| Message ID | 20191208233308.3068099-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: Skip request resubmission if lite-restore w/a already applied | expand |
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
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; }
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(-)