[Intel-gfx] [PATCH 4/4] drm/i915/execlists: Delay writing to ELSP until HW has processed the previous write

[Michel Thierry]

On 11/20/2017 4:34 AM, Chris Wilson wrote:
> The hardware needs some time to process the information received in the
> ExecList Submission Port, and expects us to not write anything more until
> it has 'acknowledged' this new submission by sending an IDLE_ACTIVE or
> PREEMPTED CSB event.
> 
> If we do not follow this, the driver could write new data into the ELSP
> before HW had finishing fetching the previous one, putting us in
> 'undefined behaviour' space.
> 
> This seems to be the problem causing the spurious PREEMPTED & COMPLETE
> events after a COMPLETE like the one below:
> 
> [] vcs0: sw rd pointer = 2, hw wr pointer = 0, current 'head' = 3.
> [] vcs0:  Execlist CSB[0]: 0x00000018 _ 0x00000007
> [] vcs0:  Execlist CSB[1]: 0x00000001 _ 0x00000000
> [] vcs0:  Execlist CSB[2]: 0x00000018 _ 0x00000007  <<< COMPLETE
> [] vcs0:  Execlist CSB[3]: 0x00000012 _ 0x00000007  <<< PREEMPTED & COMPLETE
> [] vcs0:  Execlist CSB[4]: 0x00008002 _ 0x00000006
> [] vcs0:  Execlist CSB[5]: 0x00000014 _ 0x00000006
> 
> The ELSP writes that lead to this CSB sequence show that the HW hadn't
> started executing the previous execlist (the one with only ctx 0x6) by the
> time the new one was submitted; this is a bit more clear in the data
> show in the EXECLIST_STATUS register at the time of the ELSP write.
> 
> [] vcs0: ELSP[0] = 0x0_0        [execlist1] - status_reg = 0x0_302
> [] vcs0: ELSP[1] = 0x6_fedb2119 [execlist0] - status_reg = 0x0_8302
> 
> [] vcs0: ELSP[2] = 0x7_fedaf119 [execlist1] - status_reg = 0x0_8308
> [] vcs0: ELSP[3] = 0x6_fedb2119 [execlist0] - status_reg = 0x7_8308
> 
> Note that having to wait for this ack does not disable lite-restores,
> although it may reduce their numbers.
> 
> v2: Rewrote Michel's patch, his digging and his fix, my spelling.
> v3: Reorder to ack early to allow preemption
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102035
> Suggested-by: Michel Thierry <michel.thierry at intel.com>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry at intel.com>

Isn't it nice to come back after the weekend and see everything is ok?

Reviewed-by: Michel Thierry <michel.thierry at intel.com>

Thanks,

> ---
>   drivers/gpu/drm/i915/intel_lrc.c        | 22 ++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
>   2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 4aed00323780..8d0c49388863 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -477,6 +477,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)
>   
>   		elsp_write(desc, elsp);
>   	}
> +	execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK);
>   }
>   
>   static bool ctx_single_port_submission(const struct i915_gem_context *ctx)
> @@ -529,6 +530,7 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
>   		elsp_write(0, elsp);
>   
>   	elsp_write(ce->lrc_desc, elsp);
> +	execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK);
>   }
>   
>   static void execlists_dequeue(struct intel_engine_cs *engine)
> @@ -575,9 +577,20 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   		 * know the next preemption status we see corresponds
>   		 * to this ELSP update.
>   		 */
> +		GEM_BUG_ON(!port_count(&port[0]));
>   		if (port_count(&port[0]) > 1)
>   			goto unlock;
>   
> +		/*
> +		 * If we write to ELSP a second time before the HW has had
> +		 * a chance to respond to the previous write, we can confuse
> +		 * the HW and hit "undefined behaviour". After writing to ELSP,
> +		 * we must then wait until we see a context-switch event from
> +		 * the HW to indicate that it has had a chance to respond.
> +		 */
> +		if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
> +			goto unlock;
> +
>   		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
>   		    rb_entry(rb, struct i915_priolist, node)->priority >
>   		    max(last->priotree.priority, 0)) {
> @@ -871,6 +884,15 @@ static void execlists_submission_tasklet(unsigned long data)
>   			GEM_TRACE("%s csb[%dd]: status=0x%08x:0x%08x\n",
>   				  engine->name, head,
>   				  status, buf[2*head + 1]);
> +
> +			if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
> +				      GEN8_CTX_STATUS_PREEMPTED))
> +				execlists_set_active(execlists,
> +						     EXECLISTS_ACTIVE_HWACK);
> +			if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
> +				execlists_clear_active(execlists,
> +						       EXECLISTS_ACTIVE_HWACK);
> +
>   			if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
>   				continue;
>   
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index f867aa6c31fc..add7a30c1a61 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -252,6 +252,7 @@ struct intel_engine_execlists {
>   	unsigned int active;
>   #define EXECLISTS_ACTIVE_USER 0
>   #define EXECLISTS_ACTIVE_PREEMPT 1
> +#define EXECLISTS_ACTIVE_HWACK 2
>   
>   	/**
>   	 * @port_mask: number of execlist ports - 1
>