Re: [PATCH] drm/i915/execlists: Move the assertion we have the rpm wakeref down

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx