Commit 331bf905 authored by Chris Wilson's avatar Chris Wilson

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: df403069 ("drm/i915/execlists: Lift process_csb() out of the irq-off spinlock")
Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: default avatarMika Kuoppala <mika.kuoppala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20191125094318.1630806-1-chris@chris-wilson.co.uk
parent bae21dac
...@@ -100,9 +100,7 @@ execlists_num_ports(const struct intel_engine_execlists * const execlists) ...@@ -100,9 +100,7 @@ execlists_num_ports(const struct intel_engine_execlists * const execlists)
static inline struct i915_request * static inline struct i915_request *
execlists_active(const struct intel_engine_execlists *execlists) execlists_active(const struct intel_engine_execlists *execlists)
{ {
GEM_BUG_ON(execlists->active - execlists->inflight > return *READ_ONCE(execlists->active);
execlists_num_ports(execlists));
return READ_ONCE(*execlists->active);
} }
static inline void static inline void
......
...@@ -2013,6 +2013,9 @@ cancel_port_requests(struct intel_engine_execlists * const execlists) ...@@ -2013,6 +2013,9 @@ cancel_port_requests(struct intel_engine_execlists * const execlists)
execlists_schedule_out(rq); execlists_schedule_out(rq);
memset(execlists->pending, 0, sizeof(execlists->pending)); 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++) for (port = execlists->active; (rq = *port); port++)
execlists_schedule_out(rq); execlists_schedule_out(rq);
execlists->active = execlists->active =
...@@ -2169,23 +2172,27 @@ static void process_csb(struct intel_engine_cs *engine) ...@@ -2169,23 +2172,27 @@ static void process_csb(struct intel_engine_cs *engine)
else else
promote = gen8_csb_parse(execlists, buf + 2 * head); promote = gen8_csb_parse(execlists, buf + 2 * head);
if (promote) { 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)) if (!inject_preempt_hang(execlists))
ring_set_paused(engine, 0); ring_set_paused(engine, 0);
/* cancel old inflight, prepare for switch */ /* cancel old inflight, prepare for switch */
trace_ports(execlists, "preempted", execlists->active); trace_ports(execlists, "preempted", old);
while (*execlists->active) while (*old)
execlists_schedule_out(*execlists->active++); execlists_schedule_out(*old++);
/* switch pending to inflight */ /* switch pending to inflight */
GEM_BUG_ON(!assert_pending_valid(execlists, "promote")); GEM_BUG_ON(!assert_pending_valid(execlists, "promote"));
execlists->active = WRITE_ONCE(execlists->active,
memcpy(execlists->inflight, memcpy(execlists->inflight,
execlists->pending, execlists->pending,
execlists_num_ports(execlists) * execlists_num_ports(execlists) *
sizeof(*execlists->pending)); sizeof(*execlists->pending)));
set_timeslice(engine);
WRITE_ONCE(execlists->pending[0], NULL); WRITE_ONCE(execlists->pending[0], NULL);
} else { } else {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment