On Mon, Sep 25, 2017 at 01:33:18PM +0300, Imre Deak wrote:
> On Thu, Sep 14, 2017 at 06:17:31PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > i830 seems to occasionally forget the PIPESTAT enable bits when
> > we read the register. These aren't the only registers on i830 that
> > have problems with RMW, as reading the double buffered plane
> > registers returns the latched value rather than the last written
> > value. So something similar is perhaps going on with PIPESTAT.
> >
> > This corruption results on vblank interrupts occasionally turning off
> > on their own, which leads to vblank timeouts and generally a stuck
> > display subsystem.
> >
> > So let's not RMW the pipestat enable bits, and instead use the cached
> > copy we have around.
> >
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 2 +
> > drivers/gpu/drm/i915/i915_irq.c | 135 ++++++++++++-----------------
> > drivers/gpu/drm/i915/intel_fifo_underrun.c | 14 +--
> > 3 files changed, 66 insertions(+), 85 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 28ad5dadbb18..3b4dd410cad1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3307,6 +3307,8 @@ static inline bool intel_vgpu_active(struct drm_i915_private *dev_priv)
> > return dev_priv->vgpu.active;
> > }
> >
> > +u32 i915_pipestat_enable_mask(struct drm_i915_private *dev_priv,
> > + enum pipe pipe);
> > void
> > i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> > u32 status_mask);
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 003a92857102..7c4e1a1faed7 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -567,62 +567,16 @@ void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
> > POSTING_READ(SDEIMR);
> > }
> >
> > -static void
> > -__i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> > - u32 enable_mask, u32 status_mask)
> > +u32 i915_pipestat_enable_mask(struct drm_i915_private *dev_priv,
> > + enum pipe pipe)
> > {
> > - i915_reg_t reg = PIPESTAT(pipe);
> > - u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> > -
> > - lockdep_assert_held(&dev_priv->irq_lock);
> > - WARN_ON(!intel_irqs_enabled(dev_priv));
> > -
> > - if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> > - status_mask & ~PIPESTAT_INT_STATUS_MASK,
> > - "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
> > - pipe_name(pipe), enable_mask, status_mask))
> > - return;
> > -
> > - if ((pipestat & enable_mask) == enable_mask)
> > - return;
> > -
> > - dev_priv->pipestat_irq_mask[pipe] |= status_mask;
> > -
> > - /* Enable the interrupt, clear any pending status */
> > - pipestat |= enable_mask | status_mask;
> > - I915_WRITE(reg, pipestat);
> > - POSTING_READ(reg);
> > -}
> > -
> > -static void
> > -__i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> > - u32 enable_mask, u32 status_mask)
> > -{
> > - i915_reg_t reg = PIPESTAT(pipe);
> > - u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> > + u32 status_mask = dev_priv->pipestat_irq_mask[pipe];
> > + u32 enable_mask = status_mask << 16;
> >
> > lockdep_assert_held(&dev_priv->irq_lock);
> > - WARN_ON(!intel_irqs_enabled(dev_priv));
> > -
> > - if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> > - status_mask & ~PIPESTAT_INT_STATUS_MASK,
> > - "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
> > - pipe_name(pipe), enable_mask, status_mask))
> > - return;
> > -
> > - if ((pipestat & enable_mask) == 0)
> > - return;
> > -
> > - dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> > -
> > - pipestat &= ~enable_mask;
> > - I915_WRITE(reg, pipestat);
> > - POSTING_READ(reg);
> > -}
> >
> > -static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32 status_mask)
> > -{
> > - u32 enable_mask = status_mask << 16;
> > + if (INTEL_GEN(dev_priv) < 5)
> > + goto out;
> >
> > /*
> > * On pipe A we don't support the PSR interrupt yet,
> > @@ -645,35 +599,59 @@ static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32 status_mask)
> > if (status_mask & SPRITE1_FLIP_DONE_INT_STATUS_VLV)
> > enable_mask |= SPRITE1_FLIP_DONE_INT_EN_VLV;
> >
> > +out:
> > + WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> > + status_mask & ~PIPESTAT_INT_STATUS_MASK,
> > + "pipe %c: enable_mask=0x%x, status_mask=0x%x\n",
> > + pipe_name(pipe), enable_mask, status_mask);
> > +
> > return enable_mask;
> > }
> >
> > -void
> > -i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> > - u32 status_mask)
> > +void i915_enable_pipestat(struct drm_i915_private *dev_priv,
> > + enum pipe pipe, u32 status_mask)
> > {
> > + i915_reg_t reg = PIPESTAT(pipe);
> > u32 enable_mask;
> >
> > - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > - enable_mask = vlv_get_pipestat_enable_mask(&dev_priv->drm,
> > - status_mask);
> > - else
> > - enable_mask = status_mask << 16;
> > - __i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
> > + WARN_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK,
> > + "pipe %c: status_mask=0x%x\n",
> > + pipe_name(pipe), status_mask);
> > +
> > + lockdep_assert_held(&dev_priv->irq_lock);
> > + WARN_ON(!intel_irqs_enabled(dev_priv));
> > +
> > + if ((dev_priv->pipestat_irq_mask[pipe] & status_mask) == status_mask)
> > + return;
> > +
> > + dev_priv->pipestat_irq_mask[pipe] |= status_mask;
> > + enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
> > +
> > + I915_WRITE(reg, enable_mask | status_mask);
> > + POSTING_READ(reg);
> > }
> >
> > -void
> > -i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> > - u32 status_mask)
> > +void i915_disable_pipestat(struct drm_i915_private *dev_priv,
> > + enum pipe pipe, u32 status_mask)
> > {
> > + i915_reg_t reg = PIPESTAT(pipe);
> > u32 enable_mask;
> >
> > - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > - enable_mask = vlv_get_pipestat_enable_mask(&dev_priv->drm,
> > - status_mask);
> > - else
> > - enable_mask = status_mask << 16;
> > - __i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
> > + WARN_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK,
> > + "pipe %c: status_mask=0x%x\n",
> > + pipe_name(pipe), status_mask);
> > +
> > + lockdep_assert_held(&dev_priv->irq_lock);
> > + WARN_ON(!intel_irqs_enabled(dev_priv));
> > +
> > + if ((dev_priv->pipestat_irq_mask[pipe] & status_mask) == 0)
> > + return;
> > +
> > + dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> > + enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
> > +
> > + I915_WRITE(reg, enable_mask | status_mask);
> > + POSTING_READ(reg);
> > }
> >
> > /**
> > @@ -1769,7 +1747,7 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> >
> > for_each_pipe(dev_priv, pipe) {
> > i915_reg_t reg;
> > - u32 mask, iir_bit = 0;
> > + u32 status_mask, enable_mask, iir_bit = 0;
> >
> > /*
> > * PIPESTAT bits get signalled even when the interrupt is
> > @@ -1780,7 +1758,7 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> > */
> >
> > /* fifo underruns are filterered in the underrun handler. */
> > - mask = PIPE_FIFO_UNDERRUN_STATUS;
> > + status_mask = PIPE_FIFO_UNDERRUN_STATUS;
> >
> > switch (pipe) {
> > case PIPE_A:
> > @@ -1794,21 +1772,20 @@ static void i9xx_pipestat_irq_ack(struct drm_i915_private *dev_priv,
> > break;
> > }
> > if (iir & iir_bit)
> > - mask |= dev_priv->pipestat_irq_mask[pipe];
> > + status_mask |= dev_priv->pipestat_irq_mask[pipe];
> >
> > - if (!mask)
> > + if (!status_mask)
> > continue;
>
> Not introduced here, but the above could be removed.
Indeed. I guess a separate patch would be cleaner.
> The patch looks ok:
>
> Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
Thanks. Pushed to dinq.
>
> >
> > reg = PIPESTAT(pipe);
> > - mask |= PIPESTAT_INT_ENABLE_MASK;
> > - pipe_stats[pipe] = I915_READ(reg) & mask;
> > + pipe_stats[pipe] = I915_READ(reg) & status_mask;
> > + enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
> >
> > /*
> > * Clear the PIPE*STAT regs before the IIR
> > */
> > - if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
> > - PIPESTAT_INT_STATUS_MASK))
> > - I915_WRITE(reg, pipe_stats[pipe]);
> > + if (pipe_stats[pipe])
> > + I915_WRITE(reg, enable_mask | pipe_stats[pipe]);
> > }
> > spin_unlock(&dev_priv->irq_lock);
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > index 04689600e337..77c123cc8817 100644
> > --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > @@ -88,14 +88,15 @@ static void i9xx_check_fifo_underruns(struct intel_crtc *crtc)
> > {
> > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > i915_reg_t reg = PIPESTAT(crtc->pipe);
> > - u32 pipestat = I915_READ(reg) & 0xffff0000;
> > + u32 enable_mask;
> >
> > lockdep_assert_held(&dev_priv->irq_lock);
> >
> > - if ((pipestat & PIPE_FIFO_UNDERRUN_STATUS) == 0)
> > + if ((I915_READ(reg) & PIPE_FIFO_UNDERRUN_STATUS) == 0)
> > return;
> >
> > - I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
> > + enable_mask = i915_pipestat_enable_mask(dev_priv, crtc->pipe);
> > + I915_WRITE(reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
> > POSTING_READ(reg);
> >
> > trace_intel_cpu_fifo_underrun(dev_priv, crtc->pipe);
> > @@ -108,15 +109,16 @@ static void i9xx_set_fifo_underrun_reporting(struct drm_device *dev,
> > {
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > i915_reg_t reg = PIPESTAT(pipe);
> > - u32 pipestat = I915_READ(reg) & 0xffff0000;
> >
> > lockdep_assert_held(&dev_priv->irq_lock);
> >
> > if (enable) {
> > - I915_WRITE(reg, pipestat | PIPE_FIFO_UNDERRUN_STATUS);
> > + u32 enable_mask = i915_pipestat_enable_mask(dev_priv, pipe);
> > +
> > + I915_WRITE(reg, enable_mask | PIPE_FIFO_UNDERRUN_STATUS);
> > POSTING_READ(reg);
> > } else {
> > - if (old && pipestat & PIPE_FIFO_UNDERRUN_STATUS)
> > + if (old && I915_READ(reg) & PIPE_FIFO_UNDERRUN_STATUS)
> > DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
> > }
> > }
> > --
> > 2.13.5
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
