Re: [PATCH 11/12] drm/i915: Inline I915_INTERRUPT_ENABLE_FIX

From: Jesse Barnes <jbarnes@virtuousgeek.org>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 11/12] drm/i915: Inline I915_INTERRUPT_ENABLE_FIX
Date: Wed, 25 Apr 2012 13:55:12 -0700	[thread overview]
Message-ID: <20120425135512.6307b991@jbarnes-desktop> (raw)
In-Reply-To: <1335304792-17636-11-git-send-email-chris@chris-wilson.co.uk>

On Tue, 24 Apr 2012 22:59:51 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Since there is only one remaining user of I915_INTERRUPT_ENABLE_FIX,
> expand it at the callsite. Quoting Jesse Barnes:
> 
> "I'd really like to get rid of these defines at the top of i915_irq.c.
> Some are unused and the others just make you check for the right bits
> everytime your read the code."
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |   38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1d320e0..a1150b7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -39,24 +39,6 @@
>  
>  #define MAX_NOPID ((u32)~0)
>  
> -/**
> - * Interrupts that are always left unmasked.
> - *
> - * Since pipe events are edge-triggered from the PIPESTAT register to IIR,
> - * we leave them always unmasked in IMR and then control enabling them through
> - * PIPESTAT alone.
> - */
> -#define I915_INTERRUPT_ENABLE_FIX			\
> -	(I915_ASLE_INTERRUPT |				\
> -	 I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |		\
> -	 I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |		\
> -	 I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |	\
> -	 I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT |	\
> -	 I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT)
> -
> -/** Interrupts that we mask and unmask at runtime. */
> -#define I915_INTERRUPT_ENABLE_VAR (I915_USER_INTERRUPT | I915_BSD_USER_INTERRUPT)
> -
>  #define I915_PIPE_VBLANK_STATUS	(PIPE_START_VBLANK_INTERRUPT_STATUS |\
>  				 PIPE_VBLANK_INTERRUPT_STATUS)
>  
> @@ -2549,13 +2531,29 @@ static void i965_irq_preinstall(struct drm_device * dev)
>  static int i965_irq_postinstall(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> -	u32 enable_mask = I915_INTERRUPT_ENABLE_FIX | I915_INTERRUPT_ENABLE_VAR;
> +	u32 enable_mask;
>  	u32 error_mask;
>  
>  	dev_priv->vblank_pipe = DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B;
>  
>  	/* Unmask the interrupts that we always want on. */
> -	dev_priv->irq_mask = ~I915_INTERRUPT_ENABLE_FIX;
> +	dev_priv->irq_mask = ~(I915_ASLE_INTERRUPT |
> +			       I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> +			       I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
> +			       I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
> +			       I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT |
> +			       I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT);
> +
> +	enable_mask = (I915_ASLE_INTERRUPT |
> +		       I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> +		       I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
> +		       I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
> +		       I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT |
> +		       I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT |
> +		       I915_USER_INTERRUPT);

You could set the enable mask to ~irq_mask | I915_USER_INTERRUPT I
think just to save a little duplication?  No biggie though.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Also this one is a little sneaky because it silently fixes the
BSD_USER_INTERRUPT bug on pre-Cantiga.  But I suppose that's fine...

-- 
Jesse Barnes, Intel Open Source Technology Center