Quoting Mika Kuoppala (2018-03-23 10:53:00)
> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> > Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
> > Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > Cc: Jeff McGee <jeff.mcgee@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
