[RFC] drm/i915/skl: Add DC6 disabling as a power well

Commit Message

Patrik Jakobsson Sept. 11, 2015, 11:55 a.m. UTC

We need to be able to control if DC6 is allowed or not. Much like
requesting power to a specific piece of the hardware we need to be able
to request that we don't enter DC6 during certain hw access.

To solve this without introducing too much infrastructure I'm hooking
into the power well / power domain framework. DC6 prevention is modeled
much like an enabled power well. Thus I'm using the terminology on/off
for DC states instead of enable/disable.

The problem that started this work is the need for DC6 to be disabled
when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
patch.

This is posted as an RFC since DMC and DC state handling is being
reworked and will possibly affect the outcome of this patch. The patch
has known warnings.

Signed-off-by: Patrik Jakobsson <[e-mail]>
---
 drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
 drivers/gpu/drm/i915/intel_drv.h        |  2 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
 3 files changed, 64 insertions(+), 16 deletions(-)

Comments

Daniel Vetter Sept. 14, 2015, 9:50 a.m. UTC | #1

On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> We need to be able to control if DC6 is allowed or not. Much like
> requesting power to a specific piece of the hardware we need to be able
> to request that we don't enter DC6 during certain hw access.
> 
> To solve this without introducing too much infrastructure I'm hooking
> into the power well / power domain framework. DC6 prevention is modeled
> much like an enabled power well. Thus I'm using the terminology on/off
> for DC states instead of enable/disable.
> 
> The problem that started this work is the need for DC6 to be disabled
> when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> patch.
> 
> This is posted as an RFC since DMC and DC state handling is being
> reworked and will possibly affect the outcome of this patch. The patch
> has known warnings.
> 
> Signed-off-by: Patrik Jakobsson <[e-mail]>

Despite the naming suggesting its for power wells only this is exactly
what the power well infrastructure is meant for: It's just our in-house
struct power_domain for display runtime pm: They're hierachical and and
refcounted with get/put. So from that pov lgtm.

But please cc the people working on the other dmc patches and coordinate
with them.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
>  drivers/gpu/drm/i915/intel_drv.h        |  2 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
>  3 files changed, 64 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 4823184..c2c1ad2 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  
> +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> +
>  		intel_dp_set_link_params(intel_dp, crtc->config);
>  
>  		intel_ddi_init_dp_buf_reg(intel_encoder);
> @@ -2297,6 +2299,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>  		intel_dp_complete_link_train(intel_dp);
>  		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
>  			intel_dp_stop_link_train(intel_dp);
> +
> +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
>  	} else if (type == INTEL_OUTPUT_HDMI) {
>  		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>  
> @@ -2339,9 +2343,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>  
>  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> +
>  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>  		intel_edp_panel_vdd_on(intel_dp);
>  		intel_edp_panel_off(intel_dp);
> +
> +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
>  	}
>  
>  	if (IS_SKYLAKE(dev))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 46484e4..82489ad 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1367,6 +1367,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
>  			     bool override, unsigned int mask);
>  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
>  			  enum dpio_channel ch, bool override);
> +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> +void skl_disable_dc6(struct drm_i915_private *dev_priv);
>  
>  
>  /* intel_pm.c */
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 3f682a1..e30c9a6 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -335,6 +335,10 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>  	SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |		\
>  	BIT(POWER_DOMAIN_PLLS) |			\
>  	BIT(POWER_DOMAIN_INIT))
> +#define SKL_DISPLAY_DC6_OFF_POWER_DOMAINS (		\
> +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> +	BIT(POWER_DOMAIN_AUX_A))
> +
>  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
>  	(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
>  	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> @@ -550,7 +554,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
>  		"DC6 already programmed to be disabled.\n");
>  }
>  
> -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> +void skl_enable_dc6(struct drm_i915_private *dev_priv)
>  {
>  	uint32_t val;
>  
> @@ -567,7 +571,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
>  	POSTING_READ(DC_STATE_EN);
>  }
>  
> -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> +void skl_disable_dc6(struct drm_i915_private *dev_priv)
>  {
>  	uint32_t val;
>  
> @@ -628,10 +632,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  				!I915_READ(HSW_PWR_WELL_BIOS),
>  				"Invalid for power well status to be enabled, unless done by the BIOS, \
>  				when request is to disable!\n");
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> +			if (power_well->data == SKL_DISP_PW_2) {
>  				if (SKL_ENABLE_DC6(dev)) {
> -					skl_disable_dc6(dev_priv);
>  					/*
>  					 * DDI buffer programming unnecessary during driver-load/resume
>  					 * as it's already done during modeset initialization then.
> @@ -639,10 +641,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  					 */
>  					if (!dev_priv->power_domains.initializing)
>  						intel_prepare_ddi(dev);
> -				} else {
> -					gen9_disable_dc5(dev_priv);
>  				}
>  			}
> +
>  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
>  		}
>  
> @@ -660,8 +661,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  			POSTING_READ(HSW_PWR_WELL_DRIVER);
>  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>  
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> +			if (power_well->data == SKL_DISP_PW_2) {
>  				enum csr_state state;
>  				/* TODO: wait for a completion event or
>  				 * similar here instead of busy
> @@ -669,14 +669,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  				 */
>  				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
>  						FW_UNINITIALIZED, 1000);
> -				if (state != FW_LOADED)
> +				if (state != FW_LOADED) {
>  					DRM_ERROR("CSR firmware not ready (%d)\n",
> -							state);
> -				else
> -					if (SKL_ENABLE_DC6(dev))
> -						skl_enable_dc6(dev_priv);
> -					else
> -						gen9_enable_dc5(dev_priv);
> +						  state);
> +				}
>  			}
>  		}
>  	}
> @@ -752,6 +748,34 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
>  	skl_set_power_well(dev_priv, power_well, false);
>  }
>  
> +static bool skl_dc6_state_enabled(struct drm_i915_private *dev_priv,
> +				  struct i915_power_well *power_well)
> +{
> +	/* Return true if disabling of DC6 is enabled */
> +	return !(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6);
> +}
> +
> +static void skl_dc6_state_on(struct drm_i915_private *dev_priv,
> +			     struct i915_power_well *power_well)
> +{
> +	skl_enable_dc6(dev_priv);
> +}
> +
> +static void skl_dc6_state_off(struct drm_i915_private *dev_priv,
> +			      struct i915_power_well *power_well)
> +{
> +	skl_disable_dc6(dev_priv);
> +}
> +
> +static void skl_dc6_sync_hw(struct drm_i915_private *dev_priv,
> +			    struct i915_power_well *power_well)
> +{
> +	if (power_well->count > 0)
> +		skl_disable_dc6(dev_priv);
> +	else
> +		skl_enable_dc6(dev_priv);
> +}
> +
>  static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
>  					   struct i915_power_well *power_well)
>  {
> @@ -1546,6 +1570,14 @@ static const struct i915_power_well_ops skl_power_well_ops = {
>  	.is_enabled = skl_power_well_enabled,
>  };
>  
> +static const struct i915_power_well_ops skl_dc_state_ops = {
> +	.sync_hw = skl_dc6_sync_hw,
> +	/* To enable power we turn the DC state off */
> +	.enable = skl_dc6_state_off,
> +	.disable = skl_dc6_state_on,
> +	.is_enabled = skl_dc6_state_enabled,
> +};
> +
>  static struct i915_power_well hsw_power_wells[] = {
>  	{
>  		.name = "always-on",
> @@ -1745,6 +1777,11 @@ static struct i915_power_well skl_power_wells[] = {
>  		.ops = &skl_power_well_ops,
>  		.data = SKL_DISP_PW_DDI_D,
>  	},
> +	{
> +		.name = "DC6 state off",
> +		.domains = SKL_DISPLAY_DC6_OFF_POWER_DOMAINS,
> +		.ops = &skl_power_well_ops,
> +	},
>  };
>  
>  static struct i915_power_well bxt_power_wells[] = {
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patrik Jakobsson Sept. 14, 2015, 10:35 a.m. UTC | #2

On Mon, Sep 14, 2015 at 11:50:29AM +0200, Daniel Vetter wrote:
> On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > We need to be able to control if DC6 is allowed or not. Much like
> > requesting power to a specific piece of the hardware we need to be able
> > to request that we don't enter DC6 during certain hw access.
> > 
> > To solve this without introducing too much infrastructure I'm hooking
> > into the power well / power domain framework. DC6 prevention is modeled
> > much like an enabled power well. Thus I'm using the terminology on/off
> > for DC states instead of enable/disable.
> > 
> > The problem that started this work is the need for DC6 to be disabled
> > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > patch.
> > 
> > This is posted as an RFC since DMC and DC state handling is being
> > reworked and will possibly affect the outcome of this patch. The patch
> > has known warnings.
> > 
> > Signed-off-by: Patrik Jakobsson <[e-mail]>
> 
> Despite the naming suggesting its for power wells only this is exactly
> what the power well infrastructure is meant for: It's just our in-house
> struct power_domain for display runtime pm: They're hierachical and and
> refcounted with get/put. So from that pov lgtm.
> 
> But please cc the people working on the other dmc patches and coordinate
> with them.
> -Daniel

Great. We probably need a few patches from Ville for getting the correct aux
(A/B/C/D) in a nice way. Don't think they are on the list. I'll bundle those
patches when sending out the proper version of this patch.

Also, bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91876

CCed Damien, Ville and Animesh

-Patrik

> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> >  3 files changed, 64 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 4823184..c2c1ad2 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  
> > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > +
> >  		intel_dp_set_link_params(intel_dp, crtc->config);
> >  
> >  		intel_ddi_init_dp_buf_reg(intel_encoder);
> > @@ -2297,6 +2299,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> >  		intel_dp_complete_link_train(intel_dp);
> >  		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
> >  			intel_dp_stop_link_train(intel_dp);
> > +
> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> >  	} else if (type == INTEL_OUTPUT_HDMI) {
> >  		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> >  
> > @@ -2339,9 +2343,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> >  
> >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +
> > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > +
> >  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >  		intel_edp_panel_vdd_on(intel_dp);
> >  		intel_edp_panel_off(intel_dp);
> > +
> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> >  	}
> >  
> >  	if (IS_SKYLAKE(dev))
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 46484e4..82489ad 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1367,6 +1367,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
> >  			     bool override, unsigned int mask);
> >  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
> >  			  enum dpio_channel ch, bool override);
> > +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> > +void skl_disable_dc6(struct drm_i915_private *dev_priv);
> >  
> >  
> >  /* intel_pm.c */
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 3f682a1..e30c9a6 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -335,6 +335,10 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> >  	SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |		\
> >  	BIT(POWER_DOMAIN_PLLS) |			\
> >  	BIT(POWER_DOMAIN_INIT))
> > +#define SKL_DISPLAY_DC6_OFF_POWER_DOMAINS (		\
> > +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > +	BIT(POWER_DOMAIN_AUX_A))
> > +
> >  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
> >  	(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
> >  	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > @@ -550,7 +554,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> >  		"DC6 already programmed to be disabled.\n");
> >  }
> >  
> > -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > +void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >  {
> >  	uint32_t val;
> >  
> > @@ -567,7 +571,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >  	POSTING_READ(DC_STATE_EN);
> >  }
> >  
> > -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > +void skl_disable_dc6(struct drm_i915_private *dev_priv)
> >  {
> >  	uint32_t val;
> >  
> > @@ -628,10 +632,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  				!I915_READ(HSW_PWR_WELL_BIOS),
> >  				"Invalid for power well status to be enabled, unless done by the BIOS, \
> >  				when request is to disable!\n");
> > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > -				power_well->data == SKL_DISP_PW_2) {
> > +			if (power_well->data == SKL_DISP_PW_2) {
> >  				if (SKL_ENABLE_DC6(dev)) {
> > -					skl_disable_dc6(dev_priv);
> >  					/*
> >  					 * DDI buffer programming unnecessary during driver-load/resume
> >  					 * as it's already done during modeset initialization then.
> > @@ -639,10 +641,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  					 */
> >  					if (!dev_priv->power_domains.initializing)
> >  						intel_prepare_ddi(dev);
> > -				} else {
> > -					gen9_disable_dc5(dev_priv);
> >  				}
> >  			}
> > +
> >  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> >  		}
> >  
> > @@ -660,8 +661,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> >  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> >  
> > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > -				power_well->data == SKL_DISP_PW_2) {
> > +			if (power_well->data == SKL_DISP_PW_2) {
> >  				enum csr_state state;
> >  				/* TODO: wait for a completion event or
> >  				 * similar here instead of busy
> > @@ -669,14 +669,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  				 */
> >  				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> >  						FW_UNINITIALIZED, 1000);
> > -				if (state != FW_LOADED)
> > +				if (state != FW_LOADED) {
> >  					DRM_ERROR("CSR firmware not ready (%d)\n",
> > -							state);
> > -				else
> > -					if (SKL_ENABLE_DC6(dev))
> > -						skl_enable_dc6(dev_priv);
> > -					else
> > -						gen9_enable_dc5(dev_priv);
> > +						  state);
> > +				}
> >  			}
> >  		}
> >  	}
> > @@ -752,6 +748,34 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
> >  	skl_set_power_well(dev_priv, power_well, false);
> >  }
> >  
> > +static bool skl_dc6_state_enabled(struct drm_i915_private *dev_priv,
> > +				  struct i915_power_well *power_well)
> > +{
> > +	/* Return true if disabling of DC6 is enabled */
> > +	return !(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6);
> > +}
> > +
> > +static void skl_dc6_state_on(struct drm_i915_private *dev_priv,
> > +			     struct i915_power_well *power_well)
> > +{
> > +	skl_enable_dc6(dev_priv);
> > +}
> > +
> > +static void skl_dc6_state_off(struct drm_i915_private *dev_priv,
> > +			      struct i915_power_well *power_well)
> > +{
> > +	skl_disable_dc6(dev_priv);
> > +}
> > +
> > +static void skl_dc6_sync_hw(struct drm_i915_private *dev_priv,
> > +			    struct i915_power_well *power_well)
> > +{
> > +	if (power_well->count > 0)
> > +		skl_disable_dc6(dev_priv);
> > +	else
> > +		skl_enable_dc6(dev_priv);
> > +}
> > +
> >  static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
> >  					   struct i915_power_well *power_well)
> >  {
> > @@ -1546,6 +1570,14 @@ static const struct i915_power_well_ops skl_power_well_ops = {
> >  	.is_enabled = skl_power_well_enabled,
> >  };
> >  
> > +static const struct i915_power_well_ops skl_dc_state_ops = {
> > +	.sync_hw = skl_dc6_sync_hw,
> > +	/* To enable power we turn the DC state off */
> > +	.enable = skl_dc6_state_off,
> > +	.disable = skl_dc6_state_on,
> > +	.is_enabled = skl_dc6_state_enabled,
> > +};
> > +
> >  static struct i915_power_well hsw_power_wells[] = {
> >  	{
> >  		.name = "always-on",
> > @@ -1745,6 +1777,11 @@ static struct i915_power_well skl_power_wells[] = {
> >  		.ops = &skl_power_well_ops,
> >  		.data = SKL_DISP_PW_DDI_D,
> >  	},
> > +	{
> > +		.name = "DC6 state off",
> > +		.domains = SKL_DISPLAY_DC6_OFF_POWER_DOMAINS,
> > +		.ops = &skl_power_well_ops,
> > +	},
> >  };
> >  
> >  static struct i915_power_well bxt_power_wells[] = {
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Ville Syrjälä Sept. 16, 2015, 8:10 p.m. UTC | #3

On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> We need to be able to control if DC6 is allowed or not. Much like
> requesting power to a specific piece of the hardware we need to be able
> to request that we don't enter DC6 during certain hw access.
> 
> To solve this without introducing too much infrastructure I'm hooking
> into the power well / power domain framework. DC6 prevention is modeled
> much like an enabled power well. Thus I'm using the terminology on/off
> for DC states instead of enable/disable.
> 
> The problem that started this work is the need for DC6 to be disabled
> when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> patch.
> 
> This is posted as an RFC since DMC and DC state handling is being
> reworked and will possibly affect the outcome of this patch. The patch
> has known warnings.
> 
> Signed-off-by: Patrik Jakobsson <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
>  drivers/gpu/drm/i915/intel_drv.h        |  2 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
>  3 files changed, 64 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 4823184..c2c1ad2 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  
> +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> +

These I think shouldn't be necessary with my
intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
itself grab the appropriate power domain.

That's of course assuming that AUX is the only reason why we need to
keep DC6 disabled here.

>  		intel_dp_set_link_params(intel_dp, crtc->config);
>  
>  		intel_ddi_init_dp_buf_reg(intel_encoder);
> @@ -2297,6 +2299,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>  		intel_dp_complete_link_train(intel_dp);
>  		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
>  			intel_dp_stop_link_train(intel_dp);
> +
> +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
>  	} else if (type == INTEL_OUTPUT_HDMI) {
>  		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>  
> @@ -2339,9 +2343,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>  
>  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> +
>  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>  		intel_edp_panel_vdd_on(intel_dp);
>  		intel_edp_panel_off(intel_dp);
> +
> +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
>  	}
>  
>  	if (IS_SKYLAKE(dev))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 46484e4..82489ad 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1367,6 +1367,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
>  			     bool override, unsigned int mask);
>  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
>  			  enum dpio_channel ch, bool override);
> +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> +void skl_disable_dc6(struct drm_i915_private *dev_priv);
>  
>  
>  /* intel_pm.c */
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 3f682a1..e30c9a6 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -335,6 +335,10 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>  	SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |		\
>  	BIT(POWER_DOMAIN_PLLS) |			\
>  	BIT(POWER_DOMAIN_INIT))
> +#define SKL_DISPLAY_DC6_OFF_POWER_DOMAINS (		\
> +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> +	BIT(POWER_DOMAIN_AUX_A))
> +
>  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
>  	(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
>  	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> @@ -550,7 +554,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
>  		"DC6 already programmed to be disabled.\n");
>  }
>  
> -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> +void skl_enable_dc6(struct drm_i915_private *dev_priv)
>  {
>  	uint32_t val;
>  
> @@ -567,7 +571,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
>  	POSTING_READ(DC_STATE_EN);
>  }
>  
> -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> +void skl_disable_dc6(struct drm_i915_private *dev_priv)
>  {
>  	uint32_t val;
>  
> @@ -628,10 +632,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  				!I915_READ(HSW_PWR_WELL_BIOS),
>  				"Invalid for power well status to be enabled, unless done by the BIOS, \
>  				when request is to disable!\n");
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> +			if (power_well->data == SKL_DISP_PW_2) {
>  				if (SKL_ENABLE_DC6(dev)) {
> -					skl_disable_dc6(dev_priv);

Hmm. So the ordering needs to be 
disable dc6 -> enable pw2

>  					/*
>  					 * DDI buffer programming unnecessary during driver-load/resume
>  					 * as it's already done during modeset initialization then.
> @@ -639,10 +641,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  					 */
>  					if (!dev_priv->power_domains.initializing)
>  						intel_prepare_ddi(dev);
> -				} else {
> -					gen9_disable_dc5(dev_priv);
>  				}
>  			}
> +
>  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
>  		}
>  
> @@ -660,8 +661,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  			POSTING_READ(HSW_PWR_WELL_DRIVER);
>  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>  
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> +			if (power_well->data == SKL_DISP_PW_2) {
>  				enum csr_state state;
>  				/* TODO: wait for a completion event or
>  				 * similar here instead of busy
> @@ -669,14 +669,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  				 */
>  				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
>  						FW_UNINITIALIZED, 1000);
> -				if (state != FW_LOADED)
> +				if (state != FW_LOADED) {
>  					DRM_ERROR("CSR firmware not ready (%d)\n",
> -							state);
> -				else
> -					if (SKL_ENABLE_DC6(dev))
> -						skl_enable_dc6(dev_priv);
> -					else
> -						gen9_enable_dc5(dev_priv);
> +						  state);
> +				}

and here we need 
disable pw2 -> enable dc6

That's symmetric which is great for using power wells here since we walk
the power wells array forward when enabling, and backwards when
disabling.

I'm thinking that we could also move the dc5 stuff into a power well.
Would reduce the clutter in skl_set_power_well() at least. I'm not sure
what the requirements wrt. dc5 are. If they are the same as for dc6,
then a single dc power well would do, otherwise we would need two, each
with its own domains.

>  			}
>  		}
>  	}
> @@ -752,6 +748,34 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
>  	skl_set_power_well(dev_priv, power_well, false);
>  }
>  
> +static bool skl_dc6_state_enabled(struct drm_i915_private *dev_priv,
> +				  struct i915_power_well *power_well)
> +{
> +	/* Return true if disabling of DC6 is enabled */
> +	return !(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6);
> +}
> +
> +static void skl_dc6_state_on(struct drm_i915_private *dev_priv,
> +			     struct i915_power_well *power_well)
> +{
> +	skl_enable_dc6(dev_priv);
> +}
> +
> +static void skl_dc6_state_off(struct drm_i915_private *dev_priv,
> +			      struct i915_power_well *power_well)
> +{
> +	skl_disable_dc6(dev_priv);
> +}
> +
> +static void skl_dc6_sync_hw(struct drm_i915_private *dev_priv,
> +			    struct i915_power_well *power_well)
> +{
> +	if (power_well->count > 0)
> +		skl_disable_dc6(dev_priv);
> +	else
> +		skl_enable_dc6(dev_priv);
> +}

Nit: Looks like we usuall have these in the following order
sync_hw
enable
disable

'enabled' is a bit all over the place usually. I guess before or after the
rest is fine.

I'm not really sure how I would name these. The current naming doesn't
really tell me they're power well ops. Maybe
skl_dc6_off_power_well_{enable,disable,sync_hw,enabled}() ?

> +
>  static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
>  					   struct i915_power_well *power_well)
>  {
> @@ -1546,6 +1570,14 @@ static const struct i915_power_well_ops skl_power_well_ops = {
>  	.is_enabled = skl_power_well_enabled,
>  };
>  
> +static const struct i915_power_well_ops skl_dc_state_ops = {

_dc6_, and naming to match how the function are finally named of
course.

> +	.sync_hw = skl_dc6_sync_hw,
> +	/* To enable power we turn the DC state off */
> +	.enable = skl_dc6_state_off,
> +	.disable = skl_dc6_state_on,
> +	.is_enabled = skl_dc6_state_enabled,
> +};
> +
>  static struct i915_power_well hsw_power_wells[] = {
>  	{
>  		.name = "always-on",
> @@ -1745,6 +1777,11 @@ static struct i915_power_well skl_power_wells[] = {
>  		.ops = &skl_power_well_ops,
>  		.data = SKL_DISP_PW_DDI_D,
>  	},
> +	{
> +		.name = "DC6 state off",

Just "DC6 off" perhaps?

I wasn't able to think of a nice way to name this so that it would make
more sense in the logs. With this we would get 
'enabling DC6 off' and 'disabling DC6 off' which is a bit confusing.
Maybe we should at least put quotes around the power well name in the
debug message to make it a bit less funky? Probably not worth it to
add support for sully customized enable/disable log message.

> +		.domains = SKL_DISPLAY_DC6_OFF_POWER_DOMAINS,
> +		.ops = &skl_power_well_ops,

Surely you want to use the new ops you created? :)

And here we need to be careful where in the list we insert the power
well. Based on what we saw earlier, the right place should be just
before PW2. That way DC6 gets disabled before PW2 is enabled, and
PW2 gets disabled before DC6 gets enabled.

> +	},
>  };
>  
>  static struct i915_power_well bxt_power_wells[] = {
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjälä Sept. 17, 2015, 11:14 a.m. UTC | #4

On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > We need to be able to control if DC6 is allowed or not. Much like
> > requesting power to a specific piece of the hardware we need to be able
> > to request that we don't enter DC6 during certain hw access.
> > 
> > To solve this without introducing too much infrastructure I'm hooking
> > into the power well / power domain framework. DC6 prevention is modeled
> > much like an enabled power well. Thus I'm using the terminology on/off
> > for DC states instead of enable/disable.
> > 
> > The problem that started this work is the need for DC6 to be disabled
> > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > patch.
> > 
> > This is posted as an RFC since DMC and DC state handling is being
> > reworked and will possibly affect the outcome of this patch. The patch
> > has known warnings.
> > 
> > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> >  3 files changed, 64 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 4823184..c2c1ad2 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  
> > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > +
> 
> These I think shouldn't be necessary with my
> intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> itself grab the appropriate power domain.
> 
> That's of course assuming that AUX is the only reason why we need to
> keep DC6 disabled here.
> 
> >  		intel_dp_set_link_params(intel_dp, crtc->config);
> >  
> >  		intel_ddi_init_dp_buf_reg(intel_encoder);
> > @@ -2297,6 +2299,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> >  		intel_dp_complete_link_train(intel_dp);
> >  		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
> >  			intel_dp_stop_link_train(intel_dp);
> > +
> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> >  	} else if (type == INTEL_OUTPUT_HDMI) {
> >  		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> >  
> > @@ -2339,9 +2343,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> >  
> >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +
> > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > +
> >  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >  		intel_edp_panel_vdd_on(intel_dp);
> >  		intel_edp_panel_off(intel_dp);
> > +
> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> >  	}
> >  
> >  	if (IS_SKYLAKE(dev))
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 46484e4..82489ad 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1367,6 +1367,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
> >  			     bool override, unsigned int mask);
> >  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
> >  			  enum dpio_channel ch, bool override);
> > +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> > +void skl_disable_dc6(struct drm_i915_private *dev_priv);
> >  
> >  
> >  /* intel_pm.c */
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 3f682a1..e30c9a6 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -335,6 +335,10 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> >  	SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |		\
> >  	BIT(POWER_DOMAIN_PLLS) |			\
> >  	BIT(POWER_DOMAIN_INIT))
> > +#define SKL_DISPLAY_DC6_OFF_POWER_DOMAINS (		\
> > +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > +	BIT(POWER_DOMAIN_AUX_A))
> > +
> >  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
> >  	(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
> >  	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > @@ -550,7 +554,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> >  		"DC6 already programmed to be disabled.\n");
> >  }
> >  
> > -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > +void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >  {
> >  	uint32_t val;
> >  
> > @@ -567,7 +571,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >  	POSTING_READ(DC_STATE_EN);
> >  }
> >  
> > -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > +void skl_disable_dc6(struct drm_i915_private *dev_priv)
> >  {
> >  	uint32_t val;
> >  
> > @@ -628,10 +632,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  				!I915_READ(HSW_PWR_WELL_BIOS),
> >  				"Invalid for power well status to be enabled, unless done by the BIOS, \
> >  				when request is to disable!\n");
> > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > -				power_well->data == SKL_DISP_PW_2) {
> > +			if (power_well->data == SKL_DISP_PW_2) {
> >  				if (SKL_ENABLE_DC6(dev)) {
> > -					skl_disable_dc6(dev_priv);
> 
> Hmm. So the ordering needs to be 
> disable dc6 -> enable pw2
> 
> >  					/*
> >  					 * DDI buffer programming unnecessary during driver-load/resume
> >  					 * as it's already done during modeset initialization then.
> > @@ -639,10 +641,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  					 */
> >  					if (!dev_priv->power_domains.initializing)
> >  						intel_prepare_ddi(dev);
> > -				} else {
> > -					gen9_disable_dc5(dev_priv);
> >  				}
> >  			}
> > +
> >  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> >  		}
> >  
> > @@ -660,8 +661,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> >  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> >  
> > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > -				power_well->data == SKL_DISP_PW_2) {
> > +			if (power_well->data == SKL_DISP_PW_2) {
> >  				enum csr_state state;
> >  				/* TODO: wait for a completion event or
> >  				 * similar here instead of busy
> > @@ -669,14 +669,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  				 */
> >  				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> >  						FW_UNINITIALIZED, 1000);
> > -				if (state != FW_LOADED)
> > +				if (state != FW_LOADED) {
> >  					DRM_ERROR("CSR firmware not ready (%d)\n",
> > -							state);
> > -				else
> > -					if (SKL_ENABLE_DC6(dev))
> > -						skl_enable_dc6(dev_priv);
> > -					else
> > -						gen9_enable_dc5(dev_priv);
> > +						  state);
> > +				}
> 
> and here we need 
> disable pw2 -> enable dc6
> 
> That's symmetric which is great for using power wells here since we walk
> the power wells array forward when enabling, and backwards when
> disabling.
> 
> I'm thinking that we could also move the dc5 stuff into a power well.
> Would reduce the clutter in skl_set_power_well() at least. I'm not sure
> what the requirements wrt. dc5 are. If they are the same as for dc6,
> then a single dc power well would do, otherwise we would need two, each
> with its own domains.

BTW after a bit more look, I think we could probably simplify things
quite a bit with this move. We could perhaps then set power_well->data
to DC_STATE_EN_UPTO_DC5 or DC_STATE_EN_UPTO_DC6 for each well, and
convert the enable/disable dc5/6 into somehting like:

gen9_enable_dc_state(dev_priv, uint32_t state)
{
	// csr wait and the debugmask thing could go here

	val = I915_READ(DC_STATE_EN);
	val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
	val |= state;
	I915_WRITE(DC_STATE_EN, val);
	POSTING_READ(DC_STATE_EN);
}

gen9_disable_dc_state(dev_priv, uint32_t val)
{
	uint32_t val;

	val = I915_READ(DC_STATE_EN);
	val &= ~state;
	I915_WRITE(DC_STATE_EN, val);
	POSTING_READ(DC_STATE_EN);
}

We could even put these directly in the power well hooks, and share
those for DC5 and DC6. But that would perhaps mean losing the
can_enable_disable_dc5/6 asserts or we'd need some ifs for those.
But perhaps it would be cleaners to have separate power well ops for dc5
and dc6, and each would just call the proposed gen9_enable/disable_dc_state()
functions. Oh and we also have the GEN9_ENABLE_DC5 and SKL_ENABLE_DC6
macros which I supposed we'd need to check in the power well hooks.

But this unification could be a separate patch. First we can just
introduce the new power wells using the existing dc5/dc6 enable/disable
functions we have.

I didn't look at the dc9 stuff yet, so not sure if that could be handled
in a similar fashion.

Also I think currently we just disable runtime PM when the firmware
hasn't yet been loaded. But I think we would need to change that to hold
a DC5/DC5 references. I guess to do this properly we should a new power
domain for this purpose, but I'm not sure that's really worth it for a
single user use case.

Ville Syrjälä Sept. 17, 2015, 11:45 a.m. UTC | #5

