diff mbox

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

Message ID 1404485998-4580-2-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni July 4, 2014, 2:59 p.m. UTC
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 <paulo.r.zanoni@intel.com>
---
 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.

Comments

Lespiau, Damien July 9, 2014, 4:35 p.m. UTC | #1
On Fri, Jul 04, 2014 at 11:59:58AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Daniel Vetter July 10, 2014, 6:28 a.m. UTC | #2
On Wed, Jul 09, 2014 at 05:35:52PM +0100, Damien Lespiau wrote:
> On Fri, Jul 04, 2014 at 11:59:58AM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > 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 <paulo.r.zanoni@intel.com>
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Both patches merged, thanks.
-Daniel
> 
> -- 
> Damien
> 
> > ---
> >  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.
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 3488567..3e4b13e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -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)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 6986594..3a8c9a7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -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);
> > -- 
> > 2.0.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3488567..3e4b13e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -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)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6986594..3a8c9a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);