[Intel-gfx] [PATCH] drm/i915/execlists: Move the assertion we have the rpm wakeref down
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Jul 19 11:49:13 UTC 2018
On 19/07/2018 08:50, Chris Wilson wrote:
> There's a race between idling the engine and finishing off the last
> tasklet (as we may kick the tasklets after declaring an individual
> engine idle). However, since we do not need to access the device until
> we try to submit to the ELSP register (processing the CSB just requires
> normal CPU access to the HWSP, and when idle we should not need to
> submit!) we can defer the assertion unto that point. The assertion is
> still useful as it does verify that we do hold the longterm GT wakeref
> taken from request allocation until request completion.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107274
> Fixes: 9512f985c32d ("drm/i915/execlists: Direct submission of new requests (avoid tasklet/ksoftirqd)")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 25 +++++++++++--------------
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index db5351e6a3a5..9d693e61536c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -452,6 +452,16 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)
> struct execlist_port *port = execlists->port;
> unsigned int n;
>
> + /*
> + * We can skip acquiring intel_runtime_pm_get() here as it was taken
> + * on our behalf by the request (see i915_gem_mark_busy()) and it will
> + * not be relinquished until the device is idle (see
> + * i915_gem_idle_work_handler()). As a precaution, we make sure
> + * that all ELSP are drained i.e. we have processed the CSB,
> + * before allowing ourselves to idle and calling intel_runtime_pm_put().
> + */
> + GEM_BUG_ON(!engine->i915->gt.awake);
> +
> /*
> * ELSQ note: the submit queue is not cleared after being submitted
> * to the HW so we need to make sure we always clean it up. This is
> @@ -1043,16 +1053,6 @@ static void __execlists_submission_tasklet(struct intel_engine_cs *const engine)
> {
> lockdep_assert_held(&engine->timeline.lock);
>
> - /*
> - * We can skip acquiring intel_runtime_pm_get() here as it was taken
> - * on our behalf by the request (see i915_gem_mark_busy()) and it will
> - * not be relinquished until the device is idle (see
> - * i915_gem_idle_work_handler()). As a precaution, we make sure
> - * that all ELSP are drained i.e. we have processed the CSB,
> - * before allowing ourselves to idle and calling intel_runtime_pm_put().
> - */
> - GEM_BUG_ON(!engine->i915->gt.awake);
> -
> process_csb(engine);
> if (!execlists_is_active(&engine->execlists, EXECLISTS_ACTIVE_PREEMPT))
> execlists_dequeue(engine);
> @@ -1073,10 +1073,7 @@ static void execlists_submission_tasklet(unsigned long data)
> engine->execlists.active);
>
> spin_lock_irqsave(&engine->timeline.lock, flags);
> -
> - if (engine->i915->gt.awake) /* we may be delayed until after we idle! */
> - __execlists_submission_tasklet(engine);
> -
> + __execlists_submission_tasklet(engine);
> spin_unlock_irqrestore(&engine->timeline.lock, flags);
> }
>
>
Why we won't hit the assert on the elsp submit side now? AFAIR the
discussion about this particular line concluded that direct tasklet call
can race with busy->idle transition. So even is process_csb doens't need
the assert, that part I get, the part about the race now again bothers
me. Perhaps I just forgot what I thought I understood back then.. :(
Should this call process_csb only if !gt.awake? But sounds terribly
dodgy.. Why would execlists.active be set if we are not awake..
Regards,
Tvrtko
More information about the Intel-gfx
mailing list