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

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

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

On Wed, 2014-02-12 at 17:55 +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 or enable pipestat interrupts
>   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.
> 
> v2: Ville noticed that I've broken the logic a bit with trying to
> avoid checking whether we're interested in a given pipe twice. push
> the PIPESTAT read down after we've computed the mask of interesting
> bits first to avoid that duplication properly.
> 
> 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>

Looks simpler, so:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 36 +++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/i915_reg.h |  4 ----
>  2 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 386a640b7c92..a45ed67407bd 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1559,25 +1559,39 @@ 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;
> -
> -		if (!dev_priv->pipestat_irq_mask[pipe] &&
> -		    !__cpu_fifo_underrun_reporting_enabled(dev, pipe))
> -			continue;
> -
> -		reg = PIPESTAT(pipe);
> -		pipe_stats[pipe] = I915_READ(reg);
> +		u32 mask, iir_bit;
>  
>  		/*
> -		 * Clear the PIPE*STAT regs before the IIR
> +		 * PIPESTAT 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;
>  
> +		if (!mask)
> +			continue;
> +
> +		reg = PIPESTAT(pipe);
> +		pipe_stats[pipe] = I915_READ(reg) & 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)


[-- 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