On Thu, Sep 17, 2015 at 02:14:37PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > We need to be able to control if DC6 is allowed or not. Much like
> > > requesting power to a specific piece of the hardware we need to be able
> > > to request that we don't enter DC6 during certain hw access.
> > > 
> > > To solve this without introducing too much infrastructure I'm hooking
> > > into the power well / power domain framework. DC6 prevention is modeled
> > > much like an enabled power well. Thus I'm using the terminology on/off
> > > for DC states instead of enable/disable.
> > > 
> > > The problem that started this work is the need for DC6 to be disabled
> > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > patch.
> > > 
> > > This is posted as an RFC since DMC and DC state handling is being
> > > reworked and will possibly affect the outcome of this patch. The patch
> > > has known warnings.
> > > 
> > > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> > >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > >  3 files changed, 64 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 4823184..c2c1ad2 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > >  
> > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > +
> > 
> > These I think shouldn't be necessary with my
> > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > itself grab the appropriate power domain.
> > 
> > That's of course assuming that AUX is the only reason why we need to
> > keep DC6 disabled here.
> > 
> > >  		intel_dp_set_link_params(intel_dp, crtc->config);
> > >  
> > >  		intel_ddi_init_dp_buf_reg(intel_encoder);
> > > @@ -2297,6 +2299,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > >  		intel_dp_complete_link_train(intel_dp);
> > >  		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
> > >  			intel_dp_stop_link_train(intel_dp);
> > > +
> > > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> > >  	} else if (type == INTEL_OUTPUT_HDMI) {
> > >  		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> > >  
> > > @@ -2339,9 +2343,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> > >  
> > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > +
> > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > +
> > >  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > >  		intel_edp_panel_vdd_on(intel_dp);
> > >  		intel_edp_panel_off(intel_dp);
> > > +
> > > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> > >  	}
> > >  
> > >  	if (IS_SKYLAKE(dev))
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 46484e4..82489ad 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1367,6 +1367,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
> > >  			     bool override, unsigned int mask);
> > >  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
> > >  			  enum dpio_channel ch, bool override);
> > > +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> > > +void skl_disable_dc6(struct drm_i915_private *dev_priv);
> > >  
> > >  
> > >  /* intel_pm.c */
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 3f682a1..e30c9a6 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -335,6 +335,10 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > >  	SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |		\
> > >  	BIT(POWER_DOMAIN_PLLS) |			\
> > >  	BIT(POWER_DOMAIN_INIT))
> > > +#define SKL_DISPLAY_DC6_OFF_POWER_DOMAINS (		\
> > > +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > > +	BIT(POWER_DOMAIN_AUX_A))
> > > +
> > >  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
> > >  	(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
> > >  	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > > @@ -550,7 +554,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> > >  		"DC6 already programmed to be disabled.\n");
> > >  }
> > >  
> > > -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > > +void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > >  {
> > >  	uint32_t val;
> > >  
> > > @@ -567,7 +571,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > >  	POSTING_READ(DC_STATE_EN);
> > >  }
> > >  
> > > -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > > +void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > >  {
> > >  	uint32_t val;
> > >  
> > > @@ -628,10 +632,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >  				!I915_READ(HSW_PWR_WELL_BIOS),
> > >  				"Invalid for power well status to be enabled, unless done by the BIOS, \
> > >  				when request is to disable!\n");
> > > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > > -				power_well->data == SKL_DISP_PW_2) {
> > > +			if (power_well->data == SKL_DISP_PW_2) {
> > >  				if (SKL_ENABLE_DC6(dev)) {
> > > -					skl_disable_dc6(dev_priv);
> > 
> > Hmm. So the ordering needs to be 
> > disable dc6 -> enable pw2
> > 
> > >  					/*
> > >  					 * DDI buffer programming unnecessary during driver-load/resume
> > >  					 * as it's already done during modeset initialization then.
> > > @@ -639,10 +641,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >  					 */
> > >  					if (!dev_priv->power_domains.initializing)
> > >  						intel_prepare_ddi(dev);
> > > -				} else {
> > > -					gen9_disable_dc5(dev_priv);
> > >  				}
> > >  			}
> > > +
> > >  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> > >  		}
> > >  
> > > @@ -660,8 +661,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> > >  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> > >  
> > > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > > -				power_well->data == SKL_DISP_PW_2) {
> > > +			if (power_well->data == SKL_DISP_PW_2) {
> > >  				enum csr_state state;
> > >  				/* TODO: wait for a completion event or
> > >  				 * similar here instead of busy
> > > @@ -669,14 +669,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >  				 */
> > >  				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> > >  						FW_UNINITIALIZED, 1000);
> > > -				if (state != FW_LOADED)
> > > +				if (state != FW_LOADED) {
> > >  					DRM_ERROR("CSR firmware not ready (%d)\n",
> > > -							state);
> > > -				else
> > > -					if (SKL_ENABLE_DC6(dev))
> > > -						skl_enable_dc6(dev_priv);
> > > -					else
> > > -						gen9_enable_dc5(dev_priv);
> > > +						  state);
> > > +				}
> > 
> > and here we need 
> > disable pw2 -> enable dc6
> > 
> > That's symmetric which is great for using power wells here since we walk
> > the power wells array forward when enabling, and backwards when
> > disabling.
> > 
> > I'm thinking that we could also move the dc5 stuff into a power well.
> > Would reduce the clutter in skl_set_power_well() at least. I'm not sure
> > what the requirements wrt. dc5 are. If they are the same as for dc6,
> > then a single dc power well would do, otherwise we would need two, each
> > with its own domains.
> 
> BTW after a bit more look, I think we could probably simplify things
> quite a bit with this move. We could perhaps then set power_well->data
> to DC_STATE_EN_UPTO_DC5 or DC_STATE_EN_UPTO_DC6 for each well, and
> convert the enable/disable dc5/6 into somehting like:
> 
> gen9_enable_dc_state(dev_priv, uint32_t state)
> {
> 	// csr wait and the debugmask thing could go here
> 
> 	val = I915_READ(DC_STATE_EN);
> 	val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
> 	val |= state;
> 	I915_WRITE(DC_STATE_EN, val);
> 	POSTING_READ(DC_STATE_EN);
> }
> 
> gen9_disable_dc_state(dev_priv, uint32_t val)
> {
> 	uint32_t val;
> 
> 	val = I915_READ(DC_STATE_EN);
> 	val &= ~state;
> 	I915_WRITE(DC_STATE_EN, val);
> 	POSTING_READ(DC_STATE_EN);
> }
> 
> We could even put these directly in the power well hooks, and share
> those for DC5 and DC6. But that would perhaps mean losing the
> can_enable_disable_dc5/6 asserts or we'd need some ifs for those.
> But perhaps it would be cleaners to have separate power well ops for dc5
> and dc6, and each would just call the proposed gen9_enable/disable_dc_state()
> functions. Oh and we also have the GEN9_ENABLE_DC5 and SKL_ENABLE_DC6
> macros which I supposed we'd need to check in the power well hooks.
> 
> But this unification could be a separate patch. First we can just
> introduce the new power wells using the existing dc5/dc6 enable/disable
> functions we have.
> 
> I didn't look at the dc9 stuff yet, so not sure if that could be handled
> in a similar fashion.
> 
> Also I think currently we just disable runtime PM when the firmware
> hasn't yet been loaded. But I think we would need to change that to hold
> a DC5/DC5 references. I guess to do this properly we should a new power
> domain for this purpose, but I'm not sure that's really worth it for a
> single user use case.

I just had the idea that POWER_DOMAIN_INIT might be a nice fit for this,
but that would also keep the DDI power wells on even though we don't
need the firmware for those.

Patrik Jakobsson Sept. 21, 2015, 8 a.m. UTC | #6

On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > We need to be able to control if DC6 is allowed or not. Much like
> > requesting power to a specific piece of the hardware we need to be able
> > to request that we don't enter DC6 during certain hw access.
> > 
> > To solve this without introducing too much infrastructure I'm hooking
> > into the power well / power domain framework. DC6 prevention is modeled
> > much like an enabled power well. Thus I'm using the terminology on/off
> > for DC states instead of enable/disable.
> > 
> > The problem that started this work is the need for DC6 to be disabled
> > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > patch.
> > 
> > This is posted as an RFC since DMC and DC state handling is being
> > reworked and will possibly affect the outcome of this patch. The patch
> > has known warnings.
> > 
> > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> >  3 files changed, 64 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 4823184..c2c1ad2 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  
> > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > +
> 
> These I think shouldn't be necessary with my
> intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> itself grab the appropriate power domain.
> 
> That's of course assuming that AUX is the only reason why we need to
> keep DC6 disabled here.
> 

The upside with having get/put around bigger aux transfers is that we don't get
tons of enable/disable lines in the log. My vote is that we keep this but also
have your fine-grained get/puts.

We also have an extra enable disable print in skl_disable_dc6() /
skl_enable_dc6() which I think should be removed unless various DC on/off hacks
are required outside of the power wells framework.

> >  		intel_dp_set_link_params(intel_dp, crtc->config);
> >  
> >  		intel_ddi_init_dp_buf_reg(intel_encoder);
> > @@ -2297,6 +2299,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> >  		intel_dp_complete_link_train(intel_dp);
> >  		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
> >  			intel_dp_stop_link_train(intel_dp);
> > +
> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> >  	} else if (type == INTEL_OUTPUT_HDMI) {
> >  		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> >  
> > @@ -2339,9 +2343,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> >  
> >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +
> > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > +
> >  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >  		intel_edp_panel_vdd_on(intel_dp);
> >  		intel_edp_panel_off(intel_dp);
> > +
> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> >  	}
> >  
> >  	if (IS_SKYLAKE(dev))
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 46484e4..82489ad 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1367,6 +1367,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
> >  			     bool override, unsigned int mask);
> >  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
> >  			  enum dpio_channel ch, bool override);
> > +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> > +void skl_disable_dc6(struct drm_i915_private *dev_priv);
> >  
> >  
> >  /* intel_pm.c */
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 3f682a1..e30c9a6 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -335,6 +335,10 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> >  	SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |		\
> >  	BIT(POWER_DOMAIN_PLLS) |			\
> >  	BIT(POWER_DOMAIN_INIT))
> > +#define SKL_DISPLAY_DC6_OFF_POWER_DOMAINS (		\
> > +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > +	BIT(POWER_DOMAIN_AUX_A))
> > +
> >  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
> >  	(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
> >  	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > @@ -550,7 +554,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> >  		"DC6 already programmed to be disabled.\n");
> >  }
> >  
> > -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > +void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >  {
> >  	uint32_t val;
> >  
> > @@ -567,7 +571,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >  	POSTING_READ(DC_STATE_EN);
> >  }
> >  
> > -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > +void skl_disable_dc6(struct drm_i915_private *dev_priv)
> >  {
> >  	uint32_t val;
> >  
> > @@ -628,10 +632,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  				!I915_READ(HSW_PWR_WELL_BIOS),
> >  				"Invalid for power well status to be enabled, unless done by the BIOS, \
> >  				when request is to disable!\n");
> > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > -				power_well->data == SKL_DISP_PW_2) {
> > +			if (power_well->data == SKL_DISP_PW_2) {
> >  				if (SKL_ENABLE_DC6(dev)) {
> > -					skl_disable_dc6(dev_priv);
> 
> Hmm. So the ordering needs to be 
> disable dc6 -> enable pw2

I don't know why DC6 is required for PW2 and at this point I don't see why the
ordering should matter. Could you please explain?

> >  					/*
> >  					 * DDI buffer programming unnecessary during driver-load/resume
> >  					 * as it's already done during modeset initialization then.
> > @@ -639,10 +641,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  					 */
> >  					if (!dev_priv->power_domains.initializing)
> >  						intel_prepare_ddi(dev);
> > -				} else {
> > -					gen9_disable_dc5(dev_priv);
> >  				}
> >  			}
> > +
> >  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> >  		}
> >  
> > @@ -660,8 +661,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> >  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> >  
> > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > -				power_well->data == SKL_DISP_PW_2) {
> > +			if (power_well->data == SKL_DISP_PW_2) {
> >  				enum csr_state state;
> >  				/* TODO: wait for a completion event or
> >  				 * similar here instead of busy
> > @@ -669,14 +669,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  				 */
> >  				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> >  						FW_UNINITIALIZED, 1000);
> > -				if (state != FW_LOADED)
> > +				if (state != FW_LOADED) {
> >  					DRM_ERROR("CSR firmware not ready (%d)\n",
> > -							state);
> > -				else
> > -					if (SKL_ENABLE_DC6(dev))
> > -						skl_enable_dc6(dev_priv);
> > -					else
> > -						gen9_enable_dc5(dev_priv);
> > +						  state);
> > +				}
> 
> and here we need 
> disable pw2 -> enable dc6
> 
> That's symmetric which is great for using power wells here since we walk
> the power wells array forward when enabling, and backwards when
> disabling.
> 
> I'm thinking that we could also move the dc5 stuff into a power well.
> Would reduce the clutter in skl_set_power_well() at least. I'm not sure
> what the requirements wrt. dc5 are. If they are the same as for dc6,
> then a single dc power well would do, otherwise we would need two, each
> with its own domains.

From what I've heard we don't need to care about DC5 atm. But I also think that
a power well for DC5 is the way to go. Need to check with Damien what the
requirements for DC5 are.

> >  			}
> >  		}
> >  	}
> > @@ -752,6 +748,34 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
> >  	skl_set_power_well(dev_priv, power_well, false);
> >  }
> >  
> > +static bool skl_dc6_state_enabled(struct drm_i915_private *dev_priv,
> > +				  struct i915_power_well *power_well)
> > +{
> > +	/* Return true if disabling of DC6 is enabled */
> > +	return !(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6);
> > +}
> > +
> > +static void skl_dc6_state_on(struct drm_i915_private *dev_priv,
> > +			     struct i915_power_well *power_well)
> > +{
> > +	skl_enable_dc6(dev_priv);
> > +}
> > +
> > +static void skl_dc6_state_off(struct drm_i915_private *dev_priv,
> > +			      struct i915_power_well *power_well)
> > +{
> > +	skl_disable_dc6(dev_priv);
> > +}
> > +
> > +static void skl_dc6_sync_hw(struct drm_i915_private *dev_priv,
> > +			    struct i915_power_well *power_well)
> > +{
> > +	if (power_well->count > 0)
> > +		skl_disable_dc6(dev_priv);
> > +	else
> > +		skl_enable_dc6(dev_priv);
> > +}
> 
> Nit: Looks like we usuall have these in the following order
> sync_hw
> enable
> disable
> 
> 'enabled' is a bit all over the place usually. I guess before or after the
> rest is fine.

Yes, better keep the current order.

> I'm not really sure how I would name these. The current naming doesn't
> really tell me they're power well ops. Maybe
> skl_dc6_off_power_well_{enable,disable,sync_hw,enabled}() ?

I agree, better make it clear that they are pw ops.

> > +
> >  static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
> >  					   struct i915_power_well *power_well)
> >  {
> > @@ -1546,6 +1570,14 @@ static const struct i915_power_well_ops skl_power_well_ops = {
> >  	.is_enabled = skl_power_well_enabled,
> >  };
> >  
> > +static const struct i915_power_well_ops skl_dc_state_ops = {
> 
> _dc6_, and naming to match how the function are finally named of
> course.
> 
> > +	.sync_hw = skl_dc6_sync_hw,
> > +	/* To enable power we turn the DC state off */
> > +	.enable = skl_dc6_state_off,
> > +	.disable = skl_dc6_state_on,
> > +	.is_enabled = skl_dc6_state_enabled,
> > +};
> > +
> >  static struct i915_power_well hsw_power_wells[] = {
> >  	{
> >  		.name = "always-on",
> > @@ -1745,6 +1777,11 @@ static struct i915_power_well skl_power_wells[] = {
> >  		.ops = &skl_power_well_ops,
> >  		.data = SKL_DISP_PW_DDI_D,
> >  	},
> > +	{
> > +		.name = "DC6 state off",
> 
> Just "DC6 off" perhaps?
> 
> I wasn't able to think of a nice way to name this so that it would make
> more sense in the logs. With this we would get 
> 'enabling DC6 off' and 'disabling DC6 off' which is a bit confusing.
> Maybe we should at least put quotes around the power well name in the
> debug message to make it a bit less funky? Probably not worth it to
> add support for sully customized enable/disable log message.

Agreed

> > +		.domains = SKL_DISPLAY_DC6_OFF_POWER_DOMAINS,
> > +		.ops = &skl_power_well_ops,
> 
> Surely you want to use the new ops you created? :)

Oops :)

> And here we need to be careful where in the list we insert the power
> well. Based on what we saw earlier, the right place should be just
> before PW2. That way DC6 gets disabled before PW2 is enabled, and
> PW2 gets disabled before DC6 gets enabled.

Yes, regardless of the ordering requirements it makes sense to move it up.

Thanks for having a look
-Patrik

> > +	},
> >  };
> >  
> >  static struct i915_power_well bxt_power_wells[] = {
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

Jani Nikula Sept. 21, 2015, 8:26 a.m. UTC | #7

On Mon, 21 Sep 2015, Patrik Jakobsson <[e-mail]> wrote:
> On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
>> On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
>> > We need to be able to control if DC6 is allowed or not. Much like
>> > requesting power to a specific piece of the hardware we need to be able
>> > to request that we don't enter DC6 during certain hw access.
>> > 
>> > To solve this without introducing too much infrastructure I'm hooking
>> > into the power well / power domain framework. DC6 prevention is modeled
>> > much like an enabled power well. Thus I'm using the terminology on/off
>> > for DC states instead of enable/disable.
>> > 
>> > The problem that started this work is the need for DC6 to be disabled
>> > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
>> > patch.
>> > 
>> > This is posted as an RFC since DMC and DC state handling is being
>> > reworked and will possibly affect the outcome of this patch. The patch
>> > has known warnings.
>> > 
>> > Signed-off-by: Patrik Jakobsson <[e-mail]>
>> > ---
>> >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
>> >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
>> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
>> >  3 files changed, 64 insertions(+), 16 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> > index 4823184..c2c1ad2 100644
>> > --- a/drivers/gpu/drm/i915/intel_ddi.c
>> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>> >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
>> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>> >  
>> > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
>> > +
>> 
>> These I think shouldn't be necessary with my
>> intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
>> itself grab the appropriate power domain.

Are you referring to stuff that is merged? Am I missing something?

>> 
>> That's of course assuming that AUX is the only reason why we need to
>> keep DC6 disabled here.
>> 
>
> The upside with having get/put around bigger aux transfers is that we
> don't get tons of enable/disable lines in the log. My vote is that we
> keep this but also have your fine-grained get/puts.

If it works without (and everything Ville is referring to above is
merged), I'd split these to a separate patch, and we can judge the
improvement on its own.

BR,
Jani.


>
> We also have an extra enable disable print in skl_disable_dc6() /
> skl_enable_dc6() which I think should be removed unless various DC on/off hacks
> are required outside of the power wells framework.
>
>> >  		intel_dp_set_link_params(intel_dp, crtc->config);
>> >  
>> >  		intel_ddi_init_dp_buf_reg(intel_encoder);
>> > @@ -2297,6 +2299,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>> >  		intel_dp_complete_link_train(intel_dp);
>> >  		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
>> >  			intel_dp_stop_link_train(intel_dp);
>> > +
>> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
>> >  	} else if (type == INTEL_OUTPUT_HDMI) {
>> >  		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>> >  
>> > @@ -2339,9 +2343,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>> >  
>> >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
>> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>> > +
>> > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
>> > +
>> >  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>> >  		intel_edp_panel_vdd_on(intel_dp);
>> >  		intel_edp_panel_off(intel_dp);
>> > +
>> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
>> >  	}
>> >  
>> >  	if (IS_SKYLAKE(dev))
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 46484e4..82489ad 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -1367,6 +1367,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
>> >  			     bool override, unsigned int mask);
>> >  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
>> >  			  enum dpio_channel ch, bool override);
>> > +void skl_enable_dc6(struct drm_i915_private *dev_priv);
>> > +void skl_disable_dc6(struct drm_i915_private *dev_priv);
>> >  
>> >  
>> >  /* intel_pm.c */
>> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> > index 3f682a1..e30c9a6 100644
>> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> > @@ -335,6 +335,10 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>> >  	SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |		\
>> >  	BIT(POWER_DOMAIN_PLLS) |			\
>> >  	BIT(POWER_DOMAIN_INIT))
>> > +#define SKL_DISPLAY_DC6_OFF_POWER_DOMAINS (		\
>> > +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
>> > +	BIT(POWER_DOMAIN_AUX_A))
>> > +
>> >  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
>> >  	(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
>> >  	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
>> > @@ -550,7 +554,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
>> >  		"DC6 already programmed to be disabled.\n");
>> >  }
>> >  
>> > -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
>> > +void skl_enable_dc6(struct drm_i915_private *dev_priv)
>> >  {
>> >  	uint32_t val;
>> >  
>> > @@ -567,7 +571,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
>> >  	POSTING_READ(DC_STATE_EN);
>> >  }
>> >  
>> > -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
>> > +void skl_disable_dc6(struct drm_i915_private *dev_priv)
>> >  {
>> >  	uint32_t val;
>> >  
>> > @@ -628,10 +632,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>> >  				!I915_READ(HSW_PWR_WELL_BIOS),
>> >  				"Invalid for power well status to be enabled, unless done by the BIOS, \
>> >  				when request is to disable!\n");
>> > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>> > -				power_well->data == SKL_DISP_PW_2) {
>> > +			if (power_well->data == SKL_DISP_PW_2) {
>> >  				if (SKL_ENABLE_DC6(dev)) {
>> > -					skl_disable_dc6(dev_priv);
>> 
>> Hmm. So the ordering needs to be 
>> disable dc6 -> enable pw2
>
> I don't know why DC6 is required for PW2 and at this point I don't see why the
> ordering should matter. Could you please explain?
>
>> >  					/*
>> >  					 * DDI buffer programming unnecessary during driver-load/resume
>> >  					 * as it's already done during modeset initialization then.
>> > @@ -639,10 +641,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>> >  					 */
>> >  					if (!dev_priv->power_domains.initializing)
>> >  						intel_prepare_ddi(dev);
>> > -				} else {
>> > -					gen9_disable_dc5(dev_priv);
>> >  				}
>> >  			}
>> > +
>> >  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
>> >  		}
>> >  
>> > @@ -660,8 +661,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>> >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
>> >  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>> >  
>> > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>> > -				power_well->data == SKL_DISP_PW_2) {
>> > +			if (power_well->data == SKL_DISP_PW_2) {
>> >  				enum csr_state state;
>> >  				/* TODO: wait for a completion event or
>> >  				 * similar here instead of busy
>> > @@ -669,14 +669,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>> >  				 */
>> >  				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
>> >  						FW_UNINITIALIZED, 1000);
>> > -				if (state != FW_LOADED)
>> > +				if (state != FW_LOADED) {
>> >  					DRM_ERROR("CSR firmware not ready (%d)\n",
>> > -							state);
>> > -				else
>> > -					if (SKL_ENABLE_DC6(dev))
>> > -						skl_enable_dc6(dev_priv);
>> > -					else
>> > -						gen9_enable_dc5(dev_priv);
>> > +						  state);
>> > +				}
>> 
>> and here we need 
>> disable pw2 -> enable dc6
>> 
>> That's symmetric which is great for using power wells here since we walk
>> the power wells array forward when enabling, and backwards when
>> disabling.
>> 
>> I'm thinking that we could also move the dc5 stuff into a power well.
>> Would reduce the clutter in skl_set_power_well() at least. I'm not sure
>> what the requirements wrt. dc5 are. If they are the same as for dc6,
>> then a single dc power well would do, otherwise we would need two, each
>> with its own domains.
>
> From what I've heard we don't need to care about DC5 atm. But I also think that
> a power well for DC5 is the way to go. Need to check with Damien what the
> requirements for DC5 are.
>
>> >  			}
>> >  		}
>> >  	}
>> > @@ -752,6 +748,34 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
>> >  	skl_set_power_well(dev_priv, power_well, false);
>> >  }
>> >  
>> > +static bool skl_dc6_state_enabled(struct drm_i915_private *dev_priv,
>> > +				  struct i915_power_well *power_well)
>> > +{
>> > +	/* Return true if disabling of DC6 is enabled */
>> > +	return !(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6);
>> > +}
>> > +
>> > +static void skl_dc6_state_on(struct drm_i915_private *dev_priv,
>> > +			     struct i915_power_well *power_well)
>> > +{
>> > +	skl_enable_dc6(dev_priv);
>> > +}
>> > +
>> > +static void skl_dc6_state_off(struct drm_i915_private *dev_priv,
>> > +			      struct i915_power_well *power_well)
>> > +{
>> > +	skl_disable_dc6(dev_priv);
>> > +}
>> > +
>> > +static void skl_dc6_sync_hw(struct drm_i915_private *dev_priv,
>> > +			    struct i915_power_well *power_well)
>> > +{
>> > +	if (power_well->count > 0)
>> > +		skl_disable_dc6(dev_priv);
>> > +	else
>> > +		skl_enable_dc6(dev_priv);
>> > +}
>> 
>> Nit: Looks like we usuall have these in the following order
>> sync_hw
>> enable
>> disable
>> 
>> 'enabled' is a bit all over the place usually. I guess before or after the
>> rest is fine.
>
> Yes, better keep the current order.
>
>> I'm not really sure how I would name these. The current naming doesn't
>> really tell me they're power well ops. Maybe
>> skl_dc6_off_power_well_{enable,disable,sync_hw,enabled}() ?
>
> I agree, better make it clear that they are pw ops.
>
>> > +
>> >  static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
>> >  					   struct i915_power_well *power_well)
>> >  {
>> > @@ -1546,6 +1570,14 @@ static const struct i915_power_well_ops skl_power_well_ops = {
>> >  	.is_enabled = skl_power_well_enabled,
>> >  };
>> >  
>> > +static const struct i915_power_well_ops skl_dc_state_ops = {
>> 
>> _dc6_, and naming to match how the function are finally named of
>> course.
>> 
>> > +	.sync_hw = skl_dc6_sync_hw,
>> > +	/* To enable power we turn the DC state off */
>> > +	.enable = skl_dc6_state_off,
>> > +	.disable = skl_dc6_state_on,
>> > +	.is_enabled = skl_dc6_state_enabled,
>> > +};
>> > +
>> >  static struct i915_power_well hsw_power_wells[] = {
>> >  	{
>> >  		.name = "always-on",
>> > @@ -1745,6 +1777,11 @@ static struct i915_power_well skl_power_wells[] = {
>> >  		.ops = &skl_power_well_ops,
>> >  		.data = SKL_DISP_PW_DDI_D,
>> >  	},
>> > +	{
>> > +		.name = "DC6 state off",
>> 
>> Just "DC6 off" perhaps?
>> 
>> I wasn't able to think of a nice way to name this so that it would make
>> more sense in the logs. With this we would get 
>> 'enabling DC6 off' and 'disabling DC6 off' which is a bit confusing.
>> Maybe we should at least put quotes around the power well name in the
>> debug message to make it a bit less funky? Probably not worth it to
>> add support for sully customized enable/disable log message.
>
> Agreed
>
>> > +		.domains = SKL_DISPLAY_DC6_OFF_POWER_DOMAINS,
>> > +		.ops = &skl_power_well_ops,
>> 
>> Surely you want to use the new ops you created? :)
>
> Oops :)
>
>> And here we need to be careful where in the list we insert the power
>> well. Based on what we saw earlier, the right place should be just
>> before PW2. That way DC6 gets disabled before PW2 is enabled, and
>> PW2 gets disabled before DC6 gets enabled.
>
> Yes, regardless of the ordering requirements it makes sense to move it up.
>
> Thanks for having a look
> -Patrik
>
>> > +	},
>> >  };
>> >  
>> >  static struct i915_power_well bxt_power_wells[] = {
>> > -- 
>> > 2.1.4
>> > 
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > [e-mail]
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> -- 
>> Ville Syrjälä
>> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patrik Jakobsson Sept. 21, 2015, 8:45 a.m. UTC | #8

On Mon, Sep 21, 2015 at 11:26:06AM +0300, Jani Nikula wrote:
> On Mon, 21 Sep 2015, Patrik Jakobsson <[e-mail]> wrote:
> > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> >> On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> >> > We need to be able to control if DC6 is allowed or not. Much like
> >> > requesting power to a specific piece of the hardware we need to be able
> >> > to request that we don't enter DC6 during certain hw access.
> >> > 
> >> > To solve this without introducing too much infrastructure I'm hooking
> >> > into the power well / power domain framework. DC6 prevention is modeled
> >> > much like an enabled power well. Thus I'm using the terminology on/off
> >> > for DC states instead of enable/disable.
> >> > 
> >> > The problem that started this work is the need for DC6 to be disabled
> >> > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> >> > patch.
> >> > 
> >> > This is posted as an RFC since DMC and DC state handling is being
> >> > reworked and will possibly affect the outcome of this patch. The patch
> >> > has known warnings.
> >> > 
> >> > Signed-off-by: Patrik Jakobsson <[e-mail]>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> >> >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> >> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> >> >  3 files changed, 64 insertions(+), 16 deletions(-)
> >> > 
> >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> > index 4823184..c2c1ad2 100644
> >> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> >> >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> >> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >> >  
> >> > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> >> > +
> >> 
> >> These I think shouldn't be necessary with my
> >> intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> >> itself grab the appropriate power domain.
> 
> Are you referring to stuff that is merged? Am I missing something?

It's not merged afaik. Ville's patches are at:
https://github.com/vsyrjala/linux/commits/aux_gmbus_power_domains

> >> 
> >> That's of course assuming that AUX is the only reason why we need to
> >> keep DC6 disabled here.
> >> 
> >
> > The upside with having get/put around bigger aux transfers is that we
> > don't get tons of enable/disable lines in the log. My vote is that we
> > keep this but also have your fine-grained get/puts.
> 
> If it works without (and everything Ville is referring to above is
> merged), I'd split these to a separate patch, and we can judge the
> improvement on its own.

Yes, and there are a few other places that I'd like to group so I'll split that
out.

> BR,
> Jani.
> 
> 
> >
> > We also have an extra enable disable print in skl_disable_dc6() /
> > skl_enable_dc6() which I think should be removed unless various DC on/off hacks
> > are required outside of the power wells framework.
> >
> >> >  		intel_dp_set_link_params(intel_dp, crtc->config);
> >> >  
> >> >  		intel_ddi_init_dp_buf_reg(intel_encoder);
> >> > @@ -2297,6 +2299,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> >> >  		intel_dp_complete_link_train(intel_dp);
> >> >  		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
> >> >  			intel_dp_stop_link_train(intel_dp);
> >> > +
> >> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> >> >  	} else if (type == INTEL_OUTPUT_HDMI) {
> >> >  		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> >> >  
> >> > @@ -2339,9 +2343,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> >> >  
> >> >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> >> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >> > +
> >> > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> >> > +
> >> >  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> >> >  		intel_edp_panel_vdd_on(intel_dp);
> >> >  		intel_edp_panel_off(intel_dp);
> >> > +
> >> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> >> >  	}
> >> >  
> >> >  	if (IS_SKYLAKE(dev))
> >> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> > index 46484e4..82489ad 100644
> >> > --- a/drivers/gpu/drm/i915/intel_drv.h
> >> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> > @@ -1367,6 +1367,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
> >> >  			     bool override, unsigned int mask);
> >> >  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
> >> >  			  enum dpio_channel ch, bool override);
> >> > +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> >> > +void skl_disable_dc6(struct drm_i915_private *dev_priv);
> >> >  
> >> >  
> >> >  /* intel_pm.c */
> >> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >> > index 3f682a1..e30c9a6 100644
> >> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >> > @@ -335,6 +335,10 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> >> >  	SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |		\
> >> >  	BIT(POWER_DOMAIN_PLLS) |			\
> >> >  	BIT(POWER_DOMAIN_INIT))
> >> > +#define SKL_DISPLAY_DC6_OFF_POWER_DOMAINS (		\
> >> > +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> >> > +	BIT(POWER_DOMAIN_AUX_A))
> >> > +
> >> >  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
> >> >  	(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
> >> >  	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> >> > @@ -550,7 +554,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> >> >  		"DC6 already programmed to be disabled.\n");
> >> >  }
> >> >  
> >> > -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >> > +void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >> >  {
> >> >  	uint32_t val;
> >> >  
> >> > @@ -567,7 +571,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> >> >  	POSTING_READ(DC_STATE_EN);
> >> >  }
> >> >  
> >> > -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> >> > +void skl_disable_dc6(struct drm_i915_private *dev_priv)
> >> >  {
> >> >  	uint32_t val;
> >> >  
> >> > @@ -628,10 +632,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >> >  				!I915_READ(HSW_PWR_WELL_BIOS),
> >> >  				"Invalid for power well status to be enabled, unless done by the BIOS, \
> >> >  				when request is to disable!\n");
> >> > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> >> > -				power_well->data == SKL_DISP_PW_2) {
> >> > +			if (power_well->data == SKL_DISP_PW_2) {
> >> >  				if (SKL_ENABLE_DC6(dev)) {
> >> > -					skl_disable_dc6(dev_priv);
> >> 
> >> Hmm. So the ordering needs to be 
> >> disable dc6 -> enable pw2
> >
> > I don't know why DC6 is required for PW2 and at this point I don't see why the
> > ordering should matter. Could you please explain?
> >
> >> >  					/*
> >> >  					 * DDI buffer programming unnecessary during driver-load/resume
> >> >  					 * as it's already done during modeset initialization then.
> >> > @@ -639,10 +641,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >> >  					 */
> >> >  					if (!dev_priv->power_domains.initializing)
> >> >  						intel_prepare_ddi(dev);
> >> > -				} else {
> >> > -					gen9_disable_dc5(dev_priv);
> >> >  				}
> >> >  			}
> >> > +
> >> >  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> >> >  		}
> >> >  
> >> > @@ -660,8 +661,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >> >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> >> >  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> >> >  
> >> > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> >> > -				power_well->data == SKL_DISP_PW_2) {
> >> > +			if (power_well->data == SKL_DISP_PW_2) {
> >> >  				enum csr_state state;
> >> >  				/* TODO: wait for a completion event or
> >> >  				 * similar here instead of busy
> >> > @@ -669,14 +669,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >> >  				 */
> >> >  				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> >> >  						FW_UNINITIALIZED, 1000);
> >> > -				if (state != FW_LOADED)
> >> > +				if (state != FW_LOADED) {
> >> >  					DRM_ERROR("CSR firmware not ready (%d)\n",
> >> > -							state);
> >> > -				else
> >> > -					if (SKL_ENABLE_DC6(dev))
> >> > -						skl_enable_dc6(dev_priv);
> >> > -					else
> >> > -						gen9_enable_dc5(dev_priv);
> >> > +						  state);
> >> > +				}
> >> 
> >> and here we need 
> >> disable pw2 -> enable dc6
> >> 
> >> That's symmetric which is great for using power wells here since we walk
> >> the power wells array forward when enabling, and backwards when
> >> disabling.
> >> 
> >> I'm thinking that we could also move the dc5 stuff into a power well.
> >> Would reduce the clutter in skl_set_power_well() at least. I'm not sure
> >> what the requirements wrt. dc5 are. If they are the same as for dc6,
> >> then a single dc power well would do, otherwise we would need two, each
> >> with its own domains.
> >
> > From what I've heard we don't need to care about DC5 atm. But I also think that
> > a power well for DC5 is the way to go. Need to check with Damien what the
> > requirements for DC5 are.
> >
> >> >  			}
> >> >  		}
> >> >  	}
> >> > @@ -752,6 +748,34 @@ static void skl_power_well_disable(struct drm_i915_private *dev_priv,
> >> >  	skl_set_power_well(dev_priv, power_well, false);
> >> >  }
> >> >  
> >> > +static bool skl_dc6_state_enabled(struct drm_i915_private *dev_priv,
> >> > +				  struct i915_power_well *power_well)
> >> > +{
> >> > +	/* Return true if disabling of DC6 is enabled */
> >> > +	return !(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6);
> >> > +}
> >> > +
> >> > +static void skl_dc6_state_on(struct drm_i915_private *dev_priv,
> >> > +			     struct i915_power_well *power_well)
> >> > +{
> >> > +	skl_enable_dc6(dev_priv);
> >> > +}
> >> > +
> >> > +static void skl_dc6_state_off(struct drm_i915_private *dev_priv,
> >> > +			      struct i915_power_well *power_well)
> >> > +{
> >> > +	skl_disable_dc6(dev_priv);
> >> > +}
> >> > +
> >> > +static void skl_dc6_sync_hw(struct drm_i915_private *dev_priv,
> >> > +			    struct i915_power_well *power_well)
> >> > +{
> >> > +	if (power_well->count > 0)
> >> > +		skl_disable_dc6(dev_priv);
> >> > +	else
> >> > +		skl_enable_dc6(dev_priv);
> >> > +}
> >> 
> >> Nit: Looks like we usuall have these in the following order
> >> sync_hw
> >> enable
> >> disable
> >> 
> >> 'enabled' is a bit all over the place usually. I guess before or after the
> >> rest is fine.
> >
> > Yes, better keep the current order.
> >
> >> I'm not really sure how I would name these. The current naming doesn't
> >> really tell me they're power well ops. Maybe
> >> skl_dc6_off_power_well_{enable,disable,sync_hw,enabled}() ?
> >
> > I agree, better make it clear that they are pw ops.
> >
> >> > +
> >> >  static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
> >> >  					   struct i915_power_well *power_well)
> >> >  {
> >> > @@ -1546,6 +1570,14 @@ static const struct i915_power_well_ops skl_power_well_ops = {
> >> >  	.is_enabled = skl_power_well_enabled,
> >> >  };
> >> >  
> >> > +static const struct i915_power_well_ops skl_dc_state_ops = {
> >> 
> >> _dc6_, and naming to match how the function are finally named of
> >> course.
> >> 
> >> > +	.sync_hw = skl_dc6_sync_hw,
> >> > +	/* To enable power we turn the DC state off */
> >> > +	.enable = skl_dc6_state_off,
> >> > +	.disable = skl_dc6_state_on,
> >> > +	.is_enabled = skl_dc6_state_enabled,
> >> > +};
> >> > +
> >> >  static struct i915_power_well hsw_power_wells[] = {
> >> >  	{
> >> >  		.name = "always-on",
> >> > @@ -1745,6 +1777,11 @@ static struct i915_power_well skl_power_wells[] = {
> >> >  		.ops = &skl_power_well_ops,
> >> >  		.data = SKL_DISP_PW_DDI_D,
> >> >  	},
> >> > +	{
> >> > +		.name = "DC6 state off",
> >> 
> >> Just "DC6 off" perhaps?
> >> 
> >> I wasn't able to think of a nice way to name this so that it would make
> >> more sense in the logs. With this we would get 
> >> 'enabling DC6 off' and 'disabling DC6 off' which is a bit confusing.
> >> Maybe we should at least put quotes around the power well name in the
> >> debug message to make it a bit less funky? Probably not worth it to
> >> add support for sully customized enable/disable log message.
> >
> > Agreed
> >
> >> > +		.domains = SKL_DISPLAY_DC6_OFF_POWER_DOMAINS,
> >> > +		.ops = &skl_power_well_ops,
> >> 
> >> Surely you want to use the new ops you created? :)
> >
> > Oops :)
> >
> >> And here we need to be careful where in the list we insert the power
> >> well. Based on what we saw earlier, the right place should be just
> >> before PW2. That way DC6 gets disabled before PW2 is enabled, and
> >> PW2 gets disabled before DC6 gets enabled.
> >
> > Yes, regardless of the ordering requirements it makes sense to move it up.
> >
> > Thanks for having a look
> > -Patrik
> >
> >> > +	},
> >> >  };
> >> >  
> >> >  static struct i915_power_well bxt_power_wells[] = {
> >> > -- 
> >> > 2.1.4
> >> > 
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > [e-mail]
> >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> 
> >> -- 
> >> Ville Syrjälä
> >> Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

Patrik Jakobsson Sept. 21, 2015, 10:43 a.m. UTC | #9

On Thu, Sep 17, 2015 at 02:14:37PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > We need to be able to control if DC6 is allowed or not. Much like
> > > requesting power to a specific piece of the hardware we need to be able
> > > to request that we don't enter DC6 during certain hw access.
> > > 
> > > To solve this without introducing too much infrastructure I'm hooking
> > > into the power well / power domain framework. DC6 prevention is modeled
> > > much like an enabled power well. Thus I'm using the terminology on/off
> > > for DC states instead of enable/disable.
> > > 
> > > The problem that started this work is the need for DC6 to be disabled
> > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > patch.
> > > 
> > > This is posted as an RFC since DMC and DC state handling is being
> > > reworked and will possibly affect the outcome of this patch. The patch
> > > has known warnings.
> > > 
> > > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> > >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > >  3 files changed, 64 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 4823184..c2c1ad2 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > >  
> > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > +
> > 
> > These I think shouldn't be necessary with my
> > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > itself grab the appropriate power domain.
> > 
> > That's of course assuming that AUX is the only reason why we need to
> > keep DC6 disabled here.
> > 
> > >  		intel_dp_set_link_params(intel_dp, crtc->config);
> > >  
> > >  		intel_ddi_init_dp_buf_reg(intel_encoder);
> > > @@ -2297,6 +2299,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > >  		intel_dp_complete_link_train(intel_dp);
> > >  		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
> > >  			intel_dp_stop_link_train(intel_dp);
> > > +
> > > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> > >  	} else if (type == INTEL_OUTPUT_HDMI) {
> > >  		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> > >  
> > > @@ -2339,9 +2343,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> > >  
> > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > +
> > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > +
> > >  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > >  		intel_edp_panel_vdd_on(intel_dp);
> > >  		intel_edp_panel_off(intel_dp);
> > > +
> > > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> > >  	}
> > >  
> > >  	if (IS_SKYLAKE(dev))
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 46484e4..82489ad 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1367,6 +1367,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
> > >  			     bool override, unsigned int mask);
> > >  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
> > >  			  enum dpio_channel ch, bool override);
> > > +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> > > +void skl_disable_dc6(struct drm_i915_private *dev_priv);
> > >  
> > >  
> > >  /* intel_pm.c */
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 3f682a1..e30c9a6 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -335,6 +335,10 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > >  	SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |		\
> > >  	BIT(POWER_DOMAIN_PLLS) |			\
> > >  	BIT(POWER_DOMAIN_INIT))
> > > +#define SKL_DISPLAY_DC6_OFF_POWER_DOMAINS (		\
> > > +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > > +	BIT(POWER_DOMAIN_AUX_A))
> > > +
> > >  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
> > >  	(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
> > >  	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > > @@ -550,7 +554,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> > >  		"DC6 already programmed to be disabled.\n");
> > >  }
> > >  
> > > -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > > +void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > >  {
> > >  	uint32_t val;
> > >  
> > > @@ -567,7 +571,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > >  	POSTING_READ(DC_STATE_EN);
> > >  }
> > >  
> > > -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > > +void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > >  {
> > >  	uint32_t val;
> > >  
> > > @@ -628,10 +632,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >  				!I915_READ(HSW_PWR_WELL_BIOS),
> > >  				"Invalid for power well status to be enabled, unless done by the BIOS, \
> > >  				when request is to disable!\n");
> > > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > > -				power_well->data == SKL_DISP_PW_2) {
> > > +			if (power_well->data == SKL_DISP_PW_2) {
> > >  				if (SKL_ENABLE_DC6(dev)) {
> > > -					skl_disable_dc6(dev_priv);
> > 
> > Hmm. So the ordering needs to be 
> > disable dc6 -> enable pw2
> > 
> > >  					/*
> > >  					 * DDI buffer programming unnecessary during driver-load/resume
> > >  					 * as it's already done during modeset initialization then.
> > > @@ -639,10 +641,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >  					 */
> > >  					if (!dev_priv->power_domains.initializing)
> > >  						intel_prepare_ddi(dev);
> > > -				} else {
> > > -					gen9_disable_dc5(dev_priv);
> > >  				}
> > >  			}
> > > +
> > >  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> > >  		}
> > >  
> > > @@ -660,8 +661,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> > >  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> > >  
> > > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > > -				power_well->data == SKL_DISP_PW_2) {
> > > +			if (power_well->data == SKL_DISP_PW_2) {
> > >  				enum csr_state state;
> > >  				/* TODO: wait for a completion event or
> > >  				 * similar here instead of busy
> > > @@ -669,14 +669,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > >  				 */
> > >  				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> > >  						FW_UNINITIALIZED, 1000);
> > > -				if (state != FW_LOADED)
> > > +				if (state != FW_LOADED) {
> > >  					DRM_ERROR("CSR firmware not ready (%d)\n",
> > > -							state);
> > > -				else
> > > -					if (SKL_ENABLE_DC6(dev))
> > > -						skl_enable_dc6(dev_priv);
> > > -					else
> > > -						gen9_enable_dc5(dev_priv);
> > > +						  state);
> > > +				}
> > 
> > and here we need 
> > disable pw2 -> enable dc6
> > 
> > That's symmetric which is great for using power wells here since we walk
> > the power wells array forward when enabling, and backwards when
> > disabling.
> > 
> > I'm thinking that we could also move the dc5 stuff into a power well.
> > Would reduce the clutter in skl_set_power_well() at least. I'm not sure
> > what the requirements wrt. dc5 are. If they are the same as for dc6,
> > then a single dc power well would do, otherwise we would need two, each
> > with its own domains.
> 
> BTW after a bit more look, I think we could probably simplify things
> quite a bit with this move. We could perhaps then set power_well->data
> to DC_STATE_EN_UPTO_DC5 or DC_STATE_EN_UPTO_DC6 for each well, and
> convert the enable/disable dc5/6 into somehting like:
> 
> gen9_enable_dc_state(dev_priv, uint32_t state)
> {
> 	// csr wait and the debugmask thing could go here
> 
> 	val = I915_READ(DC_STATE_EN);
> 	val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
> 	val |= state;
> 	I915_WRITE(DC_STATE_EN, val);
> 	POSTING_READ(DC_STATE_EN);
> }
> 
> gen9_disable_dc_state(dev_priv, uint32_t val)
> {
> 	uint32_t val;
> 
> 	val = I915_READ(DC_STATE_EN);
> 	val &= ~state;
> 	I915_WRITE(DC_STATE_EN, val);
> 	POSTING_READ(DC_STATE_EN);
> }
> 
> We could even put these directly in the power well hooks, and share
> those for DC5 and DC6. But that would perhaps mean losing the
> can_enable_disable_dc5/6 asserts or we'd need some ifs for those.
> But perhaps it would be cleaners to have separate power well ops for dc5
> and dc6, and each would just call the proposed gen9_enable/disable_dc_state()
> functions. Oh and we also have the GEN9_ENABLE_DC5 and SKL_ENABLE_DC6
> macros which I supposed we'd need to check in the power well hooks.

My feeling is that the complexity of DC vs PW is only going to grow so I'd
seperate DC5 and DC6. Not much overhead if we do as you say above.

Those macros seems a bit excessive to me. Can we drop them? The powerwell is
only available if we explicitly say so.

> But this unification could be a separate patch. First we can just
> introduce the new power wells using the existing dc5/dc6 enable/disable
> functions we have.
> 
> I didn't look at the dc9 stuff yet, so not sure if that could be handled
> in a similar fashion.

We don't do anything special for DC9 since GEN9_ENABLE_DC5(dev) is always zero.
We can probably handle it similarly when needed. At least lets assume we don't
have a problem just yet :)

> Also I think currently we just disable runtime PM when the firmware
> hasn't yet been loaded. But I think we would need to change that to hold
> a DC5/DC5 references. I guess to do this properly we should a new power
> domain for this purpose, but I'm not sure that's really worth it for a
> single user use case.

I like the simplicity with the POWER_DOMAIN_INIT approach you describe in the
other mail. Not sure what we aim at providing wrt powersaving when no firmware
is loaded. 

> 
> -- 
> Ville Syrjälä
> Intel OTC

Patrik Jakobsson Sept. 21, 2015, 11:12 a.m. UTC | #10

On Mon, Sep 21, 2015 at 12:43:36PM +0200, Patrik Jakobsson wrote:
> On Thu, Sep 17, 2015 at 02:14:37PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > > We need to be able to control if DC6 is allowed or not. Much like
> > > > requesting power to a specific piece of the hardware we need to be able
> > > > to request that we don't enter DC6 during certain hw access.
> > > > 
> > > > To solve this without introducing too much infrastructure I'm hooking
> > > > into the power well / power domain framework. DC6 prevention is modeled
> > > > much like an enabled power well. Thus I'm using the terminology on/off
> > > > for DC states instead of enable/disable.
> > > > 
> > > > The problem that started this work is the need for DC6 to be disabled
> > > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > > patch.
> > > > 
> > > > This is posted as an RFC since DMC and DC state handling is being
> > > > reworked and will possibly affect the outcome of this patch. The patch
> > > > has known warnings.
> > > > 
> > > > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> > > >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > > >  3 files changed, 64 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 4823184..c2c1ad2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > >  
> > > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > > +
> > > 
> > > These I think shouldn't be necessary with my
> > > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > > itself grab the appropriate power domain.
> > > 
> > > That's of course assuming that AUX is the only reason why we need to
> > > keep DC6 disabled here.
> > > 
> > > >  		intel_dp_set_link_params(intel_dp, crtc->config);
> > > >  
> > > >  		intel_ddi_init_dp_buf_reg(intel_encoder);
> > > > @@ -2297,6 +2299,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > > >  		intel_dp_complete_link_train(intel_dp);
> > > >  		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
> > > >  			intel_dp_stop_link_train(intel_dp);
> > > > +
> > > > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> > > >  	} else if (type == INTEL_OUTPUT_HDMI) {
> > > >  		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> > > >  
> > > > @@ -2339,9 +2343,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> > > >  
> > > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > +
> > > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > > +
> > > >  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > > >  		intel_edp_panel_vdd_on(intel_dp);
> > > >  		intel_edp_panel_off(intel_dp);
> > > > +
> > > > +		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
> > > >  	}
> > > >  
> > > >  	if (IS_SKYLAKE(dev))
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 46484e4..82489ad 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1367,6 +1367,8 @@ void chv_phy_powergate_lanes(struct intel_encoder *encoder,
> > > >  			     bool override, unsigned int mask);
> > > >  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
> > > >  			  enum dpio_channel ch, bool override);
> > > > +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> > > > +void skl_disable_dc6(struct drm_i915_private *dev_priv);
> > > >  
> > > >  
> > > >  /* intel_pm.c */
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index 3f682a1..e30c9a6 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -335,6 +335,10 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > > >  	SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |		\
> > > >  	BIT(POWER_DOMAIN_PLLS) |			\
> > > >  	BIT(POWER_DOMAIN_INIT))
> > > > +#define SKL_DISPLAY_DC6_OFF_POWER_DOMAINS (		\
> > > > +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > > > +	BIT(POWER_DOMAIN_AUX_A))
> > > > +
> > > >  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
> > > >  	(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
> > > >  	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
> > > > @@ -550,7 +554,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> > > >  		"DC6 already programmed to be disabled.\n");
> > > >  }
> > > >  
> > > > -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > > > +void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > > >  {
> > > >  	uint32_t val;
> > > >  
> > > > @@ -567,7 +571,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > > >  	POSTING_READ(DC_STATE_EN);
> > > >  }
> > > >  
> > > > -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > > > +void skl_disable_dc6(struct drm_i915_private *dev_priv)
> > > >  {
> > > >  	uint32_t val;
> > > >  
> > > > @@ -628,10 +632,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > > >  				!I915_READ(HSW_PWR_WELL_BIOS),
> > > >  				"Invalid for power well status to be enabled, unless done by the BIOS, \
> > > >  				when request is to disable!\n");
> > > > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > > > -				power_well->data == SKL_DISP_PW_2) {
> > > > +			if (power_well->data == SKL_DISP_PW_2) {
> > > >  				if (SKL_ENABLE_DC6(dev)) {
> > > > -					skl_disable_dc6(dev_priv);
> > > 
> > > Hmm. So the ordering needs to be 
> > > disable dc6 -> enable pw2
> > > 
> > > >  					/*
> > > >  					 * DDI buffer programming unnecessary during driver-load/resume
> > > >  					 * as it's already done during modeset initialization then.
> > > > @@ -639,10 +641,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > > >  					 */
> > > >  					if (!dev_priv->power_domains.initializing)
> > > >  						intel_prepare_ddi(dev);
> > > > -				} else {
> > > > -					gen9_disable_dc5(dev_priv);
> > > >  				}
> > > >  			}
> > > > +
> > > >  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> > > >  		}
> > > >  
> > > > @@ -660,8 +661,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > > >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> > > >  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> > > >  
> > > > -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> > > > -				power_well->data == SKL_DISP_PW_2) {
> > > > +			if (power_well->data == SKL_DISP_PW_2) {
> > > >  				enum csr_state state;
> > > >  				/* TODO: wait for a completion event or
> > > >  				 * similar here instead of busy
> > > > @@ -669,14 +669,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > > >  				 */
> > > >  				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> > > >  						FW_UNINITIALIZED, 1000);
> > > > -				if (state != FW_LOADED)
> > > > +				if (state != FW_LOADED) {
> > > >  					DRM_ERROR("CSR firmware not ready (%d)\n",
> > > > -							state);
> > > > -				else
> > > > -					if (SKL_ENABLE_DC6(dev))
> > > > -						skl_enable_dc6(dev_priv);
> > > > -					else
> > > > -						gen9_enable_dc5(dev_priv);
> > > > +						  state);
> > > > +				}
> > > 
> > > and here we need 
> > > disable pw2 -> enable dc6
> > > 
> > > That's symmetric which is great for using power wells here since we walk
> > > the power wells array forward when enabling, and backwards when
> > > disabling.
> > > 
> > > I'm thinking that we could also move the dc5 stuff into a power well.
> > > Would reduce the clutter in skl_set_power_well() at least. I'm not sure
> > > what the requirements wrt. dc5 are. If they are the same as for dc6,
> > > then a single dc power well would do, otherwise we would need two, each
> > > with its own domains.
> > 
> > BTW after a bit more look, I think we could probably simplify things
> > quite a bit with this move. We could perhaps then set power_well->data
> > to DC_STATE_EN_UPTO_DC5 or DC_STATE_EN_UPTO_DC6 for each well, and
> > convert the enable/disable dc5/6 into somehting like:
> > 
> > gen9_enable_dc_state(dev_priv, uint32_t state)
> > {
> > 	// csr wait and the debugmask thing could go here
> > 
> > 	val = I915_READ(DC_STATE_EN);
> > 	val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
> > 	val |= state;
> > 	I915_WRITE(DC_STATE_EN, val);
> > 	POSTING_READ(DC_STATE_EN);
> > }
> > 
> > gen9_disable_dc_state(dev_priv, uint32_t val)
> > {
> > 	uint32_t val;
> > 
> > 	val = I915_READ(DC_STATE_EN);
> > 	val &= ~state;
> > 	I915_WRITE(DC_STATE_EN, val);
> > 	POSTING_READ(DC_STATE_EN);
> > }
> > 
> > We could even put these directly in the power well hooks, and share
> > those for DC5 and DC6. But that would perhaps mean losing the
> > can_enable_disable_dc5/6 asserts or we'd need some ifs for those.
> > But perhaps it would be cleaners to have separate power well ops for dc5
> > and dc6, and each would just call the proposed gen9_enable/disable_dc_state()
> > functions. Oh and we also have the GEN9_ENABLE_DC5 and SKL_ENABLE_DC6
> > macros which I supposed we'd need to check in the power well hooks.
> 
> My feeling is that the complexity of DC vs PW is only going to grow so I'd
> seperate DC5 and DC6. Not much overhead if we do as you say above.
> 
> Those macros seems a bit excessive to me. Can we drop them? The powerwell is
> only available if we explicitly say so.
> 
> > But this unification could be a separate patch. First we can just
> > introduce the new power wells using the existing dc5/dc6 enable/disable
> > functions we have.
> > 
> > I didn't look at the dc9 stuff yet, so not sure if that could be handled
> > in a similar fashion.
> 
> We don't do anything special for DC9 since GEN9_ENABLE_DC5(dev) is always zero.
> We can probably handle it similarly when needed. At least lets assume we don't
> have a problem just yet :)

Ignore what I just said. DC9 for BXT seems to be what DC6 is for SKL. The
exception is that we need to save/restore ourselves. What I can see atm is that
the power well model would fit for simple enable/disable of DC9 as well.

> > Also I think currently we just disable runtime PM when the firmware
> > hasn't yet been loaded. But I think we would need to change that to hold
> > a DC5/DC5 references. I guess to do this properly we should a new power
> > domain for this purpose, but I'm not sure that's really worth it for a
> > single user use case.
> 
> I like the simplicity with the POWER_DOMAIN_INIT approach you describe in the
> other mail. Not sure what we aim at providing wrt powersaving when no firmware
> is loaded. 
> 
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Sept. 23, 2015, 8:40 a.m. UTC | #11

On Thu, Sep 17, 2015 at 02:45:34PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 17, 2015 at 02:14:37PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > > and here we need 
> > > disable pw2 -> enable dc6
> > > 
> > > That's symmetric which is great for using power wells here since we walk
> > > the power wells array forward when enabling, and backwards when
> > > disabling.
> > > 
> > > I'm thinking that we could also move the dc5 stuff into a power well.
> > > Would reduce the clutter in skl_set_power_well() at least. I'm not sure
> > > what the requirements wrt. dc5 are. If they are the same as for dc6,
> > > then a single dc power well would do, otherwise we would need two, each
> > > with its own domains.
> > 
> > BTW after a bit more look, I think we could probably simplify things
> > quite a bit with this move. We could perhaps then set power_well->data
> > to DC_STATE_EN_UPTO_DC5 or DC_STATE_EN_UPTO_DC6 for each well, and
> > convert the enable/disable dc5/6 into somehting like:
> > 
> > gen9_enable_dc_state(dev_priv, uint32_t state)
> > {
> > 	// csr wait and the debugmask thing could go here
> > 
> > 	val = I915_READ(DC_STATE_EN);
> > 	val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
> > 	val |= state;
> > 	I915_WRITE(DC_STATE_EN, val);
> > 	POSTING_READ(DC_STATE_EN);
> > }
> > 
> > gen9_disable_dc_state(dev_priv, uint32_t val)
> > {
> > 	uint32_t val;
> > 
> > 	val = I915_READ(DC_STATE_EN);
> > 	val &= ~state;
> > 	I915_WRITE(DC_STATE_EN, val);
> > 	POSTING_READ(DC_STATE_EN);
> > }
> > 
> > We could even put these directly in the power well hooks, and share
> > those for DC5 and DC6. But that would perhaps mean losing the
> > can_enable_disable_dc5/6 asserts or we'd need some ifs for those.
> > But perhaps it would be cleaners to have separate power well ops for dc5
> > and dc6, and each would just call the proposed gen9_enable/disable_dc_state()
> > functions. Oh and we also have the GEN9_ENABLE_DC5 and SKL_ENABLE_DC6
> > macros which I supposed we'd need to check in the power well hooks.
> > 
> > But this unification could be a separate patch. First we can just
> > introduce the new power wells using the existing dc5/dc6 enable/disable
> > functions we have.
> > 
> > I didn't look at the dc9 stuff yet, so not sure if that could be handled
> > in a similar fashion.
> > 
> > Also I think currently we just disable runtime PM when the firmware
> > hasn't yet been loaded. But I think we would need to change that to hold
> > a DC5/DC5 references. I guess to do this properly we should a new power
> > domain for this purpose, but I'm not sure that's really worth it for a
> > single user use case.
> 
> I just had the idea that POWER_DOMAIN_INIT might be a nice fit for this,
> but that would also keep the DDI power wells on even though we don't
> need the firmware for those.

POWER_DOMAIN_INIT is what I'm using in my csr cleanup patches to fix up
the current mess.
-Daniel

Daniel Vetter Sept. 23, 2015, 8:43 a.m. UTC | #12

On Mon, Sep 21, 2015 at 10:00:45AM +0200, Patrik Jakobsson wrote:
> On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > We need to be able to control if DC6 is allowed or not. Much like
> > > requesting power to a specific piece of the hardware we need to be able
> > > to request that we don't enter DC6 during certain hw access.
> > > 
> > > To solve this without introducing too much infrastructure I'm hooking
> > > into the power well / power domain framework. DC6 prevention is modeled
> > > much like an enabled power well. Thus I'm using the terminology on/off
> > > for DC states instead of enable/disable.
> > > 
> > > The problem that started this work is the need for DC6 to be disabled
> > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > patch.
> > > 
> > > This is posted as an RFC since DMC and DC state handling is being
> > > reworked and will possibly affect the outcome of this patch. The patch
> > > has known warnings.
> > > 
> > > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> > >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > >  3 files changed, 64 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 4823184..c2c1ad2 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > >  
> > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > +
> > 
> > These I think shouldn't be necessary with my
> > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > itself grab the appropriate power domain.
> > 
> > That's of course assuming that AUX is the only reason why we need to
> > keep DC6 disabled here.
> > 
> 
> The upside with having get/put around bigger aux transfers is that we don't get
> tons of enable/disable lines in the log. My vote is that we keep this but also
> have your fine-grained get/puts.

Imo the correct solution to avoid this is by adding a slight bit of
hystersis to the power well code. Which means that yes, we reinvent
another feature of the core power_domain code in our home-grown solution -
I hate it when my years old predictions come true ;-)

Sprinkling higher-level get/put calls all over the place is imo just
leaking the abstraction, which isn't good.
-Daniel

Patrik Jakobsson Sept. 23, 2015, 11:18 a.m. UTC | #13

On Wed, Sep 23, 2015 at 10:43:00AM +0200, Daniel Vetter wrote:
> On Mon, Sep 21, 2015 at 10:00:45AM +0200, Patrik Jakobsson wrote:
> > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > > We need to be able to control if DC6 is allowed or not. Much like
> > > > requesting power to a specific piece of the hardware we need to be able
> > > > to request that we don't enter DC6 during certain hw access.
> > > > 
> > > > To solve this without introducing too much infrastructure I'm hooking
> > > > into the power well / power domain framework. DC6 prevention is modeled
> > > > much like an enabled power well. Thus I'm using the terminology on/off
> > > > for DC states instead of enable/disable.
> > > > 
> > > > The problem that started this work is the need for DC6 to be disabled
> > > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > > patch.
> > > > 
> > > > This is posted as an RFC since DMC and DC state handling is being
> > > > reworked and will possibly affect the outcome of this patch. The patch
> > > > has known warnings.
> > > > 
> > > > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> > > >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > > >  3 files changed, 64 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 4823184..c2c1ad2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > >  
> > > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > > +
> > > 
> > > These I think shouldn't be necessary with my
> > > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > > itself grab the appropriate power domain.
> > > 
> > > That's of course assuming that AUX is the only reason why we need to
> > > keep DC6 disabled here.
> > > 
> > 
> > The upside with having get/put around bigger aux transfers is that we don't get
> > tons of enable/disable lines in the log. My vote is that we keep this but also
> > have your fine-grained get/puts.
> 
> Imo the correct solution to avoid this is by adding a slight bit of
> hystersis to the power well code. Which means that yes, we reinvent
> another feature of the core power_domain code in our home-grown solution -
> I hate it when my years old predictions come true ;-)
> 
> Sprinkling higher-level get/put calls all over the place is imo just
> leaking the abstraction, which isn't good.
> -Daniel

With Ville's patches the problem is not as bad as I first thought. We can add
hysteresis later if needed.

-Patrik

> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patrik Jakobsson Sept. 24, 2015, 12:50 p.m. UTC | #14

On Wed, Sep 23, 2015 at 01:18:00PM +0200, Patrik Jakobsson wrote:
> On Wed, Sep 23, 2015 at 10:43:00AM +0200, Daniel Vetter wrote:
> > On Mon, Sep 21, 2015 at 10:00:45AM +0200, Patrik Jakobsson wrote:
> > > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > > > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > > > We need to be able to control if DC6 is allowed or not. Much like
> > > > > requesting power to a specific piece of the hardware we need to be able
> > > > > to request that we don't enter DC6 during certain hw access.
> > > > > 
> > > > > To solve this without introducing too much infrastructure I'm hooking
> > > > > into the power well / power domain framework. DC6 prevention is modeled
> > > > > much like an enabled power well. Thus I'm using the terminology on/off
> > > > > for DC states instead of enable/disable.
> > > > > 
> > > > > The problem that started this work is the need for DC6 to be disabled
> > > > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > > > patch.
> > > > > 
> > > > > This is posted as an RFC since DMC and DC state handling is being
> > > > > reworked and will possibly affect the outcome of this patch. The patch
> > > > > has known warnings.
> > > > > 
> > > > > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> > > > >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > > > >  3 files changed, 64 insertions(+), 16 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > index 4823184..c2c1ad2 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > > > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > > > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > >  
> > > > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > > > +
> > > > 
> > > > These I think shouldn't be necessary with my
> > > > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > > > itself grab the appropriate power domain.
> > > > 
> > > > That's of course assuming that AUX is the only reason why we need to
> > > > keep DC6 disabled here.
> > > > 
> > > 
> > > The upside with having get/put around bigger aux transfers is that we don't get
> > > tons of enable/disable lines in the log. My vote is that we keep this but also
> > > have your fine-grained get/puts.
> > 
> > Imo the correct solution to avoid this is by adding a slight bit of
> > hystersis to the power well code. Which means that yes, we reinvent
> > another feature of the core power_domain code in our home-grown solution -
> > I hate it when my years old predictions come true ;-)
> > 
> > Sprinkling higher-level get/put calls all over the place is imo just
> > leaking the abstraction, which isn't good.
> > -Daniel
> 
> With Ville's patches the problem is not as bad as I first thought. We can add
> hysteresis later if needed.
> 
> -Patrik

So I discovered that we cannot have DC5 and DC6 as seperate power wells since
they are mutually exclusive. As Ville pointed out we don't use DC5 for anything
so we could get away for now with just DC6 as a power well.

As I see it there are three ways to go about this:

A. Add AUX A to Power well 2.
This would power up PW2 during DP Aux A even though we don't need it but since
we get the side effect of DC6 being disabled it should work.

B. Add DC6 off as a power well and remove DC5 off.
Fairly straight forward but assumes we don't need DC5 control.

C. Add multi-state support for the DC power well.
Would be the nice way but perhaps a bit overkill. Good thing about this would be
that we can incorporate DC9 control for BXT and unify more of the DC code.

So A, B or C?

-Patrik

> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjälä Sept. 24, 2015, 3:20 p.m. UTC | #15

On Thu, Sep 24, 2015 at 02:50:16PM +0200, Patrik Jakobsson wrote:
> On Wed, Sep 23, 2015 at 01:18:00PM +0200, Patrik Jakobsson wrote:
> > On Wed, Sep 23, 2015 at 10:43:00AM +0200, Daniel Vetter wrote:
> > > On Mon, Sep 21, 2015 at 10:00:45AM +0200, Patrik Jakobsson wrote:
> > > > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > > > > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > > > > We need to be able to control if DC6 is allowed or not. Much like
> > > > > > requesting power to a specific piece of the hardware we need to be able
> > > > > > to request that we don't enter DC6 during certain hw access.
> > > > > > 
> > > > > > To solve this without introducing too much infrastructure I'm hooking
> > > > > > into the power well / power domain framework. DC6 prevention is modeled
> > > > > > much like an enabled power well. Thus I'm using the terminology on/off
> > > > > > for DC states instead of enable/disable.
> > > > > > 
> > > > > > The problem that started this work is the need for DC6 to be disabled
> > > > > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > > > > patch.
> > > > > > 
> > > > > > This is posted as an RFC since DMC and DC state handling is being
> > > > > > reworked and will possibly affect the outcome of this patch. The patch
> > > > > > has known warnings.
> > > > > > 
> > > > > > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> > > > > >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> > > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > > > > >  3 files changed, 64 insertions(+), 16 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > index 4823184..c2c1ad2 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > > > > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > > > > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > > >  
> > > > > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > > > > +
> > > > > 
> > > > > These I think shouldn't be necessary with my
> > > > > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > > > > itself grab the appropriate power domain.
> > > > > 
> > > > > That's of course assuming that AUX is the only reason why we need to
> > > > > keep DC6 disabled here.
> > > > > 
> > > > 
> > > > The upside with having get/put around bigger aux transfers is that we don't get
> > > > tons of enable/disable lines in the log. My vote is that we keep this but also
> > > > have your fine-grained get/puts.
> > > 
> > > Imo the correct solution to avoid this is by adding a slight bit of
> > > hystersis to the power well code. Which means that yes, we reinvent
> > > another feature of the core power_domain code in our home-grown solution -
> > > I hate it when my years old predictions come true ;-)
> > > 
> > > Sprinkling higher-level get/put calls all over the place is imo just
> > > leaking the abstraction, which isn't good.
> > > -Daniel
> > 
> > With Ville's patches the problem is not as bad as I first thought. We can add
> > hysteresis later if needed.
> > 
> > -Patrik
> 
> So I discovered that we cannot have DC5 and DC6 as seperate power wells since
> they are mutually exclusive. As Ville pointed out we don't use DC5 for anything
> so we could get away for now with just DC6 as a power well.
> 
> As I see it there are three ways to go about this:
> 
> A. Add AUX A to Power well 2.
> This would power up PW2 during DP Aux A even though we don't need it but since
> we get the side effect of DC6 being disabled it should work.
> 
> B. Add DC6 off as a power well and remove DC5 off.
> Fairly straight forward but assumes we don't need DC5 control.
> 
> C. Add multi-state support for the DC power well.
> Would be the nice way but perhaps a bit overkill. Good thing about this would be
> that we can incorporate DC9 control for BXT and unify more of the DC code.
> 
> So A, B or C?

After some spitballing with Imre we came up with the following power well layout,
which I think matches the documented seuqences in the spec the best:

display well:
    domains: 
        all
    enable:
        init display sequence
            enable pch reset handshake
            enable pw1 and miscio
            enable cdclk pll
            enable dbuf
        dc_state_en = up_to_dc6
    disable:
        dc_state_en = disable
        uninit display sequence
            disable dbuf
            disable cdclk pll
            disable pw1 and miscio

dc6 off well:
    domains:
        gmbus, dc5 off well domains
    enable:
        dc_state_en = up_to_dc5
    disable:
        dc_state_en = up_to_dc6

dc5 off well:
    domains:
        aux A, pw2 well domains
    enable:
        dc_state_en = disable
    disable:
        dc_state_en = up_to_dc5

pw2 well:
    domains:
        ddi B/C/D/E, aux B/C/D/E, vga, audio, pipe B/C, pfit B/C
    enable:
        enable pw2
    disable:
        disable pw2

Patrik Jakobsson Sept. 25, 2015, 8:56 a.m. UTC | #16

On Thu, Sep 24, 2015 at 06:20:14PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 24, 2015 at 02:50:16PM +0200, Patrik Jakobsson wrote:
> > On Wed, Sep 23, 2015 at 01:18:00PM +0200, Patrik Jakobsson wrote:
> > > On Wed, Sep 23, 2015 at 10:43:00AM +0200, Daniel Vetter wrote:
> > > > On Mon, Sep 21, 2015 at 10:00:45AM +0200, Patrik Jakobsson wrote:
> > > > > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > > > > > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > > > > > We need to be able to control if DC6 is allowed or not. Much like
> > > > > > > requesting power to a specific piece of the hardware we need to be able
> > > > > > > to request that we don't enter DC6 during certain hw access.
> > > > > > > 
> > > > > > > To solve this without introducing too much infrastructure I'm hooking
> > > > > > > into the power well / power domain framework. DC6 prevention is modeled
> > > > > > > much like an enabled power well. Thus I'm using the terminology on/off
> > > > > > > for DC states instead of enable/disable.
> > > > > > > 
> > > > > > > The problem that started this work is the need for DC6 to be disabled
> > > > > > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > > > > > patch.
> > > > > > > 
> > > > > > > This is posted as an RFC since DMC and DC state handling is being
> > > > > > > reworked and will possibly affect the outcome of this patch. The patch
> > > > > > > has known warnings.
> > > > > > > 
> > > > > > > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> > > > > > >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> > > > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > > > > > >  3 files changed, 64 insertions(+), 16 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > index 4823184..c2c1ad2 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > > > > > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > > > > > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > > > >  
> > > > > > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > > > > > +
> > > > > > 
> > > > > > These I think shouldn't be necessary with my
> > > > > > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > > > > > itself grab the appropriate power domain.
> > > > > > 
> > > > > > That's of course assuming that AUX is the only reason why we need to
> > > > > > keep DC6 disabled here.
> > > > > > 
> > > > > 
> > > > > The upside with having get/put around bigger aux transfers is that we don't get
> > > > > tons of enable/disable lines in the log. My vote is that we keep this but also
> > > > > have your fine-grained get/puts.
> > > > 
> > > > Imo the correct solution to avoid this is by adding a slight bit of
> > > > hystersis to the power well code. Which means that yes, we reinvent
> > > > another feature of the core power_domain code in our home-grown solution -
> > > > I hate it when my years old predictions come true ;-)
> > > > 
> > > > Sprinkling higher-level get/put calls all over the place is imo just
> > > > leaking the abstraction, which isn't good.
> > > > -Daniel
> > > 
> > > With Ville's patches the problem is not as bad as I first thought. We can add
> > > hysteresis later if needed.
> > > 
> > > -Patrik
> > 
> > So I discovered that we cannot have DC5 and DC6 as seperate power wells since
> > they are mutually exclusive. As Ville pointed out we don't use DC5 for anything
> > so we could get away for now with just DC6 as a power well.
> > 
> > As I see it there are three ways to go about this:
> > 
> > A. Add AUX A to Power well 2.
> > This would power up PW2 during DP Aux A even though we don't need it but since
> > we get the side effect of DC6 being disabled it should work.
> > 
> > B. Add DC6 off as a power well and remove DC5 off.
> > Fairly straight forward but assumes we don't need DC5 control.
> > 
> > C. Add multi-state support for the DC power well.
> > Would be the nice way but perhaps a bit overkill. Good thing about this would be
> > that we can incorporate DC9 control for BXT and unify more of the DC code.
> > 
> > So A, B or C?
> 
> After some spitballing with Imre we came up with the following power well layout,
> which I think matches the documented seuqences in the spec the best:
> 
> display well:
>     domains: 
>         all
>     enable:
>         init display sequence
>             enable pch reset handshake
>             enable pw1 and miscio
>             enable cdclk pll
>             enable dbuf
>         dc_state_en = up_to_dc6
>     disable:
>         dc_state_en = disable
>         uninit display sequence
>             disable dbuf
>             disable cdclk pll
>             disable pw1 and miscio
> 
> dc6 off well:
>     domains:
>         gmbus, dc5 off well domains
>     enable:
>         dc_state_en = up_to_dc5
>     disable:
>         dc_state_en = up_to_dc6
> 
> dc5 off well:
>     domains:
>         aux A, pw2 well domains
>     enable:
>         dc_state_en = disable
>     disable:
>         dc_state_en = up_to_dc5
> 
> pw2 well:
>     domains:
>         ddi B/C/D/E, aux B/C/D/E, vga, audio, pipe B/C, pfit B/C
>     enable:
>         enable pw2
>     disable:
>         disable pw2
> 

Ok, that looks good, but what I'm trying to get at is that if we're going to
have two seperate power wells for DC5 and DC6 how do we keep track of DC5
enable/disable in DC6 enable/disable?

E.g.
- DC6 enable (dc_state_en = up_to_dc5) 
- DC5 enable (dc_state_en = disable)
- DC6 disable (dc_state_en = up_to_dc6)

The last line would incorrectly set dc_state_en = up_to_dc6 when it should be
dc_state_en = disable because DC5 is still enabled.

Currently we don't have the above case since DC5 is never enabled (dc_state_en =
disable) but it's still nasty. And with Aux A on DC5 (as in the description
above) we would hit this problem.

-Patrik

> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Imre Deak Sept. 25, 2015, 9:41 a.m. UTC | #17

On pe, 2015-09-25 at 10:56 +0200, Patrik Jakobsson wrote:
> On Thu, Sep 24, 2015 at 06:20:14PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 24, 2015 at 02:50:16PM +0200, Patrik Jakobsson wrote:
> > > On Wed, Sep 23, 2015 at 01:18:00PM +0200, Patrik Jakobsson wrote:
> > > > On Wed, Sep 23, 2015 at 10:43:00AM +0200, Daniel Vetter wrote:
> > > > > On Mon, Sep 21, 2015 at 10:00:45AM +0200, Patrik Jakobsson wrote:
> > > > > > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > > > > > > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > > > > > > We need to be able to control if DC6 is allowed or not. Much like
> > > > > > > > requesting power to a specific piece of the hardware we need to be able
> > > > > > > > to request that we don't enter DC6 during certain hw access.
> > > > > > > > 
> > > > > > > > To solve this without introducing too much infrastructure I'm hooking
> > > > > > > > into the power well / power domain framework. DC6 prevention is modeled
> > > > > > > > much like an enabled power well. Thus I'm using the terminology on/off
> > > > > > > > for DC states instead of enable/disable.
> > > > > > > > 
> > > > > > > > The problem that started this work is the need for DC6 to be disabled
> > > > > > > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > > > > > > patch.
> > > > > > > > 
> > > > > > > > This is posted as an RFC since DMC and DC state handling is being
> > > > > > > > reworked and will possibly affect the outcome of this patch. The patch
> > > > > > > > has known warnings.
> > > > > > > > 
> > > > > > > > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> > > > > > > >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> > > > > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > > > > > > >  3 files changed, 64 insertions(+), 16 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > index 4823184..c2c1ad2 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > > > > > > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > > > > > > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > > > > >  
> > > > > > > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > > > > > > +
> > > > > > > 
> > > > > > > These I think shouldn't be necessary with my
> > > > > > > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > > > > > > itself grab the appropriate power domain.
> > > > > > > 
> > > > > > > That's of course assuming that AUX is the only reason why we need to
> > > > > > > keep DC6 disabled here.
> > > > > > > 
> > > > > > 
> > > > > > The upside with having get/put around bigger aux transfers is that we don't get
> > > > > > tons of enable/disable lines in the log. My vote is that we keep this but also
> > > > > > have your fine-grained get/puts.
> > > > > 
> > > > > Imo the correct solution to avoid this is by adding a slight bit of
> > > > > hystersis to the power well code. Which means that yes, we reinvent
> > > > > another feature of the core power_domain code in our home-grown solution -
> > > > > I hate it when my years old predictions come true ;-)
> > > > > 
> > > > > Sprinkling higher-level get/put calls all over the place is imo just
> > > > > leaking the abstraction, which isn't good.
> > > > > -Daniel
> > > > 
> > > > With Ville's patches the problem is not as bad as I first thought. We can add
> > > > hysteresis later if needed.
> > > > 
> > > > -Patrik
> > > 
> > > So I discovered that we cannot have DC5 and DC6 as seperate power wells since
> > > they are mutually exclusive. As Ville pointed out we don't use DC5 for anything
> > > so we could get away for now with just DC6 as a power well.
> > > 
> > > As I see it there are three ways to go about this:
> > > 
> > > A. Add AUX A to Power well 2.
> > > This would power up PW2 during DP Aux A even though we don't need it but since
> > > we get the side effect of DC6 being disabled it should work.
> > > 
> > > B. Add DC6 off as a power well and remove DC5 off.
> > > Fairly straight forward but assumes we don't need DC5 control.
> > > 
> > > C. Add multi-state support for the DC power well.
> > > Would be the nice way but perhaps a bit overkill. Good thing about this would be
> > > that we can incorporate DC9 control for BXT and unify more of the DC code.
> > > 
> > > So A, B or C?
> > 
> > After some spitballing with Imre we came up with the following power well layout,
> > which I think matches the documented seuqences in the spec the best:
> > 
> > display well:
> >     domains: 
> >         all
> >     enable:
> >         init display sequence
> >             enable pch reset handshake
> >             enable pw1 and miscio
> >             enable cdclk pll
> >             enable dbuf
> >         dc_state_en = up_to_dc6
> >     disable:
> >         dc_state_en = disable
> >         uninit display sequence
> >             disable dbuf
> >             disable cdclk pll
> >             disable pw1 and miscio
> > 
> > dc6 off well:
> >     domains:
> >         gmbus, dc5 off well domains
> >     enable:
> >         dc_state_en = up_to_dc5
> >     disable:
> >         dc_state_en = up_to_dc6
> > 
> > dc5 off well:
> >     domains:
> >         aux A, pw2 well domains
> >     enable:
> >         dc_state_en = disable
> >     disable:
> >         dc_state_en = up_to_dc5
> > 
> > pw2 well:
> >     domains:
> >         ddi B/C/D/E, aux B/C/D/E, vga, audio, pipe B/C, pfit B/C
> >     enable:
> >         enable pw2
> >     disable:
> >         disable pw2
> > 
> 
> Ok, that looks good, but what I'm trying to get at is that if we're going to
> have two seperate power wells for DC5 and DC6 how do we keep track of DC5
> enable/disable in DC6 enable/disable?
> 
> E.g.
> - DC6 enable (dc_state_en = up_to_dc5) 
> - DC5 enable (dc_state_en = disable)
> - DC6 disable (dc_state_en = up_to_dc6)
> 
> The last line would incorrectly set dc_state_en = up_to_dc6 when it should be
> dc_state_en = disable because DC5 is still enabled.
> 
> Currently we don't have the above case since DC5 is never enabled (dc_state_en =
> disable) but it's still nasty. And with Aux A on DC5 (as in the description
> above) we would hit this problem.

The above sequence couldn't happen since the "DC5 off well" always keeps
a reference on the "DC6 off well" (all the DC5 domains are in DC6's
domains too) and the two wells' order is fixed so that DC6 is first.

So a sequence like yours above, let's say enabling/disabling the Aux A
domain when nothing else is on would go:

- Display well enable (dc_state_en = up_to_dc6)
- DC6 off well enable (dc_state_en = up_to_dc5)
- DC5 off well enable (dc_state_en = disable)
- DC5 off well disable (dc_state_en = up_to_dc5)
- DC6 off well disable (dc_state_en = up_to_dc6)
- Display well disable (dc_state_en = N/A)

--Imre

Ville Syrjälä Sept. 25, 2015, 11:32 a.m. UTC | #18

On Fri, Sep 25, 2015 at 10:56:31AM +0200, Patrik Jakobsson wrote:
> On Thu, Sep 24, 2015 at 06:20:14PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 24, 2015 at 02:50:16PM +0200, Patrik Jakobsson wrote:
> > > On Wed, Sep 23, 2015 at 01:18:00PM +0200, Patrik Jakobsson wrote:
> > > > On Wed, Sep 23, 2015 at 10:43:00AM +0200, Daniel Vetter wrote:
> > > > > On Mon, Sep 21, 2015 at 10:00:45AM +0200, Patrik Jakobsson wrote:
> > > > > > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > > > > > > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > > > > > > We need to be able to control if DC6 is allowed or not. Much like
> > > > > > > > requesting power to a specific piece of the hardware we need to be able
> > > > > > > > to request that we don't enter DC6 during certain hw access.
> > > > > > > > 
> > > > > > > > To solve this without introducing too much infrastructure I'm hooking
> > > > > > > > into the power well / power domain framework. DC6 prevention is modeled
> > > > > > > > much like an enabled power well. Thus I'm using the terminology on/off
> > > > > > > > for DC states instead of enable/disable.
> > > > > > > > 
> > > > > > > > The problem that started this work is the need for DC6 to be disabled
> > > > > > > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > > > > > > patch.
> > > > > > > > 
> > > > > > > > This is posted as an RFC since DMC and DC state handling is being
> > > > > > > > reworked and will possibly affect the outcome of this patch. The patch
> > > > > > > > has known warnings.
> > > > > > > > 
> > > > > > > > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> > > > > > > >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> > > > > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > > > > > > >  3 files changed, 64 insertions(+), 16 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > index 4823184..c2c1ad2 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > > > > > > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > > > > > > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > > > > >  
> > > > > > > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > > > > > > +
> > > > > > > 
> > > > > > > These I think shouldn't be necessary with my
> > > > > > > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > > > > > > itself grab the appropriate power domain.
> > > > > > > 
> > > > > > > That's of course assuming that AUX is the only reason why we need to
> > > > > > > keep DC6 disabled here.
> > > > > > > 
> > > > > > 
> > > > > > The upside with having get/put around bigger aux transfers is that we don't get
> > > > > > tons of enable/disable lines in the log. My vote is that we keep this but also
> > > > > > have your fine-grained get/puts.
> > > > > 
> > > > > Imo the correct solution to avoid this is by adding a slight bit of
> > > > > hystersis to the power well code. Which means that yes, we reinvent
> > > > > another feature of the core power_domain code in our home-grown solution -
> > > > > I hate it when my years old predictions come true ;-)
> > > > > 
> > > > > Sprinkling higher-level get/put calls all over the place is imo just
> > > > > leaking the abstraction, which isn't good.
> > > > > -Daniel
> > > > 
> > > > With Ville's patches the problem is not as bad as I first thought. We can add
> > > > hysteresis later if needed.
> > > > 
> > > > -Patrik
> > > 
> > > So I discovered that we cannot have DC5 and DC6 as seperate power wells since
> > > they are mutually exclusive. As Ville pointed out we don't use DC5 for anything
> > > so we could get away for now with just DC6 as a power well.
> > > 
> > > As I see it there are three ways to go about this:
> > > 
> > > A. Add AUX A to Power well 2.
> > > This would power up PW2 during DP Aux A even though we don't need it but since
> > > we get the side effect of DC6 being disabled it should work.
> > > 
> > > B. Add DC6 off as a power well and remove DC5 off.
> > > Fairly straight forward but assumes we don't need DC5 control.
> > > 
> > > C. Add multi-state support for the DC power well.
> > > Would be the nice way but perhaps a bit overkill. Good thing about this would be
> > > that we can incorporate DC9 control for BXT and unify more of the DC code.
> > > 
> > > So A, B or C?
> > 
> > After some spitballing with Imre we came up with the following power well layout,
> > which I think matches the documented seuqences in the spec the best:
> > 
> > display well:
> >     domains: 
> >         all
> >     enable:
> >         init display sequence
> >             enable pch reset handshake
> >             enable pw1 and miscio
> >             enable cdclk pll
> >             enable dbuf
> >         dc_state_en = up_to_dc6
> >     disable:
> >         dc_state_en = disable
> >         uninit display sequence
> >             disable dbuf
> >             disable cdclk pll
> >             disable pw1 and miscio
> > 
> > dc6 off well:
> >     domains:
> >         gmbus, dc5 off well domains
> >     enable:
> >         dc_state_en = up_to_dc5
> >     disable:
> >         dc_state_en = up_to_dc6
> > 
> > dc5 off well:
> >     domains:
> >         aux A, pw2 well domains
> >     enable:
> >         dc_state_en = disable
> >     disable:
> >         dc_state_en = up_to_dc5
> > 
> > pw2 well:
> >     domains:
> >         ddi B/C/D/E, aux B/C/D/E, vga, audio, pipe B/C, pfit B/C
> >     enable:
> >         enable pw2
> >     disable:
> >         disable pw2
> > 
> 
> Ok, that looks good, but what I'm trying to get at is that if we're going to
> have two seperate power wells for DC5 and DC6 how do we keep track of DC5
> enable/disable in DC6 enable/disable?
> 
> E.g.
> - DC6 enable (dc_state_en = up_to_dc5) 
> - DC5 enable (dc_state_en = disable)
> - DC6 disable (dc_state_en = up_to_dc6)
> 
> The last line would incorrectly set dc_state_en = up_to_dc6 when it should be
> dc_state_en = disable because DC5 is still enabled.

That can not happen because of the ordering between the power wells, and
the fact that the domains are set up in a way that DC6 includes all the
DC5 domains.

Patrik Jakobsson Sept. 25, 2015, 11:37 a.m. UTC | #19

On Fri, Sep 25, 2015 at 12:41:42PM +0300, Imre Deak wrote:
> On pe, 2015-09-25 at 10:56 +0200, Patrik Jakobsson wrote:
> > On Thu, Sep 24, 2015 at 06:20:14PM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 24, 2015 at 02:50:16PM +0200, Patrik Jakobsson wrote:
> > > > On Wed, Sep 23, 2015 at 01:18:00PM +0200, Patrik Jakobsson wrote:
> > > > > On Wed, Sep 23, 2015 at 10:43:00AM +0200, Daniel Vetter wrote:
> > > > > > On Mon, Sep 21, 2015 at 10:00:45AM +0200, Patrik Jakobsson wrote:
> > > > > > > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > > > > > > > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > > > > > > > We need to be able to control if DC6 is allowed or not. Much like
> > > > > > > > > requesting power to a specific piece of the hardware we need to be able
> > > > > > > > > to request that we don't enter DC6 during certain hw access.
> > > > > > > > > 
> > > > > > > > > To solve this without introducing too much infrastructure I'm hooking
> > > > > > > > > into the power well / power domain framework. DC6 prevention is modeled
> > > > > > > > > much like an enabled power well. Thus I'm using the terminology on/off
> > > > > > > > > for DC states instead of enable/disable.
> > > > > > > > > 
> > > > > > > > > The problem that started this work is the need for DC6 to be disabled
> > > > > > > > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > > > > > > > patch.
> > > > > > > > > 
> > > > > > > > > This is posted as an RFC since DMC and DC state handling is being
> > > > > > > > > reworked and will possibly affect the outcome of this patch. The patch
> > > > > > > > > has known warnings.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Patrik Jakobsson <[e-mail]>
> > > > > > > > > ---
> > > > > > > > >  drivers/gpu/drm/i915/intel_ddi.c        |  9 +++++
> > > > > > > > >  drivers/gpu/drm/i915/intel_drv.h        |  2 +
> > > > > > > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > > > > > > > >  3 files changed, 64 insertions(+), 16 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > > index 4823184..c2c1ad2 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > > > > > > > >  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > > > > > > > >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > > > > > >  
> > > > > > > > > +		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > These I think shouldn't be necessary with my
> > > > > > > > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > > > > > > > itself grab the appropriate power domain.
> > > > > > > > 
> > > > > > > > That's of course assuming that AUX is the only reason why we need to
> > > > > > > > keep DC6 disabled here.
> > > > > > > > 
> > > > > > > 
> > > > > > > The upside with having get/put around bigger aux transfers is that we don't get
> > > > > > > tons of enable/disable lines in the log. My vote is that we keep this but also
> > > > > > > have your fine-grained get/puts.
> > > > > > 
> > > > > > Imo the correct solution to avoid this is by adding a slight bit of
> > > > > > hystersis to the power well code. Which means that yes, we reinvent
> > > > > > another feature of the core power_domain code in our home-grown solution -
> > > > > > I hate it when my years old predictions come true ;-)
> > > > > > 
> > > > > > Sprinkling higher-level get/put calls all over the place is imo just
> > > > > > leaking the abstraction, which isn't good.
> > > > > > -Daniel
> > > > > 
> > > > > With Ville's patches the problem is not as bad as I first thought. We can add
> > > > > hysteresis later if needed.
> > > > > 
> > > > > -Patrik
> > > > 
> > > > So I discovered that we cannot have DC5 and DC6 as seperate power wells since
> > > > they are mutually exclusive. As Ville pointed out we don't use DC5 for anything
> > > > so we could get away for now with just DC6 as a power well.
> > > > 
> > > > As I see it there are three ways to go about this:
> > > > 
> > > > A. Add AUX A to Power well 2.
> > > > This would power up PW2 during DP Aux A even though we don't need it but since
> > > > we get the side effect of DC6 being disabled it should work.
> > > > 
> > > > B. Add DC6 off as a power well and remove DC5 off.
> > > > Fairly straight forward but assumes we don't need DC5 control.
> > > > 
> > > > C. Add multi-state support for the DC power well.
> > > > Would be the nice way but perhaps a bit overkill. Good thing about this would be
> > > > that we can incorporate DC9 control for BXT and unify more of the DC code.
> > > > 
> > > > So A, B or C?
> > > 
> > > After some spitballing with Imre we came up with the following power well layout,
> > > which I think matches the documented seuqences in the spec the best:
> > > 
> > > display well:
> > >     domains: 
> > >         all
> > >     enable:
> > >         init display sequence
> > >             enable pch reset handshake
> > >             enable pw1 and miscio
> > >             enable cdclk pll
> > >             enable dbuf
> > >         dc_state_en = up_to_dc6
> > >     disable:
> > >         dc_state_en = disable
> > >         uninit display sequence
> > >             disable dbuf
> > >             disable cdclk pll
> > >             disable pw1 and miscio
> > > 
> > > dc6 off well:
> > >     domains:
> > >         gmbus, dc5 off well domains
> > >     enable:
> > >         dc_state_en = up_to_dc5
> > >     disable:
> > >         dc_state_en = up_to_dc6
> > > 
> > > dc5 off well:
> > >     domains:
> > >         aux A, pw2 well domains
> > >     enable:
> > >         dc_state_en = disable
> > >     disable:
> > >         dc_state_en = up_to_dc5
> > > 
> > > pw2 well:
> > >     domains:
> > >         ddi B/C/D/E, aux B/C/D/E, vga, audio, pipe B/C, pfit B/C
> > >     enable:
> > >         enable pw2
> > >     disable:
> > >         disable pw2
> > > 
> > 
> > Ok, that looks good, but what I'm trying to get at is that if we're going to
> > have two seperate power wells for DC5 and DC6 how do we keep track of DC5
> > enable/disable in DC6 enable/disable?
> > 
> > E.g.
> > - DC6 enable (dc_state_en = up_to_dc5) 
> > - DC5 enable (dc_state_en = disable)
> > - DC6 disable (dc_state_en = up_to_dc6)
> > 
> > The last line would incorrectly set dc_state_en = up_to_dc6 when it should be
> > dc_state_en = disable because DC5 is still enabled.
> > 
> > Currently we don't have the above case since DC5 is never enabled (dc_state_en =
> > disable) but it's still nasty. And with Aux A on DC5 (as in the description
> > above) we would hit this problem.
> 
> The above sequence couldn't happen since the "DC5 off well" always keeps
> a reference on the "DC6 off well" (all the DC5 domains are in DC6's
> domains too) and the two wells' order is fixed so that DC6 is first.
> 
> So a sequence like yours above, let's say enabling/disabling the Aux A
> domain when nothing else is on would go:
> 
> - Display well enable (dc_state_en = up_to_dc6)
> - DC6 off well enable (dc_state_en = up_to_dc5)
> - DC5 off well enable (dc_state_en = disable)
> - DC5 off well disable (dc_state_en = up_to_dc5)
> - DC6 off well disable (dc_state_en = up_to_dc6)
> - Display well disable (dc_state_en = N/A)
> 
> --Imre
> 

Ah ok, that will indeed prevent us from having a reference on DC5 without having
a reference on DC6. And I can hardcode the transition reg writes since we only
go up or down one step. This could also be extended to DC9 if needed.

Thanks for explaining
-Patrik

Patch

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 4823184..c2c1ad2 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2288,6 +2288,8 @@  static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
+		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
+
 		intel_dp_set_link_params(intel_dp, crtc->config);
 
 		intel_ddi_init_dp_buf_reg(intel_encoder);
@@ -2297,6 +2299,8 @@  static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 		intel_dp_complete_link_train(intel_dp);
 		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
 			intel_dp_stop_link_train(intel_dp);
+
+		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
 	} else if (type == INTEL_OUTPUT_HDMI) {
 		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 
@@ -2339,9 +2343,14 @@  static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
 
 	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+
+		intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
+
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 		intel_edp_panel_vdd_on(intel_dp);
 		intel_edp_panel_off(intel_dp);
+
+		intel_display_power_put(dev_priv, POWER_DOMAIN_AUX_A);
 	}
 
 	if (IS_SKYLAKE(dev))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 46484e4..82489ad 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1367,6 +1367,8 @@  void chv_phy_powergate_lanes(struct intel_encoder *encoder,
 			     bool override, unsigned int mask);
 bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
 			  enum dpio_channel ch, bool override);
+void skl_enable_dc6(struct drm_i915_private *dev_priv);
+void skl_disable_dc6(struct drm_i915_private *dev_priv);
 
 
 /* intel_pm.c */
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 3f682a1..e30c9a6 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -335,6 +335,10 @@  static void hsw_set_power_well(struct drm_i915_private *dev_priv,
 	SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |		\
 	BIT(POWER_DOMAIN_PLLS) |			\
 	BIT(POWER_DOMAIN_INIT))
+#define SKL_DISPLAY_DC6_OFF_POWER_DOMAINS (		\
+	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
+	BIT(POWER_DOMAIN_AUX_A))
+
 #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
 	(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |	\
 	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
@@ -550,7 +554,7 @@  static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
 		"DC6 already programmed to be disabled.\n");
 }
 
-static void skl_enable_dc6(struct drm_i915_private *dev_priv)
+void skl_enable_dc6(struct drm_i915_private *dev_priv)
 {
 	uint32_t val;
 
@@ -567,7 +571,7 @@  static void skl_enable_dc6(struct drm_i915_private *dev_priv)
 	POSTING_READ(DC_STATE_EN);
 }
 
-static void skl_disable_dc6(struct drm_i915_private *dev_priv)
+void skl_disable_dc6(struct drm_i915_private *dev_priv)
 {
 	uint32_t val;
 
@@ -628,10 +632,8 @@  static void skl_set_power_well(struct drm_i915_private *dev_priv,
 				!I915_READ(HSW_PWR_WELL_BIOS),
 				"Invalid for power well status to be enabled, unless done by the BIOS, \
 				when request is to disable!\n");
-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
-				power_well->data == SKL_DISP_PW_2) {
+			if (power_well->data == SKL_DISP_PW_2) {
 				if (SKL_ENABLE_DC6(dev)) {
-					skl_disable_dc6(dev_priv);
 					/*
 					 * DDI buffer programming unnecessary during driver-load/resume
 					 * as it's already done during modeset initialization then.
@@ -639,10 +641,9 @@  static void skl_set_power_well(struct drm_i915_private *dev_priv,
 					 */
 					if (!dev_priv->power_domains.initializing)
 						intel_prepare_ddi(dev);
-				} else {
-					gen9_disable_dc5(dev_priv);
 				}
 			}
+
 			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
 		}
 
@@ -660,8 +661,7 @@  static void skl_set_power_well(struct drm_i915_private *dev_priv,
 			POSTING_READ(HSW_PWR_WELL_DRIVER);
 			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
 
-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
-				power_well->data == SKL_DISP_PW_2) {
+			if (power_well->data == SKL_DISP_PW_2) {
 				enum csr_state state;
 				/* TODO: wait for a completion event or
 				 * similar here instead of busy
@@ -669,14 +669,10 @@  static void skl_set_power_well(struct drm_i915_private *dev_priv,
 				 */
 				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
 						FW_UNINITIALIZED, 1000);
-				if (state != FW_LOADED)
+				if (state != FW_LOADED) {
 					DRM_ERROR("CSR firmware not ready (%d)\n",
-							state);
-				else
-					if (SKL_ENABLE_DC6(dev))
-						skl_enable_dc6(dev_priv);
-					else
-						gen9_enable_dc5(dev_priv);
+						  state);
+				}
 			}
 		}
 	}
@@ -752,6 +748,34 @@  static void skl_power_well_disable(struct drm_i915_private *dev_priv,
 	skl_set_power_well(dev_priv, power_well, false);
 }
 
+static bool skl_dc6_state_enabled(struct drm_i915_private *dev_priv,
+				  struct i915_power_well *power_well)
+{
+	/* Return true if disabling of DC6 is enabled */
+	return !(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6);
+}
+
+static void skl_dc6_state_on(struct drm_i915_private *dev_priv,
+			     struct i915_power_well *power_well)
+{
+	skl_enable_dc6(dev_priv);
+}
+
+static void skl_dc6_state_off(struct drm_i915_private *dev_priv,
+			      struct i915_power_well *power_well)
+{
+	skl_disable_dc6(dev_priv);
+}
+
+static void skl_dc6_sync_hw(struct drm_i915_private *dev_priv,
+			    struct i915_power_well *power_well)
+{
+	if (power_well->count > 0)
+		skl_disable_dc6(dev_priv);
+	else
+		skl_enable_dc6(dev_priv);
+}
+
 static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
 					   struct i915_power_well *power_well)
 {
@@ -1546,6 +1570,14 @@  static const struct i915_power_well_ops skl_power_well_ops = {
 	.is_enabled = skl_power_well_enabled,
 };
 
+static const struct i915_power_well_ops skl_dc_state_ops = {
+	.sync_hw = skl_dc6_sync_hw,
+	/* To enable power we turn the DC state off */
+	.enable = skl_dc6_state_off,
+	.disable = skl_dc6_state_on,
+	.is_enabled = skl_dc6_state_enabled,
+};
+
 static struct i915_power_well hsw_power_wells[] = {
 	{
 		.name = "always-on",
@@ -1745,6 +1777,11 @@  static struct i915_power_well skl_power_wells[] = {
 		.ops = &skl_power_well_ops,
 		.data = SKL_DISP_PW_DDI_D,
 	},
+	{
+		.name = "DC6 state off",
+		.domains = SKL_DISPLAY_DC6_OFF_POWER_DOMAINS,
+		.ops = &skl_power_well_ops,
+	},
 };
 
 static struct i915_power_well bxt_power_wells[] = {