Re: [PATCH v5 3/9] drm/i915/tgl: Add power well to enable DC3CO state

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

On Sat, Aug 10, 2019 at 12:02:17AM +0530, Anshuman Gupta wrote:
> "DC3CO Off" power well inherits its power domains from
> "DC Off" power well, these power domains will disallow
> DC3CO when any external displays are connected and at
> time of modeset and aux programming.
> Renaming "DC Off" power well to "DC5 Off" power well.
> 
> v2: commit log improvement.
> v3: Used intel_wait_for_register to wait for DC3CO exit. [Imre]
>     Used gen9_set_dc_state() to allow/disallow DC3CO. [Imre]
>     Moved transcoder psr2 exit line enablement from tgl_allow_dc3co()
>     to a appropriate place haswell_crtc_enable(). [Imre]
>     Changed the DC3CO power well enabled call back logic as
>     recommended in review comments. [Imre]
> v4: Used wait_for_us() instead of intel_wait_for_reg(). [Imre (IRC)]
> v5: using udelay() instead of waiting for DC3CO exit status.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Animesh Manna <animesh.manna@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  .../drm/i915/display/intel_display_power.c    | 69 ++++++++++++++++++-
>  1 file changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index e2ef202aeeef..c9e92d48cdab 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -791,7 +791,26 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
>  	dev_priv->csr.dc_state = val & mask;
>  }
>  
> -static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
> +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv)
> +{
> +	gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO);
> +}
> +
> +static void tgl_disallow_dc3co(struct drm_i915_private *dev_priv)
> +{
> +	u32 val;
> +
> +	val = I915_READ(DC_STATE_EN);
> +	val &= ~DC_STATE_DC3CO_STATUS;
> +	I915_WRITE(DC_STATE_EN, val);
> +	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> +	/*
> +	 * Delay of 200us DC3CO Exit time B.Spec 49196
> +	 */
> +	udelay(200);
> +}
> +
> +void bxt_enable_dc9(struct drm_i915_private *dev_priv)
>  {
>  	assert_can_enable_dc9(dev_priv);
>  
> @@ -1007,6 +1026,33 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
>  		gen9_enable_dc5(dev_priv);
>  }
>  
> +static void tgl_dc3co_power_well_enable(struct drm_i915_private *dev_priv,
> +					struct i915_power_well *power_well)

Should be called dc3co_off power well.

> +{
> +	tgl_disallow_dc3co(dev_priv);
> +}
> +
> +static void tgl_dc3co_power_well_disable(struct drm_i915_private *dev_priv,
> +					 struct i915_power_well *power_well)
> +{
> +	if (!dev_priv->psr.sink_psr2_support)
> +		return;

We could end up enabling DC3CO while PSR2 is disabled after disabling
a PSR2 capable output, which is against the spec.

I'm thinking now that we should have a single dc_off power well and a
new interface setting the max allowed DC state (DC3CO, DC5/6).

(Right now I think there is also a missing re-enabling of DC3CO when
 disabling DC5/6).

> +
> +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)
> +		tgl_allow_dc3co(dev_priv);
> +}
> +
> +static bool tgl_dc3co_power_well_enabled(struct drm_i915_private *dev_priv,
> +					 struct i915_power_well *power_well)
> +{
> +	/*
> +	 * Checking alone DC_STATE_EN is not enough as DC5 power well also
> +	 * allow/disallow DC3CO to make sure both are not enabled at same time
> +	 */
> +	return ((I915_READ(DC_STATE_EN) & DC_STATE_EN_DC3CO) == 0 &&
> +		(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0);
> +}
> +
>  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
>  					 struct i915_power_well *power_well)
>  {
> @@ -2611,6 +2657,12 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_TRANSCODER_VDSC_PW2) |	\
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  
> +#define TGL_DISPLAY_DC3CO_OFF_POWER_DOMAINS (		\
> +	TGL_PW_2_POWER_DOMAINS |			\
> +	BIT_ULL(POWER_DOMAIN_MODESET) |			\
> +	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
> +	BIT_ULL(POWER_DOMAIN_INIT))
> +
>  #define TGL_DISPLAY_DC_OFF_POWER_DOMAINS (		\
>  	TGL_PW_2_POWER_DOMAINS |			\
>  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
> @@ -2715,6 +2767,13 @@ static const struct i915_power_well_ops gen9_dc_off_power_well_ops = {
>  	.is_enabled = gen9_dc_off_power_well_enabled,
>  };
>  
> +static const struct i915_power_well_ops tgl_dc3co_power_well_ops = {
> +	.sync_hw = i9xx_power_well_sync_hw_noop,
> +	.enable = tgl_dc3co_power_well_enable,
> +	.disable = tgl_dc3co_power_well_disable,
> +	.is_enabled = tgl_dc3co_power_well_enabled,
> +};
> +
>  static const struct i915_power_well_ops bxt_dpio_cmn_power_well_ops = {
>  	.sync_hw = i9xx_power_well_sync_hw_noop,
>  	.enable = bxt_dpio_cmn_power_well_enable,
> @@ -3626,11 +3685,17 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
>  		},
>  	},
>  	{
> -		.name = "DC off",
> +		.name = "DC5 off",
>  		.domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS,
>  		.ops = &gen9_dc_off_power_well_ops,
>  		.id = DISP_PW_ID_NONE,
>  	},
> +	{
> +		.name = "DC3CO off",
> +		.domains = TGL_DISPLAY_DC3CO_OFF_POWER_DOMAINS,
> +		.ops = &tgl_dc3co_power_well_ops,
> +		.id = DISP_PW_ID_NONE,
> +	},
>  	{
>  		.name = "power well 2",
>  		.domains = TGL_PW_2_POWER_DOMAINS,
> -- 
> 2.21.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx