Re: [PATCH] drm/i915: Don't rmw PIPESTAT enable bits

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux