| Message ID | 20180611200258.27121-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> -Chris
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.
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]); + I915_WRITE(reg, enable_mask); + } } spin_unlock(&dev_priv->irq_lock); }