Re: [PATCH] drm/i915: Some polish for the new pipestat_irq_handler

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Feb 12, 2014 at 05:21:06PM +0100, Daniel Vetter wrote:
> Just a bit of polish which I hope will help me with massaging some
> internal patches to use Imre's reworked pipestat handling:
> - Don't check for underrun reporting twice.
> - Frob the comments a bit.
> - Do the iir PIPE_EVENT to pipe mapping explicitly with a switch. We
>   only have one place which does this, so better to make it explicit.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 25 ++++++++++++++++++++-----
>  drivers/gpu/drm/i915/i915_reg.h |  4 ----
>  2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 386a640b7c92..bbd65809742b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1559,25 +1559,40 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>  	spin_lock(&dev_priv->irq_lock);
>  	for_each_pipe(pipe) {
>  		int reg;
> -		u32 mask;
> +		u32 mask, iir_bit;
>  
> -		if (!dev_priv->pipestat_irq_mask[pipe] &&
> -		    !__cpu_fifo_underrun_reporting_enabled(dev, pipe))
> +		if (!dev_priv->pipestat_irq_mask[pipe])
>  			continue;

Underrun reporting doesn't have an enable bit, so if we don't check it
here we'd fail to detect underruns when no PIPESTAT interrupts are
enabled. Currently that probably wouldn't happen since we always enable
some display interrupts, but I'd keep the check nonetheless.

>  
>  		reg = PIPESTAT(pipe);
>  		pipe_stats[pipe] = I915_READ(reg);
>  
>  		/*
> -		 * Clear the PIPE*STAT regs before the IIR
> +		 * pipe_stat bits get signalled even when the interrupt is
> +		 * disabled with the mask bits, and some of the status bits do
> +		 * not generate interrupts at all (like the underrun bit). Hence
> +		 * we need to be careful that we only handle what we want to
> +		 * handle.
>  		 */
>  		mask = PIPESTAT_INT_ENABLE_MASK;
>  		if (__cpu_fifo_underrun_reporting_enabled(dev, pipe))
>  			mask |= PIPE_FIFO_UNDERRUN_STATUS;
> -		if (iir & I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe))
> +
> +		switch (pipe) {
> +		case PIPE_A:
> +			iir_bit = I915_DISPLAY_PIPE_A_EVENT_INTERRUPT;
> +			break;
> +		case PIPE_B:
> +			iir_bit = I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
> +			break;
> +		}
> +		if (iir & iir_bit)
>  			mask |= dev_priv->pipestat_irq_mask[pipe];
>  		pipe_stats[pipe] &= 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]);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 645221270c34..8344541bbb93 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -997,10 +997,6 @@
>  #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.1.4

-- 
Ville Syrjälä
Intel OTC