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

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

On to, 2015-11-12 at 15:30 +0200, Imre Deak wrote:
> On to, 2015-11-12 at 13:24 +0100, Patrik Jakobsson wrote:
> > On Wed, Nov 11, 2015 at 08:57:19PM +0200, Imre Deak wrote:
> > > On ma, 2015-11-09 at 16:48 +0100, Patrik Jakobsson wrote:
> > > > Handle DC off as a power well where enabling the power well
> > > > will
> > > > prevent
> > > > the DMC to enter selected DC states (required around modesets
> > > > and
> > > > Aux
> > > > A). Disabling the power well will allow DC states again. For
> > > > now
> > > > the
> > > > highest DC state is DC6 for Skylake and DC5 for Broxton but
> > > > will
> > > > be
> > > > configurable for Skylake in a later patch.
> > > > 
> > > > v2: Check both DC5 and DC6 bits in power well enabled function
> > > > (Ville)
> > > > 
> > > > Signed-off-by: Patrik Jakobsson <
> > > > patrik.jakobsson@linux.intel.com
> > > > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c         |   6 --
> > > >  drivers/gpu/drm/i915/i915_reg.h         |   1 +
> > > >  drivers/gpu/drm/i915/intel_display.c    |   6 ++
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 110
> > > > +++++++++++++++++++++++---------
> > > >  4 files changed, 88 insertions(+), 35 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 5a63f9a..0c7f435 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -1072,9 +1072,6 @@ static int i915_pm_resume(struct device
> > > > *dev)
> > > >  
> > > >  static int skl_suspend_complete(struct drm_i915_private
> > > > *dev_priv)
> > > >  {
> > > > -	if (dev_priv->csr.dmc_payload)
> > > > -		skl_enable_dc6(dev_priv);
> > > > -
> > > >  	return 0;
> > > >  }
> > > >  
> > > > @@ -1119,9 +1116,6 @@ static int bxt_resume_prepare(struct
> > > > drm_i915_private *dev_priv)
> > > >  
> > > >  static int skl_resume_prepare(struct drm_i915_private
> > > > *dev_priv)
> > > >  {
> > > > -	if (dev_priv->csr.dmc_payload)
> > > > -		skl_disable_dc6(dev_priv);
> > > > -
> > > >  	return 0;
> > > >  }
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 31b3a84..df445ba 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -636,6 +636,7 @@ enum skl_disp_power_wells {
> > > >  
> > > >  	/* Not actual bit groups. Used as IDs for
> > > > lookup_power_well() */
> > > >  	SKL_DISP_PW_ALWAYS_ON,
> > > > +	SKL_DISP_PW_DC_OFF,
> > > 
> > > Imo it would be less confusing to call it DC3 power well. Looking
> > > at th
> > > e DC spec, DC4 is only a transitory state to DC5/6, so what we
> > > expect
> > > when we disable DC6/5 is DC3 or shallower power states (DC2/1/0).
> > 
> > I've been changing the name quite a few times but settled on "DC
> > off"
> > to keep it
> > generic. The main mechanism for the power well is to prevent any DC
> > states that
> > can cause us problems during certain operations (i.e. modeset). The
> > DC states we
> > need to block could change or be different between platforms. For
> > that reason I
> > would prefer not to be as specific with the naming.
> 
> All the power well names are platform specific so it would be logical
> and more meaningful to give this one a platform specific name too.
> Also
> 'DC off' is kind of a double negation leading to debug messages like
> 'enable DC off' which isn't that nice. But now that I think of it my
> idea isn't that great either. 'enable DC3' is confusing too, one
> could
> think we enable some power saving state at that point, which is not
> the
> case. So let's keep things as-is for now at least:
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>

I noticed some issue only now, when testing it more:

> > >   };
> > > >  
> > > >  #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2))
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 649ac34..856d801 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -13320,6 +13320,9 @@ static int intel_atomic_commit(struct
> > > > drm_device *dev,
> > > >  			to_intel_crtc_state(crtc->state)
> > > > ->update_pipe;
> > > >  		unsigned long put_domains = 0;
> > > >  
> > > > +		if (modeset)
> > > > +			intel_display_power_get(dev_priv,
> > > > POWER_DOMAIN_MODESET);
> > > > +
> > > >  		if (modeset && crtc->state->active) {
> > > >  			update_scanline_offset(to_intel_crtc(c
> > > > rt
> > > > c));
> > > >  			dev_priv->display.crtc_enable(crtc);
> > > > @@ -13343,6 +13346,9 @@ static int intel_atomic_commit(struct
> > > > drm_device *dev,
> > > >  			modeset_put_power_domains(dev_priv,
> > > > put_domains);
> > > >  
> > > >  		intel_post_plane_update(intel_crtc);
> > > > +
> > > > +		if (modeset)
> > > > +			intel_display_power_put(dev_priv,
> > > > POWER_DOMAIN_MODESET);
> > > >  	}
> > > >  
> > > >  	/* FIXME: add subpixel order */
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index edf753e..95c3fcc 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -49,9 +49,6 @@
> > > >   * present for a given platform.
> > > >   */
> > > >  
> > > > -#define GEN9_ENABLE_DC5(dev) 0
> > > > -#define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev)
> > > > -
> > > >  #define for_each_power_well(i, power_well, domain_mask,
> > > > power_domains)	\
> > > >  	for (i = 0;						
> > > > 	
> > > > \
> > > >  	     i < (power_domains)->power_well_count &&		
> > > > 	\
> > > > @@ -309,9 +306,14 @@ static void hsw_set_power_well(struct
> > > > drm_i915_private *dev_priv,
> > > >  #define SKL_DISPLAY_DDI_D_POWER_DOMAINS (		\
> > > >  	BIT(POWER_DOMAIN_PORT_DDI_D_LANES) |		\
> > > >  	BIT(POWER_DOMAIN_INIT))
> > > > +#define SKL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> > > > +	BIT(POWER_DOMAIN_MODESET) |			\
> > > > +	BIT(POWER_DOMAIN_AUX_A) |			\
> > > > +	BIT(POWER_DOMAIN_INIT))

This needs SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS as well, so that we
enable/disable DC5/6 whenever we toggle PW_2.

> > > >  #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
> > > >  	(POWER_DOMAIN_MASK & ~(				
> > > > \
> > > > -	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS)) |	\
> > > > +	SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		
> > > > \
> > > > +	SKL_DISPLAY_DC_OFF_POWER_DOMAINS)) |		\
> > > >  	BIT(POWER_DOMAIN_INIT))
> > > >  
> > > >  #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS (		
> > > > \
> > > > @@ -339,6 +341,10 @@ static void hsw_set_power_well(struct
> > > > drm_i915_private *dev_priv,
> > > >  	BIT(POWER_DOMAIN_AUX_A) |			\
> > > >  	BIT(POWER_DOMAIN_PLLS) |			\
> > > >  	BIT(POWER_DOMAIN_INIT))
> > > > +#define BXT_DISPLAY_DC_OFF_POWER_DOMAINS (		\
> > > > +	BIT(POWER_DOMAIN_MODESET) |			\
> > > > +	BIT(POWER_DOMAIN_AUX_A) |			\
> > > > +	BIT(POWER_DOMAIN_INIT))

