Re: [PATCH 07/20] drm/i915: add GEN5_IRQ_INIT

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, Mar 07, 2014 at 08:10:23PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> And the equivalent GEN8_IRQ_INIT_NDX macro. These macros are for the
> postinstall functions. The next patch will improve this macro.
> 
> Notice that I could have included POSTING_READ calls to the macro, but
> that would mean the code would do a few more POSTING_READs than
> necessary. OTOH it would be more fail-proof. I can change that if
> needed.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 73f1125..6d4daf2 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -103,6 +103,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  	I915_WRITE(type##IIR, 0xffffffff); \
>  } while (0)
>  
> +#define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
> +	I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
> +	I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \
> +} while (0)
> +
> +#define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \
> +	I915_WRITE(type##IMR, (imr_val)); \
> +	I915_WRITE(type##IER, (ier_val)); \
> +} while (0)
> +

I don't like these macros. IMO they make the code less readable, and
only save a couple LOC. They don't prevent any programmer errors either,
since all the logic is still contained in the values you pass in.

I'll read on ahead to see if they're required in your grand scheme.

>  /* For display hotplug interrupt */
>  static void
>  ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
> @@ -2957,9 +2967,7 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
>  	}
>  
>  	I915_WRITE(GTIIR, I915_READ(GTIIR));
> -	I915_WRITE(GTIMR, dev_priv->gt_irq_mask);
> -	I915_WRITE(GTIER, gt_irqs);
> -	POSTING_READ(GTIER);
> +	GEN5_IRQ_INIT(GT, dev_priv->gt_irq_mask, gt_irqs);
>  
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		pm_irqs |= GEN6_PM_RPS_EVENTS;
> @@ -2969,10 +2977,9 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
>  
>  		dev_priv->pm_irq_mask = 0xffffffff;
>  		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> -		I915_WRITE(GEN6_PMIMR, dev_priv->pm_irq_mask);
> -		I915_WRITE(GEN6_PMIER, pm_irqs);
> -		POSTING_READ(GEN6_PMIER);
> +		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_irq_mask, pm_irqs);
>  	}
> +	POSTING_READ(GTIER);
>  }
>  
>  static int ironlake_irq_postinstall(struct drm_device *dev)
> @@ -3005,9 +3012,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  
>  	/* should always can generate irq */
>  	I915_WRITE(DEIIR, I915_READ(DEIIR));
> -	I915_WRITE(DEIMR, dev_priv->irq_mask);
> -	I915_WRITE(DEIER, display_mask | extra_mask);
> -	POSTING_READ(DEIER);
> +	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
>  
>  	gen5_gt_irq_postinstall(dev);
>  
> @@ -3172,8 +3177,7 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>  		if (tmp)
>  			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
>  				  i, tmp);
> -		I915_WRITE(GEN8_GT_IMR(i), ~gt_interrupts[i]);
> -		I915_WRITE(GEN8_GT_IER(i), gt_interrupts[i]);
> +		GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]);
>  	}
>  	POSTING_READ(GEN8_GT_IER(0));
>  }
> @@ -3196,13 +3200,12 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  		if (tmp)
>  			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
>  				  pipe, tmp);
> -		I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
> -		I915_WRITE(GEN8_DE_PIPE_IER(pipe), de_pipe_enables);
> +		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
> +				  de_pipe_enables);
>  	}
>  	POSTING_READ(GEN8_DE_PIPE_ISR(0));
>  
> -	I915_WRITE(GEN8_DE_PORT_IMR, ~GEN8_AUX_CHANNEL_A);
> -	I915_WRITE(GEN8_DE_PORT_IER, GEN8_AUX_CHANNEL_A);
> +	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
>  	POSTING_READ(GEN8_DE_PORT_IER);
>  }
>  
> -- 
> 1.8.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center