[PATCH 00/20] ILK+ interrupt improvements, v2

[PATCH 00/20] ILK+ interrupt improvements, v2

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which
was sent to the mailing list on January 22. There are no real differences,
except for the last patch, which is new.

Original cover letter:
http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html

The idea behind this series is that at some point our runtime PM code will just
call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of
using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs
to our code before we do that change. We gotta be in shape if we want to be
exposed to runtime!

Thanks,
Paulo

Paulo Zanoni (20):
  drm/i915: add GEN5_IRQ_INIT macro
  drm/i915: also use GEN5_IRQ_INIT with south display interrupts
  drm/i915: use GEN8_IRQ_INIT on GEN5
  drm/i915: add GEN5_IRQ_FINI
  drm/i915: don't forget to uninstall the PM IRQs
  drm/i915: properly clear IIR at irq_uninstall on Gen5+
  drm/i915: add GEN5_IRQ_INIT
  drm/i915: check if IIR is still zero at postinstall on Gen5+
  drm/i915: fix SERR_INT init/reset code
  drm/i915: fix GEN7_ERR_INT init/reset code
  drm/i915: fix open coded gen5_gt_irq_preinstall
  drm/i915: extract ibx_irq_uninstall
  drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall
  drm/i915: enable SDEIER later
  drm/i915: remove ibx_irq_uninstall
  drm/i915: add missing intel_hpd_irq_uninstall
  drm/i915: add ironlake_irq_reset
  drm/i915: add gen8_irq_reset
  drm/i915: only enable HWSTAM interrupts on postinstall on ILK+
  drm/i915: add POSTING_READs to the IRQ init/reset macros

 drivers/gpu/drm/i915/i915_irq.c | 270 ++++++++++++++++++----------------------
 1 file changed, 121 insertions(+), 149 deletions(-)

-- 
1.8.5.3

[PATCH 01/20] drm/i915: add GEN5_IRQ_INIT macro

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The goal is to reuse the GEN8 macros, but a few changes are needed, so
let's make things easier to review.

I could also use these macros on older code, but since I plan to
change how the interrupts are initialized, we'll risk breaking the
older code in the next commits, so I'll leave this out for now.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a5df448..30c44fb 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -80,6 +80,12 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	[HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS
 };
 
+#define GEN5_IRQ_INIT(type) do { \
+	I915_WRITE(type##IMR, 0xffffffff); \
+	I915_WRITE(type##IER, 0); \
+	POSTING_READ(type##IER); \
+} while (0)
+
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
@@ -2781,17 +2787,9 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	/* and GT */
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	POSTING_READ(GTIER);
-
-	if (INTEL_INFO(dev)->gen >= 6) {
-		/* and PM */
-		I915_WRITE(GEN6_PMIMR, 0xffffffff);
-		I915_WRITE(GEN6_PMIER, 0x0);
-		POSTING_READ(GEN6_PMIER);
-	}
+	GEN5_IRQ_INIT(GT);
+	if (INTEL_INFO(dev)->gen >= 6)
+		GEN5_IRQ_INIT(GEN6_PM);
 }
 
 /* drm_dma.h hooks
@@ -2802,9 +2800,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xeffe);
 
-	I915_WRITE(DEIMR, 0xffffffff);
-	I915_WRITE(DEIER, 0x0);
-	POSTING_READ(DEIER);
+	GEN5_IRQ_INIT(DE);
 
 	gen5_gt_irq_preinstall(dev);
 
-- 
1.8.5.3

[PATCH 02/20] drm/i915: also use GEN5_IRQ_INIT with south display interrupts

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This interrupt gets initialized with a different IER value, so it was
not using the macro. The problem is that we plan to modify the macro
to make it do additional things, and we want the SDE interrupts
updated too. So let's make sure we call the macro, then, after it, we
do the necessary SDE-specific changes.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 30c44fb..852844d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2771,8 +2771,7 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	/* south display irq */
-	I915_WRITE(SDEIMR, 0xffffffff);
+	GEN5_IRQ_INIT(SDE);
 	/*
 	 * SDEIER is also touched by the interrupt handler to work around missed
 	 * PCH interrupts. Hence we can't update it after the interrupt handler
-- 
1.8.5.3

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

[Paulo Zanoni]

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

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

[Ben Widawsky]

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

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

[Daniel Vetter]

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

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

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

[Paulo Zanoni]

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



-- 
Paulo Zanoni

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Paulo Zanoni]

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



-- 
Paulo Zanoni

[PATCH 04/20] drm/i915: add GEN5_IRQ_FINI

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Same as the _INIT macro: the goal is to reuse the GEN8 macros, but
there are still some slight differences.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7be7da1..a9f173c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -103,6 +103,11 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(type##IIR, 0xffffffff); \
 } while (0)
 
+#define GEN5_IRQ_FINI(type) do { \
+	I915_WRITE(type##IMR, 0xffffffff); \
+	I915_WRITE(type##IER, 0); \
+	I915_WRITE(type##IIR, I915_READ(type##IIR)); \
+} while (0)
 
 /* For display hotplug interrupt */
 static void
@@ -3304,22 +3309,16 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xffffffff);
 
-	I915_WRITE(DEIMR, 0xffffffff);
-	I915_WRITE(DEIER, 0x0);
-	I915_WRITE(DEIIR, I915_READ(DEIIR));
+	GEN5_IRQ_FINI(DE);
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
-	I915_WRITE(GTIMR, 0xffffffff);
-	I915_WRITE(GTIER, 0x0);
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
+	GEN5_IRQ_FINI(GT);
 
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	I915_WRITE(SDEIMR, 0xffffffff);
-	I915_WRITE(SDEIER, 0x0);
-	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
+	GEN5_IRQ_FINI(SDE);
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
 		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
 }
-- 
1.8.5.3

[PATCH 05/20] drm/i915: don't forget to uninstall the PM IRQs

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

It's the only thihg missing, apparently.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a9f173c..f681462 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3314,6 +3314,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
 	GEN5_IRQ_FINI(GT);
+	if (INTEL_INFO(dev)->gen >= 6)
+		GEN5_IRQ_FINI(GEN6_PM);
 
 	if (HAS_PCH_NOP(dev))
 		return;
-- 
1.8.5.3

Re: [PATCH 05/20] drm/i915: don't forget to uninstall the PM IRQs

[Ben Widawsky]

On Fri, Mar 07, 2014 at 08:10:21PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> It's the only thihg missing, apparently.

s/thihg/thing

There is a potential fixup in patch 3, but with or without,
everything up through here is:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index a9f173c..f681462 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3314,6 +3314,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>  
>  	GEN5_IRQ_FINI(GT);
> +	if (INTEL_INFO(dev)->gen >= 6)
> +		GEN5_IRQ_FINI(GEN6_PM);
>  
>  	if (HAS_PCH_NOP(dev))
>  		return;
> -- 
> 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

[PATCH 06/20] drm/i915: properly clear IIR at irq_uninstall on Gen5+

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The IRQ_INIT and IRQ_FINI macros are basically the same thing, with
the exception that IRQ_FINI doesn't properly clear IIR twice and
doesn't have as many POSTING_READs as IRQ_INIT. So rename the macro to
IRQ_RESET and use it everywhere.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 76 +++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f681462..73f1125 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -85,7 +85,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
  * 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 { \
+#define GEN8_IRQ_RESET_NDX(type, which) do { \
 	I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
 	POSTING_READ(GEN8_##type##_IMR(which)); \
 	I915_WRITE(GEN8_##type##_IER(which), 0); \
@@ -94,7 +94,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
 } while (0)
 
-#define GEN5_IRQ_INIT(type) do { \
+#define GEN5_IRQ_RESET(type) do { \
 	I915_WRITE(type##IMR, 0xffffffff); \
 	POSTING_READ(type##IMR); \
 	I915_WRITE(type##IER, 0); \
@@ -103,12 +103,6 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(type##IIR, 0xffffffff); \
 } while (0)
 
-#define GEN5_IRQ_FINI(type) do { \
-	I915_WRITE(type##IMR, 0xffffffff); \
-	I915_WRITE(type##IER, 0); \
-	I915_WRITE(type##IIR, I915_READ(type##IIR)); \
-} while (0)
-
 /* For display hotplug interrupt */
 static void
 ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
@@ -2794,7 +2788,7 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	GEN5_IRQ_INIT(SDE);
+	GEN5_IRQ_RESET(SDE);
 	/*
 	 * SDEIER is also touched by the interrupt handler to work around missed
 	 * PCH interrupts. Hence we can't update it after the interrupt handler
@@ -2809,9 +2803,9 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	GEN5_IRQ_INIT(GT);
+	GEN5_IRQ_RESET(GT);
 	if (INTEL_INFO(dev)->gen >= 6)
-		GEN5_IRQ_INIT(GEN6_PM);
+		GEN5_IRQ_RESET(GEN6_PM);
 	POSTING_READ(GTIIR);
 }
 
@@ -2823,7 +2817,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xeffe);
 
-	GEN5_IRQ_INIT(DE);
+	GEN5_IRQ_RESET(DE);
 
 	gen5_gt_irq_preinstall(dev);
 
@@ -2867,18 +2861,18 @@ static void gen8_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(GEN8_MASTER_IRQ, 0);
 	POSTING_READ(GEN8_MASTER_IRQ);
 
-	GEN8_IRQ_INIT_NDX(GT, 0);
-	GEN8_IRQ_INIT_NDX(GT, 1);
-	GEN8_IRQ_INIT_NDX(GT, 2);
-	GEN8_IRQ_INIT_NDX(GT, 3);
+	GEN8_IRQ_RESET_NDX(GT, 0);
+	GEN8_IRQ_RESET_NDX(GT, 1);
+	GEN8_IRQ_RESET_NDX(GT, 2);
+	GEN8_IRQ_RESET_NDX(GT, 3);
 
 	for_each_pipe(pipe) {
-		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe);
+		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
 	}
 
-	GEN5_IRQ_INIT(GEN8_DE_PORT_);
-	GEN5_IRQ_INIT(GEN8_DE_MISC_);
-	GEN5_IRQ_INIT(GEN8_PCU_);
+	GEN5_IRQ_RESET(GEN8_DE_PORT_);
+	GEN5_IRQ_RESET(GEN8_DE_MISC_);
+	GEN5_IRQ_RESET(GEN8_PCU_);
 	POSTING_READ(GEN8_PCU_IIR);
 
 	ibx_irq_preinstall(dev);
@@ -3237,32 +3231,17 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(GEN8_MASTER_IRQ, 0);
 
-#define GEN8_IRQ_FINI_NDX(type, which) do { \
-		I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
-		I915_WRITE(GEN8_##type##_IER(which), 0); \
-		I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
-	} while (0)
-
-#define GEN8_IRQ_FINI(type) do { \
-		I915_WRITE(GEN8_##type##_IMR, 0xffffffff); \
-		I915_WRITE(GEN8_##type##_IER, 0); \
-		I915_WRITE(GEN8_##type##_IIR, 0xffffffff); \
-	} while (0)
+	GEN8_IRQ_RESET_NDX(GT, 0);
+	GEN8_IRQ_RESET_NDX(GT, 1);
+	GEN8_IRQ_RESET_NDX(GT, 2);
+	GEN8_IRQ_RESET_NDX(GT, 3);
 
-	GEN8_IRQ_FINI_NDX(GT, 0);
-	GEN8_IRQ_FINI_NDX(GT, 1);
-	GEN8_IRQ_FINI_NDX(GT, 2);
-	GEN8_IRQ_FINI_NDX(GT, 3);
-
-	for_each_pipe(pipe) {
-		GEN8_IRQ_FINI_NDX(DE_PIPE, pipe);
-	}
+	for_each_pipe(pipe)
+		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
 
-	GEN8_IRQ_FINI(DE_PORT);
-	GEN8_IRQ_FINI(DE_MISC);
-	GEN8_IRQ_FINI(PCU);
-#undef GEN8_IRQ_FINI
-#undef GEN8_IRQ_FINI_NDX
+	GEN5_IRQ_RESET(GEN8_DE_PORT_);
+	GEN5_IRQ_RESET(GEN8_DE_MISC_);
+	GEN5_IRQ_RESET(GEN8_PCU_);
 
 	POSTING_READ(GEN8_PCU_IIR);
 }
@@ -3309,18 +3288,19 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xffffffff);
 
-	GEN5_IRQ_FINI(DE);
+	GEN5_IRQ_RESET(DE);
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
 
-	GEN5_IRQ_FINI(GT);
+	GEN5_IRQ_RESET(GT);
 	if (INTEL_INFO(dev)->gen >= 6)
-		GEN5_IRQ_FINI(GEN6_PM);
+		GEN5_IRQ_RESET(GEN6_PM);
+	POSTING_READ(GTIIR);
 
 	if (HAS_PCH_NOP(dev))
 		return;
 
-	GEN5_IRQ_FINI(SDE);
+	GEN5_IRQ_RESET(SDE);
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
 		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
 }
-- 
1.8.5.3

Re: [PATCH 06/20] drm/i915: properly clear IIR at irq_uninstall on Gen5+

[Chris Wilson]

On Fri, Mar 07, 2014 at 08:10:22PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The IRQ_INIT and IRQ_FINI macros are basically the same thing, with
> the exception that IRQ_FINI doesn't properly clear IIR twice and
> doesn't have as many POSTING_READs as IRQ_INIT. So rename the macro to
> IRQ_RESET and use it everywhere.

"the macro". The previous sentence talks about a pair of macros, so it
not clear to which you refer to here. By deduction and confirmed by
inspection you mean s/IRQ_INIT/IRQ_FINI/.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 06/20] drm/i915: properly clear IIR at irq_uninstall on Gen5+

[Ben Widawsky]

On Fri, Mar 07, 2014 at 08:10:22PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The IRQ_INIT and IRQ_FINI macros are basically the same thing, with
> the exception that IRQ_FINI doesn't properly clear IIR twice and
> doesn't have as many POSTING_READs as IRQ_INIT. So rename the macro to
> IRQ_RESET and use it everywhere.

Make me happy and call it GEN5_IRQ_DISABLE(). Also note, I've argued
with the, "doesn't properly clear IIR twice" in a previous patch, but
that's not a big deal.

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 76 +++++++++++++++--------------------------
>  1 file changed, 28 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index f681462..73f1125 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -85,7 +85,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>   * 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 { \
> +#define GEN8_IRQ_RESET_NDX(type, which) do { \
>  	I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
>  	POSTING_READ(GEN8_##type##_IMR(which)); \
>  	I915_WRITE(GEN8_##type##_IER(which), 0); \
> @@ -94,7 +94,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
>  } while (0)
>  
> -#define GEN5_IRQ_INIT(type) do { \
> +#define GEN5_IRQ_RESET(type) do { \
>  	I915_WRITE(type##IMR, 0xffffffff); \
>  	POSTING_READ(type##IMR); \
>  	I915_WRITE(type##IER, 0); \
> @@ -103,12 +103,6 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  	I915_WRITE(type##IIR, 0xffffffff); \
>  } while (0)
>  
> -#define GEN5_IRQ_FINI(type) do { \
> -	I915_WRITE(type##IMR, 0xffffffff); \
> -	I915_WRITE(type##IER, 0); \
> -	I915_WRITE(type##IIR, I915_READ(type##IIR)); \
> -} while (0)
> -
>  /* For display hotplug interrupt */
>  static void
>  ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
> @@ -2794,7 +2788,7 @@ static void ibx_irq_preinstall(struct drm_device *dev)
>  	if (HAS_PCH_NOP(dev))
>  		return;
>  
> -	GEN5_IRQ_INIT(SDE);
> +	GEN5_IRQ_RESET(SDE);
>  	/*
>  	 * SDEIER is also touched by the interrupt handler to work around missed
>  	 * PCH interrupts. Hence we can't update it after the interrupt handler
> @@ -2809,9 +2803,9 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	GEN5_IRQ_INIT(GT);
> +	GEN5_IRQ_RESET(GT);
>  	if (INTEL_INFO(dev)->gen >= 6)
> -		GEN5_IRQ_INIT(GEN6_PM);
> +		GEN5_IRQ_RESET(GEN6_PM);
>  	POSTING_READ(GTIIR);
>  }
>  
> @@ -2823,7 +2817,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
>  
>  	I915_WRITE(HWSTAM, 0xeffe);
>  
> -	GEN5_IRQ_INIT(DE);
> +	GEN5_IRQ_RESET(DE);
>  
>  	gen5_gt_irq_preinstall(dev);
>  
> @@ -2867,18 +2861,18 @@ static void gen8_irq_preinstall(struct drm_device *dev)
>  	I915_WRITE(GEN8_MASTER_IRQ, 0);
>  	POSTING_READ(GEN8_MASTER_IRQ);
>  
> -	GEN8_IRQ_INIT_NDX(GT, 0);
> -	GEN8_IRQ_INIT_NDX(GT, 1);
> -	GEN8_IRQ_INIT_NDX(GT, 2);
> -	GEN8_IRQ_INIT_NDX(GT, 3);
> +	GEN8_IRQ_RESET_NDX(GT, 0);
> +	GEN8_IRQ_RESET_NDX(GT, 1);
> +	GEN8_IRQ_RESET_NDX(GT, 2);
> +	GEN8_IRQ_RESET_NDX(GT, 3);
>  
>  	for_each_pipe(pipe) {
> -		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe);
> +		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>  	}
>  
> -	GEN5_IRQ_INIT(GEN8_DE_PORT_);
> -	GEN5_IRQ_INIT(GEN8_DE_MISC_);
> -	GEN5_IRQ_INIT(GEN8_PCU_);
> +	GEN5_IRQ_RESET(GEN8_DE_PORT_);
> +	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> +	GEN5_IRQ_RESET(GEN8_PCU_);
>  	POSTING_READ(GEN8_PCU_IIR);
>  
>  	ibx_irq_preinstall(dev);
> @@ -3237,32 +3231,17 @@ static void gen8_irq_uninstall(struct drm_device *dev)
>  
>  	I915_WRITE(GEN8_MASTER_IRQ, 0);
>  
> -#define GEN8_IRQ_FINI_NDX(type, which) do { \
> -		I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
> -		I915_WRITE(GEN8_##type##_IER(which), 0); \
> -		I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
> -	} while (0)
> -
> -#define GEN8_IRQ_FINI(type) do { \
> -		I915_WRITE(GEN8_##type##_IMR, 0xffffffff); \
> -		I915_WRITE(GEN8_##type##_IER, 0); \
> -		I915_WRITE(GEN8_##type##_IIR, 0xffffffff); \
> -	} while (0)
> +	GEN8_IRQ_RESET_NDX(GT, 0);
> +	GEN8_IRQ_RESET_NDX(GT, 1);
> +	GEN8_IRQ_RESET_NDX(GT, 2);
> +	GEN8_IRQ_RESET_NDX(GT, 3);
>  
> -	GEN8_IRQ_FINI_NDX(GT, 0);
> -	GEN8_IRQ_FINI_NDX(GT, 1);
> -	GEN8_IRQ_FINI_NDX(GT, 2);
> -	GEN8_IRQ_FINI_NDX(GT, 3);
> -
> -	for_each_pipe(pipe) {
> -		GEN8_IRQ_FINI_NDX(DE_PIPE, pipe);
> -	}
> +	for_each_pipe(pipe)
> +		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>  
> -	GEN8_IRQ_FINI(DE_PORT);
> -	GEN8_IRQ_FINI(DE_MISC);
> -	GEN8_IRQ_FINI(PCU);
> -#undef GEN8_IRQ_FINI
> -#undef GEN8_IRQ_FINI_NDX
> +	GEN5_IRQ_RESET(GEN8_DE_PORT_);
> +	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> +	GEN5_IRQ_RESET(GEN8_PCU_);
>  
>  	POSTING_READ(GEN8_PCU_IIR);
>  }
> @@ -3309,18 +3288,19 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  
>  	I915_WRITE(HWSTAM, 0xffffffff);
>  
> -	GEN5_IRQ_FINI(DE);
> +	GEN5_IRQ_RESET(DE);
>  	if (IS_GEN7(dev))
>  		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>  
> -	GEN5_IRQ_FINI(GT);
> +	GEN5_IRQ_RESET(GT);
>  	if (INTEL_INFO(dev)->gen >= 6)
> -		GEN5_IRQ_FINI(GEN6_PM);
> +		GEN5_IRQ_RESET(GEN6_PM);
> +	POSTING_READ(GTIIR);
>  
>  	if (HAS_PCH_NOP(dev))
>  		return;
>  
> -	GEN5_IRQ_FINI(SDE);
> +	GEN5_IRQ_RESET(SDE);
>  	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
>  		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
>  }
> -- 
> 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

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

[Paulo Zanoni]

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

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

[Ben Widawsky]

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

[PATCH 08/20] drm/i915: check if IIR is still zero at postinstall on Gen5+

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Instead of trying to clear it again. It should already be masked and
disabled and zeroed at preinstall/uninstall.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6d4daf2..4d0a8b1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -103,12 +103,24 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(type##IIR, 0xffffffff); \
 } while (0)
 
+/*
+ * We should clear IMR at preinstall/uninstall, and just check at postinstall.
+ */
+#define GEN5_ASSERT_IIR_IS_ZERO(reg) do { \
+	u32 val = I915_READ(reg); \
+	if (val) \
+		DRM_ERROR("Interrupt register 0x%x is not zero: 0x%08x\n", \
+			  (reg), val); \
+} while (0)
+
 #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
+	GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
 	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 { \
+	GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
 	I915_WRITE(type##IMR, (imr_val)); \
 	I915_WRITE(type##IER, (ier_val)); \
 } while (0)
@@ -2940,7 +2952,7 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
 	}
 
-	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
+	GEN5_ASSERT_IIR_IS_ZERO(SDEIIR);
 	I915_WRITE(SDEIMR, ~mask);
 }
 
@@ -2966,7 +2978,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
 	}
 
-	I915_WRITE(GTIIR, I915_READ(GTIIR));
 	GEN5_IRQ_INIT(GT, dev_priv->gt_irq_mask, gt_irqs);
 
 	if (INTEL_INFO(dev)->gen >= 6) {
@@ -2976,7 +2987,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
 
 		dev_priv->pm_irq_mask = 0xffffffff;
-		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
 		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_irq_mask, pm_irqs);
 	}
 	POSTING_READ(GTIER);
@@ -3010,8 +3020,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 
 	dev_priv->irq_mask = ~display_mask;
 
-	/* should always can generate irq */
-	I915_WRITE(DEIIR, I915_READ(DEIIR));
 	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
 
 	gen5_gt_irq_postinstall(dev);
@@ -3172,13 +3180,8 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 		GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT
 		};
 
-	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++) {
-		u32 tmp = I915_READ(GEN8_GT_IIR(i));
-		if (tmp)
-			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
-				  i, tmp);
+	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++)
 		GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]);
-	}
 	POSTING_READ(GEN8_GT_IER(0));
 }
 
@@ -3195,14 +3198,9 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 	dev_priv->de_irq_mask[PIPE_B] = ~de_pipe_masked;
 	dev_priv->de_irq_mask[PIPE_C] = ~de_pipe_masked;
 
-	for_each_pipe(pipe) {
-		u32 tmp = I915_READ(GEN8_DE_PIPE_IIR(pipe));
-		if (tmp)
-			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
-				  pipe, tmp);
+	for_each_pipe(pipe)
 		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
 				  de_pipe_enables);
-	}
 	POSTING_READ(GEN8_DE_PIPE_ISR(0));
 
 	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
-- 
1.8.5.3

Re: [PATCH 08/20] drm/i915: check if IIR is still zero at postinstall on Gen5+

[Ben Widawsky]

On Fri, Mar 07, 2014 at 08:10:24PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Instead of trying to clear it again. It should already be masked and
> disabled and zeroed at preinstall/uninstall.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6d4daf2..4d0a8b1 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -103,12 +103,24 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  	I915_WRITE(type##IIR, 0xffffffff); \
>  } while (0)
>  
> +/*
> + * We should clear IMR at preinstall/uninstall, and just check at postinstall.
> + */
> +#define GEN5_ASSERT_IIR_IS_ZERO(reg) do { \
> +	u32 val = I915_READ(reg); \
> +	if (val) \
> +		DRM_ERROR("Interrupt register 0x%x is not zero: 0x%08x\n", \
> +			  (reg), val); \
> +} while (0)
> +
>  #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
> +	GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
>  	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 { \
> +	GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
>  	I915_WRITE(type##IMR, (imr_val)); \
>  	I915_WRITE(type##IER, (ier_val)); \
>  } while (0)

Okay, this is replacing a POSTED_WRITE, with a (slower) POSTING_READ
which gives an error that we can do nothing about other than clear it
anyway.

I'd be in favor of dropping this patch.

> @@ -2940,7 +2952,7 @@ static void ibx_irq_postinstall(struct drm_device *dev)
>  		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
>  	}
>  
> -	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
> +	GEN5_ASSERT_IIR_IS_ZERO(SDEIIR);
>  	I915_WRITE(SDEIMR, ~mask);
>  }
>  
> @@ -2966,7 +2978,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
>  		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
>  	}
>  
> -	I915_WRITE(GTIIR, I915_READ(GTIIR));
>  	GEN5_IRQ_INIT(GT, dev_priv->gt_irq_mask, gt_irqs);
>  
>  	if (INTEL_INFO(dev)->gen >= 6) {
> @@ -2976,7 +2987,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
>  			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
>  
>  		dev_priv->pm_irq_mask = 0xffffffff;
> -		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
>  		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_irq_mask, pm_irqs);
>  	}
>  	POSTING_READ(GTIER);
> @@ -3010,8 +3020,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  
>  	dev_priv->irq_mask = ~display_mask;
>  
> -	/* should always can generate irq */
> -	I915_WRITE(DEIIR, I915_READ(DEIIR));
>  	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
>  
>  	gen5_gt_irq_postinstall(dev);
> @@ -3172,13 +3180,8 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>  		GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT
>  		};
>  
> -	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++) {
> -		u32 tmp = I915_READ(GEN8_GT_IIR(i));
> -		if (tmp)
> -			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
> -				  i, tmp);
> +	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++)
>  		GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]);
> -	}
>  	POSTING_READ(GEN8_GT_IER(0));
>  }
>  
> @@ -3195,14 +3198,9 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  	dev_priv->de_irq_mask[PIPE_B] = ~de_pipe_masked;
>  	dev_priv->de_irq_mask[PIPE_C] = ~de_pipe_masked;
>  
> -	for_each_pipe(pipe) {
> -		u32 tmp = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> -		if (tmp)
> -			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
> -				  pipe, tmp);
> +	for_each_pipe(pipe)
>  		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
>  				  de_pipe_enables);
> -	}
>  	POSTING_READ(GEN8_DE_PIPE_ISR(0));
>  
>  	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
> -- 
> 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

Re: [PATCH 08/20] drm/i915: check if IIR is still zero at postinstall on Gen5+

[Daniel Vetter]

On Tue, Mar 18, 2014 at 11:20:09AM -0700, Ben Widawsky wrote:
> On Fri, Mar 07, 2014 at 08:10:24PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Instead of trying to clear it again. It should already be masked and
> > disabled and zeroed at preinstall/uninstall.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 32 +++++++++++++++-----------------
> >  1 file changed, 15 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 6d4daf2..4d0a8b1 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -103,12 +103,24 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
> >  	I915_WRITE(type##IIR, 0xffffffff); \
> >  } while (0)
> >  
> > +/*
> > + * We should clear IMR at preinstall/uninstall, and just check at postinstall.
> > + */
> > +#define GEN5_ASSERT_IIR_IS_ZERO(reg) do { \
> > +	u32 val = I915_READ(reg); \
> > +	if (val) \
> > +		DRM_ERROR("Interrupt register 0x%x is not zero: 0x%08x\n", \
> > +			  (reg), val); \
> > +} while (0)
> > +
> >  #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
> > +	GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
> >  	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 { \
> > +	GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
> >  	I915_WRITE(type##IMR, (imr_val)); \
> >  	I915_WRITE(type##IER, (ier_val)); \
> >  } while (0)
> 
> Okay, this is replacing a POSTED_WRITE, with a (slower) POSTING_READ
> which gives an error that we can do nothing about other than clear it
> anyway.
> 
> I'd be in favor of dropping this patch.

The point of the assert is to make sure that the new IIR clearing logic
with blocking everything+clearing in the preinstall hook actually does
what it's supposed to do.

Since the point of this exercise is to reuse this code for runtime
suspend/resume where races are much easier to hit I think this is a good
self-check of the code.
-Daniel

> 
> > @@ -2940,7 +2952,7 @@ static void ibx_irq_postinstall(struct drm_device *dev)
> >  		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
> >  	}
> >  
> > -	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
> > +	GEN5_ASSERT_IIR_IS_ZERO(SDEIIR);
> >  	I915_WRITE(SDEIMR, ~mask);
> >  }
> >  
> > @@ -2966,7 +2978,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
> >  		gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT;
> >  	}
> >  
> > -	I915_WRITE(GTIIR, I915_READ(GTIIR));
> >  	GEN5_IRQ_INIT(GT, dev_priv->gt_irq_mask, gt_irqs);
> >  
> >  	if (INTEL_INFO(dev)->gen >= 6) {
> > @@ -2976,7 +2987,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
> >  			pm_irqs |= PM_VEBOX_USER_INTERRUPT;
> >  
> >  		dev_priv->pm_irq_mask = 0xffffffff;
> > -		I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> >  		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_irq_mask, pm_irqs);
> >  	}
> >  	POSTING_READ(GTIER);
> > @@ -3010,8 +3020,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> >  
> >  	dev_priv->irq_mask = ~display_mask;
> >  
> > -	/* should always can generate irq */
> > -	I915_WRITE(DEIIR, I915_READ(DEIIR));
> >  	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
> >  
> >  	gen5_gt_irq_postinstall(dev);
> > @@ -3172,13 +3180,8 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
> >  		GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT
> >  		};
> >  
> > -	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++) {
> > -		u32 tmp = I915_READ(GEN8_GT_IIR(i));
> > -		if (tmp)
> > -			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
> > -				  i, tmp);
> > +	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++)
> >  		GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]);
> > -	}
> >  	POSTING_READ(GEN8_GT_IER(0));
> >  }
> >  
> > @@ -3195,14 +3198,9 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> >  	dev_priv->de_irq_mask[PIPE_B] = ~de_pipe_masked;
> >  	dev_priv->de_irq_mask[PIPE_C] = ~de_pipe_masked;
> >  
> > -	for_each_pipe(pipe) {
> > -		u32 tmp = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> > -		if (tmp)
> > -			DRM_ERROR("Interrupt (%d) should have been masked in pre-install 0x%08x\n",
> > -				  pipe, tmp);
> > +	for_each_pipe(pipe)
> >  		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
> >  				  de_pipe_enables);
> > -	}
> >  	POSTING_READ(GEN8_DE_PIPE_ISR(0));
> >  
> >  	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
> > -- 
> > 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

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

Re: [PATCH 08/20] drm/i915: check if IIR is still zero at postinstall on Gen5+

[Ben Widawsky]