As above, this needs BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS.

> > > >  #define BXT_DISPLAY_ALWAYS_ON_POWER_DOMAINS (		\
> > > >  	(POWER_DOMAIN_MASK &
> > > > ~(BXT_DISPLAY_POWERWELL_1_POWER_DOMAINS
> > > > > 	\
> > > >  	BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS)) |	\
> > > > @@ -486,15 +492,6 @@ static void gen9_enable_dc5(struct
> > > > drm_i915_private *dev_priv)
> > > >  	gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
> > > >  }
> > > >  
> > > > -static void gen9_disable_dc5(struct drm_i915_private
> > > > *dev_priv)
> > > > -{
> > > > -	assert_can_disable_dc5(dev_priv);
> > > > -
> > > > -	DRM_DEBUG_KMS("Disabling DC5\n");
> > > > -
> > > > -	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > > > -}
> > > > -
> > > >  static void assert_can_enable_dc6(struct drm_i915_private
> > > > *dev_priv)
> > > >  {
> > > >  	struct drm_device *dev = dev_priv->dev;
> > > > @@ -522,6 +519,14 @@ static void assert_can_disable_dc6(struct
> > > > drm_i915_private *dev_priv)
> > > >  		  "DC6 already programmed to be disabled.\n");
> > > >  }
> > > >  
> > > > +static void gen9_disable_dc5_dc6(struct drm_i915_private
> > > > *dev_priv)
> > > > +{
> > > > +	assert_can_disable_dc5(dev_priv);
> > > > +	assert_can_disable_dc6(dev_priv);
> > > > +
> > > > +	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > > > +}
> > > > +
> > > >  void skl_enable_dc6(struct drm_i915_private *dev_priv)
> > > >  {
> > > >  	assert_can_enable_dc6(dev_priv);
> > > > @@ -571,6 +576,7 @@ static void skl_set_power_well(struct
> > > > drm_i915_private *dev_priv,
> > > >  	case SKL_DISP_PW_DDI_C:
> > > >  	case SKL_DISP_PW_DDI_D:
> > > >  	case SKL_DISP_PW_MISC_IO:
> > > > +	case SKL_DISP_PW_DC_OFF:
> > > >  		break;
> > > >  	default:
> > > >  		WARN(1, "Unknown power well %lu\n", power_well
> > > > ->data);
> > > > @@ -589,17 +595,13 @@ static void skl_set_power_well(struct
> > > > drm_i915_private *dev_priv,
> > > >  				"Invalid for power well status
> > > > to be
> > > > enabled, unless done by the BIOS, \
> > > >  				when request is to
> > > > disable!\n");
> > > >  			if (power_well->data == SKL_DISP_PW_2)
> > > > {
> > > > -				if (GEN9_ENABLE_DC5(dev))
> > > > -					gen9_disable_dc5(dev_p
> > > > ri
> > > > v);
> > > > -				if (SKL_ENABLE_DC6(dev)) {
> > > > -					/*
> > > > -					 * DDI buffer
> > > > programming
> > > > unnecessary during driver-load/resume
> > > > -					 * as it's already
> > > > done
> > > > during modeset initialization then.
> > > > -					 * It's also invalid
> > > > here as
> > > > encoder list is still uninitialized.
> > > > -					 */
> > > > -					if (!dev_priv
> > > > ->power_domains.initializing)
> > > > -						intel_prepare_
> > > > dd
> > > > i(de
> > > > v);
> > > > -				}
> > > > +				/*
> > > > +				 * DDI buffer programming
> > > > unnecessary during driver-load/resume
> > > > +				 * as it's already done during
> > > > modeset initialization then.
> > > > +				 * It's also invalid here as
> > > > encoder
> > > > list is still uninitialized.
> > > > +				 */
> > > > +				if (!dev_priv
> > > > ->power_domains.initializing)
> > > > +					intel_prepare_ddi(dev)
> > > > ;
> > > >  			}
> > > >  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp |
> > > > req_mask);
> > > >  		}
> > > > @@ -617,10 +619,6 @@ static void skl_set_power_well(struct
> > > > drm_i915_private *dev_priv,
> > > >  			I915_WRITE(HSW_PWR_WELL_DRIVER,	
> > > > t
> > > > mp &
> > > > ~req_mask);
> > > >  			POSTING_READ(HSW_PWR_WELL_DRIVER);
> > > >  			DRM_DEBUG_KMS("Disabling %s\n",
> > > > power_well
> > > > ->name);
> > > > -
> > > > -			if (GEN9_ENABLE_DC5(dev) &&
> > > > -				power_well->data ==
> > > > SKL_DISP_PW_2)
> > > > -					gen9_enable_dc5(dev_pr
> > > > iv
> > > > );
> > > >  		}
> > > >  	}
> > > >  
> > > > @@ -695,6 +693,40 @@ static void skl_power_well_disable(struct
> > > > drm_i915_private *dev_priv,
> > > >  	skl_set_power_well(dev_priv, power_well, false);
> > > >  }
> > > >  
> > > > +static bool gen9_dc_off_power_well_enabled(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > > +					  struct
> > > > i915_power_well
> > > > *power_well)
> > > > +{
> > > > +	return (I915_READ(DC_STATE_EN) &
> > > > DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0;
> > > > +}
> > > > +
> > > > +static void gen9_dc_off_power_well_enable(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > > +					 struct
> > > > i915_power_well
> > > > *power_well)
> > > > +{
> > > > +	gen9_disable_dc5_dc6(dev_priv);
> > > > +}
> > > > +
> > > > +static void gen9_dc_off_power_well_disable(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > > +					  struct
> > > > i915_power_well
> > > > *power_well)
> > > > +{
> > > > +	if (IS_SKYLAKE(dev_priv))
> > > > +		skl_enable_dc6(dev_priv);
> > > > +	else
> > > > +		gen9_enable_dc5(dev_priv);
> > > > +}
> > > > +
> > > > +static void gen9_dc_off_power_well_sync_hw(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > > +					  struct
> > > > i915_power_well
> > > > *power_well)
> > > > +{
> > > > +	if (power_well->count > 0) {
> > > > +		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > > > +	} else {
> > > > +		if (IS_SKYLAKE(dev_priv))
> > > > +			gen9_set_dc_state(dev_priv,
> > > > DC_STATE_EN_UPTO_DC6);
> > > > +		else
> > > > +			gen9_set_dc_state(dev_priv,
> > > > DC_STATE_EN_UPTO_DC5);
> > > > +	}
> > > > +}
> > > > +
> > > >  static void i9xx_always_on_power_well_noop(struct
> > > > drm_i915_private
> > > > *dev_priv,
> > > >  					   struct
> > > > i915_power_well
> > > > *power_well)
> > > >  {
> > > > @@ -1517,6 +1549,13 @@ static const struct i915_power_well_ops
> > > > skl_power_well_ops = {
> > > >  	.is_enabled = skl_power_well_enabled,
> > > >  };
> > > >  
> > > > +static const struct i915_power_well_ops
> > > > gen9_dc_off_power_well_ops =
> > > > {
> > > > +	.sync_hw = gen9_dc_off_power_well_sync_hw,
> > > > +	.enable = gen9_dc_off_power_well_enable,
> > > > +	.disable = gen9_dc_off_power_well_disable,
> > > > +	.is_enabled = gen9_dc_off_power_well_enabled,
> > > > +};
> > > > +
> > > >  static struct i915_power_well hsw_power_wells[] = {
> > > >  	{
> > > >  		.name = "always-on",
> > > > @@ -1697,6 +1736,12 @@ static struct i915_power_well
> > > > skl_power_wells[] = {
> > > >  		.data = SKL_DISP_PW_2,
> > > >  	},
> > > >  	{
> > > > +		.name = "DC off",
> > > > +		.domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
> > > > +		.ops = &gen9_dc_off_power_well_ops,
> > > > +		.data = SKL_DISP_PW_DC_OFF,
> > > > +	},

This should be listed right before the PW_2 well, so that the we have
the following sequences:

1. disable DC5/6
2. enable PW_2

1. disable PW_2
2. enable DC5/6

> > > > +	{
> > > >  		.name = "DDI A/E power well",
> > > >  		.domains = SKL_DISPLAY_DDI_A_E_POWER_DOMAINS,
> > > >  		.ops = &skl_power_well_ops,
> > > > @@ -1769,7 +1814,14 @@ static struct i915_power_well
> > > > bxt_power_wells[] = {
> > > >  		.domains =
> > > > BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS,
> > > >  		.ops = &skl_power_well_ops,
> > > >  		.data = SKL_DISP_PW_2,
> > > > -	}
> > > > +	},
> > > > +	{
> > > > +		.name = "DC off",
> > > > +		.domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
> > > > +		.ops = &gen9_dc_off_power_well_ops,
> > > > +		.data = SKL_DISP_PW_DC_OFF,
> > > > +	},

As above, this should be listed right before the PW_2 well.

> > > > +
> > > >  };
> > > >  
> > > >  #define set_power_wells(power_domains, __power_wells) ({	
> > > > 	
> > > > \
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx