[Intel-gfx] [PATCH] drm/i915/gt: Detect if we miss WaIdleLiteRestore

[Chris Wilson]

Quoting Chris Wilson (2019-12-09 02:01:45)
> In order to avoid confusing the HW, we must never submit an empty ring
> during lite-restore, that is we should always advance the RING_TAIL
> before submitting to stay ahead of the RING_HEAD.
> 
> Normally this is prevented by keeping a couple of spare NOPs in the
> request->wa_tail so that on resubmission we can advance the tail. This
> relies on the request only being resubmitted once, which is the normal
> condition as it is seen once for ELSP[1] and then later in ELSP[0]. On
> preemption, the requests are unwound and the tail reset back to the
> normal end point (as we know the request is incomplete and therefore its
> RING_HEAD is even earlier).
> 
> However, if this w/a should fail we would try and resubmit the request
> with the RING_TAIL already set to the location of this request's wa_tail
> potentially causing a GPU hang. We can spot when we do try and
> incorrectly resubmit without advancing the RING_TAIL and spare any
> embarrassment by forcing the context restore.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 40 +++++++++++++++++------------
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index c7ea8a055005..0b669dfe2f23 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1216,13 +1216,31 @@ execlists_schedule_out(struct i915_request *rq)
>         i915_request_put(rq);
>  }
>  
> -static u64 execlists_update_context(const struct i915_request *rq)
> +static u64 execlists_update_context(struct i915_request *rq)
>  {
>         struct intel_context *ce = rq->hw_context;
> -       u64 desc;
> +       u64 desc = ce->lrc_desc;
> +       u32 tail;
>  
> -       ce->lrc_reg_state[CTX_RING_TAIL] =
> -               intel_ring_set_tail(rq->ring, rq->tail);
> +       /*
> +        * WaIdleLiteRestore:bdw,skl
> +        *
> +        * We should never submit the context with the same RING_TAIL twice
> +        * just in case we submit an empty ring, which confuses the HW.
> +        *
> +        * We append a couple of NOOPs (gen8_emit_wa_tail) after the end of
> +        * the normal request to be able to always advance the RING_TAIL on
> +        * subsequent resubmissions (for lite restore). Should that fail us,
> +        * and we try and submit the same tail again, force the context
> +        * reload.
> +        */
> +       tail = intel_ring_set_tail(rq->ring, rq->tail);
> +       if (unlikely(ce->lrc_reg_state[CTX_RING_TAIL] == tail)) {
> +               GEM_WARN_ON(!(desc & CTX_DESC_FORCE_RESTORE));

This doesn't discriminate enough, as it is legal to resubmit an
incomplete request with rq->tail (not rq->wa_tail)....

Wait no, that is the bug, because the HW still has the request in
flight, and because we did an unsubmit we believe it is safe to reuse.

> +               desc |= CTX_DESC_FORCE_RESTORE;
> +       }
> +       ce->lrc_reg_state[CTX_RING_TAIL] = tail;
> +       rq->tail = rq->wa_tail;
>  
>         /*
>          * Make sure the context image is complete before we submit it to HW.
> @@ -1236,13 +1254,11 @@ static u64 execlists_update_context(const struct i915_request *rq)
>          */
>         wmb();
>  
> -       desc = ce->lrc_desc;
> -       ce->lrc_desc &= ~CTX_DESC_FORCE_RESTORE;
> -
>         /* Wa_1607138340:tgl */
>         if (IS_TGL_REVID(rq->i915, TGL_REVID_A0, TGL_REVID_A0))
>                 desc |= CTX_DESC_FORCE_RESTORE;
>  
> +       ce->lrc_desc &= ~CTX_DESC_FORCE_RESTORE;
>         return desc;
>  }
>  
> @@ -1776,16 +1792,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;
>                 }
>         }
>  
> -- 
> 2.24.0
>