Skip to contentSkip to conversation

drm/i915: fix hsw_write_dcomp() error message

PATCH In review Paulo Zanoni 3 participants 5 messages Jul 4, 2014 – Jul 10, 2014

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Paulo Zanoni

author
#0 lore raw

PATCH 1/2 drm/i915: fix hsw_write_dcomp() error message

From: Paulo Zanoni <[email protected]>

That function can be used to write anything on D_COMP, not just disable it, so print a more appropriate message.

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +1 -1
index c12a5da..6986594 100644
@@ -7370,7 +7370,7 @@ static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
 		mutex_lock(&dev_priv->rps.hw_lock);
 		if (sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_D_COMP,
 					    val))
-			DRM_ERROR("Failed to disable D_COMP\n");
+			DRM_ERROR("Failed to write to D_COMP\n");
 		mutex_unlock(&dev_priv->rps.hw_lock);
 	} else {
 		I915_WRITE(D_COMP, val);
Signature · 1 line
2.0.0
author
#1 lore raw

PATCH 2/2 drm/i915: fix D_COMP usage on BDW

From: Paulo Zanoni <[email protected]>

On HSW, the D_COMP register can be accessed through the mailbox (read and write) or through MMIO on a MCHBAR offset (read only). On BDW, the access should be done through MMIO on another address. So to account for all these cases, create hsw_read_dcomp() with the correct implementation for reading, and also fix hsw_write_dcomp() to do the correct thing on BDW.

With this patch, we can now get back from the PC8+ state on BDW. We were previously getting a black screen and lots of dmesg errors. Please notice that the bug only happens when you actually reach the PC8+ states, not when you only allow it.

Testcase: igt/pm_rpm/rte

Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/i915_reg.h      |  5 ++++-
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)

I tested this before my BDW machine died and it worked. I couldn't test it again since then. Please notice that this is an important patch that prevents the machine from being useless after runtime PM is enabled (and we actually reach package C8 or deeper states). This will be even easier to reproduce after we merge Daniel's "RPM on DPMS" series.

drivers/gpu/drm/i915/i915_reg.h +4 -1
index 3488567..3e4b13e 100644
@@ -5962,7 +5962,10 @@ enum punit_power_well {
 #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
 #define  LCPLL_CD_SOURCE_FCLK_DONE	(1<<19)
 
-#define D_COMP				(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+/* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
+ * since on HSW we can't write to it using I915_WRITE. */
+#define D_COMP_HSW			(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+#define D_COMP_BDW			0x138144
 #define  D_COMP_RCOMP_IN_PROGRESS	(1<<9)
 #define  D_COMP_COMP_FORCE		(1<<8)
 #define  D_COMP_COMP_DISABLE		(1<<0)
drivers/gpu/drm/i915/intel_display.c +16 -5
index 6986594..3a8c9a7 100644
@@ -7362,6 +7362,16 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	WARN(!dev_priv->pm.irqs_disabled, "IRQs enabled\n");
 }
 
+static uint32_t hsw_read_dcomp(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+
+	if (IS_HASWELL(dev))
+		return I915_READ(D_COMP_HSW);
+	else
+		return I915_READ(D_COMP_BDW);
+}
+
 static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
 {
 	struct drm_device *dev = dev_priv->dev;

@@ -7373,9 +7383,9 @@ static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
 			DRM_ERROR("Failed to write D_COMP\n");
 		mutex_unlock(&dev_priv->rps.hw_lock);
 	} else {
-		I915_WRITE(D_COMP, val);
+		I915_WRITE(D_COMP_BDW, val);
+		POSTING_READ(D_COMP_BDW);
 	}
