Re: [PATCH 03/20] drm/i915: use GEN8_IRQ_INIT on GEN5

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, Mar 07, 2014 at 08:10:19PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> And rename is to GEN5_IRQ_INIT.
> 
> We have discussed doing equivalent changes on July 2013, and I even
> sent a patch series for this: "[PATCH 00/15] Unify interrupt register
> init/reset". Now that the BDW code was merged, I have one more
> argument in favor of these changes.
> 
> Here's what really changes with the Gen 5 IRQ init code:
>   - We now clear the IIR registers at preinstall (they are also
>     cleared at postinstall, but we will change that later).
>   - We have an additional POSTING_READ at the IMR register.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 49 +++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 852844d..7be7da1 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -80,12 +80,30 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
>  };
>  
> +/*
> + * IIR can theoretically queue up two events. Be paranoid.
> + * Also, make sure callers of these macros have something equivalent to a
> + * POSTING_READ on the IIR register.
> + * */

I don't understand what you mean in this comment. If you're always going
to sending a posting read after the second IIR write, why not just put
it in the macro?

The reason it wasn't in my original macro is because we've done the
posting read on IER, and IMR - so we're not going to get new interrupts.
When the second IIR write lands is irrelevant. The POSTING_READ in
between is to prevent the [probably impossible] case of the writes
getting collapsed into one write.

> +#define GEN8_IRQ_INIT_NDX(type, which) do { \
> +	I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
> +	POSTING_READ(GEN8_##type##_IMR(which)); \
> +	I915_WRITE(GEN8_##type##_IER(which), 0); \
> +	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
> +	POSTING_READ(GEN8_##type##_IIR(which)); \
> +	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
> +} while (0)
> +
>  #define GEN5_IRQ_INIT(type) do { \
>  	I915_WRITE(type##IMR, 0xffffffff); \
> +	POSTING_READ(type##IMR); \
>  	I915_WRITE(type##IER, 0); \
> -	POSTING_READ(type##IER); \
> +	I915_WRITE(type##IIR, 0xffffffff); \
> +	POSTING_READ(type##IIR); \
> +	I915_WRITE(type##IIR, 0xffffffff); \
>  } while (0)
>  
> +
>  /* For display hotplug interrupt */
>  static void
>  ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
> @@ -2789,6 +2807,7 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev)
>  	GEN5_IRQ_INIT(GT);
>  	if (INTEL_INFO(dev)->gen >= 6)
>  		GEN5_IRQ_INIT(GEN6_PM);
> +	POSTING_READ(GTIIR);
>  }
>  
>  /* drm_dma.h hooks
> @@ -2843,25 +2862,6 @@ static void gen8_irq_preinstall(struct drm_device *dev)
>  	I915_WRITE(GEN8_MASTER_IRQ, 0);
>  	POSTING_READ(GEN8_MASTER_IRQ);
>  
> -	/* IIR can theoretically queue up two events. Be paranoid */
> -#define GEN8_IRQ_INIT_NDX(type, which) do { \
> -		I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
> -		POSTING_READ(GEN8_##type##_IMR(which)); \
> -		I915_WRITE(GEN8_##type##_IER(which), 0); \
> -		I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
> -		POSTING_READ(GEN8_##type##_IIR(which)); \
> -		I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
> -	} while (0)
> -
> -#define GEN8_IRQ_INIT(type) do { \
> -		I915_WRITE(GEN8_##type##_IMR, 0xffffffff); \
> -		POSTING_READ(GEN8_##type##_IMR); \
> -		I915_WRITE(GEN8_##type##_IER, 0); \
> -		I915_WRITE(GEN8_##type##_IIR, 0xffffffff); \
> -		POSTING_READ(GEN8_##type##_IIR); \
> -		I915_WRITE(GEN8_##type##_IIR, 0xffffffff); \
> -	} while (0)
> -
>  	GEN8_IRQ_INIT_NDX(GT, 0);
>  	GEN8_IRQ_INIT_NDX(GT, 1);
>  	GEN8_IRQ_INIT_NDX(GT, 2);
> @@ -2871,12 +2871,9 @@ static void gen8_irq_preinstall(struct drm_device *dev)
>  		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe);
>  	}
>  
> -	GEN8_IRQ_INIT(DE_PORT);
> -	GEN8_IRQ_INIT(DE_MISC);
> -	GEN8_IRQ_INIT(PCU);
> -#undef GEN8_IRQ_INIT
> -#undef GEN8_IRQ_INIT_NDX
> -
> +	GEN5_IRQ_INIT(GEN8_DE_PORT_);
> +	GEN5_IRQ_INIT(GEN8_DE_MISC_);
> +	GEN5_IRQ_INIT(GEN8_PCU_);
>  	POSTING_READ(GEN8_PCU_IIR);
>  
>  	ibx_irq_preinstall(dev);
> -- 
> 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