All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Don't rmw PIPESTAT enable bits
Date: Mon, 25 Sep 2017 16:53:33 +0300	[thread overview]
Message-ID: <20170925135333.GA10981@intel.com> (raw)
In-Reply-To: <20170925103318.irv3brn2t2kv3kfq@ideak-desk.fi.intel.com>

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@linux.intel.com>
> > 
> > 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@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  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@intel.com>

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

      reply	other threads:[~2017-09-25 13:53 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-14 15:17 [PATCH] drm/i915: Don't rmw PIPESTAT enable bits Ville Syrjala
2017-09-14 16:04 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-09-14 20:37 ` [PATCH] " Chris Wilson
2017-09-15 10:03   ` Ville Syrjälä
2017-09-25 14:05     ` Ville Syrjälä
2017-09-25 14:10       ` Chris Wilson
2017-09-14 21:20 ` ✗ Fi.CI.IGT: failure for " Patchwork
2017-09-25 10:33 ` [PATCH] " Imre Deak
2017-09-25 13:53   ` Ville Syrjälä [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170925135333.GA10981@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=imre.deak@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.