-	POSTING_READ(D_COMP);
 }
 
 /*

@@ -7413,12 +7423,13 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
 	if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
 		DRM_ERROR("LCPLL still locked\n");
 
-	val = I915_READ(D_COMP);
+	val = hsw_read_dcomp(dev_priv);
 	val |= D_COMP_COMP_DISABLE;
 	hsw_write_dcomp(dev_priv, val);
 	ndelay(100);
 
-	if (wait_for((I915_READ(D_COMP) & D_COMP_RCOMP_IN_PROGRESS) == 0, 1))
+	if (wait_for((hsw_read_dcomp(dev_priv) & D_COMP_RCOMP_IN_PROGRESS) == 0,
+		     1))
 		DRM_ERROR("D_COMP RCOMP still in progress\n");
 
 	if (allow_power_down) {

@@ -7467,7 +7478,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 		POSTING_READ(LCPLL_CTL);
 	}
 
-	val = I915_READ(D_COMP);
+	val = hsw_read_dcomp(dev_priv);
 	val |= D_COMP_COMP_FORCE;
 	val &= ~D_COMP_COMP_DISABLE;
 	hsw_write_dcomp(dev_priv, val);
Signature · 1 line
2.0.0
Reviewed-by on v1 2/2
#2 lore raw

Re: [PATCH 2/2] drm/i915: fix D_COMP usage on BDW

Quoted 9 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

On HSW, the D_COMP register can be accessed through the mailbox (read and write) or through MMIO on a MCHBAR offset (read only). On BDW, the access should be done through MMIO on another address. So to account for all these cases, create hsw_read_dcomp() with the correct implementation for reading, and also fix hsw_write_dcomp() to do the correct thing on BDW.

Quoted 7 lines

With this patch, we can now get back from the PC8+ state on BDW. We were previously getting a black screen and lots of dmesg errors. Please notice that the bug only happens when you actually reach the PC8+ states, not when you only allow it.

Testcase: igt/pm_rpm/rte

Signed-off-by Paulo Zanoni <[email protected]>
Reviewed-by Damien Lespiau <[email protected]>
Signature · 1 line
Damien
Quoted 92 lines
---
 drivers/gpu/drm/i915/i915_reg.h      |  5 ++++-
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)

I tested this before my BDW machine died and it worked. I couldn't test it again since then. Please notice that this is an important patch that prevents the machine from being useless after runtime PM is enabled (and we actually reach package C8 or deeper states). This will be even easier to reproduce after we merge Daniel's "RPM on DPMS" series.

drivers/gpu/drm/i915/i915_reg.h +4 -1
index 3488567..3e4b13e 100644
@@ -5962,7 +5962,10 @@ enum punit_power_well {
 #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
 #define  LCPLL_CD_SOURCE_FCLK_DONE	(1<<19)
 
-#define D_COMP				(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+/* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
+ * since on HSW we can't write to it using I915_WRITE. */
+#define D_COMP_HSW			(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+#define D_COMP_BDW			0x138144
 #define  D_COMP_RCOMP_IN_PROGRESS	(1<<9)
 #define  D_COMP_COMP_FORCE		(1<<8)
 #define  D_COMP_COMP_DISABLE		(1<<0)
drivers/gpu/drm/i915/intel_display.c +16 -5
index 6986594..3a8c9a7 100644
@@ -7362,6 +7362,16 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	WARN(!dev_priv->pm.irqs_disabled, "IRQs enabled\n");
 }
 
+static uint32_t hsw_read_dcomp(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+
+	if (IS_HASWELL(dev))
+		return I915_READ(D_COMP_HSW);
+	else
+		return I915_READ(D_COMP_BDW);
+}
+
 static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
 {
 	struct drm_device *dev = dev_priv->dev;

@@ -7373,9 +7383,9 @@ static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
 			DRM_ERROR("Failed to write D_COMP\n");
 		mutex_unlock(&dev_priv->rps.hw_lock);
 	} else {
-		I915_WRITE(D_COMP, val);
+		I915_WRITE(D_COMP_BDW, val);
+		POSTING_READ(D_COMP_BDW);
 	}
-	POSTING_READ(D_COMP);
 }
 
 /*

@@ -7413,12 +7423,13 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
 	if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
 		DRM_ERROR("LCPLL still locked\n");
 
-	val = I915_READ(D_COMP);
+	val = hsw_read_dcomp(dev_priv);
 	val |= D_COMP_COMP_DISABLE;
 	hsw_write_dcomp(dev_priv, val);
 	ndelay(100);
 
-	if (wait_for((I915_READ(D_COMP) & D_COMP_RCOMP_IN_PROGRESS) == 0, 1))
+	if (wait_for((hsw_read_dcomp(dev_priv) & D_COMP_RCOMP_IN_PROGRESS) == 0,
+		     1))
 		DRM_ERROR("D_COMP RCOMP still in progress\n");
 
 	if (allow_power_down) {

@@ -7467,7 +7478,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 		POSTING_READ(LCPLL_CTL);
 	}
 
-	val = I915_READ(D_COMP);
+	val = hsw_read_dcomp(dev_priv);
 	val |= D_COMP_COMP_FORCE;
 	val &= ~D_COMP_COMP_DISABLE;
 	hsw_write_dcomp(dev_priv, val);
Signature · 6 lines
2.0.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#3 lore raw

Re: [PATCH 2/2] drm/i915: fix D_COMP usage on BDW

Quoted 15 lines · Damien Lespiau
Quoted 14 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

On HSW, the D_COMP register can be accessed through the mailbox (read and write) or through MMIO on a MCHBAR offset (read only). On BDW, the access should be done through MMIO on another address. So to account for all these cases, create hsw_read_dcomp() with the correct implementation for reading, and also fix hsw_write_dcomp() to do the correct thing on BDW.

With this patch, we can now get back from the PC8+ state on BDW. We were previously getting a black screen and lots of dmesg errors. Please notice that the bug only happens when you actually reach the PC8+ states, not when you only allow it.

Quoted 4 lines
Quoted 2 lines

Testcase: igt/pm_rpm/rte

Signed-off-by Paulo Zanoni <[email protected]>
Reviewed-by Damien Lespiau <[email protected]>

Both patches merged, thanks.
-Daniel

Quoted 100 lines
Signature · 2 lines
-- 
Damien
Quoted 92 lines
---
 drivers/gpu/drm/i915/i915_reg.h      |  5 ++++-
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)

I tested this before my BDW machine died and it worked. I couldn't test it again since then. Please notice that this is an important patch that prevents the machine from being useless after runtime PM is enabled (and we actually reach package C8 or deeper states). This will be even easier to reproduce after we merge Daniel's "RPM on DPMS" series.

drivers/gpu/drm/i915/i915_reg.h +4 -1
index 3488567..3e4b13e 100644
@@ -5962,7 +5962,10 @@ enum punit_power_well {
 #define  LCPLL_CD_SOURCE_FCLK		(1<<21)
 #define  LCPLL_CD_SOURCE_FCLK_DONE	(1<<19)
 
-#define D_COMP				(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+/* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
+ * since on HSW we can't write to it using I915_WRITE. */
+#define D_COMP_HSW			(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+#define D_COMP_BDW			0x138144
 #define  D_COMP_RCOMP_IN_PROGRESS	(1<<9)
 #define  D_COMP_COMP_FORCE		(1<<8)
 #define  D_COMP_COMP_DISABLE		(1<<0)
