On Thu, Sep 14, 2017 at 06:17:31PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <[email protected]> > > 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 <[email protected]> > Signed-off-by: Ville Syrjälä <[email protected]> > --- > 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. The patch looks ok: Reviewed-by: Imre Deak <[email protected]> > > 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 > _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
