- 28 Nov, 2019 4 commits
-
-
Chris Wilson authored
After obtaining a local reference to the vm from the context, remember to drop it before it goes out of scope! Signed-off-by:
Chris Wilson <chris@chris-wilson.co.uk> Cc: Matthew Auld <matthew.auld@intel.com> Reviewed-by:
Matthew Auld <matthew.auld@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191128185402.110678-1-chris@chris-wilson.co.uk
-
Chris Wilson authored
Don't rely on the RUNTIME_INFO() when we loop over a particular context and only run on a filtered set of engines. Signed-off-by:
Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191127223252.3777141-1-chris@chris-wilson.co.uk
-
Chris Wilson authored
We have a case of a mysteriously absent pulse, so dump the engine details to see if we can find out what happened to it. References: https://bugs.freedesktop.org/show_bug.cgi?id=112405Signed-off-by:
Mika Kuoppala <mika.kuoppala@linux.intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191128102546.3857140-1-chris@chris-wilson.co.uk
-
Chris Wilson authored
One does not lightly add a new hidden struct_mutex dependency deep within the execbuf bowels! The immediate suspicion in seeing the whitelist cached on the context, is that it is intended to be preserved between batches, as the kernel is quite adept at caching small allocations itself. But no, it's sole purpose is to serialise command submission in order to save a kmalloc on a slow, slow path! By removing the whitelist dependency from the context, our freedom to chop the big struct_mutex is greatly augmented. v2: s/set_bit/__set_bit/ as the whitelist shall never be accessed concurrently. Signed-off-by:
-
- 27 Nov, 2019 5 commits
-
-
Chris Wilson authored
The design of our interrupt handlers is that we ack the receipt of the interrupt first, inside the critical section where the master interrupt control is off and other cpus cannot start processing the next interrupt; and then process the interrupt events afterwards. However, Icelake introduced a whole new set of banked GT_IIR that are inherently serialised and slow to retrieve the IIR and must be processed within the critical section. We can still push our breadcrumbs out of this critical section by using our irq_worker. On bdw+, this should not make too much of a difference as we only slightly defer the breadcrumbs, but on icl+ this should make a big difference to our throughput of interrupts from concurrently executing engines. Signed-off-by:
-
Chris Wilson authored
The expected downside to commit 58b4c1a0 ("drm/i915: Reduce nested prepare_remote_context() to a trylock") was that it would need to return -EAGAIN to userspace in order to resolve potential mutex inversion. Such an unsightly round trip is unnecessary if we could atomically insert a barrier into the i915_active_fence, so make it happen. Currently, we use the timeline->mutex (or some other named outer lock) to order insertion into the i915_active_fence (and so individual nodes of i915_active). Inside __i915_active_fence_set, we only need then serialise with the interrupt handler in order to claim the timeline for ourselves. However, if we remove the outer lock, we need to ensure the order is intact between not only multiple threads trying to insert themselves into the timeline, but also with the interrupt handler completing the previous occupant. We use xchg() on insert so that we have an ordered sequence of insertions (and each caller knows the previous fence on which to wait, preserving the chain of all fences in the timeline), but we then have to cmpxchg() in the interrupt handler to avoid overwriting the new occupant. The only nasty side-effect is having to temporarily strip off the RCU-annotations to apply the atomic operations, otherwise the rules are much more conventional! Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112402 Fixes: 58b4c1a0 ("drm/i915: Reduce nested prepare_remote_context() to a trylock") Signed-off-by:
-
Chris Wilson authored
Now that we rapidly park the GT when the GPU idles, we often find ourselves idling faster than the RC6 promotion timer. Thus if we tell the GPU to enter RC6 manually as we park, we can do so quicker (by around 50ms, half an EI on average) and marginally increase our powersaving across all execlists platforms. v2: Now with a selftest to check we can enter RC6 manually Signed-off-by:
Imre Deak <imre.deak@intel.com> Reviewed-by:
Andi Shyti <andi.shyti@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191127095657.3209854-1-chris@chris-wilson.co.uk
-
Clint Taylor authored
During the Display Interrupt Service routine the Display Interrupt Enable bit must be disabled, The interrupts handled, then the Display Interrupt Enable bit must be set to prevent possible missed interrupts. Bspec: 49212 V2: Change Title to remove SDE reference. V3: Fix TAB spacing. Cc: Lucas De Marchi <lucas.demarchi@intel.com> Cc: Aditya Swarup <aditya.swarup@intel.com> Reviewed-by:
Matt Roper <matthew.d.roper@intel.com> Signed-off-by:
Clint Taylor <clinton.a.taylor@intel.com> Signed-off-by:
Lucas De Marchi <lucas.demarchi@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191121201455.2558-1-clinton.a.taylor@intel.com
-
Kai Vehmanen authored
Starting with gen12, PORT_A can be connected to a transcoder with audio support. Modify the existing logic that disabled audio on PORT_A unconditionally. Signed-off-by:
Kai Vehmanen <kai.vehmanen@linux.intel.com> Reviewed-by:
-
- 26 Nov, 2019 3 commits
-
-
Stanislav Lisovskiy authored
According to BSpec 53998, there is a mask of max 8 SAGV/QGV points we need to support. Bumping this up to keep the CI happy(currently preventing tests to run), until all SAGV changes land. v2: Fix second plane where QGV points were hardcoded as well. v3: Change the naming of I915_NUM_SAGV_POINTS to be I915_NUM_QGV_POINTS, as more meaningful (Ville Syrjälä) Reviewed-by:Ville Syrjälä <ville.syrjala@linux.intel.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112189Signed-off-by:
Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191125160800.14740-1-stanislav.lisovskiy@intel.com [vsyrjala: Add missing braces around else (checkpatch), fix Bugzilla tag] Signed-off-by:
-
Chris Wilson authored
On context retiring, we may invoke the kernel_context to unpin this context. Elsewhere, we may use the kernel_context to modify this context. This currently leads to an AB-BA lock inversion, so we need to back-off from the contended lock, and repeat. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111732Signed-off-by:
-
Chris Wilson authored
Based on a sampling of a number of benchmarks across platforms, by default opt for a much more lenient timeout so that we should not adversely affect existing "good" clients. 640ms ought to be enough for anyone. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112169 Fixes: 3a7a92ab ("drm/i915/execlists: Force preemption") Signed-off-by:
-
- 25 Nov, 2019 7 commits
-
-
Chris Wilson authored
An i915_vma struct on the stack may push the frame over the limit, if set conservatively, so move it to the heap. Signed-off-by:
-
Chris Wilson authored
The major drawback of commit 7e34f4e4 ("drm/i915/gen8+: Add RC6 CTX corruption WA") is that it disables RC6 while Skylake (and friends) is active, and we do not consider the GPU idle until all outstanding requests have been retired and the engine switched over to the kernel context. If userspace is idle, this task falls onto our background idle worker, which only runs roughly once a second, meaning that userspace has to have been idle for a couple of seconds before we enable RC6 again. Naturally, this causes us to consume considerably more energy than before as powersaving is effectively disabled while a display server (here's looking at you Xorg) is running. As execlists will get a completion event as each context is completed, we can use this interrupt to queue a retire worker bound to this engine to cleanup idle timelines. We will then immediately notice the idle engine (without userspace intervention or the aid of the background retire worker) and start parking the GPU. Thus during light workloads, we will do much more work to idle the GPU faster... Hopefully with commensurate power saving! v2: Watch context completions and only look at those local to the engine when retiring to reduce the amount of excess work we perform. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112315 References: 7e34f4e4 ("drm/i915/gen8+: Add RC6 CTX corruption WA") References: 2248a283 ("drm/i915/gen8+: Add RC6 CTX corruption WA") Signed-off-by:
-
Chris Wilson authored
In the next patch, we will introduce a new asynchronous retirement worker, fed by execlists CS events. Here we may queue a retirement as soon as a request is submitted to HW (and completes instantly), and we also want to process that retirement as early as possible and cannot afford to postpone (as there may not be another opportunity to retire it for a few seconds). To allow the new async retirer to run in parallel with our submission, pull the __i915_request_queue (that passes the request to HW) inside the timelines spinlock so that the retirement cannot release the timeline before we have completed the submission. v2: Actually to play nicely with engine_retire, we have to raise the timeline.active_lock before releasing the HW. intel_gt_retire_requsts() is still serialised by the outer lock so they cannot see this intermediate state, and engine_retire is serialised by HW submission. Signed-off-by:
-
Chris Wilson authored
As the engine->kernel_context is used within the engine-pm barrier, we have to be careful when emitting requests outside of the barrier, as the strict timeline locking rules do not apply. Instead, we must ensure the engine_park() cannot be entered as we build the request, which is simplest by taking an explicit engine-pm wakeref around the request construction. Signed-off-by:
-
Chris Wilson authored
I rushed a last minute correction to cancel_port_requests() to prevent the snooping of *execlists->active as the inflight array was being updated, without noticing we iterated the inflight array starting from active! Oops. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112387 Fixes: 331bf905 ("drm/i915/gt: Mark the execlists->active as the primary volatile access") Signed-off-by:
-
Chris Wilson authored
Since we want to do a lockless read of the current active request, and that request is written to by process_csb also without serialisation, we need to instruct gcc to take care in reading the pointer itself. Otherwise, we have observed execlists_active() to report 0x40. [ 2400.760381] igt/para-4098 1..s. 2376479300us : process_csb: rcs0 cs-irq head=3, tail=4 [ 2400.760826] igt/para-4098 1..s. 2376479303us : process_csb: rcs0 csb[4]: status=0x00000001:0x00000000 [ 2400.761271] igt/para-4098 1..s. 2376479306us : trace_ports: rcs0: promote { b9c59:2622, b9c55:2624 } [ 2400.761726] igt/para-4097 0d... 2376479311us : __i915_schedule: rcs0: -2147483648->3, inflight:0000000000000040, rq:ffff888208c1e940 which is impossible! The answer is that as we keep the existing execlists->active pointing into the array as we copy over that array, the unserialised read may see a partial pointer value. Fixes: df403069 ("drm/i915/execlists: Lift process_csb() out of the irq-off spinlock") Signed-off-by: -
Chris Wilson authored
On converting from kunmap_atomic() to kunamp() one must remember the latter takes the struct page, the former the vaddr. Fixes: 48715f70 ("drm/i915: Avoid atomic context for error capture") Signed-off-by:
-
- 23 Nov, 2019 2 commits
-
-
Chris Wilson authored
Include the name of the failing subsubtest, should it fails. Signed-off-by:
-
Tvrtko Ursulin authored
Commit 750e76b4 ("drm/i915/gt: Move the [class][inst] lookup for engines onto the GT") changed the engine query to iterate over uabi engines but left the buffer size calculation look at the physical engine count. Difference has no practical consequence but it is nicer to align both queries. Signed-off-by:
-
- 22 Nov, 2019 5 commits
-
-
Juston Li authored
This includes other platforms that utilize the same gen graphics as CFL: AML, WHL and CML. Signed-off-by:
Juston Li <juston.li@intel.com> Reviewed-by:
Ramalingam C <ramalingam.c@intel.com> Signed-off-by:
Lyude Paul <lyude@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191011181918.29618-1-juston.li@intel.com
-
Chris Wilson authored
Before checking the current i915_active state for the asynchronous work we submitted, flush any ongoing callback. This ensures that our sampling is robust and does not sporadically fail due to bad timing as the work is running on another cpu. v2: Drop the fence callback sync, retiring under the lock should be good enough to synchronize with engine_retire() and the intel_gt_retire_requests() background worker. Signed-off-by:
-
Chris Wilson authored
Bonded request submission is designed to allow requests to execute in parallel as laid out by the user. If the master request is already finished before its bonded pair is submitted, the pair were not destined to run in parallel and we lose the information about the master engine to dictate selection of the secondary. If the second request was required to be run on a particular engine in a virtual set, that should have been specified, rather than left to the whims of a random unconnected requests! In the selftest, I made the mistake of not ensuring the master would overlap with its bonded pairs, meaning that it could indeed complete before we submitted the bonds. Those bonds were then free to select any available engine in their virtual set, and not the one expected by the test. Signed-off-by:
-
Chris Wilson authored
As we start peeking into requests for longer and longer, e.g. incorporating use of spinlocks when only protected by an rcu_read_lock(), we need to be careful in how we reset the request when recycling and need to preserve any barriers that may still be in use as the request is reset for reuse. Quoting Linus Torvalds: > If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU? .. because the object can be accessed (by RCU) after the refcount has gone down to zero, and the thing has been released. That's the whole and only point of SLAB_TYPESAFE_BY_RCU. That flag basically says: "I may end up accessing this object *after* it has been free'd, because there may be RCU lookups in flight" This has nothing to do with constructors. It's ok if the object gets reused as an object of the same type and does *not* get re-initialized, because we're perfectly fine seeing old stale data. What it guarantees is that the slab isn't shared with any other kind of object, _and_ that the underlying pages are free'd after an RCU quiescent period (so the pages aren't shared with another kind of object either during an RCU walk). And it doesn't necessarily have to have a constructor, because the thing that a RCU walk will care about is (a) guaranteed to be an object that *has* been on some RCU list (so it's not a "new" object) (b) the RCU walk needs to have logic to verify that it's still the *same* object and hasn't been re-used as something else. In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used immediately, but because it gets reused as the same kind of object, the RCU walker can "know" what parts have meaning for re-use, in a way it couidn't if the re-use was random. That said, it *is* subtle, and people should be careful. > So the re-use might initialize the fields lazily, not necessarily using a ctor. If you have a well-defined refcount, and use "atomic_inc_not_zero()" to guard the speculative RCU access section, and use "atomic_dec_and_test()" in the freeing section, then you should be safe wrt new allocations. If you have a completely new allocation that has "random stale content", you know that it cannot be on the RCU list, so there is no speculative access that can ever see that random content. So the only case you need to worry about is a re-use allocation, and you know that the refcount will start out as zero even if you don't have a constructor. So you can think of the refcount itself as always having a zero constructor, *BUT* you need to be careful with ordering. In particular, whoever does the allocation needs to then set the refcount to a non-zero value *after* it has initialized all the other fields. And in particular, it needs to make sure that it uses the proper memory ordering to do so. NOTE! One thing to be very worried about is that re-initializing whatever RCU lists means that now the RCU walker may be walking on the wrong list so the walker may do the right thing for this particular entry, but it may miss walking *other* entries. So then you can get spurious lookup failures, because the RCU walker never walked all the way to the end of the right list. That ends up being a much more subtle bug. Signed-off-by: -
Chris Wilson authored
Use our more regular igt_flush_test() to bind the wait-for-idle and error out instead of waiting around forever on critical failure. Signed-off-by:
Stuart Summers <stuart.summers@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191121233021.507400-1-chris@chris-wilson.co.uk
-
- 21 Nov, 2019 6 commits
-
-
Chris Wilson authored
Whenever we wait on a request, make sure we actually hold a reference to it and that it cannot be retired/freed on another CPU! Signed-off-by:
-
Chris Wilson authored
Assume that intel_wakeref_get() may take the mutex, and perform other sleeping actions in the course of its callbacks and so use might_sleep() to ensure that all callers abide. Anything that cannot sleep has to use e.g. intel_wakeref_get_if_active() to guarantee its avoidance of the non-atomic paths. Signed-off-by:
-
Chris Wilson authored
Since the request is already on the HW as we perform its validation, it and even its subsequent barrier may be concurrently retired before we process the assertions. If it is retired already and so off the HW, our assertions become void and we need to ignore them. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112363Signed-off-by:
Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20191121103546.146487-1-chris@chris-wilson.co.uk
-
Chris Wilson authored
As we wait upon a request, we must be holding a reference to it, and be wary that i915_request_add() consumes the passed in reference. Signed-off-by:
-
Chris Wilson authored
Since retirement may be running in a worker on another CPU, it may be skipped in the local intel_gt_wait_for_idle(). To ensure the state is consistent for our sanity checks upon load, serialise with the remote retirer by waiting on the timeline->mutex. Outside of this use case, e.g. on suspend or module unload, we expect the slack to be picked up by intel_gt_pm_wait_for_idle() and so prefer to put the special case serialisation with retirement in its single user, for now at least. Signed-off-by:
-
Chris Wilson authored
From inside an active timeline in the execbuf ioctl, we may try to reclaim some space in the GGTT. We need GGTT space for all objects on !full-ppgtt platforms, and for context images everywhere. However, to free up space in the GGTT we may need to remove some pinned objects (e.g. context images) that require flushing the idle barriers to remove. For this we use the big hammer of intel_gt_wait_for_idle() However, commit 7936a22d ("drm/i915/gt: Wait for new requests in intel_gt_retire_requests()") will continue spinning on the wait if a timeline is active but lacks requests, as is the case during execbuf reservation. Spinning forever is quite time consuming, so revert that commit and start again. In practice, the effect commit 7936a22d was trying to achieve is accomplished by commit 1683d24c ("drm/i915/gt: Move new timelines to the end of active_list"), so there is no immediate rush to replace the looping. Testcase: igt/gem_exec_reloc/basic-range Fixes: 7936a22d ("drm/i915/gt: Wait for new requests in intel_gt_retire_requests()") References: 1683d24c ("drm/i915/gt: Move new timelines to the end of active_list") Signed-off-by:
-
- 20 Nov, 2019 8 commits
-
-
Stuart Summers authored
GuC submission path can be called from an interrupt context and so should use a worker to avoid holding a mutex. References: 07779a76 ("drm/i915: Mark up the calling context for intel_wakeref_put()") Signed-off-by:
-
Chris Wilson authored
pm_suspend_target_state is declared under CONFIG_PM_SLEEP but only defined under CONFIG_SUSPEND. Play safe and only use the symbol if it is both declared and defined. Reported-by: kbuild-all@lists.01.org Signed-off-by:
-
Chris Wilson authored
Now that we never allow the intel_wakeref callbacks to be invoked from interrupt context, we do not need the irqsafe spinlock for the timeline. Signed-off-by:
-
Chris Wilson authored
In commit a79ca656 ("drm/i915: Push the wakeref->count deferral to the backend"), I erroneously concluded that we last modify the engine inside __i915_request_commit() meaning that we could enable concurrent submission for userspace as we enqueued this request. However, this falls into a trap with other users of the engine->kernel_context waking up and submitting their request before the idle-switch is queued, with the result that the kernel_context is executed out-of-sequence most likely upsetting the GPU and certainly ourselves when we try to retire the out-of-sequence requests. As such we need to hold onto the effective engine->kernel_context mutex lock (via the engine pm mutex proxy) until we have finish queuing the request to the engine. v2: Serialise against concurrent intel_gt_retire_requests() v3: Describe the hairy locking scheme with intel_gt_retire_requests() for future reference. v4: Combine timeline->lock and engine pm release; it's hairy. Fixes: a79ca656 ("drm/i915: Push the wakeref->count deferral to the backend") Signed-off-by:
-
Chris Wilson authored
The general concept was that intel_timeline.active_count was locked by the intel_timeline.mutex. The exception was for power management, where the engine->kernel_context->timeline could be manipulated under the global wakeref.mutex. This was quite solid, as we always manipulated the timeline only while we held an engine wakeref. And then we started retiring requests outside of struct_mutex, only using the timelines.active_list and the timeline->mutex. There we started manipulating intel_timeline.active_count outside of an engine wakeref, and so introduced a race between __engine_park() and intel_gt_retire_requests(), a race that could result in the engine->kernel_context not being added to the active timelines and so losing requests, which caused us to keep the system permanently powered up [and unloadable]. The race would be easy to close if we could take the engine wakeref for the timeline before we retire -- except timelines are not bound to any engine and so we would need to keep all active engines awake. The alternative is to guard intel_timeline_enter/intel_timeline_exit for use outside of the timeline->mutex. Fixes: e5dadff4 ("drm/i915: Protect request retirement with timeline->mutex") Signed-off-by:
-
Chris Wilson authored
Previously, we assumed we could use mutex_trylock() within an atomic context, falling back to a worker if contended. However, such trickery is illegal inside interrupt context, and so we need to always use a worker under such circumstances. As we normally are in process context, we can typically use a plain mutex, and only defer to a work when we know we are being called from an interrupt path. Fixes: 51fbd8de ("drm/i915/pmu: Atomically acquire the gt_pm wakeref") References: a0855d24 ("locking/mutex: Complain upon mutex API misuse in IRQ contexts") References: https://bugs.freedesktop.org/show_bug.cgi?id=111626Signed-off-by:
-
Stuart Summers authored
When display is not available, finding the memory bandwidth available for display is not useful. Skip this sequence here. References: HSDES 1209978255 Signed-off-by:
-
Stuart Summers authored
Platforms without display do not map the MCHBAR MMIO into the GFX device BAR. Skip this sequence when display is not available. Signed-off-by:
-
