[PATCH 0/6] opregion asle enable cleanups

[PATCH 0/6] opregion asle enable cleanups

[Jani Nikula]

This series is a stab at untangling some of the opregion code,
particularly related to opregion asle enable. The only "real" change
should be clearing the driver readiness status in patch 2; otherwise it
*should* be just non-functional cleanup. I've split this up into perhaps
more patches than would be strictly necessary, but it should be helpful
in case there are subtle changes in bios/driver interaction that need to
be bisected.

This should complete the cleanups outlined by Daniel [1].

Cheers,
Jani.


[1] http://thread.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/19270/focus=19437


Jani Nikula (6):
  drm/i915: cleanup opregion technology enabled indicator defines
  drm/i915: manage opregion asle driver readiness properly
  drm/i915: untie opregion init and asle irq/pipestat enable
  drm/i915: cleanup redundant checks from intel_enable_asle
  drm/i915: cleanup opregion asle pipestat enable
  drm/i915: drop locking from asle pipestat enable

 drivers/gpu/drm/i915/i915_drv.h       |    4 ----
 drivers/gpu/drm/i915/i915_irq.c       |   31 ++++++++-----------------
 drivers/gpu/drm/i915/intel_opregion.c |   40 ++++++++++++++++-----------------
 3 files changed, 28 insertions(+), 47 deletions(-)

-- 
1.7.9.5

[PATCH 1/6] drm/i915: cleanup opregion technology enabled indicator defines

[Jani Nikula]

Move near other defines, add TCHE in the name. No functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index dda1828..75062e0 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -123,6 +123,12 @@ struct opregion_asle {
 #define ASLE_PFIT_FAILED	(1<<14)
 #define ASLE_PWM_FREQ_FAILED	(1<<16)
 
+/* Technology enabled indicator */
+#define ASLE_TCHE_ALS_EN	(1 << 0)
+#define ASLE_TCHE_BLC_EN	(1 << 1)
+#define ASLE_TCHE_PFIT_EN	(1 << 2)
+#define ASLE_TCHE_PFMB_EN	(1 << 3)
+
 /* ASLE backlight brightness to set */
 #define ASLE_BCLP_VALID                (1<<31)
 #define ASLE_BCLP_MSK          (~(1<<31))
@@ -222,11 +228,6 @@ void intel_opregion_asle_intr(struct drm_device *dev)
 	iowrite32(asle_stat, &asle->aslc);
 }
 
-#define ASLE_ALS_EN    (1<<0)
-#define ASLE_BLC_EN    (1<<1)
-#define ASLE_PFIT_EN   (1<<2)
-#define ASLE_PFMB_EN   (1<<3)
-
 void intel_opregion_enable_asle(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -236,7 +237,7 @@ void intel_opregion_enable_asle(struct drm_device *dev)
 		if (IS_MOBILE(dev))
 			intel_enable_asle(dev);
 
-		iowrite32(ASLE_BLC_EN, &asle->tche);
+		iowrite32(ASLE_TCHE_BLC_EN, &asle->tche);
 		iowrite32(1, &asle->ardy);
 	}
 }
-- 
1.7.9.5

[PATCH 2/6] drm/i915: manage opregion asle driver readiness properly

[Jani Nikula]

Only set ASLE driver readiness (ARDY) and technology enabled indicator
(TCHE) once per opregion init. There should be no need to do that at irq
postinstall time. Also clear driver readiness at fini.

While at it, add defines for driver readiness.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 75062e0..5b6d202 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -110,6 +110,10 @@ struct opregion_asle {
 	u8 rsvd[102];
 } __attribute__((packed));
 
+/* Driver readiness indicator */
+#define ASLE_ARDY_READY		(1 << 0)
+#define ASLE_ARDY_NOT_READY	(0 << 0)
+
 /* ASLE irq request bits */
 #define ASLE_SET_ALS_ILLUM     (1 << 0)
 #define ASLE_SET_BACKLIGHT     (1 << 1)
@@ -236,9 +240,6 @@ void intel_opregion_enable_asle(struct drm_device *dev)
 	if (asle) {
 		if (IS_MOBILE(dev))
 			intel_enable_asle(dev);
-
-		iowrite32(ASLE_TCHE_BLC_EN, &asle->tche);
-		iowrite32(1, &asle->ardy);
 	}
 }
 
@@ -425,8 +426,12 @@ void intel_opregion_init(struct drm_device *dev)
 		register_acpi_notifier(&intel_opregion_notifier);
 	}
 
