Re: [PATCH 7/7] drm/i915: vlv: handle only enabled pipestat interrupt events

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5707 bytes --]

On Wed, 2014-02-05 at 17:11 +0200, Ville Syrjälä wrote:
> On Tue, Feb 04, 2014 at 09:35:51PM +0200, Imre Deak wrote:
> > Atm we call the handlers for pending pipestat interrupt events even if
> > they aren't explicitly enabled by i915_enable_pipestat(). This isn't an
> > issue for events other than the vblank start event, since those are
> > always enabled anyways. Otoh, we enable the vblank start event
> > on-demand, so we'll end up calling the vblank handler at times when they
> > are disabled.
> > 
> > I haven't checked if this causes any real problem, but for consistency
> > and to remove some overhead we should still fix this by clearing /
> > handling only the enabled interrupt events. Also this is a dependency
> > for the upcoming VLV power domain patchset where we need to disable all
> > the pipestat interrupts whenever the display power well is off.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  drivers/gpu/drm/i915/i915_irq.c | 33 ++++++++++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/i915_reg.h |  4 ++++
> >  3 files changed, 35 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 43f37ca..faca5b4 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1427,6 +1427,7 @@ typedef struct drm_i915_private {
> >  	};
> >  	u32 gt_irq_mask;
> >  	u32 pm_irq_mask;
> > +	u32 pipestat_irq_mask[I915_MAX_PIPES];
> >  
> >  	struct work_struct hotplug_work;
> >  	bool enable_hotplug_processing;
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index eea5398..2cac477 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -419,6 +419,16 @@ done:
> >  	return ret;
> >  }
> >  
> > +static bool __cpu_fifo_underrun_reporting_enabled(struct drm_device *dev,
> > +						  enum pipe pipe)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +
> > +	return !intel_crtc->cpu_fifo_underrun_disabled;
> > +}
> > +
> >  /**
> >   * intel_set_pch_fifo_underrun_reporting - enable/disable FIFO underrun messages
> >   * @dev: drm device
> > @@ -520,6 +530,8 @@ i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> >  	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);
> > @@ -550,6 +562,8 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> >  	if ((pipestat & enable_mask) == 0)
> >  		return;
> >  
> > +	dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> > +
> >  	pipestat &= ~enable_mask;
> >  	I915_WRITE(reg, pipestat);
> >  	POSTING_READ(reg);
> > @@ -1530,18 +1544,31 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> >  static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > -	u32 pipe_stats[I915_MAX_PIPES];
> > +	u32 pipe_stats[I915_MAX_PIPES] = { };
> >  	int pipe;
> >  
> >  	spin_lock(&dev_priv->irq_lock);
> >  	for_each_pipe(pipe) {
> > -		int reg = PIPESTAT(pipe);
> > +		int reg;
> > +		u32 mask;
> > +
> > +		if (!dev_priv->pipestat_irq_mask[pipe] &&
> > +		    !__cpu_fifo_underrun_reporting_enabled(dev, pipe))
> > +			continue;
> > +
> > +		reg = PIPESTAT(pipe);
> >  		pipe_stats[pipe] = I915_READ(reg);
> >  
> >  		/*
> >  		 * Clear the PIPE*STAT regs before the IIR
> >  		 */
> > -		if (pipe_stats[pipe] & 0x8000ffff)
> > +		mask = PIPESTAT_INT_ENABLE_MASK | PIPE_FIFO_UNDERRUN_STATUS;
> 
> Maybe we should add PIPE_FIFO_UNDERRUN_STATUS to the mask only when the
> underrun reporting is enabled. If someone goes and enables underrun
> reporting just after valleyview_pipestat_irq_handler(), we'd
> potentially report a stale underrun.

Ok, will fix it.

--Imre

> 
> You get to blame me for that one though, since the bug is already there
> in the current code, which I implemented.
> 
> > +		if (iir & I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe))
> > +			mask |= dev_priv->pipestat_irq_mask[pipe];
> > +		pipe_stats[pipe] &= mask;
> > +
> > +		if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
> > +					PIPESTAT_INT_STATUS_MASK))
> >  			I915_WRITE(reg, pipe_stats[pipe]);
> >  	}
> >  	spin_unlock(&dev_priv->irq_lock);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index c998c4f..47e0635 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -997,6 +997,10 @@
> >  #define I915_DISPLAY_PIPE_A_EVENT_INTERRUPT		(1<<6)
> >  #define I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT		(1<<5)
> >  #define I915_DISPLAY_PIPE_B_EVENT_INTERRUPT		(1<<4)
> > +#define I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe)				\
> > +	((pipe) == PIPE_A ? I915_DISPLAY_PIPE_A_EVENT_INTERRUPT :	\
> > +	 I915_DISPLAY_PIPE_B_EVENT_INTERRUPT)
> > +
> >  #define I915_DEBUG_INTERRUPT				(1<<2)
> >  #define I915_USER_INTERRUPT				(1<<1)
> >  #define I915_ASLE_INTERRUPT				(1<<0)
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx