[Intel-gfx] [PATCH v2] drm/i915: Actually flush interrupts on reset not just wedging

[Chris Wilson]

Quoting Mika Kuoppala (2018-03-23 10:53:00)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > Commit 0f36a85c3bd5 ("drm/i915: Flush pending interrupt following a GPU
> > reset") got confused and only applied the flush to the set-wedge path
> > (which itself is proving troublesome), but we also need the
> > serialisation on the regular reset path. Oops.
> >
> > Move the interrupt into reset_irq() and make it common to the reset and
> > final set-wedge.
> >
> > v2: reset_irq() after port cancellation, as we assert that
> > execlists->active is sane for cancellation (and is being reset by
> > reset_irq).
> >
> > References: 0f36a85c3bd5 ("drm/i915: Flush pending interrupt following a GPU reset")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > Cc: Michel Thierry <michel.thierry at intel.com>
> > Cc: Michał Winiarski <michal.winiarski at intel.com>
> > Cc: Jeff McGee <jeff.mcgee at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 107 +++++++++++++++++++--------------------
> >  1 file changed, 53 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index ce09c5ad334f..b4ab06b05e58 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -740,6 +740,57 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
> >       }
> >  }
> >  
> > +static void clear_gtiir(struct intel_engine_cs *engine)
> > +{
> > +     static const u8 gtiir[] = {
> > +             [RCS]  = 0,
> > +             [BCS]  = 0,
> > +             [VCS]  = 1,
> > +             [VCS2] = 1,
> > +             [VECS] = 3,
> > +     };
> > +     struct drm_i915_private *dev_priv = engine->i915;
> > +     int i;
> > +
> > +     /* TODO: correctly reset irqs for gen11 */
> > +     if (WARN_ON_ONCE(INTEL_GEN(engine->i915) >= 11))
> > +             return;
> > +
> > +     GEM_BUG_ON(engine->id >= ARRAY_SIZE(gtiir));
> > +
> > +     /*
> > +      * Clear any pending interrupt state.
> > +      *
> > +      * We do it twice out of paranoia that some of the IIR are
> > +      * double buffered, and so if we only reset it once there may
> > +      * still be an interrupt pending.
> > +      */
> > +     for (i = 0; i < 2; i++) {
> > +             I915_WRITE(GEN8_GT_IIR(gtiir[engine->id]),
> > +                        engine->irq_keep_mask);
> > +             POSTING_READ(GEN8_GT_IIR(gtiir[engine->id]));
> > +     }
> > +     GEM_BUG_ON(I915_READ(GEN8_GT_IIR(gtiir[engine->id])) &
> > +                engine->irq_keep_mask);
> > +}
> > +
> > +static void reset_irq(struct intel_engine_cs *engine)
> > +{
> > +     /* Mark all CS interrupts as complete */
> > +     smp_store_mb(engine->execlists.active, 0);
> > +     synchronize_hardirq(engine->i915->drm.irq);
> > +
> > +     clear_gtiir(engine);
> 
> Should clearing the iir be before we synchronize?
> Our master irq is off, so no bit can light up,
> clear iir, prevent tasklet with active and synchronize.

After I think. Before we risk running at the same time as an interrupt
handler on another thread. Hence the mb on disabling the execlists
(corresponding with the READ_ONE(active) in gen8_cs_irq_handler).

Not that is makes much difference (since we don't trust the consistentcy
of IIR inside the irq handler, because we do randomly reset it), but I
think it makes sense; clear off the interrupt handler, cancel IIR, let
the system resume processing interrupts.
 
> For documentation it would be nice that we have
> assert that the tasklet is indeed disabled.

Not without poking inside tasklet :) In a couple of patches time I plan
to have tasklet_disable/tasklet_enable from inside intel_lrc. I'll
shovel something in then.
-Chris