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

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

 



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




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