[Intel-gfx] [PATCH 3/4] drm/i915: Use full serialisation around engine->irq_posted

[Chris Wilson]

Quoting Jeff McGee (2018-03-22 15:34:45)
> On Thu, Mar 22, 2018 at 07:35:32AM +0000, Chris Wilson wrote:
> > Using engine->irq_posted for execlists, we are not always serialised by
> > the tasklet as we supposed. On the reset paths, the tasklet is disabled
> > and ignored. Instead, we manipulate the engine->irq_posted directly to
> > account for the reset, but if an interrupt fired before the reset and so
> > wrote to engine->irq_posted, that write may not be flushed from the
> > local CPU's cacheline until much later as the tasklet is already active
> > and so does not generate a mb(). To correctly serialise the interrupt
> > with reset, we need serialisation on the set_bit() itself.
> > 
> > And at last Mika can be happy.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > Cc: Michał Winiarski <michal.winiarski at intel.com>
> > CC: Michel Thierry <michel.thierry at intel.com>
> > Cc: Jeff McGee <jeff.mcgee at intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index fa7310766217..27aee25429b7 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1405,10 +1405,9 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir)
> >       bool tasklet = false;
> >  
> >       if (iir & GT_CONTEXT_SWITCH_INTERRUPT) {
> > -             if (READ_ONCE(engine->execlists.active)) {
> > -                     __set_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> > -                     tasklet = true;
> > -             }
> > +             if (READ_ONCE(engine->execlists.active))
> > +                     tasklet = !test_and_set_bit(ENGINE_IRQ_EXECLIST,
> > +                                                 &engine->irq_posted);
> >       }
> >  
> >       if (iir & GT_RENDER_USER_INTERRUPT) {
> > -- 
> > 2.16.2
> > 
> 
> Confirmed that this along with the interrupt flush eliminates the cases
> of finding CSB tail at its reset value (0x7) in the tasklet in my force
> preemption tests.

At the moment, I'm concerned about the failures we have in CI before we
go building on top. So care to complete the set of r-b for us to move
on?
-Chris