On Wed, Mar 19, 2014 at 09:28:32AM +0100, Daniel Vetter wrote:
> On Tue, Mar 18, 2014 at 11:20:09AM -0700, Ben Widawsky wrote:
> > On Fri, Mar 07, 2014 at 08:10:24PM -0300, Paulo Zanoni wrote:
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > Instead of trying to clear it again. It should already be masked and
> > > disabled and zeroed at preinstall/uninstall.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 32 +++++++++++++++-----------------
> > >  1 file changed, 15 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 6d4daf2..4d0a8b1 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -103,12 +103,24 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
> > >  	I915_WRITE(type##IIR, 0xffffffff); \
> > >  } while (0)
> > >  
> > > +/*
> > > + * We should clear IMR at preinstall/uninstall, and just check at postinstall.
> > > + */
> > > +#define GEN5_ASSERT_IIR_IS_ZERO(reg) do { \
> > > +	u32 val = I915_READ(reg); \
> > > +	if (val) \
> > > +		DRM_ERROR("Interrupt register 0x%x is not zero: 0x%08x\n", \
> > > +			  (reg), val); \
> > > +} while (0)
> > > +
> > >  #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
> > > +	GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
> > >  	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 { \
> > > +	GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
> > >  	I915_WRITE(type##IMR, (imr_val)); \
> > >  	I915_WRITE(type##IER, (ier_val)); \
> > >  } while (0)
> > 
> > Okay, this is replacing a POSTED_WRITE, with a (slower) POSTING_READ
> > which gives an error that we can do nothing about other than clear it
> > anyway.
> > 
> > I'd be in favor of dropping this patch.
> 
> The point of the assert is to make sure that the new IIR clearing logic
> with blocking everything+clearing in the preinstall hook actually does
> what it's supposed to do.
> 
> Since the point of this exercise is to reuse this code for runtime
> suspend/resume where races are much easier to hit I think this is a good
> self-check of the code.
> -Daniel
> 

Okay, I am feeling somewhat pressured to stick a reviewed-by on this
since Daniel likes it.

Change the macro to WARN instead of DRM_ERROR, and, clear the IIR if
it's non-zero. With that change, it's:
Reviewed-by-with-reservations: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 08/20] drm/i915: check if IIR is still zero at postinstall on Gen5+

[Paulo Zanoni]

2014-03-19 14:50 GMT-03:00 Ben Widawsky <ben@bwidawsk.net>:
> On Wed, Mar 19, 2014 at 09:28:32AM +0100, Daniel Vetter wrote:
>> On Tue, Mar 18, 2014 at 11:20:09AM -0700, Ben Widawsky wrote:
>> > On Fri, Mar 07, 2014 at 08:10:24PM -0300, Paulo Zanoni wrote:
>> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > >
>> > > Instead of trying to clear it again. It should already be masked and
>> > > disabled and zeroed at preinstall/uninstall.
>> > >
>> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/i915_irq.c | 32 +++++++++++++++-----------------
>> > >  1 file changed, 15 insertions(+), 17 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> > > index 6d4daf2..4d0a8b1 100644
>> > > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > > @@ -103,12 +103,24 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>> > >   I915_WRITE(type##IIR, 0xffffffff); \
>> > >  } while (0)
>> > >
>> > > +/*
>> > > + * We should clear IMR at preinstall/uninstall, and just check at postinstall.
>> > > + */
>> > > +#define GEN5_ASSERT_IIR_IS_ZERO(reg) do { \
>> > > + u32 val = I915_READ(reg); \
>> > > + if (val) \
>> > > +         DRM_ERROR("Interrupt register 0x%x is not zero: 0x%08x\n", \
>> > > +                   (reg), val); \
>> > > +} while (0)
>> > > +
>> > >  #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \
>> > > + GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
>> > >   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 { \
>> > > + GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
>> > >   I915_WRITE(type##IMR, (imr_val)); \
>> > >   I915_WRITE(type##IER, (ier_val)); \
>> > >  } while (0)
>> >
>> > Okay, this is replacing a POSTED_WRITE, with a (slower) POSTING_READ
>> > which gives an error that we can do nothing about other than clear it
>> > anyway.
>> >
>> > I'd be in favor of dropping this patch.
>>
>> The point of the assert is to make sure that the new IIR clearing logic
>> with blocking everything+clearing in the preinstall hook actually does
>> what it's supposed to do.
>>
>> Since the point of this exercise is to reuse this code for runtime
>> suspend/resume where races are much easier to hit I think this is a good
>> self-check of the code.

Just an additional comment: the current code just writes to IIR once
in case it's not zero, and this is not a guarantee that it will be
cleared. So if we decided to drop this patch, I would suggest to fix
the IIR-clearing code.

>> -Daniel
>>
>
> Okay, I am feeling somewhat pressured to stick a reviewed-by on this
> since Daniel likes it.
>
> Change the macro to WARN instead of DRM_ERROR, and, clear the IIR if
> it's non-zero. With that change, it's:
> Reviewed-by-with-reservations: Ben Widawsky <ben@bwidawsk.net>
>
> --
> 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

[PATCH 09/20] drm/i915: fix SERR_INT init/reset code

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The SERR_INT register is very similar to the other IIR registers, so
let's zero it at preinstall/uninstall and WARN for a non-zero value at
postinstall, just like we do with the other IIR registers. For this
one, there's no need to double-clear since it can't store more than
one interrupt.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4d0a8b1..d295624 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2811,6 +2811,10 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 		return;
 
 	GEN5_IRQ_RESET(SDE);
+
+	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
+		I915_WRITE(SERR_INT, 0xffffffff);
+
 	/*
 	 * SDEIER is also touched by the interrupt handler to work around missed
 	 * PCH interrupts. Hence we can't update it after the interrupt handler
@@ -2949,7 +2953,7 @@ static void ibx_irq_postinstall(struct drm_device *dev)
 	} else {
 		mask = SDE_GMBUS_CPT | SDE_AUX_MASK_CPT | SDE_ERROR_CPT;
 
-		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
+		GEN5_ASSERT_IIR_IS_ZERO(SERR_INT);
 	}
 
 	GEN5_ASSERT_IIR_IS_ZERO(SDEIIR);
@@ -3303,7 +3307,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	GEN5_IRQ_RESET(SDE);
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
-		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
+		I915_WRITE(SERR_INT, 0xffffffff);
 }
 
 static void i8xx_irq_preinstall(struct drm_device * dev)
-- 
1.8.5.3

Re: [PATCH 09/20] drm/i915: fix SERR_INT init/reset code

[Ben Widawsky]

On Fri, Mar 07, 2014 at 08:10:25PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The SERR_INT register is very similar to the other IIR registers, so
> let's zero it at preinstall/uninstall and WARN for a non-zero value at
> postinstall, just like we do with the other IIR registers. For this
> one, there's no need to double-clear since it can't store more than
> one interrupt.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Without the assert that I don't like, this is
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4d0a8b1..d295624 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2811,6 +2811,10 @@ static void ibx_irq_preinstall(struct drm_device *dev)
>  		return;
>  
>  	GEN5_IRQ_RESET(SDE);
> +
> +	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
> +		I915_WRITE(SERR_INT, 0xffffffff);
> +
>  	/*
>  	 * SDEIER is also touched by the interrupt handler to work around missed
>  	 * PCH interrupts. Hence we can't update it after the interrupt handler
> @@ -2949,7 +2953,7 @@ static void ibx_irq_postinstall(struct drm_device *dev)
>  	} else {
>  		mask = SDE_GMBUS_CPT | SDE_AUX_MASK_CPT | SDE_ERROR_CPT;
>  
> -		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
> +		GEN5_ASSERT_IIR_IS_ZERO(SERR_INT);
>  	}
>  
>  	GEN5_ASSERT_IIR_IS_ZERO(SDEIIR);
> @@ -3303,7 +3307,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  
>  	GEN5_IRQ_RESET(SDE);
>  	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
> -		I915_WRITE(SERR_INT, I915_READ(SERR_INT));
> +		I915_WRITE(SERR_INT, 0xffffffff);
>  }
>  
>  static void i8xx_irq_preinstall(struct drm_device * 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

[PATCH 10/20] drm/i915: fix GEN7_ERR_INT init/reset code

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Same as SERR_INT and the other IIR registers: reset on
preinstall/uninstall and WARN for non-zero values at postinstall. This
one also doesn't need double-clear.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d295624..02eb493 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2845,6 +2845,9 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	GEN5_IRQ_RESET(DE);
 
+	if (IS_GEN7(dev))
+		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
+
 	gen5_gt_irq_preinstall(dev);
 
 	ibx_irq_preinstall(dev);
@@ -3011,7 +3014,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 		extra_mask = (DE_PIPEC_VBLANK_IVB | DE_PIPEB_VBLANK_IVB |
 			      DE_PIPEA_VBLANK_IVB);
 
-		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
+		GEN5_ASSERT_IIR_IS_ZERO(GEN7_ERR_INT);
 	} else {
 		display_mask = (DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
 				DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
@@ -3295,7 +3298,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	GEN5_IRQ_RESET(DE);
 	if (IS_GEN7(dev))
-		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
+		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
 
 	GEN5_IRQ_RESET(GT);
 	if (INTEL_INFO(dev)->gen >= 6)
-- 
1.8.5.3

Re: [PATCH 10/20] drm/i915: fix GEN7_ERR_INT init/reset code

[Ben Widawsky]

On Fri, Mar 07, 2014 at 08:10:26PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Same as SERR_INT and the other IIR registers: reset on
> preinstall/uninstall and WARN for non-zero values at postinstall. This
> one also doesn't need double-clear.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

This one just like patch 9 is:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Like that, I'd prefer to get rid of the IIR assertion

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d295624..02eb493 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2845,6 +2845,9 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
>  
>  	GEN5_IRQ_RESET(DE);
>  
> +	if (IS_GEN7(dev))
> +		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
> +
>  	gen5_gt_irq_preinstall(dev);
>  
>  	ibx_irq_preinstall(dev);
> @@ -3011,7 +3014,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  		extra_mask = (DE_PIPEC_VBLANK_IVB | DE_PIPEB_VBLANK_IVB |
>  			      DE_PIPEA_VBLANK_IVB);
>  
> -		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
> +		GEN5_ASSERT_IIR_IS_ZERO(GEN7_ERR_INT);
>  	} else {
>  		display_mask = (DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
>  				DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
> @@ -3295,7 +3298,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>  
>  	GEN5_IRQ_RESET(DE);
>  	if (IS_GEN7(dev))
> -		I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
> +		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
>  
>  	GEN5_IRQ_RESET(GT);
>  	if (INTEL_INFO(dev)->gen >= 6)
> -- 
> 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

[PATCH 11/20] drm/i915: fix open coded gen5_gt_irq_preinstall

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The duplicate was at an _uninstall function, so rename it to
gen5_gt_irq_reset.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 02eb493..b162ddd 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2825,7 +2825,7 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 	POSTING_READ(SDEIER);
 }
 
-static void gen5_gt_irq_preinstall(struct drm_device *dev)
+static void gen5_gt_irq_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -2848,7 +2848,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
 
-	gen5_gt_irq_preinstall(dev);
+	gen5_gt_irq_reset(dev);
 
 	ibx_irq_preinstall(dev);
 }
@@ -2868,7 +2868,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
 	I915_WRITE(GTIIR, I915_READ(GTIIR));
 
-	gen5_gt_irq_preinstall(dev);
+	gen5_gt_irq_reset(dev);
 
 	I915_WRITE(DPINVGTT, 0xff);
 
@@ -3300,10 +3300,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
 
-	GEN5_IRQ_RESET(GT);
-	if (INTEL_INFO(dev)->gen >= 6)
-		GEN5_IRQ_RESET(GEN6_PM);
-	POSTING_READ(GTIIR);
+	gen5_gt_irq_reset(dev);
 
 	if (HAS_PCH_NOP(dev))
 		return;
-- 
1.8.5.3

[PATCH 12/20] drm/i915: extract ibx_irq_uninstall

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Just like ibx_irq_preinstall. We'll call this from somewhere else in
the next patch.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b162ddd..ff0c63d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2844,7 +2844,6 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 	I915_WRITE(HWSTAM, 0xeffe);
 
 	GEN5_IRQ_RESET(DE);
-
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
 
@@ -3229,6 +3228,19 @@ static int gen8_irq_postinstall(struct drm_device *dev)
 	return 0;
 }
 
+static void ibx_irq_uninstall(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (HAS_PCH_NOP(dev))
+		return;
+
+	GEN5_IRQ_RESET(SDE);
+
+	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
+		I915_WRITE(SERR_INT, 0xffffffff);
+}
+
 static void gen8_irq_uninstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3302,12 +3314,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	gen5_gt_irq_reset(dev);
 
-	if (HAS_PCH_NOP(dev))
-		return;
-
-	GEN5_IRQ_RESET(SDE);
-	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
-		I915_WRITE(SERR_INT, 0xffffffff);
+	ibx_irq_uninstall(dev);
 }
 
 static void i8xx_irq_preinstall(struct drm_device * dev)
-- 
1.8.5.3

[PATCH 13/20] drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

After all, we call ibx_irq_preinstall from gen8_irq_preinstall.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ff0c63d..95f535b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3264,6 +3264,8 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 	GEN5_IRQ_RESET(GEN8_PCU_);
 
 	POSTING_READ(GEN8_PCU_IIR);
+
+	ibx_irq_uninstall(dev);
 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)
-- 
1.8.5.3

[PATCH 14/20] drm/i915: enable SDEIER later

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

On the preinstall stage we should just disable all the interrupts, but
we currently enable all the south display interrupts due to the way we
touch SDEIER at the IRQ handlers (note: they are still masked and our
IRQ handler is disabled). Instead of doing that, let's make the
preinstall stage just disable all the south interrupts, and do the
proper interrupt dance/ordering at the postinstall stage, including an
assert to check if everything is behaving as expected.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 95f535b..4479e29 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2814,13 +2814,24 @@ static void ibx_irq_preinstall(struct drm_device *dev)
 
 	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
 		I915_WRITE(SERR_INT, 0xffffffff);
+}
 
-	/*
-	 * SDEIER is also touched by the interrupt handler to work around missed
-	 * PCH interrupts. Hence we can't update it after the interrupt handler
-	 * is enabled - instead we unconditionally enable all PCH interrupt
-	 * sources here, but then only unmask them as needed with SDEIMR.
-	 */
+/*
+ * SDEIER is also touched by the interrupt handler to work around missed PCH
+ * interrupts. Hence we can't update it after the interrupt handler is enabled -
+ * instead we unconditionally enable all PCH interrupt sources here, but then
+ * only unmask them as needed with SDEIMR.
+ *
+ * This function needs to be called before interrupts are enabled.
+ */
+static void ibx_irq_pre_postinstall(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (HAS_PCH_NOP(dev))
+		return;
+
+	WARN_ON(I915_READ(SDEIER) != 0);
 	I915_WRITE(SDEIER, 0xffffffff);
 	POSTING_READ(SDEIER);
 }
@@ -3026,6 +3037,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 
 	dev_priv->irq_mask = ~display_mask;
 
+	ibx_irq_pre_postinstall(dev);
+
 	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
 
 	gen5_gt_irq_postinstall(dev);
@@ -3217,6 +3230,8 @@ static int gen8_irq_postinstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	ibx_irq_pre_postinstall(dev);
+
 	gen8_gt_irq_postinstall(dev_priv);
 	gen8_de_irq_postinstall(dev_priv);
 
-- 
1.8.5.3

Re: [PATCH 14/20] drm/i915: enable SDEIER later

[Ben Widawsky]

On Fri, Mar 07, 2014 at 08:10:30PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> On the preinstall stage we should just disable all the interrupts, but
> we currently enable all the south display interrupts due to the way we
> touch SDEIER at the IRQ handlers (note: they are still masked and our
> IRQ handler is disabled).

I think this statement is false. The interrupt is enabled right after
preinstall(). For the nomodeset case, this actually seems to make some
difference. It still looks fine to me though.

> Instead of doing that, let's make the
> preinstall stage just disable all the south interrupts, and do the
> proper interrupt dance/ordering at the postinstall stage, including an
> assert to check if everything is behaving as expected.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 95f535b..4479e29 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2814,13 +2814,24 @@ static void ibx_irq_preinstall(struct drm_device *dev)
>  
>  	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
>  		I915_WRITE(SERR_INT, 0xffffffff);
> +}
>  
> -	/*
> -	 * SDEIER is also touched by the interrupt handler to work around missed
> -	 * PCH interrupts. Hence we can't update it after the interrupt handler
> -	 * is enabled - instead we unconditionally enable all PCH interrupt
> -	 * sources here, but then only unmask them as needed with SDEIMR.
> -	 */
> +/*
> + * SDEIER is also touched by the interrupt handler to work around missed PCH
> + * interrupts. Hence we can't update it after the interrupt handler is enabled -
> + * instead we unconditionally enable all PCH interrupt sources here, but then
> + * only unmask them as needed with SDEIMR.
> + *
> + * This function needs to be called before interrupts are enabled.
> + */
> +static void ibx_irq_pre_postinstall(struct drm_device *dev)

sde_irq_postinstall()?

> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (HAS_PCH_NOP(dev))
> +		return;
> +
> +	WARN_ON(I915_READ(SDEIER) != 0);
>  	I915_WRITE(SDEIER, 0xffffffff);
>  	POSTING_READ(SDEIER);
>  }
> @@ -3026,6 +3037,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>  
>  	dev_priv->irq_mask = ~display_mask;
>  
> +	ibx_irq_pre_postinstall(dev);
> +
>  	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
>  
>  	gen5_gt_irq_postinstall(dev);
> @@ -3217,6 +3230,8 @@ static int gen8_irq_postinstall(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	ibx_irq_pre_postinstall(dev);
> +
>  	gen8_gt_irq_postinstall(dev_priv);
>  	gen8_de_irq_postinstall(dev_priv);
>  
> -- 
> 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

Re: [PATCH 14/20] drm/i915: enable SDEIER later

[Paulo Zanoni]

2014-03-18 17:29 GMT-03:00 Ben Widawsky <ben@bwidawsk.net>:
> On Fri, Mar 07, 2014 at 08:10:30PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> On the preinstall stage we should just disable all the interrupts, but
>> we currently enable all the south display interrupts due to the way we
>> touch SDEIER at the IRQ handlers (note: they are still masked and our
>> IRQ handler is disabled).
>
> I think this statement is false. The interrupt is enabled right after
> preinstall(). For the nomodeset case, this actually seems to make some
> difference. It still looks fine to me though.

Could you please clarify this paragraph?

We currently enable SDEIER at ibx_irq_preinstall, but the reason why
we do this is because we change SDEIER at the IRQ handler. So if we
move that code from preinstall to postinstall, but at a point where no
interrupts are enabled yet, we should be safe, and this is what the
patch does.

>
>> Instead of doing that, let's make the
>> preinstall stage just disable all the south interrupts, and do the
>> proper interrupt dance/ordering at the postinstall stage, including an
>> assert to check if everything is behaving as expected.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++++++------
>>  1 file changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 95f535b..4479e29 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -2814,13 +2814,24 @@ static void ibx_irq_preinstall(struct drm_device *dev)
>>
>>       if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
>>               I915_WRITE(SERR_INT, 0xffffffff);
>> +}
>>
>> -     /*
>> -      * SDEIER is also touched by the interrupt handler to work around missed
>> -      * PCH interrupts. Hence we can't update it after the interrupt handler
>> -      * is enabled - instead we unconditionally enable all PCH interrupt
>> -      * sources here, but then only unmask them as needed with SDEIMR.
>> -      */
>> +/*
>> + * SDEIER is also touched by the interrupt handler to work around missed PCH
>> + * interrupts. Hence we can't update it after the interrupt handler is enabled -
>> + * instead we unconditionally enable all PCH interrupt sources here, but then
>> + * only unmask them as needed with SDEIMR.
>> + *
>> + * This function needs to be called before interrupts are enabled.
>> + */
>> +static void ibx_irq_pre_postinstall(struct drm_device *dev)
>
> sde_irq_postinstall()?

I agree the current name is bad, but we use the IBX naming for
everything on the PCH at i915_irq.c, and ironlake_irq_postinstall()
already calls a function named ibx_irq_postinstall(), so I needed a
name that means "call this just before the postinstall() steps". If we
rename it to sde_irq_postinstall we may confuse users due to the fact
that we also have ibx_irq_postinstall. So with the current patch, we
have this:

void ironlake_irq_postinstall()
{
    /* We have to call this before enabling the interrupts */
    ibx_irq_pre_postinstall();
    /* Enable the interrupts */
    GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
    /* Now do the necessary postinstall adjustments to the PCH stuff */
    ibx_irq_postinstall();
}

But I'm still open to new naming suggestions.

>
>> +{
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     if (HAS_PCH_NOP(dev))
>> +             return;
>> +
>> +     WARN_ON(I915_READ(SDEIER) != 0);
>>       I915_WRITE(SDEIER, 0xffffffff);
>>       POSTING_READ(SDEIER);
>>  }
>> @@ -3026,6 +3037,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>>
>>       dev_priv->irq_mask = ~display_mask;
>>
>> +     ibx_irq_pre_postinstall(dev);
>> +
>>       GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
>>
>>       gen5_gt_irq_postinstall(dev);
>> @@ -3217,6 +3230,8 @@ static int gen8_irq_postinstall(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> +     ibx_irq_pre_postinstall(dev);
>> +
>>       gen8_gt_irq_postinstall(dev_priv);
>>       gen8_de_irq_postinstall(dev_priv);
>>
>> --
>> 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



-- 
Paulo Zanoni

Re: [PATCH 14/20] drm/i915: enable SDEIER later

[Ben Widawsky]

On Thu, Mar 27, 2014 at 11:39:36AM -0300, Paulo Zanoni wrote:
> 2014-03-18 17:29 GMT-03:00 Ben Widawsky <ben@bwidawsk.net>:
> > On Fri, Mar 07, 2014 at 08:10:30PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> On the preinstall stage we should just disable all the interrupts, but
> >> we currently enable all the south display interrupts due to the way we
> >> touch SDEIER at the IRQ handlers (note: they are still masked and our
> >> IRQ handler is disabled).
> >
> > I think this statement is false. The interrupt is enabled right after
> > preinstall(). For the nomodeset case, this actually seems to make some
> > difference. It still looks fine to me though.
> 
> Could you please clarify this paragraph?

The point was, "IRQ handler is disabled" is false. At least when I last
checked. We've registered the interrupt by then, which means we can
potentially get interrupts from the hardware.

I think we just might disagree on what "enabled" means. Perhaps this is
due to me working for too long with buggy hardware. In any event, as I
said, it does look fine to me as is.

> 
> We currently enable SDEIER at ibx_irq_preinstall, but the reason why
> we do this is because we change SDEIER at the IRQ handler. So if we
> move that code from preinstall to postinstall, but at a point where no
> interrupts are enabled yet, we should be safe, and this is what the
> patch does.
> 
> >
> >> Instead of doing that, let's make the
> >> preinstall stage just disable all the south interrupts, and do the
> >> proper interrupt dance/ordering at the postinstall stage, including an
> >> assert to check if everything is behaving as expected.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_irq.c | 27 +++++++++++++++++++++------
> >>  1 file changed, 21 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index 95f535b..4479e29 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -2814,13 +2814,24 @@ static void ibx_irq_preinstall(struct drm_device *dev)
> >>
> >>       if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
> >>               I915_WRITE(SERR_INT, 0xffffffff);
> >> +}
> >>
> >> -     /*
> >> -      * SDEIER is also touched by the interrupt handler to work around missed
> >> -      * PCH interrupts. Hence we can't update it after the interrupt handler
> >> -      * is enabled - instead we unconditionally enable all PCH interrupt
> >> -      * sources here, but then only unmask them as needed with SDEIMR.
> >> -      */
> >> +/*
> >> + * SDEIER is also touched by the interrupt handler to work around missed PCH
> >> + * interrupts. Hence we can't update it after the interrupt handler is enabled -
> >> + * instead we unconditionally enable all PCH interrupt sources here, but then
> >> + * only unmask them as needed with SDEIMR.
> >> + *
> >> + * This function needs to be called before interrupts are enabled.
> >> + */
> >> +static void ibx_irq_pre_postinstall(struct drm_device *dev)
> >
> > sde_irq_postinstall()?
> 
> I agree the current name is bad, but we use the IBX naming for
> everything on the PCH at i915_irq.c, and ironlake_irq_postinstall()
> already calls a function named ibx_irq_postinstall(), so I needed a
> name that means "call this just before the postinstall() steps". If we
> rename it to sde_irq_postinstall we may confuse users due to the fact
> that we also have ibx_irq_postinstall. So with the current patch, we
> have this:
> 
> void ironlake_irq_postinstall()
> {
>     /* We have to call this before enabling the interrupts */
>     ibx_irq_pre_postinstall();
>     /* Enable the interrupts */
>     GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
>     /* Now do the necessary postinstall adjustments to the PCH stuff */
>     ibx_irq_postinstall();
> }
> 
> But I'm still open to new naming suggestions.
> 

I gave my suggestion. If you and or the maintainer think it's not a
better one due to the existing scheme, then by all means, feel free to
move on. There's nothing functionally incorrect that I can spot.

> >
> >> +{
> >> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >> +
> >> +     if (HAS_PCH_NOP(dev))
> >> +             return;
> >> +
> >> +     WARN_ON(I915_READ(SDEIER) != 0);
> >>       I915_WRITE(SDEIER, 0xffffffff);
> >>       POSTING_READ(SDEIER);
> >>  }
> >> @@ -3026,6 +3037,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> >>
> >>       dev_priv->irq_mask = ~display_mask;
> >>
> >> +     ibx_irq_pre_postinstall(dev);
> >> +
> >>       GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
> >>
> >>       gen5_gt_irq_postinstall(dev);
> >> @@ -3217,6 +3230,8 @@ static int gen8_irq_postinstall(struct drm_device *dev)
> >>  {
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >>
> >> +     ibx_irq_pre_postinstall(dev);
> >> +
> >>       gen8_gt_irq_postinstall(dev_priv);
> >>       gen8_de_irq_postinstall(dev_priv);
> >>
> >> --
> >> 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
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 14/20] drm/i915: enable SDEIER later

[Daniel Vetter]

On Fri, Mar 28, 2014 at 7:20 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Thu, Mar 27, 2014 at 11:39:36AM -0300, Paulo Zanoni wrote:
>> 2014-03-18 17:29 GMT-03:00 Ben Widawsky <ben@bwidawsk.net>:
>> > On Fri, Mar 07, 2014 at 08:10:30PM -0300, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >>
>> >> On the preinstall stage we should just disable all the interrupts, but
>> >> we currently enable all the south display interrupts due to the way we
>> >> touch SDEIER at the IRQ handlers (note: they are still masked and our
>> >> IRQ handler is disabled).
>> >
>> > I think this statement is false. The interrupt is enabled right after
>> > preinstall(). For the nomodeset case, this actually seems to make some
>> > difference. It still looks fine to me though.
>>
>> Could you please clarify this paragraph?
>
> The point was, "IRQ handler is disabled" is false. At least when I last
> checked. We've registered the interrupt by then, which means we can
> potentially get interrupts from the hardware.
>
> I think we just might disagree on what "enabled" means. Perhaps this is
> due to me working for too long with buggy hardware. In any event, as I
> said, it does look fine to me as is.

I think a polished comment to explain that despite the irq handler
being register we shouldn't yet get interrupts and hence won't race is
justified here?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 15/20] drm/i915: remove ibx_irq_uninstall

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

After the latest changes, ibx_irq_preinstall and ibx_irq_uninstall are
the same, so remove one of the copies and rename the other to
ibx_irq_reset (since we're using the "reset" name for things which are
called both at preinstall and uninstall).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4479e29..3584a16d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2803,7 +2803,7 @@ void i915_queue_hangcheck(struct drm_device *dev)
 		  round_jiffies_up(jiffies + DRM_I915_HANGCHECK_JIFFIES));
 }
 
-static void ibx_irq_preinstall(struct drm_device *dev)
+static void ibx_irq_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -2860,7 +2860,7 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 
 	gen5_gt_irq_reset(dev);
 
-	ibx_irq_preinstall(dev);
+	ibx_irq_reset(dev);
 }
 
 static void valleyview_irq_preinstall(struct drm_device *dev)
@@ -2914,7 +2914,7 @@ static void gen8_irq_preinstall(struct drm_device *dev)
 	GEN5_IRQ_RESET(GEN8_PCU_);
 	POSTING_READ(GEN8_PCU_IIR);
 
-	ibx_irq_preinstall(dev);
+	ibx_irq_reset(dev);
 }
 
 static void ibx_hpd_irq_setup(struct drm_device *dev)
@@ -3243,19 +3243,6 @@ static int gen8_irq_postinstall(struct drm_device *dev)
 	return 0;
 }
 
-static void ibx_irq_uninstall(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (HAS_PCH_NOP(dev))
-		return;
-
-	GEN5_IRQ_RESET(SDE);
-
-	if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
-		I915_WRITE(SERR_INT, 0xffffffff);
-}
-
 static void gen8_irq_uninstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3280,7 +3267,7 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 
 	POSTING_READ(GEN8_PCU_IIR);
 
-	ibx_irq_uninstall(dev);
+	ibx_irq_reset(dev);
 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)
@@ -3331,7 +3318,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	gen5_gt_irq_reset(dev);
 
-	ibx_irq_uninstall(dev);
+	ibx_irq_reset(dev);
 }
 
 static void i8xx_irq_preinstall(struct drm_device * dev)
-- 
1.8.5.3

[PATCH 16/20] drm/i915: add missing intel_hpd_irq_uninstall

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Missing from gen8_irq_uninstall.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3584a16d..1e5cc5b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3251,6 +3251,8 @@ static void gen8_irq_uninstall(struct drm_device *dev)
 	if (!dev_priv)
 		return;
 
+	intel_hpd_irq_uninstall(dev_priv);
+
 	I915_WRITE(GEN8_MASTER_IRQ, 0);
 
 	GEN8_IRQ_RESET_NDX(GT, 0);
-- 
1.8.5.3

Re: [PATCH 16/20] drm/i915: add missing intel_hpd_irq_uninstall

[Ben Widawsky]

On Fri, Mar 07, 2014 at 08:10:32PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Missing from gen8_irq_uninstall.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 3584a16d..1e5cc5b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3251,6 +3251,8 @@ static void gen8_irq_uninstall(struct drm_device *dev)
>  	if (!dev_priv)
>  		return;
>  
> +	intel_hpd_irq_uninstall(dev_priv);
> +
>  	I915_WRITE(GEN8_MASTER_IRQ, 0);
>  
>  	GEN8_IRQ_RESET_NDX(GT, 0);
> -- 
> 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

[PATCH 17/20] drm/i915: add ironlake_irq_reset

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

To merge the common code of ironlake_irq_preinstall and
ironlake_irq_uninstall.

We should also probably do something about that HSWSTAM write on a
later commit.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1e5cc5b..4917a8c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2848,11 +2848,9 @@ static void gen5_gt_irq_reset(struct drm_device *dev)
 
 /* drm_dma.h hooks
 */
-static void ironlake_irq_preinstall(struct drm_device *dev)
+static void ironlake_irq_reset(struct drm_device *dev)
 {
-	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-
-	I915_WRITE(HWSTAM, 0xeffe);
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	GEN5_IRQ_RESET(DE);
 	if (IS_GEN7(dev))
@@ -2863,6 +2861,15 @@ static void ironlake_irq_preinstall(struct drm_device *dev)
 	ibx_irq_reset(dev);
 }
 
+static void ironlake_irq_preinstall(struct drm_device *dev)
+{
+	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
+
+	I915_WRITE(HWSTAM, 0xeffe);
+
+	ironlake_irq_reset(dev);
+}
+
 static void valleyview_irq_preinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -3314,13 +3321,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	I915_WRITE(HWSTAM, 0xffffffff);
 
