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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 21 Sep 2015, Patrik Jakobsson <patrik.jakobsson@linux.intel.com> 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 <patrik.jakobsson@linux.intel.com>
>> > ---
>> >  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
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>> -- 
>> Ville Syrjälä
>> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx