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

[Daniel Vetter <daniel@ffwll.ch>]

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

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