-	GEN5_IRQ_RESET(DE);
-	if (IS_GEN7(dev))
-		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
-
-	gen5_gt_irq_reset(dev);
-
-	ibx_irq_reset(dev);
+	ironlake_irq_reset(dev);
 }
 
 static void i8xx_irq_preinstall(struct drm_device * dev)
-- 
1.8.5.3

[PATCH 18/20] drm/i915: add gen8_irq_reset

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

So we can merge all the common code from postinstall and uninstall.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4917a8c..d6723ab 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2899,7 +2899,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
 	POSTING_READ(VLV_IER);
 }
 
-static void gen8_irq_preinstall(struct drm_device *dev)
+static void gen8_irq_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipe;
@@ -2924,6 +2924,11 @@ static void gen8_irq_preinstall(struct drm_device *dev)
 	ibx_irq_reset(dev);
 }
 
+static void gen8_irq_preinstall(struct drm_device *dev)
+{
+	gen8_irq_reset(dev);
+}
+
 static void ibx_hpd_irq_setup(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -3253,30 +3258,13 @@ static int gen8_irq_postinstall(struct drm_device *dev)
 static void gen8_irq_uninstall(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int pipe;
 
 	if (!dev_priv)
 		return;
 
 	intel_hpd_irq_uninstall(dev_priv);
 
-	I915_WRITE(GEN8_MASTER_IRQ, 0);
-
-	GEN8_IRQ_RESET_NDX(GT, 0);
-	GEN8_IRQ_RESET_NDX(GT, 1);
-	GEN8_IRQ_RESET_NDX(GT, 2);
-	GEN8_IRQ_RESET_NDX(GT, 3);
-
-	for_each_pipe(pipe)
-		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
-
-	GEN5_IRQ_RESET(GEN8_DE_PORT_);
-	GEN5_IRQ_RESET(GEN8_DE_MISC_);
-	GEN5_IRQ_RESET(GEN8_PCU_);
-
-	POSTING_READ(GEN8_PCU_IIR);
-
-	ibx_irq_reset(dev);
+	gen8_irq_reset(dev);
 }
 
 static void valleyview_irq_uninstall(struct drm_device *dev)
-- 
1.8.5.3

Re: [PATCH 18/20] drm/i915: add gen8_irq_reset

[Ben Widawsky]

On Fri, Mar 07, 2014 at 08:10:34PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> So we can merge all the common code from postinstall and uninstall.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 26 +++++++-------------------
>  1 file changed, 7 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4917a8c..d6723ab 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2899,7 +2899,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
>  	POSTING_READ(VLV_IER);
>  }
>  
> -static void gen8_irq_preinstall(struct drm_device *dev)
> +static void gen8_irq_reset(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int pipe;
> @@ -2924,6 +2924,11 @@ static void gen8_irq_preinstall(struct drm_device *dev)
>  	ibx_irq_reset(dev);
>  }
>  
> +static void gen8_irq_preinstall(struct drm_device *dev)
> +{
> +	gen8_irq_reset(dev);
> +}
> +
>  static void ibx_hpd_irq_setup(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> @@ -3253,30 +3258,13 @@ static int gen8_irq_postinstall(struct drm_device *dev)
>  static void gen8_irq_uninstall(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int pipe;
>  
>  	if (!dev_priv)
>  		return;
>  
>  	intel_hpd_irq_uninstall(dev_priv);
>  
> -	I915_WRITE(GEN8_MASTER_IRQ, 0);
> -
> -	GEN8_IRQ_RESET_NDX(GT, 0);
> -	GEN8_IRQ_RESET_NDX(GT, 1);
> -	GEN8_IRQ_RESET_NDX(GT, 2);
> -	GEN8_IRQ_RESET_NDX(GT, 3);
> -
> -	for_each_pipe(pipe)
> -		GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
> -
> -	GEN5_IRQ_RESET(GEN8_DE_PORT_);
> -	GEN5_IRQ_RESET(GEN8_DE_MISC_);
> -	GEN5_IRQ_RESET(GEN8_PCU_);
> -
> -	POSTING_READ(GEN8_PCU_IIR);
> -
> -	ibx_irq_reset(dev);
> +	gen8_irq_reset(dev);

BTW: This looks like a bad hunk. I've merged up to this point, and I do
not have ibx_irq_reset().


>  }
>  
>  static void valleyview_irq_uninstall(struct drm_device *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

Re: [PATCH 18/20] drm/i915: add gen8_irq_reset

[Paulo Zanoni]

2014-03-18 17:43 GMT-03:00 Ben Widawsky <ben@bwidawsk.net>:
> On Fri, Mar 07, 2014 at 08:10:34PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> So we can merge all the common code from postinstall and uninstall.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 26 +++++++-------------------
>>  1 file changed, 7 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 4917a8c..d6723ab 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -2899,7 +2899,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev)
>>       POSTING_READ(VLV_IER);
>>  }
>>
>> -static void gen8_irq_preinstall(struct drm_device *dev)
>> +static void gen8_irq_reset(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       int pipe;
>> @@ -2924,6 +2924,11 @@ static void gen8_irq_preinstall(struct drm_device *dev)
>>       ibx_irq_reset(dev);
>>  }
>>
>> +static void gen8_irq_preinstall(struct drm_device *dev)
>> +{
>> +     gen8_irq_reset(dev);
>> +}
>> +
>>  static void ibx_hpd_irq_setup(struct drm_device *dev)
>>  {
>>       drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>> @@ -3253,30 +3258,13 @@ static int gen8_irq_postinstall(struct drm_device *dev)
>>  static void gen8_irq_uninstall(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>> -     int pipe;
>>
>>       if (!dev_priv)
>>               return;
>>
>>       intel_hpd_irq_uninstall(dev_priv);
>>
>> -     I915_WRITE(GEN8_MASTER_IRQ, 0);
>> -
>> -     GEN8_IRQ_RESET_NDX(GT, 0);
>> -     GEN8_IRQ_RESET_NDX(GT, 1);
>> -     GEN8_IRQ_RESET_NDX(GT, 2);
>> -     GEN8_IRQ_RESET_NDX(GT, 3);
>> -
>> -     for_each_pipe(pipe)
>> -             GEN8_IRQ_RESET_NDX(DE_PIPE, pipe);
>> -
>> -     GEN5_IRQ_RESET(GEN8_DE_PORT_);
>> -     GEN5_IRQ_RESET(GEN8_DE_MISC_);
>> -     GEN5_IRQ_RESET(GEN8_PCU_);
>> -
>> -     POSTING_READ(GEN8_PCU_IIR);
>> -
>> -     ibx_irq_reset(dev);
>> +     gen8_irq_reset(dev);
>
> BTW: This looks like a bad hunk. I've merged up to this point, and I do
> not have ibx_irq_reset().

It was introduced by patch 15.

>
>
>>  }
>>
>>  static void valleyview_irq_uninstall(struct drm_device *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



-- 
Paulo Zanoni

[PATCH 19/20] drm/i915: only enable HWSTAM interrupts on postinstall on ILK+

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We should only enable interrupts at postinstall.

And now on ILK/SNB/IVB/HSW the irq_preinstall and irq_postinstall
functions leave the hardware in the same state.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d6723ab..79a8196 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2852,6 +2852,8 @@ static void ironlake_irq_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	I915_WRITE(HWSTAM, 0xffffffff);
+
 	GEN5_IRQ_RESET(DE);
 	if (IS_GEN7(dev))
 		I915_WRITE(GEN7_ERR_INT, 0xffffffff);
@@ -2863,10 +2865,6 @@ static void ironlake_irq_reset(struct drm_device *dev)
 
 static void ironlake_irq_preinstall(struct drm_device *dev)
 {
-	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-
-	I915_WRITE(HWSTAM, 0xeffe);
-
 	ironlake_irq_reset(dev);
 }
 
@@ -3049,6 +3047,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 
 	dev_priv->irq_mask = ~display_mask;
 
+	I915_WRITE(HWSTAM, 0xeffe);
+
 	ibx_irq_pre_postinstall(dev);
 
 	GEN5_IRQ_INIT(DE, dev_priv->irq_mask, display_mask | extra_mask);
@@ -3307,8 +3307,6 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
 
 	intel_hpd_irq_uninstall(dev_priv);
 
-	I915_WRITE(HWSTAM, 0xffffffff);
-
 	ironlake_irq_reset(dev);
 }
 
-- 
1.8.5.3

[PATCH 20/20] drm/i915: add POSTING_READs to the IRQ init/reset macros

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

I previously chose to keep the POSTING_READ calls as something to be
done by the macro callers, but the conclusion after discussing this on
the mailing list is that leaving the POSTING_READ calls to the macros
makes the code safer, and the additional useless register reads
shouldn't be noticeable. So move the POSTING_READ calls to the
callers.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 79a8196..dee3a3b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -80,11 +80,7 @@ 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.
- * */
+/* IIR can theoretically queue up two events. Be paranoid. */
 #define GEN8_IRQ_RESET_NDX(type, which) do { \
 	I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
 	POSTING_READ(GEN8_##type##_IMR(which)); \
@@ -92,6 +88,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
 	POSTING_READ(GEN8_##type##_IIR(which)); \
 	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
+	POSTING_READ(GEN8_##type##_IIR(which)); \
 } while (0)
 
 #define GEN5_IRQ_RESET(type) do { \
@@ -101,6 +98,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	I915_WRITE(type##IIR, 0xffffffff); \
 	POSTING_READ(type##IIR); \
 	I915_WRITE(type##IIR, 0xffffffff); \
+	POSTING_READ(type##IIR); \
 } while (0)
 
 /*
@@ -117,12 +115,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
 	GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
 	I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
 	I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \
+	POSTING_READ(GEN8_##type##_IER(which)); \
 } while (0)
 
 #define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \
 	GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
 	I915_WRITE(type##IMR, (imr_val)); \
 	I915_WRITE(type##IER, (ier_val)); \
+	POSTING_READ(type##IER); \
 } while (0)
 
 /* For display hotplug interrupt */
@@ -2843,7 +2843,6 @@ static void gen5_gt_irq_reset(struct drm_device *dev)
 	GEN5_IRQ_RESET(GT);
 	if (INTEL_INFO(dev)->gen >= 6)
 		GEN5_IRQ_RESET(GEN6_PM);
-	POSTING_READ(GTIIR);
 }
 
 /* drm_dma.h hooks
@@ -2917,7 +2916,6 @@ static void gen8_irq_reset(struct drm_device *dev)
 	GEN5_IRQ_RESET(GEN8_DE_PORT_);
 	GEN5_IRQ_RESET(GEN8_DE_MISC_);
 	GEN5_IRQ_RESET(GEN8_PCU_);
-	POSTING_READ(GEN8_PCU_IIR);
 
 	ibx_irq_reset(dev);
 }
@@ -3016,7 +3014,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
 		dev_priv->pm_irq_mask = 0xffffffff;
 		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_irq_mask, pm_irqs);
 	}
-	POSTING_READ(GTIER);
 }
 
 static int ironlake_irq_postinstall(struct drm_device *dev)
@@ -3213,7 +3210,6 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
 
 	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++)
 		GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]);
-	POSTING_READ(GEN8_GT_IER(0));
 }
 
 static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
@@ -3232,10 +3228,8 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 	for_each_pipe(pipe)
 		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
 				  de_pipe_enables);
-	POSTING_READ(GEN8_DE_PIPE_ISR(0));
 
 	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
-	POSTING_READ(GEN8_DE_PORT_IER);
 }
 
 static int gen8_irq_postinstall(struct drm_device *dev)
-- 
1.8.5.3

Re: [PATCH 20/20] drm/i915: add POSTING_READs to the IRQ init/reset macros

[Ben Widawsky]

On Fri, Mar 07, 2014 at 08:10:36PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I previously chose to keep the POSTING_READ calls as something to be
> done by the macro callers, but the conclusion after discussing this on
> the mailing list is that leaving the POSTING_READ calls to the macros
> makes the code safer, and the additional useless register reads
> shouldn't be noticeable. So move the POSTING_READ calls to the
> callers.

Can you just squash this into the earlier patch? Either way, 
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 79a8196..dee3a3b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -80,11 +80,7 @@ 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.
> - * */
> +/* IIR can theoretically queue up two events. Be paranoid. */
>  #define GEN8_IRQ_RESET_NDX(type, which) do { \
>  	I915_WRITE(GEN8_##type##_IMR(which), 0xffffffff); \
>  	POSTING_READ(GEN8_##type##_IMR(which)); \
> @@ -92,6 +88,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
>  	POSTING_READ(GEN8_##type##_IIR(which)); \
>  	I915_WRITE(GEN8_##type##_IIR(which), 0xffffffff); \
> +	POSTING_READ(GEN8_##type##_IIR(which)); \
>  } while (0)
>  
>  #define GEN5_IRQ_RESET(type) do { \
> @@ -101,6 +98,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  	I915_WRITE(type##IIR, 0xffffffff); \
>  	POSTING_READ(type##IIR); \
>  	I915_WRITE(type##IIR, 0xffffffff); \
> +	POSTING_READ(type##IIR); \
>  } while (0)
>  
>  /*
> @@ -117,12 +115,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */
>  	GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \
>  	I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \
>  	I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \
> +	POSTING_READ(GEN8_##type##_IER(which)); \
>  } while (0)
>  
>  #define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \
>  	GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \
>  	I915_WRITE(type##IMR, (imr_val)); \
>  	I915_WRITE(type##IER, (ier_val)); \
> +	POSTING_READ(type##IER); \
>  } while (0)
>  
>  /* For display hotplug interrupt */
> @@ -2843,7 +2843,6 @@ static void gen5_gt_irq_reset(struct drm_device *dev)
>  	GEN5_IRQ_RESET(GT);
>  	if (INTEL_INFO(dev)->gen >= 6)
>  		GEN5_IRQ_RESET(GEN6_PM);
> -	POSTING_READ(GTIIR);
>  }
>  
>  /* drm_dma.h hooks
> @@ -2917,7 +2916,6 @@ static void gen8_irq_reset(struct drm_device *dev)
>  	GEN5_IRQ_RESET(GEN8_DE_PORT_);
>  	GEN5_IRQ_RESET(GEN8_DE_MISC_);
>  	GEN5_IRQ_RESET(GEN8_PCU_);
> -	POSTING_READ(GEN8_PCU_IIR);
>  
>  	ibx_irq_reset(dev);
>  }
> @@ -3016,7 +3014,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev)
>  		dev_priv->pm_irq_mask = 0xffffffff;
>  		GEN5_IRQ_INIT(GEN6_PM, dev_priv->pm_irq_mask, pm_irqs);
>  	}
> -	POSTING_READ(GTIER);
>  }
>  
>  static int ironlake_irq_postinstall(struct drm_device *dev)
> @@ -3213,7 +3210,6 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv)
>  
>  	for (i = 0; i < ARRAY_SIZE(gt_interrupts); i++)
>  		GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]);
> -	POSTING_READ(GEN8_GT_IER(0));
>  }
>  
>  static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
> @@ -3232,10 +3228,8 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  	for_each_pipe(pipe)
>  		GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv->de_irq_mask[pipe],
>  				  de_pipe_enables);
> -	POSTING_READ(GEN8_DE_PIPE_ISR(0));
>  
>  	GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A);
> -	POSTING_READ(GEN8_DE_PORT_IER);
>  }
>  
>  static int gen8_irq_postinstall(struct drm_device *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

Re: [PATCH 00/20] ILK+ interrupt improvements, v2

[Ben Widawsky]

On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Hi
> 
> This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which
> was sent to the mailing list on January 22. There are no real differences,
> except for the last patch, which is new.
> 
> Original cover letter:
> http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html
> 
> The idea behind this series is that at some point our runtime PM code will just
> call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of
> using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs
> to our code before we do that change. We gotta be in shape if we want to be
> exposed to runtime!
> 
> Thanks,
> Paulo
> 
> Paulo Zanoni (20):
>   drm/i915: add GEN5_IRQ_INIT macro
>   drm/i915: also use GEN5_IRQ_INIT with south display interrupts
>   drm/i915: use GEN8_IRQ_INIT on GEN5
>   drm/i915: add GEN5_IRQ_FINI
>   drm/i915: don't forget to uninstall the PM IRQs
>   drm/i915: properly clear IIR at irq_uninstall on Gen5+
>   drm/i915: add GEN5_IRQ_INIT
>   drm/i915: check if IIR is still zero at postinstall on Gen5+
>   drm/i915: fix SERR_INT init/reset code
>   drm/i915: fix GEN7_ERR_INT init/reset code
>   drm/i915: fix open coded gen5_gt_irq_preinstall
>   drm/i915: extract ibx_irq_uninstall
>   drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall
>   drm/i915: enable SDEIER later
>   drm/i915: remove ibx_irq_uninstall
>   drm/i915: add missing intel_hpd_irq_uninstall
>   drm/i915: add ironlake_irq_reset
>   drm/i915: add gen8_irq_reset
>   drm/i915: only enable HWSTAM interrupts on postinstall on ILK+
>   drm/i915: add POSTING_READs to the IRQ init/reset macros
> 
>  drivers/gpu/drm/i915/i915_irq.c | 270 ++++++++++++++++++----------------------
>  1 file changed, 121 insertions(+), 149 deletions(-)
> 

Okay, here is the summary of my review. At first I was complaining to
myself about how many patches you used to do a simple thing. But, I must
admit it made reviewing the thing a lot easier, and when I look back at
how much stuff you combined, I'm really glad you did it this way. I'm
sure I've missed something silly though, since every patch looks so
similar :P

1-5: Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (with possible comment
improvement on #3)

7: I don't like. Can we drop? I guess doing this would make a decent
amount of churn, so if you don't want to drop it, that's fine, and it's
functionally correct:
     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

8: I'd really like to drop this one.

9-10: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

12-13: I wouldn't mind cpt_irq_* rename, but either way
       Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

14: With the requested change in the mail:
    Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

16: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

20: Should be squashed, but
    Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which
seems to always mean "disable." I think disable makes the code so much
clearer, and would really love if you can apply this simple rename. With
the rename, they're:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 00/20] ILK+ interrupt improvements, v2

[Daniel Vetter]

On Tue, Mar 18, 2014 at 01:53:53PM -0700, Ben Widawsky wrote:
> On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Hi
> > 
> > This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which
> > was sent to the mailing list on January 22. There are no real differences,
> > except for the last patch, which is new.
> > 
> > Original cover letter:
> > http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html
> > 
> > The idea behind this series is that at some point our runtime PM code will just
> > call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of
> > using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs
> > to our code before we do that change. We gotta be in shape if we want to be
> > exposed to runtime!
> > 
> > Thanks,
> > Paulo
> > 
> > Paulo Zanoni (20):
> >   drm/i915: add GEN5_IRQ_INIT macro
> >   drm/i915: also use GEN5_IRQ_INIT with south display interrupts
> >   drm/i915: use GEN8_IRQ_INIT on GEN5
> >   drm/i915: add GEN5_IRQ_FINI
> >   drm/i915: don't forget to uninstall the PM IRQs
> >   drm/i915: properly clear IIR at irq_uninstall on Gen5+
> >   drm/i915: add GEN5_IRQ_INIT
> >   drm/i915: check if IIR is still zero at postinstall on Gen5+
> >   drm/i915: fix SERR_INT init/reset code
> >   drm/i915: fix GEN7_ERR_INT init/reset code
> >   drm/i915: fix open coded gen5_gt_irq_preinstall
> >   drm/i915: extract ibx_irq_uninstall
> >   drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall
> >   drm/i915: enable SDEIER later
> >   drm/i915: remove ibx_irq_uninstall
> >   drm/i915: add missing intel_hpd_irq_uninstall
> >   drm/i915: add ironlake_irq_reset
> >   drm/i915: add gen8_irq_reset
> >   drm/i915: only enable HWSTAM interrupts on postinstall on ILK+
> >   drm/i915: add POSTING_READs to the IRQ init/reset macros
> > 
> >  drivers/gpu/drm/i915/i915_irq.c | 270 ++++++++++++++++++----------------------
> >  1 file changed, 121 insertions(+), 149 deletions(-)
> > 
> 
> Okay, here is the summary of my review. At first I was complaining to
> myself about how many patches you used to do a simple thing. But, I must
> admit it made reviewing the thing a lot easier, and when I look back at
> how much stuff you combined, I'm really glad you did it this way. I'm
> sure I've missed something silly though, since every patch looks so
> similar :P
> 
> 1-5: Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (with possible comment
> improvement on #3)
> 
> 7: I don't like. Can we drop? I guess doing this would make a decent
> amount of churn, so if you don't want to drop it, that's fine, and it's
> functionally correct:
>      Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 8: I'd really like to drop this one.

Comment on this and I think with a pimped commit message this is good to
go imo. I really think the added self-checks are required to start using
this code for runtime pm.

> 9-10: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 12-13: I wouldn't mind cpt_irq_* rename, but either way
>        Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 14: With the requested change in the mail:
>     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 16: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 20: Should be squashed, but
>     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> 6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which
> seems to always mean "disable." I think disable makes the code so much
> clearer, and would really love if you can apply this simple rename. With
> the rename, they're:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Paulo's using "reset" functions/macros both in the preinstall hooks and in
the uninstall/disable code. We already use reset for stuff run before
init/enable code to get the hw in a state we expect it to, so I think
Paulo's naming choice is accurate and a plain "disable" more misleading.

I think you raise some good points in your review, and besides the 3 cases
I commented on I lack the detailed knowledge to avoid looking like a fool
;-) So I think I'll wait for Paulo's comments before pulling this all in.

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

Re: [PATCH 00/20] ILK+ interrupt improvements, v2

[Ben Widawsky]

On Wed, Mar 19, 2014 at 09:36:04AM +0100, Daniel Vetter wrote:
> On Tue, Mar 18, 2014 at 01:53:53PM -0700, Ben Widawsky wrote:
> > On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote:
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > Hi
> > > 
> > > This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which
> > > was sent to the mailing list on January 22. There are no real differences,
> > > except for the last patch, which is new.
> > > 
> > > Original cover letter:
> > > http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html
> > > 
> > > The idea behind this series is that at some point our runtime PM code will just
> > > call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of
> > > using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs
> > > to our code before we do that change. We gotta be in shape if we want to be
> > > exposed to runtime!
> > > 
> > > Thanks,
> > > Paulo
> > > 
> > > Paulo Zanoni (20):
> > >   drm/i915: add GEN5_IRQ_INIT macro
> > >   drm/i915: also use GEN5_IRQ_INIT with south display interrupts
> > >   drm/i915: use GEN8_IRQ_INIT on GEN5
> > >   drm/i915: add GEN5_IRQ_FINI
> > >   drm/i915: don't forget to uninstall the PM IRQs
> > >   drm/i915: properly clear IIR at irq_uninstall on Gen5+
> > >   drm/i915: add GEN5_IRQ_INIT
> > >   drm/i915: check if IIR is still zero at postinstall on Gen5+
> > >   drm/i915: fix SERR_INT init/reset code
> > >   drm/i915: fix GEN7_ERR_INT init/reset code
> > >   drm/i915: fix open coded gen5_gt_irq_preinstall
> > >   drm/i915: extract ibx_irq_uninstall
> > >   drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall
> > >   drm/i915: enable SDEIER later
> > >   drm/i915: remove ibx_irq_uninstall
> > >   drm/i915: add missing intel_hpd_irq_uninstall
> > >   drm/i915: add ironlake_irq_reset
> > >   drm/i915: add gen8_irq_reset
> > >   drm/i915: only enable HWSTAM interrupts on postinstall on ILK+
> > >   drm/i915: add POSTING_READs to the IRQ init/reset macros
> > > 
> > >  drivers/gpu/drm/i915/i915_irq.c | 270 ++++++++++++++++++----------------------
> > >  1 file changed, 121 insertions(+), 149 deletions(-)
> > > 
> > 
> > Okay, here is the summary of my review. At first I was complaining to
> > myself about how many patches you used to do a simple thing. But, I must
> > admit it made reviewing the thing a lot easier, and when I look back at
> > how much stuff you combined, I'm really glad you did it this way. I'm
> > sure I've missed something silly though, since every patch looks so
> > similar :P
> > 
> > 1-5: Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (with possible comment
> > improvement on #3)
> > 
> > 7: I don't like. Can we drop? I guess doing this would make a decent
> > amount of churn, so if you don't want to drop it, that's fine, and it's
> > functionally correct:
> >      Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > 8: I'd really like to drop this one.
> 
> Comment on this and I think with a pimped commit message this is good to
> go imo. I really think the added self-checks are required to start using
> this code for runtime pm.
> 

So you don't need my reviewed-by then. I don't like it...

> > 9-10: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > 12-13: I wouldn't mind cpt_irq_* rename, but either way
> >        Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > 14: With the requested change in the mail:
> >     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > 16: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > 20: Should be squashed, but
> >     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> > 
> > 6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which
> > seems to always mean "disable." I think disable makes the code so much
> > clearer, and would really love if you can apply this simple rename. With
> > the rename, they're:
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Paulo's using "reset" functions/macros both in the preinstall hooks and in
> the uninstall/disable code. We already use reset for stuff run before
> init/enable code to get the hw in a state we expect it to, so I think
> Paulo's naming choice is accurate and a plain "disable" more misleading.
> 

I cannot disagree more. Every time I read "reset" it confuses me. But it
seems like I am the minority.

> I think you raise some good points in your review, and besides the 3 cases
> I commented on I lack the detailed knowledge to avoid looking like a fool
> ;-) So I think I'll wait for Paulo's comments before pulling this all in.
> 
> Thanks,
> Daniel

Once Paulo responds, I'll make it a top priority to re-review whatever
is needed. Sorry for the original delay.

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

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 00/20] ILK+ interrupt improvements, v2

[Paulo Zanoni]

2014-03-19 14:25 GMT-03:00 Ben Widawsky <ben@bwidawsk.net>:
> On Wed, Mar 19, 2014 at 09:36:04AM +0100, Daniel Vetter wrote:
>> On Tue, Mar 18, 2014 at 01:53:53PM -0700, Ben Widawsky wrote:
>> > On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote:
>> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > >
>> > > Hi
>> > >
>> > > This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which
>> > > was sent to the mailing list on January 22. There are no real differences,
>> > > except for the last patch, which is new.
>> > >
>> > > Original cover letter:
>> > > http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html
>> > >
>> > > The idea behind this series is that at some point our runtime PM code will just
>> > > call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of
>> > > using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs
>> > > to our code before we do that change. We gotta be in shape if we want to be
>> > > exposed to runtime!
>> > >
>> > > Thanks,
>> > > Paulo
>> > >
>> > > Paulo Zanoni (20):
>> > >   drm/i915: add GEN5_IRQ_INIT macro
>> > >   drm/i915: also use GEN5_IRQ_INIT with south display interrupts
>> > >   drm/i915: use GEN8_IRQ_INIT on GEN5
>> > >   drm/i915: add GEN5_IRQ_FINI
>> > >   drm/i915: don't forget to uninstall the PM IRQs
>> > >   drm/i915: properly clear IIR at irq_uninstall on Gen5+
>> > >   drm/i915: add GEN5_IRQ_INIT
>> > >   drm/i915: check if IIR is still zero at postinstall on Gen5+
>> > >   drm/i915: fix SERR_INT init/reset code
>> > >   drm/i915: fix GEN7_ERR_INT init/reset code
>> > >   drm/i915: fix open coded gen5_gt_irq_preinstall
>> > >   drm/i915: extract ibx_irq_uninstall
>> > >   drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall
>> > >   drm/i915: enable SDEIER later
>> > >   drm/i915: remove ibx_irq_uninstall
>> > >   drm/i915: add missing intel_hpd_irq_uninstall
>> > >   drm/i915: add ironlake_irq_reset
>> > >   drm/i915: add gen8_irq_reset
>> > >   drm/i915: only enable HWSTAM interrupts on postinstall on ILK+
>> > >   drm/i915: add POSTING_READs to the IRQ init/reset macros
>> > >
>> > >  drivers/gpu/drm/i915/i915_irq.c | 270 ++++++++++++++++++----------------------
>> > >  1 file changed, 121 insertions(+), 149 deletions(-)
>> > >
>> >
>> > Okay, here is the summary of my review. At first I was complaining to
>> > myself about how many patches you used to do a simple thing. But, I must
>> > admit it made reviewing the thing a lot easier, and when I look back at
>> > how much stuff you combined, I'm really glad you did it this way. I'm
>> > sure I've missed something silly though, since every patch looks so
>> > similar :P
>> >
>> > 1-5: Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (with possible comment
>> > improvement on #3)
>> >
>> > 7: I don't like. Can we drop? I guess doing this would make a decent
>> > amount of churn, so if you don't want to drop it, that's fine, and it's
>> > functionally correct:
>> >      Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>> >
>> > 8: I'd really like to drop this one.
>>
>> Comment on this and I think with a pimped commit message this is good to
>> go imo. I really think the added self-checks are required to start using
>> this code for runtime pm.
>>
>
> So you don't need my reviewed-by then. I don't like it...
>
>> > 9-10: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>> >
>> > 12-13: I wouldn't mind cpt_irq_* rename, but either way
>> >        Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>> >
>> > 14: With the requested change in the mail:
>> >     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>> >
>> > 16: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>> >
>> > 20: Should be squashed, but
>> >     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>> >
>> > 6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which
>> > seems to always mean "disable." I think disable makes the code so much
>> > clearer, and would really love if you can apply this simple rename. With
>> > the rename, they're:
>> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>>
>> Paulo's using "reset" functions/macros both in the preinstall hooks and in
>> the uninstall/disable code. We already use reset for stuff run before
>> init/enable code to get the hw in a state we expect it to, so I think
>> Paulo's naming choice is accurate and a plain "disable" more misleading.
>>
>
> I cannot disagree more. Every time I read "reset" it confuses me. But it
> seems like I am the minority.

I understand "reset" may not be the best name, I was already expecting
to see suggestions on the naming here. IMHO "disable" is also usable,
and I could rename, but Daniel just called it "misleading". So how
about we rename it to "clear" instead?

(let's see if I can make Ben and Daniel agree on something!)

I'll leave discussion of the other topics to the other emails.

>
>> I think you raise some good points in your review, and besides the 3 cases
>> I commented on I lack the detailed knowledge to avoid looking like a fool
>> ;-) So I think I'll wait for Paulo's comments before pulling this all in.
>>
>> Thanks,
>> Daniel
>
> Once Paulo responds, I'll make it a top priority to re-review whatever
> is needed. Sorry for the original delay.
>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> --
> Ben Widawsky, Intel Open Source Technology Center



-- 
Paulo Zanoni

Re: [PATCH 00/20] ILK+ interrupt improvements, v2

[Ben Widawsky]

On Wed, Mar 26, 2014 at 05:33:20PM -0300, Paulo Zanoni wrote:
> 2014-03-19 14:25 GMT-03:00 Ben Widawsky <ben@bwidawsk.net>:
> > On Wed, Mar 19, 2014 at 09:36:04AM +0100, Daniel Vetter wrote:
> >> On Tue, Mar 18, 2014 at 01:53:53PM -0700, Ben Widawsky wrote:
> >> > On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote:
> >> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > >
> >> > > Hi
> >> > >
> >> > > This is basically a rebase of "[PATCH 00/19] ILK+ interrupt improvements", which
> >> > > was sent to the mailing list on January 22. There are no real differences,
> >> > > except for the last patch, which is new.
> >> > >
> >> > > Original cover letter:
> >> > > http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html
> >> > >
> >> > > The idea behind this series is that at some point our runtime PM code will just
> >> > > call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of
> >> > > using dev_priv->pc8.regsave, so I decided to audit, cleanup and add a few WARNs
> >> > > to our code before we do that change. We gotta be in shape if we want to be
> >> > > exposed to runtime!
> >> > >
> >> > > Thanks,
> >> > > Paulo
> >> > >
> >> > > Paulo Zanoni (20):
> >> > >   drm/i915: add GEN5_IRQ_INIT macro
> >> > >   drm/i915: also use GEN5_IRQ_INIT with south display interrupts
> >> > >   drm/i915: use GEN8_IRQ_INIT on GEN5
> >> > >   drm/i915: add GEN5_IRQ_FINI
> >> > >   drm/i915: don't forget to uninstall the PM IRQs
> >> > >   drm/i915: properly clear IIR at irq_uninstall on Gen5+
> >> > >   drm/i915: add GEN5_IRQ_INIT
> >> > >   drm/i915: check if IIR is still zero at postinstall on Gen5+
> >> > >   drm/i915: fix SERR_INT init/reset code
> >> > >   drm/i915: fix GEN7_ERR_INT init/reset code
> >> > >   drm/i915: fix open coded gen5_gt_irq_preinstall
> >> > >   drm/i915: extract ibx_irq_uninstall
> >> > >   drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall
> >> > >   drm/i915: enable SDEIER later
> >> > >   drm/i915: remove ibx_irq_uninstall
> >> > >   drm/i915: add missing intel_hpd_irq_uninstall
> >> > >   drm/i915: add ironlake_irq_reset
> >> > >   drm/i915: add gen8_irq_reset
> >> > >   drm/i915: only enable HWSTAM interrupts on postinstall on ILK+
> >> > >   drm/i915: add POSTING_READs to the IRQ init/reset macros
> >> > >
> >> > >  drivers/gpu/drm/i915/i915_irq.c | 270 ++++++++++++++++++----------------------
> >> > >  1 file changed, 121 insertions(+), 149 deletions(-)
> >> > >
> >> >
> >> > Okay, here is the summary of my review. At first I was complaining to
> >> > myself about how many patches you used to do a simple thing. But, I must
> >> > admit it made reviewing the thing a lot easier, and when I look back at
> >> > how much stuff you combined, I'm really glad you did it this way. I'm
> >> > sure I've missed something silly though, since every patch looks so
> >> > similar :P
> >> >
> >> > 1-5: Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (with possible comment
> >> > improvement on #3)
> >> >
> >> > 7: I don't like. Can we drop? I guess doing this would make a decent
> >> > amount of churn, so if you don't want to drop it, that's fine, and it's
> >> > functionally correct:
> >> >      Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> >> >
> >> > 8: I'd really like to drop this one.
> >>
> >> Comment on this and I think with a pimped commit message this is good to
> >> go imo. I really think the added self-checks are required to start using
> >> this code for runtime pm.
> >>
> >
> > So you don't need my reviewed-by then. I don't like it...
> >
> >> > 9-10: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> >> >
> >> > 12-13: I wouldn't mind cpt_irq_* rename, but either way
> >> >        Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> >> >
> >> > 14: With the requested change in the mail:
> >> >     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> >> >
> >> > 16: Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> >> >
> >> > 20: Should be squashed, but
> >> >     Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> >> >
> >> > 6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which
> >> > seems to always mean "disable." I think disable makes the code so much
> >> > clearer, and would really love if you can apply this simple rename. With
> >> > the rename, they're:
> >> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> >>
> >> Paulo's using "reset" functions/macros both in the preinstall hooks and in
> >> the uninstall/disable code. We already use reset for stuff run before
> >> init/enable code to get the hw in a state we expect it to, so I think
> >> Paulo's naming choice is accurate and a plain "disable" more misleading.
> >>
> >
> > I cannot disagree more. Every time I read "reset" it confuses me. But it
> > seems like I am the minority.
> 
> I understand "reset" may not be the best name, I was already expecting
> to see suggestions on the naming here. IMHO "disable" is also usable,
> and I could rename, but Daniel just called it "misleading". So how
> about we rename it to "clear" instead?
> 
> (let's see if I can make Ben and Daniel agree on something!)
> 
> I'll leave discussion of the other topics to the other emails.
> 

"clear" has a distinct definition, and in the case of the mask, you are
not clearing. It's better than "reset"

I still like, "disable"
I can live with "disable_and_mask"

> >
> >> I think you raise some good points in your review, and besides the 3 cases
> >> I commented on I lack the detailed knowledge to avoid looking like a fool
> >> ;-) So I think I'll wait for Paulo's comments before pulling this all in.
> >>
> >> Thanks,
> >> Daniel
> >
> > Once Paulo responds, I'll make it a top priority to re-review whatever
> > is needed. Sorry for the original delay.
> >
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> > --
> > Ben Widawsky, Intel Open Source Technology Center
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 00/20] ILK+ interrupt improvements, v2

[Daniel Vetter]

On Wed, Mar 26, 2014 at 9:54 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>> >> Paulo's using "reset" functions/macros both in the preinstall hooks and in
>> >> the uninstall/disable code. We already use reset for stuff run before
>> >> init/enable code to get the hw in a state we expect it to, so I think
>> >> Paulo's naming choice is accurate and a plain "disable" more misleading.
>> >>
>> >
>> > I cannot disagree more. Every time I read "reset" it confuses me. But it
>> > seems like I am the minority.
>>
>> I understand "reset" may not be the best name, I was already expecting
>> to see suggestions on the naming here. IMHO "disable" is also usable,
>> and I could rename, but Daniel just called it "misleading". So how
>> about we rename it to "clear" instead?
>>
>> (let's see if I can make Ben and Daniel agree on something!)
>>
>> I'll leave discussion of the other topics to the other emails.
>>
>
> "clear" has a distinct definition, and in the case of the mask, you are
> not clearing. It's better than "reset"
>
> I still like, "disable"
> I can live with "disable_and_mask"

I still don't like "disable" really. Imo reset is totally ok, or
disable_and_clear (disable_and_mask_and_clear is a bit too much, and
disable_and_mask leaves out the important part that we clear the damn
thing).

Really, reset has imo clearly defined semantics of "clear to default,
quiescent state so that we can move on". Everything else is more
verbose and or falls short in conveying meaning imo. I know that Bspec
talks about "reset" all the time and means "clear to 0, but imo that
Bspec convention isn't the best choice really. At least it confuses me
to no end.

And since reset is what's in the patch, reset it is. Let's move on.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch