[Intel-gfx] [PATCH] drm/i915/gen6: Flush RING_IMR changes before changing the global GT IMR

[Chris Wilson]

Quoting Mika Kuoppala (2019-01-03 10:25:37)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > On Baytail, notably, we can still detect missed interrupt syndrome
> > (where we never spot a completed request). In this case, it can be
> > alleviated by always keeping the interrupt unmasked, implying that the
> > interrupt is being lost in the window after modifying the IMR. (This is
> > the reason we still have the posting reads on enable_irq, if we remove
> > them we miss interrupts!) Having narrowed the issue down to the IMR,
> > rather than keeping it always enabled, applying the usual posting
> > read/flush of the RING_IMR before unmasking the GT IMR also seems to
> > prevent the missed interrupt. So be it.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 8967dcb5f58f..3d5d6b908148 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -974,6 +974,10 @@ gen6_irq_enable(struct intel_engine_cs *engine)
> >       I915_WRITE_IMR(engine,
> >                      ~(engine->irq_enable_mask |
> >                        engine->irq_keep_mask));
> > +
> > +     /* Flush/delay to ensure the RING_IMR is active before the GT IMR */
> > +     POSTING_READ_FW(RING_IMR(engine->mmio_base));
> > +
> 
> I guess lifting this to be part of I915_WRITE_IMR would have caused
> unwanted latencies on platforms that demonstratedly are free
> of this sort of weirdness. I mean, the write should go in order
> so the gpu view of events is consistent. Perhaps there
> is some asyncness we can't grasp...or it is the delay like
> the comment explains.

The only plausible explanation I can give as to why this might actually
be a HW device and not just a delay is some latching, but even then I
would expect it to be tied to the IIR (if the ring had any!) rather than
the IMR.

Also better if we don't bury away magic too deeply if it's only required
for a single reason.

Fwiw, I tried removing the POSTING_READ(RING_IMR) elsewhere, and
bsw/bxt/glk all failed gem_sync (in CI no less, so \o/ for BAT sometimes
doing its job).
-Chris