-	if (opregion->asle)
+	if (opregion->asle) {
 		intel_opregion_enable_asle(dev);
+
+		iowrite32(ASLE_TCHE_BLC_EN, &opregion->asle->tche);
+		iowrite32(ASLE_ARDY_READY, &opregion->asle->ardy);
+	}
 }
 
 void intel_opregion_fini(struct drm_device *dev)
@@ -437,6 +442,9 @@ void intel_opregion_fini(struct drm_device *dev)
 	if (!opregion->header)
 		return;
 
+	if (opregion->asle)
+		iowrite32(ASLE_ARDY_NOT_READY, &opregion->asle->ardy);
+
 	if (opregion->acpi) {
 		iowrite32(0, &opregion->acpi->drdy);
 
@@ -499,6 +507,8 @@ int intel_opregion_setup(struct drm_device *dev)
 	if (mboxes & MBOX_ASLE) {
 		DRM_DEBUG_DRIVER("ASLE supported\n");
 		opregion->asle = base + OPREGION_ASLE_OFFSET;
+
+		iowrite32(ASLE_ARDY_NOT_READY, &opregion->asle->ardy);
 	}
 
 	return 0;
-- 
1.7.9.5

[PATCH 3/6] drm/i915: untie opregion init and asle irq/pipestat enable

[Jani Nikula]

Stop calling intel_opregion_enable_asle() and consequently
intel_enable_asle() on opregion init. It should not be necessary for
these reasons:

1) On PCH split platforms, it only enables GSE interrupt, which is
   enabled in irq postinstall anyway. Moreover, the irq enable uses the
   wrong bit on IVB+.

2) On gen 2, it would enable a reserved pipestat bit. If there were gen
   2 systems with opregion asle support, that is. And the gen 2 irq
   handler won't handle it anyway.

3) On gen 3-4, the irq postinstall will call
   intel_opregion_enable_asle() to enable the pipestat.

In short, move the asle irq/pipestat enable responsibility to irq
postinstall, which already happens to be in place.

This should not cause any functional changes, but only do the one line
change here for easier bisectability, just in case, and leave all the
cleanups this allows to followup patches.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 5b6d202..4e69799 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -427,8 +427,6 @@ void intel_opregion_init(struct drm_device *dev)
 	}
 
 	if (opregion->asle) {
-		intel_opregion_enable_asle(dev);
-
 		iowrite32(ASLE_TCHE_BLC_EN, &opregion->asle->tche);
 		iowrite32(ASLE_ARDY_READY, &opregion->asle->ardy);
 	}
-- 
1.7.9.5

[PATCH 4/6] drm/i915: cleanup redundant checks from intel_enable_asle

[Jani Nikula]

Realize that intel_enable_asle() is never called on PCH-split platforms
or on VLV. Rip out the GSE irq enable for PCH-split platforms, which
also happens to be incorrect for IVB+.

This should not cause any functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c |   16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b0dbf4c..1783ebe 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -356,21 +356,11 @@ void intel_enable_asle(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 
-	/* FIXME: opregion/asle for VLV */
-	if (IS_VALLEYVIEW(dev))
-		return;
-
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 
-	if (HAS_PCH_SPLIT(dev))
-		ironlake_enable_display_irq(dev_priv, DE_GSE);
-	else {
-		i915_enable_pipestat(dev_priv, 1,
-				     PIPE_LEGACY_BLC_EVENT_ENABLE);
-		if (INTEL_INFO(dev)->gen >= 4)
-			i915_enable_pipestat(dev_priv, 0,
-					     PIPE_LEGACY_BLC_EVENT_ENABLE);
-	}
+	i915_enable_pipestat(dev_priv, 1, PIPE_LEGACY_BLC_EVENT_ENABLE);
+	if (INTEL_INFO(dev)->gen >= 4)
+		i915_enable_pipestat(dev_priv, 0, PIPE_LEGACY_BLC_EVENT_ENABLE);
 
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
-- 
1.7.9.5

[PATCH 5/6] drm/i915: cleanup opregion asle pipestat enable

[Jani Nikula]

Both intel_opregion_enable_asle() and intel_enable_asle() have shrunk
considerably. Merge them together into a static function in i915_irq.c,
and rename to better reflect the purpose and the related platforms.

No functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |    4 ----
 drivers/gpu/drm/i915/i915_irq.c       |   11 +++++++----
 drivers/gpu/drm/i915/intel_opregion.c |   11 -----------
 3 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 14156f2..c34103d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1484,8 +1484,6 @@ i915_enable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask);
 void
 i915_disable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask);
 
-void intel_enable_asle(struct drm_device *dev);
-
 #ifdef CONFIG_DEBUG_FS
 extern void i915_destroy_error_state(struct drm_device *dev);
 #else
@@ -1819,12 +1817,10 @@ extern int intel_opregion_setup(struct drm_device *dev);
 extern void intel_opregion_init(struct drm_device *dev);
 extern void intel_opregion_fini(struct drm_device *dev);
 extern void intel_opregion_asle_intr(struct drm_device *dev);
-extern void intel_opregion_enable_asle(struct drm_device *dev);
 #else
 static inline void intel_opregion_init(struct drm_device *dev) { return; }
 static inline void intel_opregion_fini(struct drm_device *dev) { return; }
 static inline void intel_opregion_asle_intr(struct drm_device *dev) { return; }
-static inline void intel_opregion_enable_asle(struct drm_device *dev) { return; }
 #endif
 
 /* intel_acpi.c */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1783ebe..03a31be 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -349,13 +349,16 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask)
 }
 
 /**
- * intel_enable_asle - enable ASLE interrupt for OpRegion
+ * i915_enable_asle_pipestat - enable ASLE pipestat for OpRegion
  */
-void intel_enable_asle(struct drm_device *dev)
+static void i915_enable_asle_pipestat(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	unsigned long irqflags;
 
+	if (!dev_priv->opregion.asle || !IS_MOBILE(dev))
+		return;
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 
 	i915_enable_pipestat(dev_priv, 1, PIPE_LEGACY_BLC_EVENT_ENABLE);
@@ -2964,7 +2967,7 @@ static int i915_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(IER, enable_mask);
 	POSTING_READ(IER);
 
-	intel_opregion_enable_asle(dev);
+	i915_enable_asle_pipestat(dev);
 
 	return 0;
 }
@@ -3198,7 +3201,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	POSTING_READ(PORT_HOTPLUG_EN);
 
-	intel_opregion_enable_asle(dev);
+	i915_enable_asle_pipestat(dev);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 4e69799..62b64e4 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -232,17 +232,6 @@ void intel_opregion_asle_intr(struct drm_device *dev)
 	iowrite32(asle_stat, &asle->aslc);
 }
 
-void intel_opregion_enable_asle(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct opregion_asle __iomem *asle = dev_priv->opregion.asle;
-
-	if (asle) {
-		if (IS_MOBILE(dev))
-			intel_enable_asle(dev);
-	}
-}
-
 #define ACPI_EV_DISPLAY_SWITCH (1<<0)
 #define ACPI_EV_LID            (1<<1)
 #define ACPI_EV_DOCK           (1<<2)
-- 
1.7.9.5

[PATCH 6/6] drm/i915: drop locking from asle pipestat enable

[Jani Nikula]

