Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
> Quoting Mika Kuoppala (2019-11-25 09:16:30)
>> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
>>
>> > 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
>>
>> Where is this exact tracepoint? My grep skills are failing me.
>
> I added to see
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7388/fi-bsw-n3050/igt@i915_selftest@live_gem_contexts.html
>
>> >
>> > 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.
>>
>> ...otherwise we will see ?
>>
>> Also, the 0x40 is bothering me as I didn't find the tracepoint. If we
>> only displayed pointer values, where did the offset appear.
>
> Because we did a byte-by-byte copy of pending to inflight as
> execlists_active() reads *active [pointing into inflight]
>
> So inflight is a random mix of NULL + rq, starting at the LSB.
Seems so, yeah we can't really assume memcpy would do anything
fancier.
Ok, put a WRITE_ONCE for changing the active on
cancel_port_requests() too, for symmetry.
Reviewed-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
>
>> > Fixes: df403069029d ("drm/i915/execlists: Lift process_csb() out of the irq-off spinlock")
>> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> > ---
>> > drivers/gpu/drm/i915/gt/intel_engine.h | 4 +---
>> > drivers/gpu/drm/i915/gt/intel_lrc.c | 24 ++++++++++++++----------
>> > 2 files changed, 15 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
>> > index bc3b72bfa9e3..01765a7ec18f 100644
>> > --- 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);
>>
>> Yes this seems proper as we need apriori read before deferencing.
>>
>> > }
>> >
>> > static inline void
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > index 0e2065a13f24..0d0dca3d6724 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > @@ -2169,23 +2169,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 we set the active to pending here...
>>
>> > +
>> > 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)));
>>
>> Why we rewrite it in here, is the pending moving beneath us?
>
> Yes. Pending is where we track the next submit, inflight + active the
> current. pending[0] = NULL is the next line, and pending[] is then set
> in dequeue.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
