Re: [PATCH v4 10/12] drm/i915/gen9: Turn DC handling into a power well

[Imre Deak <imre.deak@intel.com>]

On Mon, 2015-11-23 at 14:58 -0800, Matt Roper wrote:
> On Mon, Nov 16, 2015 at 04:20:01PM +0100, Patrik Jakobsson wrote:
> > Handle DC off as a power well where enabling the power well will
> > prevent
> > the DMC to enter selected DC states (required around modesets and
> > Aux
> > A). Disabling the power well will allow DC states again. For now
> > the
> > highest DC state is DC6 for Skylake and DC5 for Broxton but will be
> > configurable for Skylake in a later patch.
> > 
> > v2: Check both DC5 and DC6 bits in power well enabled function
> > (Ville)
> > v3:
> > - Remove unneeded DC_OFF case in skl_set_power_well() (Imre)
> > - Add PW2 dependency to DC_OFF (Imre)
> > v4: Put DC_OFF before PW2 in BXT power well array
> > 
> > Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> I've been seeing a BXT regression on recent di-nightly where DPMS off
> causes the entire platform to power down[1] instead of just the
> display;
> my bisect lands on this commit as the culprit.  Any idea what the
> cause
> could be?  I can reproduce by either letting the system sit idle long
> enough at an fb console, or by doing an "xset dpms force off" in X.
> Unfortunately I don't have a functioning serial console on this
> platform, so I can't get any messages that may show up around the
> DPMS
> operation.  I've attached my boot-time dmesg output in case that
> helps.
> 
> Subsequent commits seem to depend on the changes here, so I haven't
> reverted this commit directly on di-nightly, but I confirmed that if
> I
> checkout this commit directly I see DPMS problems, whereas its HEAD~1
> works as expected.

The power well support on BXT is not stable atm, we need to apply at
least a similar set of fixes as we did for SKL. So for now I would
suggest disabling it, by booting with i915.disable_power_well=0 until
things are fixed. This should've been made the default option earlier,
I forgot about this. I will follow up with the patch to that extent.

Thanks for the report,
Imre

> Matt
> 
> 
> [1] My BIOS and/or hardware is a bit flaky so even when operating
> "normally" most attempts to reboot/poweroff/S3 suspend all result in
> the
> platform going into some "mostly off" state with an error LED lit and
> require a full power cycle to resurrect.  That's the same state I
> wind
> up in after DPMS off when this patch is present.
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c         |   6 --
> >  drivers/gpu/drm/i915/i915_reg.h         |   1 +
> >  drivers/gpu/drm/i915/intel_display.c    |   6 ++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 110
> > +++++++++++++++++++++++---------
> >  4 files changed, 88 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 5a63f9a..0c7f435 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1072,9 +1072,6 @@ static int i915_pm_resume(struct device *dev)
> >  
> >  static int skl_suspend_complete(struct drm_i915_private *dev_priv)
> >  {
> > -	if (dev_priv->csr.dmc_payload)
> > -		skl_enable_dc6(dev_priv);
> > -
> >  	return 0;
> >  }
> >  
> > @@ -1119,9 +1116,6 @@ static int bxt_resume_prepare(struct
> > drm_i915_private *dev_priv)
> >  
> >  static int skl_resume_prepare(struct drm_i915_private *dev_priv)
> >  {
> > -	if (dev_priv->csr.dmc_payload)
> > -		skl_disable_dc6(dev_priv);
> > -
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 0f3849f..b6a3525 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -638,6 +638,7 @@ enum skl_disp_power_wells {
> >  
> >  	/* Not actual bit groups. Used as IDs for
> > lookup_power_well() */
> >  	SKL_DISP_PW_ALWAYS_ON,
> > +	SKL_DISP_PW_DC_OFF,
> >  };
> >  
> >  #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2))
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 978b1b9..21385a0 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13323,6 +13323,9 @@ static int intel_atomic_commit(struct
> > drm_device *dev,
> >  			to_intel_crtc_state(crtc->state)-
> > >update_pipe;
> >  		unsigned long put_domains = 0;
> >  
> > +		if (modeset)
> > +			intel_display_power_get(dev_priv,
> > POWER_DOMAIN_MODESET);
> > +
> >  		if (modeset && crtc->state->active) {
> >  			update_scanline_offset(to_intel_crtc(crtc)
> > );
> >  			dev_priv->display.crtc_enable(crtc);
> > @@ -13346,6 +13349,9 @@ static int intel_atomic_commit(struct
> > drm_device *dev,
> >  			modeset_put_power_domains(dev_priv,
> > put_domains);
> >  
> >  		intel_post_plane_update(intel_crtc);
> > +
> > +		if (modeset)
> > +			intel_display_power_put(dev_priv,
> > POWER_DOMAIN_MODESET);
> >  	}
> >  
> >  	/* FIXME: add subpixel order */
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index edf753e..0ff1646 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -49,9 +49,6 @@
> >   * present for a given platform.
> >   */
> >  
> > -#define GEN9_ENABLE_DC5(dev) 0
> > -#define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev)
> > -
> >  #define for_each_power_well(i, power_well, domain_mask,
> > power_domains)	\
> >  	for (i = 0;						
> > 	\
> >  	     i < (power_domains)->power_well_count &&		
> > 	\
> > @@ -309,9 +306,15 @@ static void hsw_set_power_well(struct
> > drm_i915_private *dev_priv,
> >  #define SKL_DISPLAY_DDI_D_POWER_DOMAINS (		\
> >  	BIT(POWER_DOMAIN_PORT_DDI_D_LANES) |		\
> >  	BIT(POWER_DOMAIN_INIT))
> > +#define SKL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> > +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > +	BIT(POWER_DOMAIN_MODESET) |			\
> > +	BIT(POWER_DOMAIN_AUX_A) |			\
> > +	BIT(POWER_DOMAIN_INIT))
> >  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
> >  	(POWER_DOMAIN_MASK & ~(				\
> > -	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS)) |	\
> > +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > +	SKL_DISPLAY_DC_OFF_POWER_DOMAINS)) |		\
> >  	BIT(POWER_DOMAIN_INIT))
> >  
> >  #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS (		\
> > @@ -339,6 +342,11 @@ static void hsw_set_power_well(struct
> > drm_i915_private *dev_priv,
> >  	BIT(POWER_DOMAIN_AUX_A) |			\
> >  	BIT(POWER_DOMAIN_PLLS) |			\
> >  	BIT(POWER_DOMAIN_INIT))
> > +#define BXT_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> > +	BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > +	BIT(POWER_DOMAIN_MODESET) |			\
> > +	BIT(POWER_DOMAIN_AUX_A) |			\
> > +	BIT(POWER_DOMAIN_INIT))
> >  #define BXT_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
> >  	(POWER_DOMAIN_MASK &
> > ~(BXT_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
> >  	BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS)) |	\
> > @@ -486,15 +494,6 @@ static void gen9_enable_dc5(struct
> > drm_i915_private *dev_priv)
> >  	gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
> >  }
> >  
> > -static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
> > -{
> > -	assert_can_disable_dc5(dev_priv);
> > -
> > -	DRM_DEBUG_KMS("Disabling DC5\n");
> > -
> > -	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > -}
> > -
> >  static void assert_can_enable_dc6(struct drm_i915_private
> > *dev_priv)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> > @@ -522,6 +521,14 @@ static void assert_can_disable_dc6(struct
> > drm_i915_private *dev_priv)
> >  		  "DC6 already programmed to be disabled.\n");
> >  }
> >  
> > +static void gen9_disable_dc5_dc6(struct drm_i915_private
> > *dev_priv)
> > +{
> > +	assert_can_disable_dc5(dev_priv);
> > +	assert_can_disable_dc6(dev_priv);
> > +
> > +	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > +}
> > +
> >  void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >  {
> >  	assert_can_enable_dc6(dev_priv);
> > @@ -589,17 +596,13 @@ static void skl_set_power_well(struct
> > drm_i915_private *dev_priv,
> >  				"Invalid for power well status to
> > be enabled, unless done by the BIOS, \
> >  				when request is to disable!\n");
> >  			if (power_well->data == SKL_DISP_PW_2) {
> > -				if (GEN9_ENABLE_DC5(dev))
> > -					gen9_disable_dc5(dev_priv)
> > ;
> > -				if (SKL_ENABLE_DC6(dev)) {
> > -					/*
> > -					 * DDI buffer programming
> > unnecessary during driver-load/resume
> > -					 * as it's already done
> > during modeset initialization then.
> > -					 * It's also invalid here
> > as encoder list is still uninitialized.
> > -					 */
> > -					if (!dev_priv-
> > >power_domains.initializing)
> > -						intel_prepare_ddi(
> > dev);
> > -				}
> > +				/*
> > +				 * DDI buffer programming
> > unnecessary during driver-load/resume
> > +				 * as it's already done during
> > modeset initialization then.
> > +				 * It's also invalid here as
> > encoder list is still uninitialized.
> > +				 */
> > +				if (!dev_priv-
> > >power_domains.initializing)
> > +					intel_prepare_ddi(dev);
> >  			}
> >  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp |
> > req_mask);
> >  		}
> > @@ -617,10 +620,6 @@ static void skl_set_power_well(struct
> > drm_i915_private *dev_priv,
> >  			I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp
> > & ~req_mask);
> >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> >  			DRM_DEBUG_KMS("Disabling %s\n",
> > power_well->name);
> > -
> > -			if (GEN9_ENABLE_DC5(dev) &&
> > -				power_well->data == SKL_DISP_PW_2)
> > -					gen9_enable_dc5(dev_priv);
> >  		}
> >  	}
> >  
> > @@ -695,6 +694,40 @@ static void skl_power_well_disable(struct
> > drm_i915_private *dev_priv,
> >  	skl_set_power_well(dev_priv, power_well, false);
> >  }
> >  
> > +static bool gen9_dc_off_power_well_enabled(struct drm_i915_private
> > *dev_priv,
> > +					  struct i915_power_well
> > *power_well)
> > +{
> > +	return (I915_READ(DC_STATE_EN) &
> > DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0;
> > +}
> > +
> > +static void gen9_dc_off_power_well_enable(struct drm_i915_private
> > *dev_priv,
> > +					 struct i915_power_well
> > *power_well)
> > +{
> > +	gen9_disable_dc5_dc6(dev_priv);
> > +}
> > +
> > +static void gen9_dc_off_power_well_disable(struct drm_i915_private
> > *dev_priv,
> > +					  struct i915_power_well
> > *power_well)
> > +{
> > +	if (IS_SKYLAKE(dev_priv))
> > +		skl_enable_dc6(dev_priv);
> > +	else
> > +		gen9_enable_dc5(dev_priv);
> > +}
> > +
> > +static void gen9_dc_off_power_well_sync_hw(struct drm_i915_private
> > *dev_priv,
> > +					  struct i915_power_well
> > *power_well)
> > +{
> > +	if (power_well->count > 0) {
> > +		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > +	} else {
> > +		if (IS_SKYLAKE(dev_priv))
> > +			gen9_set_dc_state(dev_priv,
> > DC_STATE_EN_UPTO_DC6);
> > +		else
> > +			gen9_set_dc_state(dev_priv,
> > DC_STATE_EN_UPTO_DC5);
> > +	}
> > +}
> > +
> >  static void i9xx_always_on_power_well_noop(struct drm_i915_private
> > *dev_priv,
> >  					   struct i915_power_well
> > *power_well)
> >  {
> > @@ -1517,6 +1550,13 @@ static const struct i915_power_well_ops
> > skl_power_well_ops = {
> >  	.is_enabled = skl_power_well_enabled,
> >  };
> >  
> > +static const struct i915_power_well_ops gen9_dc_off_power_well_ops
> > = {
> > +	.sync_hw = gen9_dc_off_power_well_sync_hw,
> > +	.enable = gen9_dc_off_power_well_enable,
> > +	.disable = gen9_dc_off_power_well_disable,
> > +	.is_enabled = gen9_dc_off_power_well_enabled,
> > +};
> > +
> >  static struct i915_power_well hsw_power_wells[] = {
> >  	{
> >  		.name = "always-on",
> > @@ -1691,6 +1731,12 @@ static struct i915_power_well
> > skl_power_wells[] = {
> >  		.data = SKL_DISP_PW_MISC_IO,
> >  	},
> >  	{
> > +		.name = "DC off",
> > +		.domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
> > +		.ops = &gen9_dc_off_power_well_ops,
> > +		.data = SKL_DISP_PW_DC_OFF,
> > +	},
> > +	{
> >  		.name = "power well 2",
> >  		.domains = SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS,
> >  		.ops = &skl_power_well_ops,
> > @@ -1765,11 +1811,17 @@ static struct i915_power_well
> > bxt_power_wells[] = {
> >  		.data = SKL_DISP_PW_1,
> >  	},
> >  	{
> > +		.name = "DC off",
> > +		.domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
> > +		.ops = &gen9_dc_off_power_well_ops,
> > +		.data = SKL_DISP_PW_DC_OFF,
> > +	},
> > +	{
> >  		.name = "power well 2",
> >  		.domains = BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS,
> >  		.ops = &skl_power_well_ops,
> >  		.data = SKL_DISP_PW_2,
> > -	}
> > +	},
> >  };
> >  
> >  #define set_power_wells(power_domains, __power_wells) ({		
> > \
> > -- 
> > 2.5.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