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@xxxxxxxxxxxxxxxxxx>
> ---
> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
