drm/i915/gt: Mark the execlists->active as the primary volatile access

Since we want to do a lockless read of the current active request, and
that request is written to by process_csb also without serialisation, we
need to instruct gcc to take care in reading the pointer itself.

Otherwise, we have observed execlists_active() to report 0x40.

[ 2400.760381] igt/para-4098    1..s. 2376479300us : process_csb: rcs0 cs-irq head=3, tail=4
[ 2400.760826] igt/para-4098    1..s. 2376479303us : process_csb: rcs0 csb[4]: status=0x00000001:0x00000000
[ 2400.761271] igt/para-4098    1..s. 2376479306us : trace_ports: rcs0: promote { b9c59:2622, b9c55:2624 }
[ 2400.761726] igt/para-4097    0d... 2376479311us : __i915_schedule: rcs0: -2147483648->3, inflight:0000000000000040, rq:ffff888208c1e940

which is impossible!

The answer is that as we keep the existing execlists->active pointing
into the array as we copy over that array, the unserialised read may see
a partial pointer value.

Fixes: 4a24537b5914 ("drm/i915/execlists: Lift process_csb() out of the irq-off spinlock")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20191125094318.1630806-1-chris@chris-wilson.co.uk
(cherry picked from commit 331bf90591573dfe6c8e892239713ef9702f1396)
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index bc3b72bfa9e3be8b39d4b1d9111bc219f67710e3..01765a7ec18f0b862ae6b54a6413b18dc79fb1f9 100644 (file)
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -100,9 +100,7 @@ execlists_num_ports(const struct intel_engine_execlists * const execlists)
 static inline struct i915_request *
 execlists_active(const struct intel_engine_execlists *execlists)
 {
-       GEM_BUG_ON(execlists->active - execlists->inflight >
-                  execlists_num_ports(execlists));
-       return READ_ONCE(*execlists->active);
+       return *READ_ONCE(execlists->active);
 }
 
 static inline void

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 37fe72aa8e27ec7977cc41cd92907c88e65b8a49..a692e6c9ea75eb9f6441073c76f408aff65ae773 100644 (file)
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1943,6 +1943,9 @@ cancel_port_requests(struct intel_engine_execlists * const execlists)
                execlists_schedule_out(rq);
        memset(execlists->pending, 0, sizeof(execlists->pending));
 
+       /* Mark the end of active before we overwrite *active */
+       WRITE_ONCE(execlists->active, execlists->pending);
+
        for (port = execlists->active; (rq = *port); port++)
                execlists_schedule_out(rq);
        execlists->active =
@@ -2099,23 +2102,27 @@ static void process_csb(struct intel_engine_cs *engine)
                else
                        promote = gen8_csb_parse(execlists, buf + 2 * head);
                if (promote) {
+                       struct i915_request * const *old = execlists->active;
+
+                       /* Point active to the new ELSP; prevent overwriting */
+                       WRITE_ONCE(execlists->active, execlists->pending);
+                       set_timeslice(engine);
+
                        if (!inject_preempt_hang(execlists))
                                ring_set_paused(engine, 0);
 
                        /* cancel old inflight, prepare for switch */
-                       trace_ports(execlists, "preempted", execlists->active);
-                       while (*execlists->active)
-                               execlists_schedule_out(*execlists->active++);
+                       trace_ports(execlists, "preempted", old);
+                       while (*old)
+                               execlists_schedule_out(*old++);
 
                        /* switch pending to inflight */
                        GEM_BUG_ON(!assert_pending_valid(execlists, "promote"));
-                       execlists->active =
-                               memcpy(execlists->inflight,
-                                      execlists->pending,
-                                      execlists_num_ports(execlists) *
-                                      sizeof(*execlists->pending));
-
-                       set_timeslice(engine);
+                       WRITE_ONCE(execlists->active,
+                                  memcpy(execlists->inflight,
+                                         execlists->pending,
+                                         execlists_num_ports(execlists) *
+                                         sizeof(*execlists->pending)));
 
                        WRITE_ONCE(execlists->pending[0], NULL);
                } else {