drm/i915/execlists: Delay writing to ELSP until HW has processed the previous write

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.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102035
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/<20171118003038.7935-1-michel.thierry@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20171120123458.23242-4-chris@chris-wilson.co.uk
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 4aed003237808e16360c3666dd6127a85154e5cc..8d0c493888634bea4558fb4fbb819071f2c36973 100644 (file)
--- 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 c00804ed64c6634b6aca783c3952fe6426c18d54..e437a9556c087dd60473da40ad2e2c0992eb006b 100644 (file)
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -251,6 +251,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