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.
The patch looks ok:
Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
>
> 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
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
