On 11/20/2017 4:34 AM, Chris Wilson wrote:
The hardware needs some time to process the information received in the
ExecList Submission Port, and expects us to not write anything more until
it has 'acknowledged' this new submission by sending an IDLE_ACTIVE or
PREEMPTED CSB event.
If we do not follow this, the driver could write new data into the ELSP
before HW had finishing fetching the previous one, putting us in
'undefined behaviour' space.
This seems to be the problem causing the spurious PREEMPTED & COMPLETE
events after a COMPLETE like the one below:
[] vcs0: sw rd pointer = 2, hw wr pointer = 0, current 'head' = 3.
[] vcs0: Execlist CSB[0]: 0x00000018 _ 0x00000007
[] vcs0: Execlist CSB[1]: 0x00000001 _ 0x00000000
[] vcs0: Execlist CSB[2]: 0x00000018 _ 0x00000007 <<< COMPLETE
[] vcs0: Execlist CSB[3]: 0x00000012 _ 0x00000007 <<< PREEMPTED & COMPLETE
[] vcs0: Execlist CSB[4]: 0x00008002 _ 0x00000006
[] vcs0: Execlist CSB[5]: 0x00000014 _ 0x00000006
The ELSP writes that lead to this CSB sequence show that the HW hadn't
started executing the previous execlist (the one with only ctx 0x6) by the
time the new one was submitted; this is a bit more clear in the data
show in the EXECLIST_STATUS register at the time of the ELSP write.
[] vcs0: ELSP[0] = 0x0_0 [execlist1] - status_reg = 0x0_302
[] vcs0: ELSP[1] = 0x6_fedb2119 [execlist0] - status_reg = 0x0_8302
[] vcs0: ELSP[2] = 0x7_fedaf119 [execlist1] - status_reg = 0x0_8308
[] vcs0: ELSP[3] = 0x6_fedb2119 [execlist0] - status_reg = 0x7_8308
Note that having to wait for this ack does not disable lite-restores,
although it may reduce their numbers.
v2: Rewrote Michel's patch, his digging and his fix, my spelling.
v3: Reorder to ack early to allow preemption
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102035
Suggested-by: Michel Thierry <michel.thierry@xxxxxxxxx>
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
Isn't it nice to come back after the weekend and see everything is ok?
Reviewed-by: Michel Thierry <michel.thierry@xxxxxxxxx>
Thanks,
---
drivers/gpu/drm/i915/intel_lrc.c | 22 ++++++++++++++++++++++
drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
2 files changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 4aed00323780..8d0c49388863 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -477,6 +477,7 @@ static void execlists_submit_ports(struct intel_engine_cs *engine)
elsp_write(desc, elsp);
}
+ execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK);
}
static bool ctx_single_port_submission(const struct i915_gem_context *ctx)
@@ -529,6 +530,7 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
elsp_write(0, elsp);
elsp_write(ce->lrc_desc, elsp);
+ execlists_clear_active(&engine->execlists, EXECLISTS_ACTIVE_HWACK);
}
static void execlists_dequeue(struct intel_engine_cs *engine)
@@ -575,9 +577,20 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
* know the next preemption status we see corresponds
* to this ELSP update.
*/
+ GEM_BUG_ON(!port_count(&port[0]));
if (port_count(&port[0]) > 1)
goto unlock;
+ /*
+ * If we write to ELSP a second time before the HW has had
+ * a chance to respond to the previous write, we can confuse
+ * the HW and hit "undefined behaviour". After writing to ELSP,
+ * we must then wait until we see a context-switch event from
+ * the HW to indicate that it has had a chance to respond.
+ */
+ if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
+ goto unlock;
+
if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
rb_entry(rb, struct i915_priolist, node)->priority >
max(last->priotree.priority, 0)) {
@@ -871,6 +884,15 @@ static void execlists_submission_tasklet(unsigned long data)
GEM_TRACE("%s csb[%dd]: status=0x%08x:0x%08x\n",
engine->name, head,
status, buf[2*head + 1]);
+
+ if (status & (GEN8_CTX_STATUS_IDLE_ACTIVE |
+ GEN8_CTX_STATUS_PREEMPTED))
+ execlists_set_active(execlists,
+ EXECLISTS_ACTIVE_HWACK);
+ if (status & GEN8_CTX_STATUS_ACTIVE_IDLE)
+ execlists_clear_active(execlists,
+ EXECLISTS_ACTIVE_HWACK);
+
if (!(status & GEN8_CTX_STATUS_COMPLETED_MASK))
continue;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index f867aa6c31fc..add7a30c1a61 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -252,6 +252,7 @@ struct intel_engine_execlists {
unsigned int active;
#define EXECLISTS_ACTIVE_USER 0
#define EXECLISTS_ACTIVE_PREEMPT 1
+#define EXECLISTS_ACTIVE_HWACK 2
/**
* @port_mask: number of execlist ports - 1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
