drm/i915/gt: Skip request resubmission if lite-restore w/a already applied
diff mbox series

Message ID 20191208233308.3068099-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915/gt: Skip request resubmission if lite-restore w/a already applied
Related show

Commit Message

Chris Wilson Dec. 8, 2019, 11:33 p.m. UTC
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@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(-)

Comments

Chris Wilson Dec. 8, 2019, 11:37 p.m. UTC | #1
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@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

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..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;
 				merge = false;
 			}