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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <patrik.jakobsson@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  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
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux