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

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

On to, 2015-11-12 at 13:24 +0100, Patrik Jakobsson wrote:
> On Wed, Nov 11, 2015 at 08:57:19PM +0200, Imre Deak wrote:
> > On ma, 2015-11-09 at 16:48 +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)
> > > 
> > > Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com
> > > >
> > > ---
> > >  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 31b3a84..df445ba 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -636,6 +636,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,
> > 
> > Imo it would be less confusing to call it DC3 power well. Looking
> > at th
> > e DC spec, DC4 is only a transitory state to DC5/6, so what we
> > expect
> > when we disable DC6/5 is DC3 or shallower power states (DC2/1/0).
> 
> I've been changing the name quite a few times but settled on "DC off"
> to keep it
> generic. The main mechanism for the power well is to prevent any DC
> states that
> can cause us problems during certain operations (i.e. modeset). The
> DC states we
> need to block could change or be different between platforms. For
> that reason I
> would prefer not to be as specific with the naming.

All the power well names are platform specific so it would be logical
and more meaningful to give this one a platform specific name too. Also
'DC off' is kind of a double negation leading to debug messages like
'enable DC off' which isn't that nice. But now that I think of it my
idea isn't that great either. 'enable DC3' is confusing too, one could
think we enable some power saving state at that point, which is not the
case. So let's keep things as-is for now at least:

Reviewed-by: Imre Deak <imre.deak@intel.com>

> 
> > 
> > >  };
> > >  
> > >  #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 649ac34..856d801 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13320,6 +13320,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(crt
> > > c));
> > >  			dev_priv->display.crtc_enable(crtc);
> > > @@ -13343,6 +13346,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..95c3fcc 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,14 @@ 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 (		\
> > > +	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 +341,10 @@ 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 (		\
> > > +	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 +492,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 +519,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);
> > > @@ -571,6 +576,7 @@ static void skl_set_power_well(struct
> > > drm_i915_private *dev_priv,
> > >  	case SKL_DISP_PW_DDI_C:
> > >  	case SKL_DISP_PW_DDI_D:
> > >  	case SKL_DISP_PW_MISC_IO:
> > > +	case SKL_DISP_PW_DC_OFF:
> > >  		break;
> > >  	default:
> > >  		WARN(1, "Unknown power well %lu\n", power_well
> > > ->data);
> > > @@ -589,17 +595,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_pri
> > > v);
> > > -				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_dd
> > > i(de
> > > v);
> > > -				}
> > > +				/*
> > > +				 * 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 +619,6 @@ static void skl_set_power_well(struct
> > > drm_i915_private *dev_priv,
> > >  			I915_WRITE(HSW_PWR_WELL_DRIVER,	t
> > > mp &
> > > ~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 +693,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 +1549,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",
> > > @@ -1697,6 +1736,12 @@ static struct i915_power_well
> > > skl_power_wells[] = {
> > >  		.data = SKL_DISP_PW_2,
> > >  	},
> > >  	{
> > > +		.name = "DC off",
> > > +		.domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
> > > +		.ops = &gen9_dc_off_power_well_ops,
> > > +		.data = SKL_DISP_PW_DC_OFF,
> > > +	},
> > > +	{
> > >  		.name = "DDI A/E power well",
> > >  		.domains = SKL_DISPLAY_DDI_A_E_POWER_DOMAINS,
> > >  		.ops = &skl_power_well_ops,
> > > @@ -1769,7 +1814,14 @@ static struct i915_power_well
> > > bxt_power_wells[] = {
> > >  		.domains =
> > > BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS,
> > >  		.ops = &skl_power_well_ops,
> > >  		.data = SKL_DISP_PW_2,
> > > -	}
> > > +	},
> > > +	{
> > > +		.name = "DC off",
> > > +		.domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
> > > +		.ops = &gen9_dc_off_power_well_ops,
> > > +		.data = SKL_DISP_PW_DC_OFF,
> > > +	},
> > > +
> > >  };
> > >  
> > >  #define set_power_wells(power_domains, __power_wells) ({		
> > > \
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx