Re: [Intel-gfx] [PATCH 1/4] drm/i915: Fix PIPESTATE irq ack on i965/g4x

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jun 11, 2018 at 09:53:52PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-06-11 21:02:55)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On i965/g4x IIR is edge triggered. So in order for IIR to notice that
> > there is still a pending interrupt we have to force and edge in ISR.
> > For the ISR/IIR pipe event bits we can do that by temporarily
> > clearing all the PIPESTAT enable bits when we ack the status bits.
> > This will force the ISR pipe event bit low, and it can then go back
> > high when we restore the PIPESTAT enable bits.
> > 
> > This avoids the following race:
> > 1. stat = read(PIPESTAT)
> > 2. an enabled PIPESTAT status bit goes high
> > 3. write(PIPESTAT, enable|stat);
> > 4. write(IIR, PIPE_EVENT)
> > 
> > The end result is IIR==0 and ISR!=0. This can lead to nasty
> > vblank wait/flip_done timeouts if another interrupt source
> > doesn't trick us into looking at the PIPESTAT status bits despite
> > the IIR PIPE_EVENT bit being low.
> > 
> > Before i965 IIR was level triggered so this problem can't actually
> > happen there. And curiously VLV/CHV went back to the level triggered
> > scheme as well. But for simplicity we'll use the same i965/g4x
> > compatible code for all platforms.
> > 
> > Cc: stable@vger.kernel.org
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106033
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105225
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106030
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 2fd92a886789..364e1c85315e 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1893,9 +1893,17 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> >  
> >                 /*
> >                  * Clear the PIPE*STAT regs before the IIR
> > +                *
> > +                * Toggle the enable bits to make sure we get an
> > +                * edge in the ISR pipe event bit if we don't clear
> > +                * all the enabled status bits. Otherwise the edge
> > +                * triggered IIR on i965/g4x wouldn't notice that
> > +                * an interrupt is still pending.
> >                  */
> > -               if (pipe_stats[pipe])
> > -                       I915_WRITE(reg, enable_mask | pipe_stats[pipe]);
> > +               if (pipe_stats[pipe]) {
> > +                       I915_WRITE(reg, pipe_stats[pipe]);
> 
> Ack status, disable all.
> 
> > +                       I915_WRITE(reg, enable_mask);
> 
> Enable, an asserted bit should trigger a new edge.
> 
> It certainly does as you explain, now I just hope the hw feels the same!
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

s/PIPESTATE/PIPESTAT/ (can't count how many times I've done that one
wrong) and pushed to dinq. Thanks for the review.

-- 
Ville Syrjälä
Intel