Re: [PATCH 2/4] drm/i915/gen9: Sanitize handling of allowed DC states

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

On ma, 2016-02-29 at 14:02 +0100, Patrik Jakobsson wrote:
> On Wed, Feb 24, 2016 at 07:57:44PM +0200, Imre Deak wrote:
> > We can simplify the conditions selecting the target DC state during
> > runtime by calculating the allowed DC states in advance during
> > driver
> > loading. This also makes it easier to disable DC states depending
> > on the
> > i915.disable_power_well module option, added in the next patch.
> > 
> > CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |  1 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 74
> > +++++++++++++++++++++++----------
> >  2 files changed, 54 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 9e76bfc..b563de5 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -750,6 +750,7 @@ struct intel_csr {
> >  	i915_reg_t mmioaddr[8];
> >  	uint32_t mmiodata[8];
> >  	uint32_t dc_state;
> > +	uint32_t allowed_dc_mask;
> >  };
> >  
> >  #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 8276dc2..88df99e 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -538,12 +538,8 @@ static void gen9_set_dc_state(struct
> > drm_i915_private *dev_priv, uint32_t state)
> >  	else
> >  		mask |= DC_STATE_EN_UPTO_DC6;
> >  
> > -	WARN_ON_ONCE(state & ~mask);
> > -
> > -	if (i915.enable_dc == 0)
> > -		state = DC_STATE_DISABLE;
> > -	else if (i915.enable_dc == 1 && state >
> > DC_STATE_EN_UPTO_DC5)
> > -		state = DC_STATE_EN_UPTO_DC5;
> > +	if (WARN_ON_ONCE(state & ~dev_priv->csr.allowed_dc_mask))
> > +		state &= dev_priv->csr.allowed_dc_mask;
> >  
> >  	val = I915_READ(DC_STATE_EN);
> >  	DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n",
> > @@ -659,8 +655,7 @@ static void gen9_disable_dc5_dc6(struct
> > drm_i915_private *dev_priv)
> >  {
> >  	assert_can_disable_dc5(dev_priv);
> >  
> > -	if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> > -	    i915.enable_dc != 0 && i915.enable_dc != 1)
> > +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> >  		assert_can_disable_dc6(dev_priv);
> >  
> >  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > @@ -839,26 +834,19 @@ static void
> > gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
> >  static void gen9_dc_off_power_well_disable(struct drm_i915_private
> > *dev_priv,
> >  					   struct i915_power_well
> > *power_well)
> >  {
> > -	if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> > -	    i915.enable_dc != 0 && i915.enable_dc != 1)
> > +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> >  		skl_enable_dc6(dev_priv);
> > -	else
> > +	else if (dev_priv->csr.allowed_dc_mask &
> > DC_STATE_EN_UPTO_DC5)
> >  		gen9_enable_dc5(dev_priv);
> >  }
> >  
> >  static void gen9_dc_off_power_well_sync_hw(struct drm_i915_private
> > *dev_priv,
> >  					   struct i915_power_well
> > *power_well)
> >  {
> > -	if (power_well->count > 0) {
> > -		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > -	} else {
> > -		if ((IS_SKYLAKE(dev_priv) ||
> > IS_KABYLAKE(dev_priv)) &&
> > -		    i915.enable_dc != 0 &&
> > -		    i915.enable_dc != 1)
> > -			gen9_set_dc_state(dev_priv,
> > DC_STATE_EN_UPTO_DC6);
> > -		else
> > -			gen9_set_dc_state(dev_priv,
> > DC_STATE_EN_UPTO_DC5);
> > -	}
> > +	if (power_well->count > 0)
> > +		gen9_dc_off_power_well_enable(dev_priv,
> > power_well);
> > +	else
> > +		gen9_dc_off_power_well_disable(dev_priv,
> > power_well);
> >  }
> >  
> >  static void i9xx_always_on_power_well_noop(struct drm_i915_private
> > *dev_priv,
> > @@ -2023,6 +2011,48 @@ sanitize_disable_power_well_option(const
> > struct drm_i915_private *dev_priv,
> >  	return 1;
> >  }
> >  
> > +static uint32_t get_allowed_dc_mask(const struct drm_i915_private
> > *dev_priv,
> > +				    int enable_dc)
> > +{
> > +	uint32_t mask = 0;
> > +
> > +	/*
> > +	 * DC9 has a separate HW flow from the rest of the DC
> > states, not
> > +	 * depending on the DMC firmware. It's needed by system
> > +	 * suspend/resume, so allow it unconditionally.
> > +	 */
> > +	if (IS_BROXTON(dev_priv))
> > +		mask |= DC_STATE_EN_DC9;
> > +
> > +	if (!enable_dc)
> > +		return mask;
> > +
> > +	if (!HAS_CSR(dev_priv))
> > +		return mask;
> > +
> > +	mask |= DC_STATE_EN_UPTO_DC5;
> > +	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> > +		mask |= DC_STATE_EN_UPTO_DC6;
> > +	} else if (!IS_BROXTON(dev_priv)) {
> > +		MISSING_CASE(INTEL_DEVID(dev_priv));
> > +		mask = 0;
> > +	}
> > +
> > +	switch (enable_dc) {
> > +	case 1:
> > +		mask &= ~DC_STATE_EN_UPTO_DC6;
> > +		break;
> > +	case 2:
> 
> Should we make some noise here on BXT? Just a thought, not sure it's
> useful.

Yes, I added this now. I'll resend the whole patchset with this change
since patchwork didn't pick up the first version for testing.

--Imre

> 
> > +	case -1:
> > +		break;
> > +	default:
> > +		DRM_ERROR("Unexpected value for enable_dc (%d)\n",
> > enable_dc);
> > +		break;
> > +	}
> > +
> > +	return mask;
> > +}
> > +
> >  #define set_power_wells(power_domains, __power_wells) ({		
> > \
> >  	(power_domains)->power_wells = (__power_wells);		
> > 	\
> >  	(power_domains)->power_well_count =
> > ARRAY_SIZE(__power_wells);	\
> > @@ -2041,6 +2071,8 @@ int intel_power_domains_init(struct
> > drm_i915_private *dev_priv)
> >  
> >  	i915.disable_power_well =
> > sanitize_disable_power_well_option(dev_priv,
> >  						     i915.disable_
> > power_well);
> > +	dev_priv->csr.allowed_dc_mask =
> > get_allowed_dc_mask(dev_priv,
> > +							    i915.e
> > nable_dc);
> >  
> >  	BUILD_BUG_ON(POWER_DOMAIN_NUM > 31);
> >  
> > -- 
> > 2.5.0
> > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx