Re: [PATCH 1/2] drm/i915/execlists: Add a paranoid flush of the CSB pointers upon reset

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-09-12 08:51:38)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > After a GPU reset, we need to drain all the CS events so that we have an
>> > accurate picture of the execlists state at the time of the reset. Be
>> > paranoid and force a read of the CSB write pointer from memory.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/gt/intel_lrc.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > index 3d83c7e0d9de..61a38a4ccbca 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > @@ -2836,6 +2836,10 @@ static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
>> >       struct i915_request *rq;
>> >       u32 *regs;
>> >  
>> > +     mb(); /* paranoia: read the CSB pointers from after the reset */
>> > +     clflush(execlists->csb_write);
>> > +     mb();
>> > +
>> 
>> We know there is always a cost. We do invalidate the csb
>> on each pass on process_csb.
>> 
>> Add csb_write in to invalidate_csb entries along
>> with mbs. Rename it to invalidate_csb and use it
>> always?
>> 
>> By doing so, we could prolly throw out the rmb() at
>> the start of the process_csb as we would have invalidated
>> the write pointer along with the entries we read,
>> on previous pass.
>
> No. That rmb is essential for the read ordering at that moment in time.

Ah yes indeed it is. head vs entries coherency.

>
> All I have in mind here is a delay, not really a barrier per se, just
> this is a nice way of saying no speculation either.

Forgetting the rmb(), there is similar pattern of mb()+flush
elsewhere. Just saw the profiliferation and opportunity to converge.

But syncing with the hardware on moment of reset, this should
do.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx