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

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

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).

>  };
>  
>  #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(crtc));
>  			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))
>  #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))
>  #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_priv);
> -				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_ddi(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,	tmp &
> ~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_priv);
>  		}
>  	}
>  
> @@ -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,
> +	},
> +	{
>  		.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,
> +	},
> +
>  };
>  
>  #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