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@xxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxxx > > 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@xxxxxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxx> 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
