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ä <[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.
Indeed. I guess a separate patch would be cleaner. > The patch looks ok: > > Reviewed-by: Imre Deak <[email protected]> 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
