[Intel-gfx] [PATCH 3/7] drm/i915/selftests: Relax timeout for error-interrupt reset processing

[Chris Wilson]

Quoting Mika Kuoppala (2020-02-11 15:54:07)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > Quoting Mika Kuoppala (2020-02-11 15:23:24)
> >> Chris Wilson <chris at chris-wilson.co.uk> writes:
> >> > +                             /* Kick the tasklet to process the error */
> >> > +                             intel_engine_flush_submission(engine);
> >> 
> >> This pattern of usage in selftests makes me want to add mb(); to
> >> intel_engine_flush_submission(), as it does not seem to do it
> >> implicitly.
> >> 
> >> We want to flush submission and observe the effects, thus
> >> it seems like a good place.
> >
> > Hmm. From the cpu perspective the memory barrier would be on the other
> > side in clear_bit_unlock(), and flush_submission does unlock_wait.
> >
> > But, then, we have to factor in that we have to communicate with an
> > external client the GPU, so perhaps an explicit memory barrier...
> >
> > We certainly do perform the memory barrier in order to set the GPU in
> > motion, but have not relied on them for observing effects (aside from
> > the CSB ringbuf).
> >
> > I don't see a strong argument that adding a 'mb/rmb' here will make any
> > difference, I don't see what we are pairing with from the GPU
> > perspective. But maybe there is?
> 
> I don't have a strong argument from gpu side. But what if the
> flush only does the nonatomic wait and returns, and our
> CPU has read the state up front for the next comparison.
> 
> Or now thinking it, the saving grace is that if we don't need
> to flush in here, the tasklet has finished and finish has
> the barrier?

That's how we synchronize with the tasklet, yes. So by this point (as we
expect to have a request ready and scheduled the tasklet) we know that
the tasklet has completed.

So we know we've *initiated* some action on the GPU. Most of the
side-effects from doing so are without barriers.
-Chris