Enable asle pipestat earlier in i915/i965 irq postinstall to not need
irq_lock in i915_enable_asle_pipestat().

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c |   12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 03a31be..0243db1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -354,18 +354,13 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask)
 static void i915_enable_asle_pipestat(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	unsigned long irqflags;
 
 	if (!dev_priv->opregion.asle || !IS_MOBILE(dev))
 		return;
 
-	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-
 	i915_enable_pipestat(dev_priv, 1, PIPE_LEGACY_BLC_EVENT_ENABLE);
 	if (INTEL_INFO(dev)->gen >= 4)
 		i915_enable_pipestat(dev_priv, 0, PIPE_LEGACY_BLC_EVENT_ENABLE);
-
-	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
 /**
@@ -2953,6 +2948,8 @@ static int i915_irq_postinstall(struct drm_device *dev)
 		I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT |
 		I915_USER_INTERRUPT;
 
+	i915_enable_asle_pipestat(dev);
+
 	if (I915_HAS_HOTPLUG(dev)) {
 		I915_WRITE(PORT_HOTPLUG_EN, 0);
 		POSTING_READ(PORT_HOTPLUG_EN);
@@ -2967,8 +2964,6 @@ static int i915_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(IER, enable_mask);
 	POSTING_READ(IER);
 
-	i915_enable_asle_pipestat(dev);
-
 	return 0;
 }
 
@@ -3178,6 +3173,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
 		enable_mask |= I915_BSD_USER_INTERRUPT;
 
 	i915_enable_pipestat(dev_priv, 0, PIPE_GMBUS_EVENT_ENABLE);
+	i915_enable_asle_pipestat(dev);
 
 	/*
 	 * Enable some error detection, note the instruction error mask
@@ -3201,8 +3197,6 @@ static int i965_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	POSTING_READ(PORT_HOTPLUG_EN);
 
-	i915_enable_asle_pipestat(dev);
-
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH 1/6] drm/i915: cleanup opregion technology enabled indicator defines

[Damien Lespiau]

On Mon, Apr 29, 2013 at 01:02:50PM +0300, Jani Nikula wrote:
> Move near other defines, add TCHE in the name. No functional changes.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 2/6] drm/i915: manage opregion asle driver readiness properly

[Damien Lespiau]

On Mon, Apr 29, 2013 at 01:02:51PM +0300, Jani Nikula wrote:
> Only set ASLE driver readiness (ARDY) and technology enabled indicator
> (TCHE) once per opregion init. There should be no need to do that at irq
> postinstall time. Also clear driver readiness at fini.
> 
> While at it, add defines for driver readiness.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 3/6] drm/i915: untie opregion init and asle irq/pipestat enable

[Damien Lespiau]

On Mon, Apr 29, 2013 at 01:02:52PM +0300, Jani Nikula wrote:
> Stop calling intel_opregion_enable_asle() and consequently
> intel_enable_asle() on opregion init. It should not be necessary for
> these reasons:
> 
> 1) On PCH split platforms, it only enables GSE interrupt, which is
>    enabled in irq postinstall anyway. Moreover, the irq enable uses the
>    wrong bit on IVB+.
> 
> 2) On gen 2, it would enable a reserved pipestat bit. If there were gen
>    2 systems with opregion asle support, that is. And the gen 2 irq
>    handler won't handle it anyway.
> 
> 3) On gen 3-4, the irq postinstall will call
>    intel_opregion_enable_asle() to enable the pipestat.
> 
> In short, move the asle irq/pipestat enable responsibility to irq
> postinstall, which already happens to be in place.
> 
> This should not cause any functional changes, but only do the one line
> change here for easier bisectability, just in case, and leave all the
> cleanups this allows to followup patches.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 3/6] drm/i915: untie opregion init and asle irq/pipestat enable

[Damien Lespiau]

On Mon, Apr 29, 2013 at 01:02:52PM +0300, Jani Nikula wrote:
> Stop calling intel_opregion_enable_asle() and consequently
> intel_enable_asle() on opregion init. It should not be necessary for
> these reasons:
> 
> 1) On PCH split platforms, it only enables GSE interrupt, which is
>    enabled in irq postinstall anyway. Moreover, the irq enable uses the
>    wrong bit on IVB+.
> 
> 2) On gen 2, it would enable a reserved pipestat bit. If there were gen
>    2 systems with opregion asle support, that is. And the gen 2 irq
>    handler won't handle it anyway.
> 
> 3) On gen 3-4, the irq postinstall will call
>    intel_opregion_enable_asle() to enable the pipestat.
> 
> In short, move the asle irq/pipestat enable responsibility to irq
> postinstall, which already happens to be in place.
> 
> This should not cause any functional changes, but only do the one line
> change here for easier bisectability, just in case, and leave all the
> cleanups this allows to followup patches.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Oh wait, this means that at this commit we don't enable pipestat on gen
3/4 and end up with a broken backlight? maybe this untangling could also
make the postinstall functions call intel_enable_asle()?

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_opregion.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 5b6d202..4e69799 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -427,8 +427,6 @@ void intel_opregion_init(struct drm_device *dev)
>  	}
>  
>  	if (opregion->asle) {
> -		intel_opregion_enable_asle(dev);
> -
>  		iowrite32(ASLE_TCHE_BLC_EN, &opregion->asle->tche);
>  		iowrite32(ASLE_ARDY_READY, &opregion->asle->ardy);
>  	}
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915: cleanup redundant checks from intel_enable_asle

[Damien Lespiau]

On Mon, Apr 29, 2013 at 01:02:53PM +0300, Jani Nikula wrote:
> Realize that intel_enable_asle() is never called on PCH-split platforms
> or on VLV. Rip out the GSE irq enable for PCH-split platforms, which
> also happens to be incorrect for IVB+.
> 
> This should not cause any functional changes.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 3/6] drm/i915: untie opregion init and asle irq/pipestat enable

[Damien Lespiau]

On Mon, Apr 29, 2013 at 12:29:24PM +0100, Damien Lespiau wrote:
> On Mon, Apr 29, 2013 at 01:02:52PM +0300, Jani Nikula wrote:
> > Stop calling intel_opregion_enable_asle() and consequently
> > intel_enable_asle() on opregion init. It should not be necessary for
> > these reasons:
> > 
> > 1) On PCH split platforms, it only enables GSE interrupt, which is
> >    enabled in irq postinstall anyway. Moreover, the irq enable uses the
> >    wrong bit on IVB+.
> > 
> > 2) On gen 2, it would enable a reserved pipestat bit. If there were gen
> >    2 systems with opregion asle support, that is. And the gen 2 irq
> >    handler won't handle it anyway.
> > 
> > 3) On gen 3-4, the irq postinstall will call
> >    intel_opregion_enable_asle() to enable the pipestat.
> > 
> > In short, move the asle irq/pipestat enable responsibility to irq
> > postinstall, which already happens to be in place.
> > 
> > This should not cause any functional changes, but only do the one line
> > change here for easier bisectability, just in case, and leave all the
> > cleanups this allows to followup patches.
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> Oh wait, this means that at this commit we don't enable pipestat on gen
> 3/4 and end up with a broken backlight? maybe this untangling could also
> make the postinstall functions call intel_enable_asle()?

Disregard that comment, the naming was confusing after all...

-- 
Damien

Re: [PATCH 5/6] drm/i915: cleanup opregion asle pipestat enable

[Damien Lespiau]

On Mon, Apr 29, 2013 at 01:02:54PM +0300, Jani Nikula wrote:
> Both intel_opregion_enable_asle() and intel_enable_asle() have shrunk
> considerably. Merge them together into a static function in i915_irq.c,
> and rename to better reflect the purpose and the related platforms.
> 
> No functional changes.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 6/6] drm/i915: drop locking from asle pipestat enable

[Damien Lespiau]

On Mon, Apr 29, 2013 at 01:02:55PM +0300, Jani Nikula wrote:
> Enable asle pipestat earlier in i915/i965 irq postinstall to not need
> irq_lock in i915_enable_asle_pipestat().
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 6/6] drm/i915: drop locking from asle pipestat enable

[Daniel Vetter]

On Mon, Apr 29, 2013 at 01:02:55PM +0300, Jani Nikula wrote:
> Enable asle pipestat earlier in i915/i965 irq postinstall to not need
> irq_lock in i915_enable_asle_pipestat().
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Honestly I'm not too fond of too clever init sequence ordering - we
already get the inherent constraints of the setup sequence wrong way too
often. Trying to avoid a spinlock at setup time with clever ordering feels
like the wrong tradeoff here.

So I'd prefer a WARN(!spin_is_locked) instead in enable_pipestat (plus
lock grabbing in the vlv/i965 postinstall code).

All other patches from this series merged, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c |   12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 03a31be..0243db1 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -354,18 +354,13 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask)
>  static void i915_enable_asle_pipestat(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> -	unsigned long irqflags;
>  
>  	if (!dev_priv->opregion.asle || !IS_MOBILE(dev))
>  		return;
>  
> -	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -
>  	i915_enable_pipestat(dev_priv, 1, PIPE_LEGACY_BLC_EVENT_ENABLE);
>  	if (INTEL_INFO(dev)->gen >= 4)
>  		i915_enable_pipestat(dev_priv, 0, PIPE_LEGACY_BLC_EVENT_ENABLE);
> -
> -	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
>  /**
> @@ -2953,6 +2948,8 @@ static int i915_irq_postinstall(struct drm_device *dev)
>  		I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT |
>  		I915_USER_INTERRUPT;
>  
> +	i915_enable_asle_pipestat(dev);
> +
>  	if (I915_HAS_HOTPLUG(dev)) {
>  		I915_WRITE(PORT_HOTPLUG_EN, 0);
>  		POSTING_READ(PORT_HOTPLUG_EN);
> @@ -2967,8 +2964,6 @@ static int i915_irq_postinstall(struct drm_device *dev)
>  	I915_WRITE(IER, enable_mask);
>  	POSTING_READ(IER);
>  
> -	i915_enable_asle_pipestat(dev);
> -
>  	return 0;
>  }
>  
> @@ -3178,6 +3173,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
>  		enable_mask |= I915_BSD_USER_INTERRUPT;
>  
>  	i915_enable_pipestat(dev_priv, 0, PIPE_GMBUS_EVENT_ENABLE);
> +	i915_enable_asle_pipestat(dev);
>  
>  	/*
>  	 * Enable some error detection, note the instruction error mask
> @@ -3201,8 +3197,6 @@ static int i965_irq_postinstall(struct drm_device *dev)
>  	I915_WRITE(PORT_HOTPLUG_EN, 0);
>  	POSTING_READ(PORT_HOTPLUG_EN);
>  
> -	i915_enable_asle_pipestat(dev);
> -
>  	return 0;
>  }
>  
> -- 
> 1.7.9.5
> 

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

[PATCH] drm/i915: make locking requirement for pipestat changes more explicit

[Jani Nikula]

Warn on missing locking in pipestat enable/disable, and fix calls that
would trigger this.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Daniel, is this more to your liking?

I don't have a machine handy to actually test this right now...
---
 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 03a31be..efedf61 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -325,6 +325,8 @@ i915_enable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask)
 	u32 reg = PIPESTAT(pipe);
 	u32 pipestat = I915_READ(reg) & 0x7fff0000;
 
+	WARN_ON(!spin_is_locked(&dev_priv->irq_lock));
+
 	if ((pipestat & mask) == mask)
 		return;
 
@@ -340,6 +342,8 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask)
 	u32 reg = PIPESTAT(pipe);
 	u32 pipestat = I915_READ(reg) & 0x7fff0000;
 
+	WARN_ON(!spin_is_locked(&dev_priv->irq_lock));
+
 	if ((pipestat & mask) == 0)
 		return;
 
@@ -354,18 +358,13 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask)
 static void i915_enable_asle_pipestat(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	unsigned long irqflags;
 
 	if (!dev_priv->opregion.asle || !IS_MOBILE(dev))
 		return;
 
-	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-
 	i915_enable_pipestat(dev_priv, 1, PIPE_LEGACY_BLC_EVENT_ENABLE);
 	if (INTEL_INFO(dev)->gen >= 4)
 		i915_enable_pipestat(dev_priv, 0, PIPE_LEGACY_BLC_EVENT_ENABLE);
-
-	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
 /**
@@ -2650,6 +2649,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	u32 enable_mask;
 	u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV;
 	u32 render_irqs;
+	unsigned long irqflags;
 
 	enable_mask = I915_DISPLAY_PORT_INTERRUPT;
 	enable_mask |= I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
@@ -2675,9 +2675,11 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(PIPESTAT(1), 0xffff);
 	POSTING_READ(VLV_IER);
 
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, 0, pipestat_enable);
 	i915_enable_pipestat(dev_priv, 0, PIPE_GMBUS_EVENT_ENABLE);
 	i915_enable_pipestat(dev_priv, 1, pipestat_enable);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	I915_WRITE(VLV_IIR, 0xffffffff);
 	I915_WRITE(VLV_IIR, 0xffffffff);
@@ -2934,6 +2936,7 @@ static int i915_irq_postinstall(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	u32 enable_mask;
+	unsigned long irqflags;
 
 	I915_WRITE(EMR, ~(I915_ERROR_PAGE_TABLE | I915_ERROR_MEMORY_REFRESH));
 
@@ -2967,7 +2970,9 @@ static int i915_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(IER, enable_mask);
 	POSTING_READ(IER);
 
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_asle_pipestat(dev);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return 0;
 }
@@ -3159,6 +3164,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	u32 enable_mask;
 	u32 error_mask;
+	unsigned long irqflags;
 
 	/* Unmask the interrupts that we always want on. */
 	dev_priv->irq_mask = ~(I915_ASLE_INTERRUPT |
@@ -3177,7 +3183,10 @@ static int i965_irq_postinstall(struct drm_device *dev)
 	if (IS_G4X(dev))
 		enable_mask |= I915_BSD_USER_INTERRUPT;
 
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, 0, PIPE_GMBUS_EVENT_ENABLE);
+	i915_enable_asle_pipestat(dev);
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	/*
 	 * Enable some error detection, note the instruction error mask
@@ -3201,8 +3210,6 @@ static int i965_irq_postinstall(struct drm_device *dev)
 	I915_WRITE(PORT_HOTPLUG_EN, 0);
 	POSTING_READ(PORT_HOTPLUG_EN);
 
-	i915_enable_asle_pipestat(dev);
-
 	return 0;
 }
 
-- 
1.7.10.4