diff mbox

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

Message ID 1394233836-3827-4-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni March 7, 2014, 11:10 p.m. UTC
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(-)

Comments

Ben Widawsky March 18, 2014, 5:11 p.m. UTC | #1
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
Daniel Vetter March 18, 2014, 5:41 p.m. UTC | #2
On Tue, Mar 18, 2014 at 10:11:38AM -0700, Ben Widawsky wrote:
> 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?

Bspec says if there's a 2nd interrupt queued up IMR/IER don't affect that
bit and it will show up /sometimes/ after the first bit has been cleared
from IIR. My best guess for that was "on the next read", hence the
POSTING_READ to make sure it happens.

Or do you mean something else?

In any case a bit of Bspec citation for this would be appropriate.
-Daniel

> 
> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Paulo Zanoni March 26, 2014, 8 p.m. UTC | #3
2014-03-18 14:11 GMT-03:00 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?

Because if the next piece of the code is still reading registers, you
don't need it, so you can save it. This is exactly what is done by the
Gen8 code, which I copied. So the comment is telling the user to not
forget to do what you have done, for example, with the call to
POSTING_READ(GEN8_PCU_IIR) at gen8_irq_preinstall and
gen8_irq_uninstall. In other words, the comment is supposed to mean
"hey, the macros don't do the posting read, so please remember to do
them if you need". If we leave the POSTING_READs out of the macro, we
can save a few calls, just like we do on the Gen8 code.

Anyway, we have discussed this and it seems everybody prefers to have
the POSTING_READ calls on the macro, so I'll just change the patches
to do this, and then the comment will disappear.

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

I thought the reason it wasn't in your original macro was because you
always had I915_READ calls after the macro calls. And on the case you
don't have this, you have that that POSTING_READ(GEN8_PCU_IIR) which I
mentioned.

>
>> +#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
Daniel Vetter March 26, 2014, 9:37 p.m. UTC | #4
On Wed, Mar 26, 2014 at 9:00 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Anyway, we have discussed this and it seems everybody prefers to have
> the POSTING_READ calls on the macro, so I'll just change the patches
> to do this, and then the comment will disappear.

I think for consistency we should do the same change in gen8. So maybe
as a follow-up patch on top of this which changes everything? Since
this unifies the irq setup code it's slightly irksome if we deviate
right away ;-)
-Daniel
Paulo Zanoni March 27, 2014, 12:06 p.m. UTC | #5
2014-03-26 18:37 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Mar 26, 2014 at 9:00 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> Anyway, we have discussed this and it seems everybody prefers to have
>> the POSTING_READ calls on the macro, so I'll just change the patches
>> to do this, and then the comment will disappear.
>
> I think for consistency we should do the same change in gen8. So maybe
> as a follow-up patch on top of this which changes everything? Since
> this unifies the irq setup code it's slightly irksome if we deviate
> right away ;-)

It's what patch 20 does. I was just squashing it into the earlier
patches, as requested.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff mbox

Patch

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.
+ * */
+#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);