summaryrefslogtreecommitdiffstats
path: root/drivers/gpu/drm/i915/i915_gem.c
Commit message (Collapse)AuthorAgeFilesLines
...
* | | drm/i915: Refactor duplicate object vmap functionsChris Wilson2016-04-111-0/+44
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | We now have two implementations for vmapping a whole object, one for dma-buf and one for the ringbuffer. If we couple the mapping into the obj->pages lifetime, then we can reuse an obj->mapping for both and at the same time couple it into the shrinker. There is a third vmapping routine in the cmdparser that maps only a range within the object, for the time being that is left alone, but will eventually use these routines in order to cache the mapping between invocations. v2: Mark the failable kmalloc() as __GFP_NOWARN (vsyrjala) v3: Call unpin_vmap from the right dmabuf unmapper v4: Rename vmap to map as we don't wish to imply the type of mapping involved, just that it contiguously maps the object into kernel space. Add kerneldoc and lockdep annotations Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Dave Gordon <david.s.gordon@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460113874-17366-4-git-send-email-chris@chris-wilson.co.uk Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
* | | drm/i915: Apply a mb between emitting the request and hangcheckChris Wilson2016-04-081-17/+22
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Seal the request and mark it as pending execution before we submit it to hardware. We assume that the actual submission cannot fail (that guarantee is provided by preallocating space in the request for the submission). As we may inspect this state without holding any locks during hangcheck we should apply a barrier to ensure that we do not see a more recent value in the HWS than we are tracking. Based on a patch by Mika Kuoppala. Suggested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460010558-10705-8-git-send-email-chris@chris-wilson.co.uk Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
* | | drm/i915: Move the hw semaphore initialisation from GEM to the engineChris Wilson2016-04-081-6/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Since we are setting engine local values that are tied to the hardware, move it out of i915_gem_init_seqno() into the intel_ring_init_seqno() backend, next to where the other hw semaphore registers are written. v2: Make the explanatory comment about always resetting the semaphores to 0 irrespective of the value of the reset seqno. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460010558-10705-4-git-send-email-chris@chris-wilson.co.uk
* | | drm/i915: On GPU reset, set the HWS breadcrumb to the last seqnoChris Wilson2016-04-081-0/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | After the GPU reset and we discard all of the incomplete requests, mark the GPU as having advanced to the last_submitted_seqno (as having completed the requests and ready for fresh work). The impact of this is negligible, as all the requests will be considered completed by this point, it just brings the HWS into line with expectations for external viewers. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460010558-10705-2-git-send-email-chris@chris-wilson.co.uk
* | | drm/i915: Move execlists irq handler to a bottom halfTvrtko Ursulin2016-04-041-4/+6
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Doing a lot of work in the interrupt handler introduces huge latencies to the system as a whole. Most dramatic effect can be seen by running an all engine stress test like igt/gem_exec_nop/all where, when the kernel config is lean enough, the whole system can be brought into multi-second periods of complete non-interactivty. That can look for example like this: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 23s! [kworker/u8:3:143] Modules linked in: [redacted for brevity] CPU: 0 PID: 143 Comm: kworker/u8:3 Tainted: G U L 4.5.0-160321+ #183 Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1 Workqueue: i915 gen6_pm_rps_work [i915] task: ffff8800aae88000 ti: ffff8800aae90000 task.ti: ffff8800aae90000 RIP: 0010:[<ffffffff8104a3c2>] [<ffffffff8104a3c2>] __do_softirq+0x72/0x1d0 RSP: 0000:ffff88014f403f38 EFLAGS: 00000206 RAX: ffff8800aae94000 RBX: 0000000000000000 RCX: 00000000000006e0 RDX: 0000000000000020 RSI: 0000000004208060 RDI: 0000000000215d80 RBP: ffff88014f403f80 R08: 0000000b1b42c180 R09: 0000000000000022 R10: 0000000000000004 R11: 00000000ffffffff R12: 000000000000a030 R13: 0000000000000082 R14: ffff8800aa4d0080 R15: 0000000000000082 FS: 0000000000000000(0000) GS:ffff88014f400000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fa53b90c000 CR3: 0000000001a0a000 CR4: 00000000001406f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Stack: 042080601b33869f ffff8800aae94000 00000000fffc2678 ffff88010000000a 0000000000000000 000000000000a030 0000000000005302 ffff8800aa4d0080 0000000000000206 ffff88014f403f90 ffffffff8104a716 ffff88014f403fa8 Call Trace: <IRQ> [<ffffffff8104a716>] irq_exit+0x86/0x90 [<ffffffff81031e7d>] smp_apic_timer_interrupt+0x3d/0x50 [<ffffffff814f3eac>] apic_timer_interrupt+0x7c/0x90 <EOI> [<ffffffffa01c5b40>] ? gen8_write64+0x1a0/0x1a0 [i915] [<ffffffff814f2b39>] ? _raw_spin_unlock_irqrestore+0x9/0x20 [<ffffffffa01c5c44>] gen8_write32+0x104/0x1a0 [i915] [<ffffffff8132c6a2>] ? n_tty_receive_buf_common+0x372/0xae0 [<ffffffffa017cc9e>] gen6_set_rps_thresholds+0x1be/0x330 [i915] [<ffffffffa017eaf0>] gen6_set_rps+0x70/0x200 [i915] [<ffffffffa0185375>] intel_set_rps+0x25/0x30 [i915] [<ffffffffa01768fd>] gen6_pm_rps_work+0x10d/0x2e0 [i915] [<ffffffff81063852>] ? finish_task_switch+0x72/0x1c0 [<ffffffff8105ab29>] process_one_work+0x139/0x350 [<ffffffff8105b186>] worker_thread+0x126/0x490 [<ffffffff8105b060>] ? rescuer_thread+0x320/0x320 [<ffffffff8105fa64>] kthread+0xc4/0xe0 [<ffffffff8105f9a0>] ? kthread_create_on_node+0x170/0x170 [<ffffffff814f351f>] ret_from_fork+0x3f/0x70 [<ffffffff8105f9a0>] ? kthread_create_on_node+0x170/0x170 I could not explain, or find a code path, which would explain a +20 second lockup, but from some instrumentation it was apparent the interrupts off proportion of time was between 10-25% under heavy load which is quite bad. When a interrupt "cliff" is reached, which was >~320k irq/s on my machine, the whole system goes into a terrible state of the above described multi-second lockups. By moving the GT interrupt handling to a tasklet in a most simple way, the problem above disappears completely. Testing the effect on sytem-wide latencies using igt/gem_syslatency shows the following before this patch: gem_syslatency: cycles=1532739, latency mean=416531.829us max=2499237us gem_syslatency: cycles=1839434, latency mean=1458099.157us max=4998944us gem_syslatency: cycles=1432570, latency mean=2688.451us max=1201185us gem_syslatency: cycles=1533543, latency mean=416520.499us max=2498886us This shows that the unrelated process is experiencing huge delays in its wake-up latency. After the patch the results look like this: gem_syslatency: cycles=808907, latency mean=53.133us max=1640us gem_syslatency: cycles=862154, latency mean=62.778us max=2117us gem_syslatency: cycles=856039, latency mean=58.079us max=2123us gem_syslatency: cycles=841683, latency mean=56.914us max=1667us Showing a huge improvement in the unrelated process wake-up latency. It also shows an approximate halving in the number of total empty batches submitted during the test. This may not be worrying since the test puts the driver under a very unrealistic load with ncpu threads doing empty batch submission to all GPU engines each. Another benefit compared to the hard-irq handling is that now work on all engines can be dispatched in parallel since we can have up to number of CPUs active tasklets. (While previously a single hard-irq would serially dispatch on one engine after another.) More interesting scenario with regards to throughput is "gem_latency -n 100" which shows 25% better throughput and CPU usage, and 14% better dispatch latencies. I did not find any gains or regressions with Synmark2 or GLbench under light testing. More benchmarking is certainly required. v2: * execlists_lock should be taken as spin_lock_bh when queuing work from userspace now. (Chris Wilson) * uncore.lock must be taken with spin_lock_irq when submitting requests since that now runs from either softirq or process context. v3: * Expanded commit message with more testing data; * converted missed locking sites to _bh; * added execlist_lock comment. (Chris Wilson) v4: * Mention dispatch parallelism in commit. (Chris Wilson) * Do not hold uncore.lock over MMIO reads since the block is already serialised per-engine via the tasklet itself. (Chris Wilson) * intel_lrc_irq_handler should be static. (Chris Wilson) * Cancel/sync the tasklet on GPU reset. (Chris Wilson) * Document and WARN that tasklet cannot be active/pending on engine cleanup. (Chris Wilson/Imre Deak) Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Imre Deak <imre.deak@intel.com> Testcase: igt/gem_exec_nop/all Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94350 Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1459768316-6670-1-git-send-email-tvrtko.ursulin@linux.intel.com
* | | drm/i915: Refer to GGTT {,VM} consistentlyJoonas Lahtinen2016-03-311-18/+32
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Refer to the GGTT VM consistently as "ggtt->base" instead of just "ggtt", "vm" or indirectly through other variables like "dev_priv->ggtt.base" to avoid confusion with the i915_ggtt object itself and PPGTT VMs. Refer to the GGTT as "ggtt" instead of indirectly through chaining. As a bonus gets rid of the long-standing i915_obj_to_ggtt vs. i915_gem_obj_to_ggtt conflict, due to removal of i915_obj_to_ggtt! v2: - Added some more after grepping sources with Chris v3: - Refer to GGTT VM through ggtt->base consistently instead of ggtt_vm (Chris) v4: - Convert all dev_priv->ggtt->foo accesses to ggtt->foo. v5: - Make patch checker happy Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
* | | drm/i915: Rename GGTT init functionsJoonas Lahtinen2016-03-301-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Rename and document the GGTT init functions to give a better idea of the context where they are called from. i915_gem_gtt_init => i915_ggtt_init_hw i915_gem_init_global_gtt => i915_gem_init_ggtt i915_global_gtt_cleanup => i915_ggtt_cleanup_hw Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1458830866-12578-1-git-send-email-joonas.lahtinen@linux.intel.com
* | | drm/i915: BUG_ON when ggtt_view is NULLMatthew Auld2016-03-301-7/+2
| |/ |/| | | | | | | | | | | | | | | | | | | | | | | | | | | Lets BUG_ON and don't bother with a WARN and returning an error, so we can remove the need to pollute the code with error handling, after all it is a programmer error to provide NULL view. Also while we're here remove redundant NULL ggtt_view check. Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Signed-off-by: Matthew Auld <matthew.auld@intel.com> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1458834860-7898-1-git-send-email-matthew.auld@intel.com
* | drm/i915: replace for_each_engine()Dave Gordon2016-03-241-28/+22
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Having provided for_each_engine_id() for cases where the third (id) argument is useful, we can now replace all the remaining instances with a simpler version that takes only two parameters. In many cases, this also allows the elimination of the local variable used in the iterator (usually 'i'). v2: s/dev_priv/(dev_priv__)/ in body of for_each_engine_masked() [Chris Wilson] Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1458757194-17783-2-git-send-email-david.s.gordon@intel.com
* | drm/i915: Rename dev_priv->gtt to dev_priv->ggttJoonas Lahtinen2016-03-181-8/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | Refer to Global GTT consistently as GGTT, thus rename dev_priv->gtt to dev_priv->ggtt and struct i915_gtt to struct i915_ggtt. Fix a couple of whitespace problems while at it. v2: - Fix a typo in commit message. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
* | drm/i915: Use shorter route to dev_private where possibleTvrtko Ursulin2016-03-181-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Where we have a request we can use req->i915 directly instead of going through the engine and device. Coccinelle script: @@ function f; identifier r; @@ f(..., struct drm_i915_gem_request *r, ...) { ... - engine->dev->dev_private + r->i915 ... } @@ struct drm_i915_gem_request *req; @@ ( req-> - engine->dev->dev_private + i915 ) Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1458219850-21007-1-git-send-email-tvrtko.ursulin@linux.intel.com
* | drm/i915: Remove unused variable in i915_gem_request_add_to_clientTvrtko Ursulin2016-03-181-2/+0
| | | | | | | | | | Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
* | drm/i915: Move load time gem_load_init earlierImre Deak2016-03-171-14/+24
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The only steps requiring device access is the fence and swizzling initialization, so split these out keeping them in their current place and move the rest of init steps earlier. v2-v3: - unchanged v4: - move call to i915_gem_detect_bit_6_swizzle() to i915_gem_load_init_fences() and preserve the original order of the detection of HW fence capailities wrt. swizzling (Chris) CC: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Imre Deak <imre.deak@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1458132843-21860-1-git-send-email-imre.deak@intel.com
* | drm/i915: Modify reset func to handle per engine resetsMika Kuoppala2016-03-171-7/+7
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | In full gpu reset we prime all engines and reset domains corresponding to each engine. Per engine reset is just a special case of this process wherein only a single engine is reset. This change is aimed to modify relevant functions to achieve this. There are some other steps we carry out in case of engine reset which are addressed in later patches. Reset func now accepts a mask of all engines that need to be reset. Where per engine resets are supported, error handler populates the mask accordingly otherwise all engines are specified. v2: ALL_ENGINES mask fixup, better for_each_ring_masked (Chris) v3: Whitespace fixes (Chris) v4: Rebase due to s/ring/engine Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1458143640-20563-1-git-send-email-mika.kuoppala@intel.com
* | drm/i915: More renaming of rings to enginesTvrtko Ursulin2016-03-161-25/+25
| | | | | | | | | | | | | | | | | | | | | | This time using only sed and a few by hand. v2: Rename also intel_ring_id and intel_ring_initialized. v3: Fixed typo in intel_ring_initialized. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1458126040-33105-1-git-send-email-tvrtko.ursulin@linux.intel.com
* | drm/i915: More intel_engine_cs renamingTvrtko Ursulin2016-03-161-37/+37
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Some trivial ones, first pass done with Coccinelle: @@ @@ ( - I915_NUM_RINGS + I915_NUM_ENGINES | - intel_ring_flag + intel_engine_flag | - for_each_ring + for_each_engine | - i915_gem_request_get_ring + i915_gem_request_get_engine | - intel_ring_idle + intel_engine_idle | - i915_gem_reset_ring_status + i915_gem_reset_engine_status | - i915_gem_reset_ring_cleanup + i915_gem_reset_engine_cleanup | - init_ring_lists + init_engine_lists ) But that didn't fully work so I cleaned it up with: for f in *.[hc]; do sed -i -e s/I915_NUM_RINGS/I915_NUM_ENGINES/ $f; done for f in *.[hc]; do sed -i -e s/i915_gem_request_get_ring/i915_gem_request_get_engine/ $f; done for f in *.[hc]; do sed -i -e s/intel_ring_flag/intel_engine_flag/ $f; done for f in *.[hc]; do sed -i -e s/intel_ring_idle/intel_engine_idle/ $f; done for f in *.[hc]; do sed -i -e s/init_ring_lists/init_engine_lists/ $f; done for f in *.[hc]; do sed -i -e s/i915_gem_reset_ring_cleanup/i915_gem_reset_engine_cleanup/ $f; done for f in *.[hc]; do sed -i -e s/i915_gem_reset_ring_status/i915_gem_reset_engine_status/ $f; done v2: Rebase. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
* | drm/i915: Rename intel_engine_cs struct membersTvrtko Ursulin2016-03-161-19/+19
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | below and a couple manual fixups. @@ identifier I, J; @@ struct I { ... - struct intel_engine_cs *J; + struct intel_engine_cs *engine; ... } @@ identifier I, J; @@ struct I { ... - struct intel_engine_cs J; + struct intel_engine_cs engine; ... } @@ struct drm_i915_private *d; @@ ( - d->ring + d->engine ) @@ struct i915_execbuffer_params *p; @@ ( - p->ring + p->engine ) @@ struct intel_ringbuffer *r; @@ ( - r->ring + r->engine ) @@ struct drm_i915_gem_request *req; @@ ( - req->ring + req->engine ) v2: Script missed the tracepoint code - fixed up by hand. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
* | drm/i915: Rename intel_engine_cs function parametersTvrtko Ursulin2016-03-161-43/+43
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | @@ identifier func; @@ func(..., struct intel_engine_cs * - ring + engine , ...) { <... - ring + engine ...> } @@ identifier func; type T; @@ T func(..., struct intel_engine_cs * - ring + engine , ...); Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
* | drm/i915: Rename local struct intel_engine_cs variablesTvrtko Ursulin2016-03-161-68/+68
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Done by the Coccinelle script below plus a manual intervention to GEN8_RING_SEMAPHORE_INIT. @@ expression E; @@ - struct intel_engine_cs *ring = E; + struct intel_engine_cs *engine = E; <+... - ring + engine ...+> @@ @@ - struct intel_engine_cs *ring; + struct intel_engine_cs *engine; <+... - ring + engine ...+> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
* | drm/i915: Avoid snooping with userptr where not supportedTvrtko Ursulin2016-03-021-1/+1
|/ | | | | | | | | | | | | | | | | | | | | | | | | commit e5756c10d841ddb448293c849392f3d6b809561f Author: Imre Deak <imre.deak@intel.com> Date: Fri Aug 14 18:43:30 2015 +0300 drm/i915/bxt: don't allow cached GEM mappings on A stepping Added an exception of disallowing snooping for Broxton A stepping hardware but userptr was still enabling it regardless. Move the check to HAS_SNOOP now that it is used from multiple call sites and use it. v2: Userptr cannot be supported when it cannot be coherent and generalize the code better. (Chris Wilson) v3: Make has_snoop true only when !has_llc. (Chris Wilson) Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Imre Deak <imre.deak@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1456920631-34302-1-git-send-email-tvrtko.ursulin@linux.intel.com
* drm/i915: Reduce the pointer dance of i915_is_ggtt()Chris Wilson2016-02-261-11/+7
| | | | | | | | | | | The multiple levels of indirect do nothing but hinder the compiler and the pointer chasing turns to be quite painful but painless to fix. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Dave Gordon <david.s.gordon@intel.com> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1456484600-11477-1-git-send-email-tvrtko.ursulin@linux.intel.com
* drm/i915: Rename vma->*_list to *_link for consistencyChris Wilson2016-02-261-25/+25
| | | | | | | | | | | | | | Elsewhere we have adopted the convention of using '_link' to denote elements in the list (and '_list' for the actual list_head itself), and that the name should indicate which list the link belongs to (and preferrably not just where the link is being stored). s/vma_link/obj_link/ (we iterate over obj->vma_list) s/mm_list/vm_link/ (we iterate over vm->[in]active_list) Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
* drm/i915: Check for get_pages instead of shmem (filp)Ben Widawsky2016-02-151-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | This behavior of checking for a shmem backed GEM object was introduced here: commit 4c914c0c7c787b8f730128a8cdcca9c50b0784ab Author: Brad Volkin <bradley.d.volkin@intel.com> Date: Tue Feb 18 10:15:45 2014 -0800 drm/i915: Refactor shmem pread setup It is possible for an object to not be a shmem backed GEM object (for example userptr objects). An example of how we hit this failure can be found through copy_batch() in the command parser because we allocate a userptr object for the batch which contains privileged instructions. Userptr calls drm_gem_private_object_init() which explicitly sets the filp to none. NOTE: I manually retyped this from a test machine. So I haven't even compiled this exact patch. v2: Use same logic as from a2a4f916c2f (Kristian, Dave Gordon) Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Kristian Høgsberg <krh@bitplanet.net> Cc: Dave Gordon <david.s.gordon@intel.com> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> Tested-by: Jordan Justen <jordan.l.justen@intel.com> (v1) Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> (v1) Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1455047053-2644-1-git-send-email-benjamin.widawsky@intel.com
* drm/i915: Use appropriate spinlock flavourTvrtko Ursulin2016-02-151-4/+2
| | | | | | | | We know this never runs from interrupt context so don't need to use the flags variant. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
* Revert "drm/i915: fix context/engine cleanup order"Daniel Vetter2016-02-151-12/+11
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This reverts commit 1b39a917a9e00378c02c50ad86632ed3d872bfad. Chris retracted his reviewed-by (which I failed to notice) and somehow it blows up (I did it again!) as reported by Mika with the below backtrace on module reload: [ 58.170374] IP: [<ffffffffa00e04d3>] intel_logical_ring_cleanup+0x83/0x100 [i915] ... [ 58.170469] Call Trace: [ 58.170479] [<ffffffffa00d0ed4>] i915_gem_cleanup_engines+0x34/0x60 [i915] [ 58.170493] [<ffffffffa0154520>] i915_driver_unload+0x140/0x220 [i915] [ 58.170497] [<ffffffff8154a4f4>] drm_dev_unregister+0x24/0xa0 [ 58.170501] [<ffffffff8154aace>] drm_put_dev+0x1e/0x60 [ 58.170506] [<ffffffffa00912a0>] i915_pci_remove+0x10/0x20 [i915] [ 58.170510] [<ffffffff814766e4>] pci_device_remove+0x34/0xb0 [ 58.170514] [<ffffffff8156e7d5>] __device_release_driver+0x95/0x140 [ 58.170518] [<ffffffff8156e97c>] driver_detach+0xbc/0xc0 [ 58.170521] [<ffffffff8156d883>] bus_remove_driver+0x53/0xd0 [ 58.170525] [<ffffffff8156f3a7>] driver_unregister+0x27/0x50 [ 58.170528] [<ffffffff81475725>] pci_unregister_driver+0x25/0x70 [ 58.170531] [<ffffffff8154c274>] drm_pci_exit+0x74/0x90 [ 58.170543] [<ffffffffa0154cb0>] i915_exit+0x20/0x1aa [i915] [ 58.170548] [<ffffffff8111846f>] SyS_delete_module+0x18f/0x1f0 Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Dave Gordon <david.s.gordon@intel.com> Cc: Nick Hoath <nicholas.hoath@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
* drm/i915: fix context/engine cleanup orderNick Hoath2016-02-101-11/+12
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | Swap the order of context & engine cleanup, so that contexts are cleaned up first, and *then* engines. This is a more sensible order anyway, but in particular has become necessary since the 'intel_ring_initialized() must be simple and inline' patch, which now uses ring->dev as an 'initialised' flag, so it can now be NULL after engine teardown. This in turn can cause a problem in the context code, which (used to) check the ring->dev->struct_mutex -- causing a fault if ring->dev was NULL. Also rename the cleanup function to reflect what it actually does (cleanup engines, not a ringbuffer), and fix an annoying whitespace issue. v2: Also make the fix in i915_load_modeset_init, not just in i915_driver_unload (Chris Wilson) v3: Had extra stuff in it. v4: Reverted extra stuff (so we're back to v2). Rebased and updated commentary above (Dave Gordon). Signed-off-by: Nick Hoath <nicholas.hoath@intel.com> Signed-off-by: David Gordon <david.s.gordon@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1453504211-7982-2-git-send-email-david.s.gordon@intel.com
* drm/i915: Allow i915_gem_object_get_page() on userptr as wellChris Wilson2016-02-031-1/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | commit 033908aed5a596f6202c848c6bbc8a40fb1a8490 Author: Dave Gordon <david.s.gordon@intel.com> Date: Thu Dec 10 18:51:23 2015 +0000 drm/i915: mark GEM object pages dirty when mapped & written by the CPU introduced a check into i915_gem_object_get_dirty_pages() that returned a NULL pointer when called with a bad object, one that was not backed by shmemfs. This WARN was too strict as we can work on all struct page backed objects, and resulted in a WARN + GPF for existing userspace. In order to differentiate the various types of objects, add a new flags field to the i915_gem_object_ops struct to describe their capabilities, with the first flag being whether the object has struct pages. v2: Drop silly const before an integer in the structure declaration. Testcase: igt/gem_userptr_blits/relocations Reported-and-tested-by: Kristian Høgsberg Kristensen <krh@bitplanet.net> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Dave Gordon <david.s.gordon@intel.com> Cc: Kristian Høgsberg Kristensen <krh@bitplanet.net> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Dave Gordon <david.s.gordon@intel.com> Reviewed-by: Kristian Høgsberg Kristensen <krh@bitplanet.net> Tested-by: Michal Winiarski <michal.winiarski@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1453487551-16799-1-git-send-email-chris@chris-wilson.co.uk
* drm/i915: Make LRC (un)pinning work on context and engineTvrtko Ursulin2016-01-281-1/+1
| | | | | | | | | | | | | | | Previously intel_lr_context_(un)pin were operating on requests which is in conflict with their names. If we make them take a context and an engine, it makes the names make more sense and it also makes future fixes possible. v2: Rebase for default_context/kernel_context change. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Nick Hoath <nicholas.hoath@intel.com>
* drm/i915: Sanitize i915_gem_load() init and clean-upImre Deak2016-01-271-1/+10
| | | | | | | | | | | | Factor out common clean-up code for the GEM load time init function. Also rename i915_gem_load() to i915_gem_load_init() to have a better match with its new clean-up function. No functional change. Signed-off-by: Imre Deak <imre.deak@intel.com> Reviewed-by: David Weinehall <david.weinehall@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1453209992-25995-5-git-send-email-imre.deak@intel.com
* drm/i915: Sanitize GEM shrinker init and clean-upImre Deak2016-01-271-2/+0
| | | | | | | | | | | Factor out the common GEM shrinker clean-up code and call the shrinker init function from the same function from where the corresponding shrinker clean-up function is called. Also add sanity checking to the shrinker and OOM registration calls. Signed-off-by: Imre Deak <imre.deak@intel.com> Reviewed-by: David Weinehall <david.weinehall@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1453209992-25995-4-git-send-email-imre.deak@intel.com
* Revert "drm/i915: Fix context/engine cleanup order"Daniel Vetter2016-01-271-12/+11
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This reverts commit 1803c035efb88afb9d3e7feb279ac29a83216382. It seems to blow up on module unload due to a use-after free hitting a BUG_ON with CONFIG_DEBUG_SG. Quoting from Tvrtko's mail: "I've decoded the instructions and it pointed to SG_MAGIC checking: 488b8098010000 mov 0x198(%rax),%rax ba21436587 mov $0x87654321,%edx 488b00 mov (%rax),%rax *** CRASH "Grep showed 0x87654321 is SG_MAGIC, so likely candidate for this code pattern is: static inline struct page *sg_page(struct scatterlist *sg) { BUG_ON(sg->sg_magic != SG_MAGIC); BUG_ON(sg_is_chain(sg)); return (struct page *)((sg)->page_link & ~0x3); } "Which would mean the offender is in intel_logical_ring_cleanup is most likely: ... if (ring->status_page.obj) { kunmap(sg_page(ring->status_page.obj->pages->sgl)); ring->status_page.obj = NULL; } ... "I think that the i915_gem_context_fini will do a final unref on dev_priv->kernel_context and then the ring buff has a copy which is left dangling because: lrc_setup_hardware_status_page(ring, dev_priv->kernel_context->engine[ring->id].state); and: ring->status_page.obj = default_ctx_obj; "Where default_ctx_obj == dev_priv->kernel_context->engine[ring->id].state So indeed looks like the unload ordering is the trigger. In fact it is almost the same fragility wrt/ kernel_context hidden dependency I expressed my worry about in an e-mail yesterday or so. It only shows if CONFIG_DEBUG_SG is set, otherwise it accesses freed memory and probably just survives." This causes serious trouble in our CI system since it took out all gen8+ machines. Not yet clear why this wasn't caught in pre-merge testing. Backtrace from CI, for posterity: [ 163.737836] general protection fault: 0000 [#1] PREEMPT SMP [ 163.737849] Modules linked in: ax88179_178a usbnet mii snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915(-) x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_codec snd_hwdep snd_hda_core snd_pcm mei_me mei i2c_hid e1000e ptp pps_core [last unloaded: snd_hda_intel] [ 163.737902] CPU: 0 PID: 5812 Comm: rmmod Tainted: G U W 4.5.0-rc1-gfxbench+ #1 [ 163.737911] Hardware name: System manufacturer System Product Name/Z170M-PLUS, BIOS 0505 11/16/2015 [ 163.737920] task: ffff8800bb99cf80 ti: ffff88022ff2c000 task.ti: ffff88022ff2c000 [ 163.737928] RIP: 0010:[<ffffffffa018f723>] [<ffffffffa018f723>] intel_logical_ring_cleanup+0x83/0x100 [i915] [ 163.737969] RSP: 0018:ffff88022ff2fd30 EFLAGS: 00010282 [ 163.737975] RAX: 6b6b6b6b6b6b6b6b RBX: ffff8800bb2f31b8 RCX: 0000000000000002 [ 163.737982] RDX: 0000000087654321 RSI: 000000000000000d RDI: ffff8800bb2f31f0 [ 163.737989] RBP: ffff88022ff2fd40 R08: 0000000000000000 R09: 0000000000000001 [ 163.737996] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800bb2f0000 [ 163.738003] R13: ffff8800bb2f8fc8 R14: ffff8800bb285668 R15: 000055af1ae55210 [ 163.738010] FS: 00007f187014b700(0000) GS:ffff88023bc00000(0000) knlGS:0000000000000000 [ 163.738021] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 163.738030] CR2: 0000558f84e4cbc8 CR3: 000000022cd55000 CR4: 00000000003406f0 [ 163.738039] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 163.738048] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 163.738057] Stack: [ 163.738062] ffff8800bb2f31b8 ffff8800bb2f0000 ffff88022ff2fd70 ffffffffa0180414 [ 163.738079] ffff8800bb2f0000 ffff8800bb285668 ffff8800bb2856c8 ffffffffa0242460 [ 163.738094] ffff88022ff2fd98 ffffffffa0202d30 ffff8800bb285668 ffff8800bb285668 [ 163.738109] Call Trace: [ 163.738140] [<ffffffffa0180414>] i915_gem_cleanup_engines+0x34/0x60 [i915] [ 163.738185] [<ffffffffa0202d30>] i915_driver_unload+0x150/0x270 [i915] [ 163.738198] [<ffffffff815100f4>] drm_dev_unregister+0x24/0xa0 [ 163.738208] [<ffffffff815106ce>] drm_put_dev+0x1e/0x60 [ 163.738225] [<ffffffffa01412a0>] i915_pci_remove+0x10/0x20 [i915] [ 163.738237] [<ffffffff8143d9b4>] pci_device_remove+0x34/0xb0 [ 163.738249] [<ffffffff81533d15>] __device_release_driver+0x95/0x140 [ 163.738259] [<ffffffff81533eb6>] driver_detach+0xb6/0xc0 [ 163.738268] [<ffffffff81532de3>] bus_remove_driver+0x53/0xd0 [ 163.738278] [<ffffffff815348d7>] driver_unregister+0x27/0x50 [ 163.738289] [<ffffffff8143ca15>] pci_unregister_driver+0x25/0x70 [ 163.738299] [<ffffffff81511de4>] drm_pci_exit+0x74/0x90 [ 163.738337] [<ffffffffa02034a9>] i915_exit+0x20/0x1a5 [i915] [ 163.738349] [<ffffffff8110400f>] SyS_delete_module+0x18f/0x1f0 [ 163.738361] [<ffffffff817b8a9b>] entry_SYSCALL_64_fastpath+0x16/0x73 [ 163.738370] Code: ff d0 48 89 df e8 de a1 fd ff 48 8d 7b 38 e8 25 ab fd ff 48 8b 83 90 00 00 00 48 85 c0 74 25 48 8b 80 98 01 00 00 ba 21 43 65 87 <48> 8b 00 48 39 10 75 3c f6 40 08 01 75 38 48 c7 83 90 00 00 00 [ 163.738459] RIP [<ffffffffa018f723>] intel_logical_ring_cleanup+0x83/0x100 [i915] [ 163.738498] RSP <ffff88022ff2fd30> [ 163.738507] ---[ end trace 68f69ce4740fa44f ]--- Cc: Nick Hoath <nicholas.hoath@intel.com> Cc: Dave Gordon <david.s.gordon@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
* drm/i915: Fix context/engine cleanup orderNick Hoath2016-01-251-11/+12
| | | | | | | | | | | | | | | | | | | | | | | | | | | | Swap the order of context & engine cleanup, so that contexts are cleaned up first, and *then* engines. This is a more sensible order anyway, but in particular has become necessary since the 'intel_ring_initialized() must be simple and inline' patch, which now uses ring->dev as an 'initialised' flag, so it can now be NULL after engine teardown. This in turn can cause a problem in the context code, which (used to) check the ring->dev->struct_mutex -- causing a fault if ring->dev was NULL. Also rename the cleanup function to reflect what it actually does (cleanup engines, not a ringbuffer), and fix an annoying whitespace issue. v2: Also make the fix in i915_load_modeset_init, not just in i915_driver_unload (Chris Wilson) v3: Had extra stuff in it. v4: Reverted extra stuff (so we're back to v2). Rebased and updated commentary above (Dave Gordon). Signed-off-by: Nick Hoath <nicholas.hoath@intel.com> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v2) Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1453405067-32890-3-git-send-email-david.s.gordon@intel.com Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
* drm/i915: Seal busy-ioctl uABI and prevent leaking of internal idsChris Wilson2016-01-211-4/+14
| | | | | | | | | | | | | | | | | | | | | | Tvrtko was looking through the execbuffer-ioctl and noticed that the uABI was tightly coupled to our internal engine identifiers. Close inspection also revealed that we leak those internal engine identifiers through the busy-ioctl, and those internal identifiers already do not match the user identifiers. Fortuitiously, there is only one user of the set of busy rings from the busy-ioctl, and they only wish to choose between the RENDER and the BLT engines. Let's fix the userspace ABI while we still can. v2: Update the uAPI documentation to explain the identifiers. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Testcase: igt/gem_busy Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1452876706-21620-1-git-send-email-chris@chris-wilson.co.uk
* drm/i915: Decouple execbuf uAPI from internal implementationTvrtko Ursulin2016-01-211-0/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | At the moment execbuf ring selection is fully coupled to internal ring ids which is not a good thing on its own. This dependency is also spread between two source files and not spelled out at either side which makes it hidden and fragile. This patch decouples this dependency by introducing an explicit translation table of execbuf uAPI to ring id close to the only call site (i915_gem_do_execbuffer). This way we are free to change driver internal implementation details without breaking userspace. All state relating to the uAPI is now contained in, or next to, i915_gem_do_execbuffer. As a side benefit, this patch decreases the compiled size of i915_gem_do_execbuffer. v2: Extract ring selection into eb_select_ring. (Chris Wilson) Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1452870770-13981-1-git-send-email-tvrtko.ursulin@linux.intel.com
* drm/i915: tidy up a few leftoversDave Gordon2016-01-211-4/+2
| | | | | | | | | | | | | | | | | | | There are a few bits of code which the transformations implemented by the previous patch reveal to be suboptimal, once the notion of a per- ring default context has gone away. So this tidies up the leftovers. It could have been squashed into the previous patch, but that would have made that patch less clearly a simple transformation. In particular, any change which alters the code block structure or indentation has been deferred into this separate patch, because such things tend to make diffs more difficult to read. v4: Rebased Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Reviewed-by: Nick Hoath <nicholas.hoath@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1453230175-19330-4-git-send-email-david.s.gordon@intel.com Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
* drm/i915: abolish separate per-ring default_context pointersDave Gordon2016-01-211-3/+3
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Now that we've eliminated a lot of uses of ring->default_context, we can eliminate the pointer itself. All the engines share the same default intel_context, so we can just keep a single reference to it in the dev_priv structure rather than one in each of the engine[] elements. This make refcounting more sensible too, as we now have a refcount of one for the one pointer, rather than a refcount of one but multiple pointers. From an idea by Chris Wilson. v2: transform an extra instance of ring->default_context introduced by 42f1cae8c drm/i915: Restore inhibiting the load of the default context That patch's commentary includes: v2: Mark the global default context as uninitialized on GPU reset so that the context-local workarounds are reloaded upon re-enabling The code implementing that now also benefits from the replacement of the multiple (per-ring) pointers to the default context with a single pointer to the unique kernel context. v4: Rebased, remove underused local (Nick Hoath) Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Reviewed-by: Nick Hoath <nicholas.hoath@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1453230175-19330-3-git-send-email-david.s.gordon@intel.com Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
* drm/i915: simplify allocation of driver-internal requestsDave Gordon2016-01-211-13/+42
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | There are a number of places where the driver needs a request, but isn't working on behalf of any specific user or in a specific context. At present, we associate them with the per-engine default context. A future patch will abolish those per-engine context pointers; but we can already eliminate a lot of the references to them, just by making the allocator allow NULL as a shorthand for "an appropriate context for this ring", which will mean that the callers don't need to know anything about how the "appropriate context" is found (e.g. per-ring vs per-device, etc). So this patch renames the existing i915_gem_request_alloc(), and makes it local (static inline), and replaces it with a wrapper that provides a default if the context is NULL, and also has a nicer calling convention (doesn't require a pointer to an output parameter). Then we change all callers to use the new convention: OLD: err = i915_gem_request_alloc(ring, user_ctx, &req); if (err) ... NEW: req = i915_gem_request_alloc(ring, user_ctx); if (IS_ERR(req)) ... OLD: err = i915_gem_request_alloc(ring, ring->default_context, &req); if (err) ... NEW: req = i915_gem_request_alloc(ring, NULL); if (IS_ERR(req)) ... v4: Rebased Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Reviewed-by: Nick Hoath <nicholas.hoath@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1453230175-19330-2-git-send-email-david.s.gordon@intel.com Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
* drm/i915: Only grab timestamps when neededTvrtko Ursulin2016-01-181-5/+7
| | | | | | | | | | | | No need to call ktime_get_raw_ns twice per unlimited wait and can also elimate a local variable. v2: Added comment about silencing the compiler warning. (Daniel Vetter) Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Dave Gordon <david.s.gordon@intel.com> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1452870672-13901-1-git-send-email-tvrtko.ursulin@linux.intel.com
* Merge tag 'drm-intel-next-fixes-2016-01-14' of ↵Dave Airlie2016-01-181-1/+1
|\ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | git://anongit.freedesktop.org/drm-intel into drm-next misc i915 fixes all over the place. * tag 'drm-intel-next-fixes-2016-01-14' of git://anongit.freedesktop.org/drm-intel: drm/i915/gen9: Set PIN_ZONE_4G end to 4GB - 1 page drm/i915: Widen return value for reservation_object_wait_timeout_rcu to long. drm/i915: intel_hpd_init(): Fix suspend/resume reprobing drm/i915: shut up gen8+ SDE irq dmesg noise, again drm/i915: Restore inhibiting the load of the default context drm/i915: Tune down rpm wakelock debug checks drm/i915: Avoid writing relocs with addresses in non-canonical form drm/i915: Move Braswell stop_machine GGTT insertion workaround
| * drm/i915/gen9: Set PIN_ZONE_4G end to 4GB - 1 pageMichel Thierry2016-01-131-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Kernel and userspace are able to handle 4GB (1<<32) address space range, but "A32 Stateless Model" is not. According to documentation, A32 accesses are based on General State Base Address and bound checking is in place. Because size field (instruction State Base Address) limitation, it is not possible to address full 4GB memory region. A32 Stateless Model is used by some libraries and without this patch, the last page of 4GB address space is not accessible in 32bit processes. Reported-by: Artur Harasimiuk <artur.harasimiuk@intel.com> Signed-off-by: Michel Thierry <michel.thierry@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1452512367-23614-1-git-send-email-michel.thierry@intel.com Cc: drm-intel-fixes@lists.freedesktop.org Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (cherry picked from commit 1892faa9ec5d51b07d646cbd5597cd30e049aa51) Signed-off-by: Jani Nikula <jani.nikula@intel.com>
* | Merge tag 'drm-intel-next-2015-12-18' of ↵Dave Airlie2015-12-231-67/+149
|\| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | git://anongit.freedesktop.org/drm-intel into drm-next - fix atomic watermark recomputation logic (Maarten) - modeset sequence fixes for LPT (Ville) - more kbl enabling&prep work (Rodrigo, Wayne) - first bits for mst audio - page dirty tracking fixes from Dave Gordon - new get_eld hook from Takashi, also included in the sound tree - fixup cursor handling when placed at address 0 (Ville) - refactor VBT parsing code (Jani) - rpm wakelock debug infrastructure ( Imre) - fbdev is pinned again (Chris) - tune the busywait logic to avoid wasting cpu cycles (Chris) * tag 'drm-intel-next-2015-12-18' of git://anongit.freedesktop.org/drm-intel: (81 commits) drm/i915: Update DRIVER_DATE to 20151218 drm/i915/skl: Default to noncoherent access up to F0 drm/i915: Only spin whilst waiting on the current request drm/i915: Limit the busy wait on requests to 5us not 10ms! drm/i915: Break busywaiting for requests on pending signals drm/i915: don't enable autosuspend on platforms without RPM support drm/i915/backlight: prefer dev_priv over dev pointer drm/i915: Disable primary plane if we fail to reconstruct BIOS fb (v2) drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping drm/i915: Set the map-and-fenceable flag for preallocated objects drm/i915: mdelay(10) considered harmful drm/i915: check that we are in an RPM atomic section in GGTT PTE updaters drm/i915: add support for checking RPM atomic sections drm/i915: check that we hold an RPM wakelock ref before we put it drm/i915: add support for checking if we hold an RPM reference drm/i915: use assert_rpm_wakelock_held instead of opencoding it drm/i915: add assert_rpm_wakelock_held helper drm/i915: remove HAS_RUNTIME_PM check from RPM get/put/assert helpers drm/i915: get a permanent RPM reference on platforms w/o RPM support drm/i915: refactor RPM disabling due to RC6 being disabled ...
| * drm/i915: Only spin whilst waiting on the current requestChris Wilson2015-12-181-1/+7
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Limit busywaiting only to the request currently being processed by the GPU. If the request is not currently being processed by the GPU, there is a very low likelihood of it being completed within the 2 microsecond spin timeout and so we will just be wasting CPU cycles. v2: Check for logical inversion when rebasing - we were incorrectly checking for this request being active, and instead busywaiting for when the GPU was not yet processing the request of interest. v3: Try another colour for the seqno names. v4: Another colour for the function names. v5: Remove the forced coherency when checking for the active request. On reflection and plenty of recent experimentation, the issue is not a cache coherency problem - but an irq/seqno ordering problem (timing issue). Here, we do not need the w/a to force ordering of the read with an interrupt. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Eero Tamminen <eero.t.tamminen@intel.com> Cc: "Rantala, Valtteri" <valtteri.rantala@intel.com> Cc: stable@vger.kernel.org Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1449833608-22125-4-git-send-email-chris@chris-wilson.co.uk
| * drm/i915: Limit the busy wait on requests to 5us not 10ms!Chris Wilson2015-12-181-2/+45
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When waiting for high frequency requests, the finite amount of time required to set up the irq and wait upon it limits the response rate. By busywaiting on the request completion for a short while we can service the high frequency waits as quick as possible. However, if it is a slow request, we want to sleep as quickly as possible. The tradeoff between waiting and sleeping is roughly the time it takes to sleep on a request, on the order of a microsecond. Based on measurements of synchronous workloads from across big core and little atom, I have set the limit for busywaiting as 10 microseconds. In most of the synchronous cases, we can reduce the limit down to as little as 2 miscroseconds, but that leaves quite a few test cases regressing by factors of 3 and more. The code currently uses the jiffie clock, but that is far too coarse (on the order of 10 milliseconds) and results in poor interactivity as the CPU ends up being hogged by slow requests. To get microsecond resolution we need to use a high resolution timer. The cheapest of which is polling local_clock(), but that is only valid on the same CPU. If we switch CPUs because the task was preempted, we can also use that as an indicator that the system is too busy to waste cycles on spinning and we should sleep instead. __i915_spin_request was introduced in commit 2def4ad99befa25775dd2f714fdd4d92faec6e34 [v4.2] Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Tue Apr 7 16:20:41 2015 +0100 drm/i915: Optimistically spin for the request completion v2: Drop full u64 for unsigned long - the timer is 32bit wraparound safe, so we can use native register sizes on smaller architectures. Mention the approximate microseconds units for elapsed time and add some extra comments describing the reason for busywaiting. v3: Raise the limit to 10us v4: Now 5us. Reported-by: Jens Axboe <axboe@kernel.dk> Link: https://lkml.org/lkml/2015/11/12/621 Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Eero Tamminen <eero.t.tamminen@intel.com> Cc: "Rantala, Valtteri" <valtteri.rantala@intel.com> Cc: stable@vger.kernel.org Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1449833608-22125-3-git-send-email-chris@chris-wilson.co.uk
| * drm/i915: Break busywaiting for requests on pending signalsChris Wilson2015-12-181-5/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The busywait in __i915_spin_request() does not respect pending signals and so may consume the entire timeslice for the task instead of returning to userspace to handle the signal. In the worst case this could cause a delay in signal processing of 20ms, which would be a noticeable jitter in cursor tracking. If a higher resolution signal was being used, for example to provide fairness of a server timeslices between clients, we could expect to detect some unfairness between clients (i.e. some windows not updating as fast as others). This issue was noticed when inspecting a report of poor interactivity resulting from excessively high __i915_spin_request usage. Fixes regression from commit 2def4ad99befa25775dd2f714fdd4d92faec6e34 [v4.2] Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Tue Apr 7 16:20:41 2015 +0100 drm/i915: Optimistically spin for the request completion v2: Try to assess the impact of the bug Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Jens Axboe <axboe@kernel.dk> Cc; "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Eero Tamminen <eero.t.tamminen@intel.com> Cc: "Rantala, Valtteri" <valtteri.rantala@intel.com> Cc: stable@vger.kernel.org Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1449833608-22125-2-git-send-email-chris@chris-wilson.co.uk
| * drm/i915: Set the map-and-fenceable flag for preallocated objectsChris Wilson2015-12-171-19/+24
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | As we mark the preallocated objects as bound, we should also flag them correctly as being map-and-fenceable (if appropriate!) so that later users do not get confused and try and rebind the pinned vma in order to get a map-and-fenceable binding. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: "Goel, Akash" <akash.goel@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Cc: drm-intel-fixes@lists.freedesktop.org Link: http://patchwork.freedesktop.org/patch/msgid/1448029000-10616-1-git-send-email-chris@chris-wilson.co.uk Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
| * Merge tag 'drm-i915-get-eld' of tiwai/sound into drm-intel-next-queuedDaniel Vetter2015-12-111-2/+10
| |\ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Add get_eld audio component for i915/HD-audio Currently, the HDMI/DP audio status and ELD are notified and obtained via the hardware-level communication over HD-audio unsolicited event and verbs although the graphics driver holds the exactly same information. As we already have a notification via audio component, this is another step forward; namely, the audio driver may fetch directly the audio status and ELD via the new component op. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
| * | drm/i915: mark a newly-created GEM object dirty when filled with dataDave Gordon2015-12-111-0/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When creating a new (pageable) GEM object and filling it with data, we must mark it as 'dirty', i.e. backing store is out-of-date w.r.t. the newly-written content. This ensures that if the object is evicted under memory pressure, its pages in the pagecache will be written to backing store rather than discarded. Based on an original version by Alex Dai. Signed-off-by: Alex Dai <yu.dai@intel.com> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1449773486-30822-3-git-send-email-david.s.gordon@intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
| * | drm/i915: mark GEM object pages dirty when mapped & written by the CPUDave Gordon2015-12-111-0/+15
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | In various places, a single page of a (regular) GEM object is mapped into CPU address space and updated. In each such case, either the page or the the object should be marked dirty, to ensure that the modifications are not discarded if the object is evicted under memory pressure. The typical sequence is: va = kmap_atomic(i915_gem_object_get_page(obj, pageno)); *(va+offset) = ... kunmap_atomic(va); Here we introduce i915_gem_object_get_dirty_page(), which performs the same operation as i915_gem_object_get_page() but with the side-effect of marking the returned page dirty in the pagecache. This will ensure that if the object is subsequently evicted (due to memory pressure), the changes are written to backing store rather than discarded. Note that it works only for regular (shmfs-backed) GEM objects, but (at least for now) those are the only ones that are updated in this way -- the objects in question are contexts and batchbuffers, which are always shmfs-backed. Separate patches deal with the cases where whole objects are (or may be) dirtied. v3: Mark two more pages dirty in the page-boundary-crossing cases of the execbuffer relocation code [Chris Wilson] Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1449773486-30822-2-git-send-email-david.s.gordon@intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
| * | drm/i915: Update to post-reset execlist queue clean-upTomas Elf2015-12-111-11/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When clearing an execlist queue, instead of traversing it and unreferencing all requests while holding the spinlock (which might lead to thread sleeping with IRQs are turned off - bad news!), just move all requests to the retire request list while holding spinlock and then drop spinlock and invoke the execlists request retirement path, which already deals with the intricacies of purging/dereferencing execlist queue requests. This patch can be considered v3 of: commit b96db8b81c54ef30485ddb5992d63305d86ea8d3 Author: Tomas Elf <tomas.elf@intel.com> drm/i915: Grab execlist spinlock to avoid post-reset concurrency issues This patch assumes v2 of the above patch is part of the baseline, reverts v2 and adds changes on top to turn it into v3. Signed-off-by: Tomas Elf <tomas.elf@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1445619757-19822-1-git-send-email-tomas.elf@intel.com Reviewed-by: Thomas Daniel <thomas.daniel@intel.com> Reviewed-by: Dave Gordon <dave.gordon@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
| * | drm/i915: Remove VLV A0 hackWayne Boyer2015-12-101-8/+0
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Do some further clean up based on the initial review of drm/i915: Separate cherryview from valleyview. In this case remove a hack for VLV A0. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Wayne Boyer <wayne.boyer@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1449514270-15171-4-git-send-email-wayne.boyer@intel.com Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
OpenPOWER on IntegriCloud