Re: [PATCH] drm/i915: Clear PIPE.STAT before IIR on VLV/CHV

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

 



On Fri, Aug 14, 2015 at 06:24:32PM +0100, Chris Wilson wrote:
> The PIPE.STAT register contains some interrupt status bits per pipe, and
> if assert cause the corresponding bit in the IIR to be asserted (thus
> raising an interrupt). When handling an interrupt, we should clear the
> PIPE.STAT generator first before clearing the IIR so that we do not miss
> events or cause spurious work.
> 
> This ordering was broken by
> 
> commit 27b6c122512ca30399bb1b39cc42eda83901f304
> Author: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> Date:   Mon Jun 16 16:11:00 2014 +0100
> 
>     drm/i915/chv: Ack interrupts before handling them (CHV)
> 
> commit 3ff60f89bc4836583f5bd195062f16c563bd97aa
> Author: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> Date:   Mon Jun 16 16:10:58 2014 +0100
> 
>     drm/i915/vlv: Ack interrupts before handling them (VLV)
> 
> in attempting to reduce the amount of work done between receiving an
> interrupt and acknowledging it. In light of this motivation, split the
> pipestat interrupt handler into two, one to acknowledge and clear the
> interrupt and a later pass to process it.
> 
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> Cc: Bob Beckett <robert.beckett@xxxxxxxxx>
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>

Oscar/Imre, where's your review on this?

Thanks, Daniel
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 55 +++++++++++++++++++++++------------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 66064511cffb..92bdfe6f91d8 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1457,24 +1457,21 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>  	}
>  }
>  
> -static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> +static inline bool
> +intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
>  {
> -	if (!drm_handle_vblank(dev, pipe))
> -		return false;
> -
> -	return true;
> +	return drm_handle_vblank(dev, pipe);
>  }
>  
> -static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
> +static void valleyview_pipestat_irq_get(struct drm_i915_private *dev_priv,
> +					u32 iir, u32 *pipe_stats)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 pipe_stats[I915_MAX_PIPES] = { };
>  	int pipe;
>  
>  	spin_lock(&dev_priv->irq_lock);
>  	for_each_pipe(dev_priv, pipe) {
> +		u32 mask, val, iir_bit = 0;
>  		int reg;
> -		u32 mask, iir_bit = 0;
>  
>  		/*
>  		 * PIPESTAT bits get signalled even when the interrupt is
> @@ -1486,6 +1483,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>  
>  		/* fifo underruns are filterered in the underrun handler. */
>  		mask = PIPE_FIFO_UNDERRUN_STATUS;
> +		mask |= PIPESTAT_INT_ENABLE_MASK;
>  
>  		switch (pipe) {
>  		case PIPE_A:
> @@ -1501,21 +1499,25 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>  		if (iir & iir_bit)
>  			mask |= dev_priv->pipestat_irq_mask[pipe];
>  
> -		if (!mask)
> -			continue;
> -
>  		reg = PIPESTAT(pipe);
> -		mask |= PIPESTAT_INT_ENABLE_MASK;
> -		pipe_stats[pipe] = I915_READ(reg) & mask;
> +		val = I915_READ(reg);
> +		pipe_stats[pipe] |= val & mask;
>  
>  		/*
>  		 * 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 (val & (PIPE_FIFO_UNDERRUN_STATUS |
> +			   PIPESTAT_INT_STATUS_MASK))
> +			I915_WRITE(reg, val);
>  	}
>  	spin_unlock(&dev_priv->irq_lock);
> +}
> +
> +static void valleyview_pipestat_irq_handler(struct drm_i915_private *dev_priv,
> +					    u32 *pipe_stats)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	int pipe;
>  
>  	for_each_pipe(dev_priv, pipe) {
>  		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS &&
> @@ -1578,6 +1580,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = arg;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 pipe_stats[I915_MAX_PIPES] = {};
>  	u32 iir, gt_iir, pm_iir;
>  	irqreturn_t ret = IRQ_NONE;
>  
> @@ -1600,6 +1603,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  			/* Consume port before clearing IIR or we'll miss events */
>  			if (iir & I915_DISPLAY_PORT_INTERRUPT)
>  				i9xx_hpd_irq_handler(dev);
> +			valleyview_pipestat_irq_get(dev_priv, iir, pipe_stats);
>  			I915_WRITE(VLV_IIR, iir);
>  		}
>  
> @@ -1612,12 +1616,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  			snb_gt_irq_handler(dev, dev_priv, gt_iir);
>  		if (pm_iir)
>  			gen6_rps_irq_handler(dev_priv, pm_iir);
> -		/* Call regardless, as some status bits might not be
> -		 * signalled in iir */
> -		valleyview_pipestat_irq_handler(dev, iir);
>  	}
>  
>  out:
> +
> +	/* Call regardless, as some status bits might not be
> +	 * signalled in iir */
> +	valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
>  	return ret;
>  }
>  
> @@ -1625,6 +1630,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = arg;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 pipe_stats[I915_MAX_PIPES] = {};
>  	u32 master_ctl, iir;
>  	irqreturn_t ret = IRQ_NONE;
>  
> @@ -1648,18 +1654,19 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>  			/* Consume port before clearing IIR or we'll miss events */
>  			if (iir & I915_DISPLAY_PORT_INTERRUPT)
>  				i9xx_hpd_irq_handler(dev);
> +			valleyview_pipestat_irq_get(dev_priv, iir, pipe_stats);
>  			I915_WRITE(VLV_IIR, iir);
>  		}
>  
>  		gen8_gt_irq_handler(dev_priv, master_ctl);
>  
> -		/* Call regardless, as some status bits might not be
> -		 * signalled in iir */
> -		valleyview_pipestat_irq_handler(dev, iir);
> -
>  		I915_WRITE_FW(GEN8_MASTER_IRQ, DE_MASTER_IRQ_CONTROL);
>  	}
>  
> +	/* Call regardless, as some status bits might not be
> +	 * signalled in iir */
> +	valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
> +
>  	return ret;
>  }
>  
> -- 
> 2.5.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://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