Re: [PATCH 11/19] drm/i915/execlists: Double check rpm wakeref

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

On 17/05/2018 08:40, Chris Wilson wrote:
> As we are splitting processing the CSB events from submitting the ELSP,
> we also need to duplicate the check that we hold a device wakeref for our
> hardware access to the disjoint locations.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 26 ++++++++++++++++----------
>   1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 4928e9ad7826..6d3b03299b0c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -449,6 +449,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);

Hmm.. I think it would be better to leave it in the tasklet and just add 
another instance to process_csb.

Having it only deep in execlists_submit_ports could miss confusions when 
upper layers want to submit and state say it is not possible yet.

> +
>   	/*
>   	 * 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
> @@ -959,6 +969,12 @@ static void process_csb(struct intel_engine_cs *engine)
>   	struct drm_i915_private *i915 = engine->i915;
>   	bool fw = false;
>   
> +	/*
> +	 * We must never release our device wakeref until after we have
> +	 * finished processing all potential interrupts from the hardware.
> +	 */
> +	GEM_BUG_ON(!engine->i915->gt.awake);
> +
>   	do {
>   		/* The HWSP contains a (cacheable) mirror of the CSB */
>   		const u32 *buf =
> @@ -1139,16 +1155,6 @@ static void execlists_submission_tasklet(unsigned long data)
>   		  engine->execlists.active,
>   		  test_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted));
>   
> -	/*
> -	 * 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);
> -
>   	/*
>   	 * Prefer doing test_and_clear_bit() as a two stage operation to avoid
>   	 * imposing the cost of a locked atomic transaction when submitting a
> 

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx