[Intel-gfx] [PATCH] drm/i915/gt: Skip request resubmission if lite-restore w/a already applied

[Chris Wilson]

Quoting Chris Wilson (2019-12-08 23:33:08)
> Be careful that we never submit the same request again if we have
> already applied the LiteRestore w/a upon it -- as the HW may complete
> the request as we submit the ELSP and so become confused by the request
> to execute an empty ring.
> 
> To submit the same request in ELSP[0] three times (so triggering the
> LiteRestore w/a and resubmitting with it already applied) would require
> preemption, and on preemption we should be unwinding the request tail as
> we know the HW hasn't advanced beyond the normal time. So in practice,
> this should never occur...
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> git add fail for the GEM_WARN_ON tell-tale
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index c7ea8a055005..fcd9bb771223 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1776,16 +1776,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  
>                                 return;
>                         }
> -
> -                       /*
> -                        * WaIdleLiteRestore:bdw,skl
> -                        * Apply the wa NOOPs to prevent
> -                        * ring:HEAD == rq:TAIL as we resubmit the
> -                        * request. See gen8_emit_fini_breadcrumb() for
> -                        * where we prepare the padding after the
> -                        * end of the request.
> -                        */
> -                       last->tail = last->wa_tail;
>                 }
>         }
>  
> @@ -1943,6 +1933,24 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>                                     ctx_single_port_submission(rq->hw_context))
>                                         goto done;
>  
> +                               /*
> +                                * WaIdleLiteRestore:bdw,skl
> +                                *
> +                                * If we have already submitted this request
> +                                * using the wa_tail, we race with the HW and
> +                                * HEAD may reach wa_tail before it processes
> +                                * the ELSP[]. If it sees a context with an
> +                                * empty ring, the HW gets confused.
> +                                *
> +                                * To prevent subsequent resubmission (lite
> +                                * restores) with an empty ring, we emitted a
> +                                * couple of NOOPs in gen8_emit_wa_tail()
> +                                * beyond the normal end of the request.
> +                                */
> +                               if (GEM_WARN_ON(last->tail == last->wa_tail))
> +                                       goto done;
> +
> +                               last->tail = last->wa_tail;

Bah, this means we update the tail before the current submission, so
while the warn is meaningful, the ELSP[] is b0rked.
-Chris