drivers/gpu/drm/i915/intel_display.c +16 -5
index 6986594..3a8c9a7 100644
@@ -7362,6 +7362,16 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	WARN(!dev_priv->pm.irqs_disabled, "IRQs enabled\n");
 }
 
+static uint32_t hsw_read_dcomp(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+
+	if (IS_HASWELL(dev))
+		return I915_READ(D_COMP_HSW);
+	else
+		return I915_READ(D_COMP_BDW);
+}
+
 static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
 {
 	struct drm_device *dev = dev_priv->dev;

@@ -7373,9 +7383,9 @@ static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
 			DRM_ERROR("Failed to write D_COMP\n");
 		mutex_unlock(&dev_priv->rps.hw_lock);
 	} else {
-		I915_WRITE(D_COMP, val);
+		I915_WRITE(D_COMP_BDW, val);
+		POSTING_READ(D_COMP_BDW);
 	}
-	POSTING_READ(D_COMP);
 }
 
 /*

@@ -7413,12 +7423,13 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
 	if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
 		DRM_ERROR("LCPLL still locked\n");
 
-	val = I915_READ(D_COMP);
+	val = hsw_read_dcomp(dev_priv);
 	val |= D_COMP_COMP_DISABLE;
 	hsw_write_dcomp(dev_priv, val);
 	ndelay(100);
 
-	if (wait_for((I915_READ(D_COMP) & D_COMP_RCOMP_IN_PROGRESS) == 0, 1))
+	if (wait_for((hsw_read_dcomp(dev_priv) & D_COMP_RCOMP_IN_PROGRESS) == 0,
+		     1))
 		DRM_ERROR("D_COMP RCOMP still in progress\n");
 
 	if (allow_power_down) {

@@ -7467,7 +7478,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 		POSTING_READ(LCPLL_CTL);
 	}
 
-	val = I915_READ(D_COMP);
+	val = hsw_read_dcomp(dev_priv);
 	val |= D_COMP_COMP_FORCE;
 	val &= ~D_COMP_COMP_DISABLE;
 	hsw_write_dcomp(dev_priv, val);
Signature · 6 lines
2.0.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Reviewed-by on v1 1/2
#4 lore raw
In reply to Paulo Zanoni
Quoted 6 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

That function can be used to write anything on D_COMP, not just disable it, so print a more appropriate message.

Signed-off-by Paulo Zanoni <[email protected]>
Reviewed-by Damien Lespiau <[email protected]>
Quoted 24 lines
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_display.c +1 -1
index c12a5da..6986594 100644
@@ -7370,7 +7370,7 @@ static void hsw_write_dcomp(struct drm_i915_private *dev_priv, uint32_t val)
 		mutex_lock(&dev_priv->rps.hw_lock);
 		if (sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_D_COMP,
 					    val))
-			DRM_ERROR("Failed to disable D_COMP\n");
+			DRM_ERROR("Failed to write to D_COMP\n");
 		mutex_unlock(&dev_priv->rps.hw_lock);
 	} else {
 		I915_WRITE(D_COMP, val);
Signature · 6 lines
2.0.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx