drm/i915/gt: Detect if we miss WaIdleLiteRestore
diff mbox series

Message ID 20191209020145.3495050-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915/gt: Detect if we miss WaIdleLiteRestore
Related show

Commit Message

Chris Wilson Dec. 9, 2019, 2:01 a.m. UTC
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@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 40 +++++++++++++++++------------
 1 file changed, 23 insertions(+), 17 deletions(-)

Comments

Chris Wilson Dec. 9, 2019, 2:17 a.m. UTC | #1
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@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
>

Patch
diff mbox series

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));
+		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;
 		}
 	}