[Intel-gfx] [PATCH] drm/i915/execlists: Drop request-before-CS assertion

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> When we migrated to execlists, one of the conditions we wanted to test
> for was whether the breadcrumb seqno was being written before the
> breadcumb interrupt was delivered. This was following on from issues
> observed on previous generations which were not so strong ordered. With

strongly?

> the removal of the missed interrupt detection, we have not reliable
> means of detecting the out-of-order seqno/interupt but instead tried to
interrupt

> assert that the relationship between the CS event interrupt and the
> breadwrite should be strongly ordered. However, Icelake proves it is
> possible for the HW implementation to forget about minor little details
> such as write ordering and so we the order between *processing* the CS

s/we//

> event and the breadcrumb is unreliable.
>
> Remove the unreliable assertion, but leave a debug telltale in case we
> have reason to suspect.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1658
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 27 ++-------------------------
>  1 file changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index d42a9d6767d4..eb0d6f1964f4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -2384,13 +2384,6 @@ gen8_csb_parse(const struct intel_engine_execlists *execlists, const u32 *csb)
>  	return *csb & (GEN8_CTX_STATUS_IDLE_ACTIVE | GEN8_CTX_STATUS_PREEMPTED);
>  }
>  
> -static inline void flush_hwsp(const struct i915_request *rq)
> -{
> -	mb();
> -	clflush((void *)READ_ONCE(rq->hwsp_seqno));
> -	mb();
> -}
> -
>  static void process_csb(struct intel_engine_cs *engine)
>  {
>  	struct intel_engine_execlists * const execlists = &engine->execlists;
> @@ -2506,19 +2499,8 @@ static void process_csb(struct intel_engine_cs *engine)
>  				const u32 *regs __maybe_unused =
>  					rq->context->lrc_reg_state;
>  

The comment above this scope is quite strong about
strong ordering so that might need amending too.

Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>


> -				/*
> -				 * Flush the breadcrumb before crying foul.
> -				 *
> -				 * Since we have hit this on icl and seen the
> -				 * breadcrumb advance as we print out the debug
> -				 * info (so the problem corrected itself without
> -				 * lasting damage), and we know that icl suffers
> -				 * from missing global observation points in
> -				 * execlists, presume that affects even more
> -				 * coherency.
> -				 */
> -				flush_hwsp(rq);
> -
> +				ENGINE_TRACE(engine,
> +					     "context completed before request!\n");
>  				ENGINE_TRACE(engine,
>  					     "ring:{start:0x%08x, head:%04x, tail:%04x, ctl:%08x, mode:%08x}\n",
>  					     ENGINE_READ(engine, RING_START),
> @@ -2538,11 +2520,6 @@ static void process_csb(struct intel_engine_cs *engine)
>  					     regs[CTX_RING_START],
>  					     regs[CTX_RING_HEAD],
>  					     regs[CTX_RING_TAIL]);
> -
> -				/* Still? Declare it caput! */
> -				if (!i915_request_completed(rq) &&
> -				    !reset_in_progress(execlists))
> -					GEM_BUG_ON("context completed before request");
>  			}
>  
>  			execlists_schedule_out(*execlists->active++);
> -- 
> 2.20.1