[v7,3/7] drm/i915/tgl: Enable DC3CO state in "DC Off" power well

Submitted by Anshuamn Gupta on Sept. 7, 2019, 5:14 p.m.

Details

Message ID 20190907171443.16181-4-anshuman.gupta@intel.com
State New
Headers show
Series "DC3CO Support for TGL" ( rev: 8 7 ) in Intel GFX

Browsing this patch as part of:
"DC3CO Support for TGL" rev 8 in Intel GFX
<< prev patch [3/7] next patch >>

Commit Message

Anshuamn Gupta Sept. 7, 2019, 5:14 p.m.
Add max_dc_state and tgl_set_target_dc_state() API
in order to enable DC3CO state with existing DC states.
max_dc_state will enable/disable the desired DC state in
DC_STATE_EN reg when "DC Off" power well gets disable/enable.

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.
v6: Fixed minor unwanted change.
v7: Removed DC3CO powerwell and POWER_DOMAIN_VIDEO.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 111 ++++++++++++++----
 .../drm/i915/display/intel_display_power.h    |   3 +
 drivers/gpu/drm/i915/i915_drv.h               |   1 +
 3 files changed, 95 insertions(+), 20 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 496fa1b53ffb..83b10f61ee42 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -772,6 +772,29 @@  static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
 	dev_priv->csr.dc_state = val & mask;
 }
 
+static void tgl_allow_dc3co(struct drm_i915_private *dev_priv)
+{
+	if (!dev_priv->psr.sink_psr2_support)
+		return;
+
+	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)
+		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);
+}
+
 static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
 {
 	assert_can_enable_dc9(dev_priv);
@@ -939,7 +962,8 @@  static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv)
 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;
+	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 gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv)
@@ -955,24 +979,32 @@  static void gen9_disable_dc_states(struct drm_i915_private *dev_priv)
 {
 	struct intel_cdclk_state cdclk_state = {};
 
-	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
+	if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {
+		tgl_disallow_dc3co(dev_priv);
+	} else {
+		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 
-	dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
-	/* Can't read out voltage_level so can't use intel_cdclk_changed() */
-	WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state));
+		dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
+		/*
+		 * Can't read out voltage_level so can't use
+		 * intel_cdclk_changed()
+		 */
+		WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw,
+						  &cdclk_state));
 
-	gen9_assert_dbuf_enabled(dev_priv);
+		gen9_assert_dbuf_enabled(dev_priv);
 
-	if (IS_GEN9_LP(dev_priv))
-		bxt_verify_ddi_phy_power_wells(dev_priv);
+		if (IS_GEN9_LP(dev_priv))
+			bxt_verify_ddi_phy_power_wells(dev_priv);
 
-	if (INTEL_GEN(dev_priv) >= 11)
-		/*
-		 * DMC retains HW context only for port A, the other combo
-		 * PHY's HW context for port B is lost after DC transitions,
-		 * so we need to restore it manually.
-		 */
-		intel_combo_phy_init(dev_priv);
+		if (INTEL_GEN(dev_priv) >= 11)
+			/*
+			 * DMC retains HW context only for port A, the other
+			 * combo PHY's HW context for port B is lost after
+			 * DC transitions, so we need to restore it manually.
+			 */
+			intel_combo_phy_init(dev_priv);
+	}
 }
 
 static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
@@ -987,10 +1019,48 @@  static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
 	if (!dev_priv->csr.dmc_payload)
 		return;
 
-	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
-		skl_enable_dc6(dev_priv);
-	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
-		gen9_enable_dc5(dev_priv);
+	if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {
+		tgl_allow_dc3co(dev_priv);
+	} else if (dev_priv->csr.max_dc_state & DC_STATE_EN_UPTO_DC5_DC6_MASK) {
+		if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
+			skl_enable_dc6(dev_priv);
+		else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
+			gen9_enable_dc5(dev_priv);
+	}
+}
+
+void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
+			     bool psr2_deep_sleep)
+{
+	struct i915_power_well *power_well;
+	bool dc_off_enabled;
+	struct i915_power_domains *power_domains = &dev_priv->power_domains;
+
+	mutex_lock(&power_domains->lock);
+	power_well = lookup_power_well(dev_priv, TGL_DISP_DC_OFF);
+
+	if (!power_well)
+		goto unlock;
+
+	dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv,
+							   power_well);
+	if (state == dev_priv->csr.max_dc_state)
+		goto unlock;
+
+	if (!dc_off_enabled) {
+		/*
+		 * Need to disable the DC off power well to
+		 * effect target DC state.
+		 */
+		power_well->desc->ops->enable(dev_priv, power_well);
+		dev_priv->csr.max_dc_state = state;
+		power_well->desc->ops->disable(dev_priv, power_well);
+		goto unlock;
+	}
+		dev_priv->csr.max_dc_state = state;
+
+unlock:
+	mutex_unlock(&power_domains->lock);
 }
 
 static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
@@ -3610,7 +3680,7 @@  static const struct i915_power_well_desc tgl_power_wells[] = {
 		.name = "DC off",
 		.domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS,
 		.ops = &gen9_dc_off_power_well_ops,
-		.id = DISP_PW_ID_NONE,
+		.id = TGL_DISP_DC_OFF,
 	},
 	{
 		.name = "power well 2",
@@ -4039,6 +4109,7 @@  int intel_power_domains_init(struct drm_i915_private *dev_priv)
 	dev_priv->csr.allowed_dc_mask =
 		get_allowed_dc_mask(dev_priv, i915_modparams.enable_dc);
 
+	dev_priv->csr.max_dc_state = DC_STATE_EN_UPTO_DC5_DC6_MASK;
 	BUILD_BUG_ON(POWER_DOMAIN_NUM > 64);
 
 	mutex_init(&power_domains->lock);
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
index 737b5def7fc6..69ebde992342 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power.h
@@ -100,6 +100,7 @@  enum i915_power_well_id {
 	SKL_DISP_PW_MISC_IO,
 	SKL_DISP_PW_1,
 	SKL_DISP_PW_2,
+	TGL_DISP_DC_OFF,
 };
 
 #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
@@ -256,6 +257,8 @@  void intel_display_power_suspend_late(struct drm_i915_private *i915);
 void intel_display_power_resume_early(struct drm_i915_private *i915);
 void intel_display_power_suspend(struct drm_i915_private *i915);
 void intel_display_power_resume(struct drm_i915_private *i915);
+void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
+			     bool psr2_deep_sleep);
 
 const char *
 intel_display_power_domain_str(enum intel_display_power_domain domain);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index db7480831e52..999da5d2da0b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -336,6 +336,7 @@  struct intel_csr {
 	i915_reg_t mmioaddr[20];
 	u32 mmiodata[20];
 	u32 dc_state;
+	u32 max_dc_state;
 	u32 allowed_dc_mask;
 	intel_wakeref_t wakeref;
 };

Comments

On Sat, Sep 07, 2019 at 10:44:39PM +0530, Anshuman Gupta wrote:
> Add max_dc_state and tgl_set_target_dc_state() API
> in order to enable DC3CO state with existing DC states.
> max_dc_state will enable/disable the desired DC state in
> DC_STATE_EN reg when "DC Off" power well gets disable/enable.
> 
> 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.
> v6: Fixed minor unwanted change.
> v7: Removed DC3CO powerwell and POWER_DOMAIN_VIDEO.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  .../drm/i915/display/intel_display_power.c    | 111 ++++++++++++++----
>  .../drm/i915/display/intel_display_power.h    |   3 +
>  drivers/gpu/drm/i915/i915_drv.h               |   1 +
>  3 files changed, 95 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 496fa1b53ffb..83b10f61ee42 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -772,6 +772,29 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
>  	dev_priv->csr.dc_state = val & mask;
>  }
>  
> +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv)

Should be tgl_enable_dc3co(), to match the rest of DC state helpers.

> +{
> +	if (!dev_priv->psr.sink_psr2_support)
> +		return;

PSR knows when to enable DC3co, so no need to double-check that here.

> +
> +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)

This check is out-of-place wrt. the same checks for other DC states.

> +		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);
> +}
> +
>  static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
>  {
>  	assert_can_enable_dc9(dev_priv);
> @@ -939,7 +962,8 @@ static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv)
>  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;
> +	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 gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv)
> @@ -955,24 +979,32 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_cdclk_state cdclk_state = {};
>  
> -	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> +	if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {
> +		tgl_disallow_dc3co(dev_priv);
> +	} else {

With an early return you can avoid the extra diff and make reviewing
easier.

> +		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>  
> -	dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> -	/* Can't read out voltage_level so can't use intel_cdclk_changed() */
> -	WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state));
> +		dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> +		/*
> +		 * Can't read out voltage_level so can't use
> +		 * intel_cdclk_changed()
> +		 */
> +		WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw,
> +						  &cdclk_state));
>  
> -	gen9_assert_dbuf_enabled(dev_priv);
> +		gen9_assert_dbuf_enabled(dev_priv);
>  
> -	if (IS_GEN9_LP(dev_priv))
> -		bxt_verify_ddi_phy_power_wells(dev_priv);
> +		if (IS_GEN9_LP(dev_priv))
> +			bxt_verify_ddi_phy_power_wells(dev_priv);
>  
> -	if (INTEL_GEN(dev_priv) >= 11)
> -		/*
> -		 * DMC retains HW context only for port A, the other combo
> -		 * PHY's HW context for port B is lost after DC transitions,
> -		 * so we need to restore it manually.
> -		 */
> -		intel_combo_phy_init(dev_priv);
> +		if (INTEL_GEN(dev_priv) >= 11)
> +			/*
> +			 * DMC retains HW context only for port A, the other
> +			 * combo PHY's HW context for port B is lost after
> +			 * DC transitions, so we need to restore it manually.
> +			 */
> +			intel_combo_phy_init(dev_priv);
> +	}
>  }
>  
>  static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
> @@ -987,10 +1019,48 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
>  	if (!dev_priv->csr.dmc_payload)
>  		return;
>  
> -	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> -		skl_enable_dc6(dev_priv);
> -	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> -		gen9_enable_dc5(dev_priv);
> +	if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {

target_dc_state would be a better name and it shold be an exact state
instead of a mask.

> +		tgl_allow_dc3co(dev_priv);
> +	} else if (dev_priv->csr.max_dc_state & DC_STATE_EN_UPTO_DC5_DC6_MASK) {
> +		if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)

We should make these checks more uniform, by checking here only
target_dc_state.

> +			skl_enable_dc6(dev_priv);
> +		else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> +			gen9_enable_dc5(dev_priv);
> +	}
> +}
> +
> +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
> +			     bool psr2_deep_sleep)

psr2_deep_sleep is a PSR internal stuff, while PSR is using the power
domains framework, so let's keep the deep sleep programming in the PSR
code.

> +{
> +	struct i915_power_well *power_well;
> +	bool dc_off_enabled;
> +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +
> +	mutex_lock(&power_domains->lock);
> +	power_well = lookup_power_well(dev_priv, TGL_DISP_DC_OFF);
> +
> +	if (!power_well)
> +		goto unlock;
> +
> +	dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv,
> +							   power_well);
> +	if (state == dev_priv->csr.max_dc_state)
> +		goto unlock;
> +
> +	if (!dc_off_enabled) {
> +		/*
> +		 * Need to disable the DC off power well to
> +		 * effect target DC state.
> +		 */
> +		power_well->desc->ops->enable(dev_priv, power_well);
> +		dev_priv->csr.max_dc_state = state;
> +		power_well->desc->ops->disable(dev_priv, power_well);
> +		goto unlock;
> +	}
> +		dev_priv->csr.max_dc_state = state;
> +
> +unlock:
> +	mutex_unlock(&power_domains->lock);
>  }
>  
>  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> @@ -3610,7 +3680,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
>  		.name = "DC off",
>  		.domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS,
>  		.ops = &gen9_dc_off_power_well_ops,
> -		.id = DISP_PW_ID_NONE,
> +		.id = TGL_DISP_DC_OFF,

Let's assign this ID on all platforms for consistency.

>  	},
>  	{
>  		.name = "power well 2",
> @@ -4039,6 +4109,7 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>  	dev_priv->csr.allowed_dc_mask =
>  		get_allowed_dc_mask(dev_priv, i915_modparams.enable_dc);
>  
> +	dev_priv->csr.max_dc_state = DC_STATE_EN_UPTO_DC5_DC6_MASK;
>  	BUILD_BUG_ON(POWER_DOMAIN_NUM > 64);
>  
>  	mutex_init(&power_domains->lock);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> index 737b5def7fc6..69ebde992342 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -100,6 +100,7 @@ enum i915_power_well_id {
>  	SKL_DISP_PW_MISC_IO,
>  	SKL_DISP_PW_1,
>  	SKL_DISP_PW_2,
> +	TGL_DISP_DC_OFF,
>  };
>  
>  #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
> @@ -256,6 +257,8 @@ void intel_display_power_suspend_late(struct drm_i915_private *i915);
>  void intel_display_power_resume_early(struct drm_i915_private *i915);
>  void intel_display_power_suspend(struct drm_i915_private *i915);
>  void intel_display_power_resume(struct drm_i915_private *i915);
> +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
> +			     bool psr2_deep_sleep);
>  
>  const char *
>  intel_display_power_domain_str(enum intel_display_power_domain domain);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index db7480831e52..999da5d2da0b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -336,6 +336,7 @@ struct intel_csr {
>  	i915_reg_t mmioaddr[20];
>  	u32 mmiodata[20];
>  	u32 dc_state;
> +	u32 max_dc_state;
>  	u32 allowed_dc_mask;
>  	intel_wakeref_t wakeref;
>  };
> -- 
> 2.21.0
>
On 2019-09-08 at 19:44:35 +0300, Imre Deak wrote:
> On Sat, Sep 07, 2019 at 10:44:39PM +0530, Anshuman Gupta wrote:
Hi Imre,
Thanks for reviewing the pacthes i will rework the patches.
There are few comments from my side which will help to rework. 
> > Add max_dc_state and tgl_set_target_dc_state() API
> > in order to enable DC3CO state with existing DC states.
> > max_dc_state will enable/disable the desired DC state in
> > DC_STATE_EN reg when "DC Off" power well gets disable/enable.
> > 
> > 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.
> > v6: Fixed minor unwanted change.
> > v7: Removed DC3CO powerwell and POWER_DOMAIN_VIDEO.
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_power.c    | 111 ++++++++++++++----
> >  .../drm/i915/display/intel_display_power.h    |   3 +
> >  drivers/gpu/drm/i915/i915_drv.h               |   1 +
> >  3 files changed, 95 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 496fa1b53ffb..83b10f61ee42 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -772,6 +772,29 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
> >  	dev_priv->csr.dc_state = val & mask;
> >  }
> >  
> > +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv)
> 
> Should be tgl_enable_dc3co(), to match the rest of DC state helpers.
> 
> > +{
> > +	if (!dev_priv->psr.sink_psr2_support)
> > +		return;
> 
> PSR knows when to enable DC3co, so no need to double-check that here.
> 
> > +
> > +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)
> 
> This check is out-of-place wrt. the same checks for other DC states.
> 
> > +		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);
> > +}
> > +
> >  static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
> >  {
> >  	assert_can_enable_dc9(dev_priv);
> > @@ -939,7 +962,8 @@ static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv)
> >  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;
> > +	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 gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv)
> > @@ -955,24 +979,32 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv)
> >  {
> >  	struct intel_cdclk_state cdclk_state = {};
> >  
> > -	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > +	if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {
> > +		tgl_disallow_dc3co(dev_priv);
> > +	} else {
> 
> With an early return you can avoid the extra diff and make reviewing
> easier.
> 
> > +		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> >  
> > -	dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> > -	/* Can't read out voltage_level so can't use intel_cdclk_changed() */
> > -	WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state));
> > +		dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> > +		/*
> > +		 * Can't read out voltage_level so can't use
> > +		 * intel_cdclk_changed()
> > +		 */
> > +		WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw,
> > +						  &cdclk_state));
> >  
> > -	gen9_assert_dbuf_enabled(dev_priv);
> > +		gen9_assert_dbuf_enabled(dev_priv);
> >  
> > -	if (IS_GEN9_LP(dev_priv))
> > -		bxt_verify_ddi_phy_power_wells(dev_priv);
> > +		if (IS_GEN9_LP(dev_priv))
> > +			bxt_verify_ddi_phy_power_wells(dev_priv);
> >  
> > -	if (INTEL_GEN(dev_priv) >= 11)
> > -		/*
> > -		 * DMC retains HW context only for port A, the other combo
> > -		 * PHY's HW context for port B is lost after DC transitions,
> > -		 * so we need to restore it manually.
> > -		 */
> > -		intel_combo_phy_init(dev_priv);
> > +		if (INTEL_GEN(dev_priv) >= 11)
> > +			/*
> > +			 * DMC retains HW context only for port A, the other
> > +			 * combo PHY's HW context for port B is lost after
> > +			 * DC transitions, so we need to restore it manually.
> > +			 */
> > +			intel_combo_phy_init(dev_priv);
> > +	}
> >  }
> >  
> >  static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
> > @@ -987,10 +1019,48 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
> >  	if (!dev_priv->csr.dmc_payload)
> >  		return;
> >  
> > -	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> > -		skl_enable_dc6(dev_priv);
> > -	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> > -		gen9_enable_dc5(dev_priv);
> > +	if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {
> 
> target_dc_state would be a better name and it shold be an exact state
> instead of a mask.
> 
> > +		tgl_allow_dc3co(dev_priv);
> > +	} else if (dev_priv->csr.max_dc_state & DC_STATE_EN_UPTO_DC5_DC6_MASK) {
> > +		if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> 
> We should make these checks more uniform, by checking here only
> target_dc_state.
If i understand correctly this comment is about to have a uniform 
condition target_dc_state in order to decide which DC state we 
interested. Then we may require to check allowed_dc_mask accordingly in 
tgl_set_target_dc_state(), but that would not be fitting for 
older platforms.
What is your input on this? please correct me if i am wrong here.
> 
> > +			skl_enable_dc6(dev_priv);
> > +		else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> > +			gen9_enable_dc5(dev_priv);
> > +	}
> > +}
> > +
> > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
> > +			     bool psr2_deep_sleep)
> 
> psr2_deep_sleep is a PSR internal stuff, while PSR is using the power
> domains framework, so let's keep the deep sleep programming in the PSR
> code.
I wanted not to enable/disable psr2 deep sleep for each flip flush request.
Though PSR2 deep sleep is PSR internal stuff but it is H/W has tied with DC3CO.
That was intention to keep psr2_deep sleep enable/diable here (Doing in Patch 6).
In order to switch between DC3CO and DC5 we need to call 
tgl_psr2_deep_sleep_disable/enable() from outside of PSR code.
please correct me if i am wrong here.
Thanks ,
Anshuman Gupta.  
> 
> > +{
> > +	struct i915_power_well *power_well;
> > +	bool dc_off_enabled;
> > +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > +
> > +	mutex_lock(&power_domains->lock);
> > +	power_well = lookup_power_well(dev_priv, TGL_DISP_DC_OFF);
> > +
> > +	if (!power_well)
> > +		goto unlock;
> > +
> > +	dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv,
> > +							   power_well);
> > +	if (state == dev_priv->csr.max_dc_state)
> > +		goto unlock;
> > +
> > +	if (!dc_off_enabled) {
> > +		/*
> > +		 * Need to disable the DC off power well to
> > +		 * effect target DC state.
> > +		 */
> > +		power_well->desc->ops->enable(dev_priv, power_well);
> > +		dev_priv->csr.max_dc_state = state;
> > +		power_well->desc->ops->disable(dev_priv, power_well);
> > +		goto unlock;
> > +	}
> > +		dev_priv->csr.max_dc_state = state;
> > +
> > +unlock:
> > +	mutex_unlock(&power_domains->lock);
> >  }
> >  
> >  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> > @@ -3610,7 +3680,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
> >  		.name = "DC off",
> >  		.domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS,
> >  		.ops = &gen9_dc_off_power_well_ops,
> > -		.id = DISP_PW_ID_NONE,
> > +		.id = TGL_DISP_DC_OFF,
> 
> Let's assign this ID on all platforms for consistency.
IMO that should be done in different patch, as tgl_set_target_dc_state() is 
only consumer of this id ?
What is your opinion on this ?
> 
> >  	},
> >  	{
> >  		.name = "power well 2",
> > @@ -4039,6 +4109,7 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
> >  	dev_priv->csr.allowed_dc_mask =
> >  		get_allowed_dc_mask(dev_priv, i915_modparams.enable_dc);
> >  
> > +	dev_priv->csr.max_dc_state = DC_STATE_EN_UPTO_DC5_DC6_MASK;
> >  	BUILD_BUG_ON(POWER_DOMAIN_NUM > 64);
> >  
> >  	mutex_init(&power_domains->lock);
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> > index 737b5def7fc6..69ebde992342 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> > @@ -100,6 +100,7 @@ enum i915_power_well_id {
> >  	SKL_DISP_PW_MISC_IO,
> >  	SKL_DISP_PW_1,
> >  	SKL_DISP_PW_2,
> > +	TGL_DISP_DC_OFF,
> >  };
> >  
> >  #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
> > @@ -256,6 +257,8 @@ void intel_display_power_suspend_late(struct drm_i915_private *i915);
> >  void intel_display_power_resume_early(struct drm_i915_private *i915);
> >  void intel_display_power_suspend(struct drm_i915_private *i915);
> >  void intel_display_power_resume(struct drm_i915_private *i915);
> > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
> > +			     bool psr2_deep_sleep);
> >  
> >  const char *
> >  intel_display_power_domain_str(enum intel_display_power_domain domain);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index db7480831e52..999da5d2da0b 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -336,6 +336,7 @@ struct intel_csr {
> >  	i915_reg_t mmioaddr[20];
> >  	u32 mmiodata[20];
> >  	u32 dc_state;
> > +	u32 max_dc_state;
> >  	u32 allowed_dc_mask;
> >  	intel_wakeref_t wakeref;
> >  };
> > -- 
> > 2.21.0
> >
On Mon, Sep 09, 2019 at 09:49:17PM +0530, Anshuman Gupta wrote:
> On 2019-09-08 at 19:44:35 +0300, Imre Deak wrote:
> > On Sat, Sep 07, 2019 at 10:44:39PM +0530, Anshuman Gupta wrote:
> Hi Imre,
> Thanks for reviewing the pacthes i will rework the patches.
> There are few comments from my side which will help to rework. 
> > > Add max_dc_state and tgl_set_target_dc_state() API
> > > in order to enable DC3CO state with existing DC states.
> > > max_dc_state will enable/disable the desired DC state in
> > > DC_STATE_EN reg when "DC Off" power well gets disable/enable.
> > > 
> > > 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.
> > > v6: Fixed minor unwanted change.
> > > v7: Removed DC3CO powerwell and POWER_DOMAIN_VIDEO.
> > > 
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Animesh Manna <animesh.manna@intel.com>
> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_power.c    | 111 ++++++++++++++----
> > >  .../drm/i915/display/intel_display_power.h    |   3 +
> > >  drivers/gpu/drm/i915/i915_drv.h               |   1 +
> > >  3 files changed, 95 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > index 496fa1b53ffb..83b10f61ee42 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > @@ -772,6 +772,29 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
> > >  	dev_priv->csr.dc_state = val & mask;
> > >  }
> > >  
> > > +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv)
> > 
> > Should be tgl_enable_dc3co(), to match the rest of DC state helpers.
> > 
> > > +{
> > > +	if (!dev_priv->psr.sink_psr2_support)
> > > +		return;
> > 
> > PSR knows when to enable DC3co, so no need to double-check that here.
> > 
> > > +
> > > +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)
> > 
> > This check is out-of-place wrt. the same checks for other DC states.
> > 
> > > +		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);
> > > +}
> > > +
> > >  static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
> > >  {
> > >  	assert_can_enable_dc9(dev_priv);
> > > @@ -939,7 +962,8 @@ static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv)
> > >  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;
> > > +	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 gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv)
> > > @@ -955,24 +979,32 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv)
> > >  {
> > >  	struct intel_cdclk_state cdclk_state = {};
> > >  
> > > -	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > > +	if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {
> > > +		tgl_disallow_dc3co(dev_priv);
> > > +	} else {
> > 
> > With an early return you can avoid the extra diff and make reviewing
> > easier.
> > 
> > > +		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > >  
> > > -	dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> > > -	/* Can't read out voltage_level so can't use intel_cdclk_changed() */
> > > -	WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state));
> > > +		dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> > > +		/*
> > > +		 * Can't read out voltage_level so can't use
> > > +		 * intel_cdclk_changed()
> > > +		 */
> > > +		WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw,
> > > +						  &cdclk_state));
> > >  
> > > -	gen9_assert_dbuf_enabled(dev_priv);
> > > +		gen9_assert_dbuf_enabled(dev_priv);
> > >  
> > > -	if (IS_GEN9_LP(dev_priv))
> > > -		bxt_verify_ddi_phy_power_wells(dev_priv);
> > > +		if (IS_GEN9_LP(dev_priv))
> > > +			bxt_verify_ddi_phy_power_wells(dev_priv);
> > >  
> > > -	if (INTEL_GEN(dev_priv) >= 11)
> > > -		/*
> > > -		 * DMC retains HW context only for port A, the other combo
> > > -		 * PHY's HW context for port B is lost after DC transitions,
> > > -		 * so we need to restore it manually.
> > > -		 */
> > > -		intel_combo_phy_init(dev_priv);
> > > +		if (INTEL_GEN(dev_priv) >= 11)
> > > +			/*
> > > +			 * DMC retains HW context only for port A, the other
> > > +			 * combo PHY's HW context for port B is lost after
> > > +			 * DC transitions, so we need to restore it manually.
> > > +			 */
> > > +			intel_combo_phy_init(dev_priv);
> > > +	}
> > >  }
> > >  
> > >  static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
> > > @@ -987,10 +1019,48 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
> > >  	if (!dev_priv->csr.dmc_payload)
> > >  		return;
> > >  
> > > -	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> > > -		skl_enable_dc6(dev_priv);
> > > -	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> > > -		gen9_enable_dc5(dev_priv);
> > > +	if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {
> > 
> > target_dc_state would be a better name and it shold be an exact state
> > instead of a mask.
> > 
> > > +		tgl_allow_dc3co(dev_priv);
> > > +	} else if (dev_priv->csr.max_dc_state & DC_STATE_EN_UPTO_DC5_DC6_MASK) {
> > > +		if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> > 
> > We should make these checks more uniform, by checking here only
> > target_dc_state.
> If i understand correctly this comment is about to have a uniform 
> condition target_dc_state in order to decide which DC state we 
> interested. Then we may require to check allowed_dc_mask accordingly in 
> tgl_set_target_dc_state(),

Yes, we could check here only target_dc_state, setting that one only to
values permitted by allowed_dc_mask.

> but that would not be fitting for 
> older platforms.
> What is your input on this? please correct me if i am wrong here.
> > 
> > > +			skl_enable_dc6(dev_priv);
> > > +		else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> > > +			gen9_enable_dc5(dev_priv);
> > > +	}
> > > +}
> > > +
> > > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
> > > +			     bool psr2_deep_sleep)
> > 
> > psr2_deep_sleep is a PSR internal stuff, while PSR is using the power
> > domains framework, so let's keep the deep sleep programming in the PSR
> > code.
> I wanted not to enable/disable psr2 deep sleep for each flip flush request.
> Though PSR2 deep sleep is PSR internal stuff but it is H/W has tied with DC3CO.
> That was intention to keep psr2_deep sleep enable/diable here (Doing in Patch 6).
> In order to switch between DC3CO and DC5 we need to call 
> tgl_psr2_deep_sleep_disable/enable() from outside of PSR code.
> please correct me if i am wrong here.

I think it still belongs to the PSR code with other steps needed for
DC3co setup, like the idle frame programming. PSR deep sleep could also
be enabled/disabled independently of the DC state we enable (for
instance enable/disable deep sleep when both DC3co and DC5 is
disabled). It also needs the PSR lock, which again is internal to the
PSR code.

> Thanks ,
> Anshuman Gupta.  
> > 
> > > +{
> > > +	struct i915_power_well *power_well;
> > > +	bool dc_off_enabled;
> > > +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > > +
> > > +	mutex_lock(&power_domains->lock);
> > > +	power_well = lookup_power_well(dev_priv, TGL_DISP_DC_OFF);
> > > +
> > > +	if (!power_well)
> > > +		goto unlock;
> > > +
> > > +	dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv,
> > > +							   power_well);
> > > +	if (state == dev_priv->csr.max_dc_state)
> > > +		goto unlock;
> > > +
> > > +	if (!dc_off_enabled) {
> > > +		/*
> > > +		 * Need to disable the DC off power well to
> > > +		 * effect target DC state.
> > > +		 */
> > > +		power_well->desc->ops->enable(dev_priv, power_well);
> > > +		dev_priv->csr.max_dc_state = state;
> > > +		power_well->desc->ops->disable(dev_priv, power_well);
> > > +		goto unlock;
> > > +	}
> > > +		dev_priv->csr.max_dc_state = state;
> > > +
> > > +unlock:
> > > +	mutex_unlock(&power_domains->lock);
> > >  }
> > >  
> > >  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> > > @@ -3610,7 +3680,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
> > >  		.name = "DC off",
> > >  		.domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS,
> > >  		.ops = &gen9_dc_off_power_well_ops,
> > > -		.id = DISP_PW_ID_NONE,
> > > +		.id = TGL_DISP_DC_OFF,
> > 
> > Let's assign this ID on all platforms for consistency.
> IMO that should be done in different patch, as tgl_set_target_dc_state() is 
> only consumer of this id ?
> What is your opinion on this ?
> > 
> > >  	},
> > >  	{
> > >  		.name = "power well 2",
> > > @@ -4039,6 +4109,7 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
> > >  	dev_priv->csr.allowed_dc_mask =
> > >  		get_allowed_dc_mask(dev_priv, i915_modparams.enable_dc);
> > >  
> > > +	dev_priv->csr.max_dc_state = DC_STATE_EN_UPTO_DC5_DC6_MASK;
> > >  	BUILD_BUG_ON(POWER_DOMAIN_NUM > 64);
> > >  
> > >  	mutex_init(&power_domains->lock);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> > > index 737b5def7fc6..69ebde992342 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> > > @@ -100,6 +100,7 @@ enum i915_power_well_id {
> > >  	SKL_DISP_PW_MISC_IO,
> > >  	SKL_DISP_PW_1,
> > >  	SKL_DISP_PW_2,
> > > +	TGL_DISP_DC_OFF,
> > >  };
> > >  
> > >  #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
> > > @@ -256,6 +257,8 @@ void intel_display_power_suspend_late(struct drm_i915_private *i915);
> > >  void intel_display_power_resume_early(struct drm_i915_private *i915);
> > >  void intel_display_power_suspend(struct drm_i915_private *i915);
> > >  void intel_display_power_resume(struct drm_i915_private *i915);
> > > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
> > > +			     bool psr2_deep_sleep);
> > >  
> > >  const char *
> > >  intel_display_power_domain_str(enum intel_display_power_domain domain);
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index db7480831e52..999da5d2da0b 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -336,6 +336,7 @@ struct intel_csr {
> > >  	i915_reg_t mmioaddr[20];
> > >  	u32 mmiodata[20];
> > >  	u32 dc_state;
> > > +	u32 max_dc_state;
> > >  	u32 allowed_dc_mask;
> > >  	intel_wakeref_t wakeref;
> > >  };
> > > -- 
> > > 2.21.0
> > >
On 2019-09-11 at 11:21:42 +0300, Imre Deak wrote:
> On Mon, Sep 09, 2019 at 09:49:17PM +0530, Anshuman Gupta wrote:
> > On 2019-09-08 at 19:44:35 +0300, Imre Deak wrote:
> > > On Sat, Sep 07, 2019 at 10:44:39PM +0530, Anshuman Gupta wrote:
> > Hi Imre,
> > Thanks for reviewing the pacthes i will rework the patches.
> > There are few comments from my side which will help to rework. 
> > > > Add max_dc_state and tgl_set_target_dc_state() API
> > > > in order to enable DC3CO state with existing DC states.
> > > > max_dc_state will enable/disable the desired DC state in
> > > > DC_STATE_EN reg when "DC Off" power well gets disable/enable.
> > > > 
> > > > 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.
> > > > v6: Fixed minor unwanted change.
> > > > v7: Removed DC3CO powerwell and POWER_DOMAIN_VIDEO.
> > > > 
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Cc: Animesh Manna <animesh.manna@intel.com>
> > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > > ---
> > > >  .../drm/i915/display/intel_display_power.c    | 111 ++++++++++++++----
> > > >  .../drm/i915/display/intel_display_power.h    |   3 +
> > > >  drivers/gpu/drm/i915/i915_drv.h               |   1 +
> > > >  3 files changed, 95 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > > index 496fa1b53ffb..83b10f61ee42 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > > @@ -772,6 +772,29 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
> > > >  	dev_priv->csr.dc_state = val & mask;
> > > >  }
> > > >  
> > > > +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv)
> > > 
> > > Should be tgl_enable_dc3co(), to match the rest of DC state helpers.
> > > 
> > > > +{
> > > > +	if (!dev_priv->psr.sink_psr2_support)
> > > > +		return;
> > > 
> > > PSR knows when to enable DC3co, so no need to double-check that here.
> > > 
> > > > +
> > > > +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)
> > > 
> > > This check is out-of-place wrt. the same checks for other DC states.
> > > 
> > > > +		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);
> > > > +}
> > > > +
> > > >  static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
> > > >  {
> > > >  	assert_can_enable_dc9(dev_priv);
> > > > @@ -939,7 +962,8 @@ static void bxt_verify_ddi_phy_power_wells(struct drm_i915_private *dev_priv)
> > > >  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;
> > > > +	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 gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv)
> > > > @@ -955,24 +979,32 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv)
> > > >  {
> > > >  	struct intel_cdclk_state cdclk_state = {};
> > > >  
> > > > -	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > > > +	if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {
> > > > +		tgl_disallow_dc3co(dev_priv);
> > > > +	} else {
> > > 
> > > With an early return you can avoid the extra diff and make reviewing
> > > easier.
> > > 
> > > > +		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > > >  
> > > > -	dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> > > > -	/* Can't read out voltage_level so can't use intel_cdclk_changed() */
> > > > -	WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state));
> > > > +		dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> > > > +		/*
> > > > +		 * Can't read out voltage_level so can't use
> > > > +		 * intel_cdclk_changed()
> > > > +		 */
> > > > +		WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw,
> > > > +						  &cdclk_state));
> > > >  
> > > > -	gen9_assert_dbuf_enabled(dev_priv);
> > > > +		gen9_assert_dbuf_enabled(dev_priv);
> > > >  
> > > > -	if (IS_GEN9_LP(dev_priv))
> > > > -		bxt_verify_ddi_phy_power_wells(dev_priv);
> > > > +		if (IS_GEN9_LP(dev_priv))
> > > > +			bxt_verify_ddi_phy_power_wells(dev_priv);
> > > >  
> > > > -	if (INTEL_GEN(dev_priv) >= 11)
> > > > -		/*
> > > > -		 * DMC retains HW context only for port A, the other combo
> > > > -		 * PHY's HW context for port B is lost after DC transitions,
> > > > -		 * so we need to restore it manually.
> > > > -		 */
> > > > -		intel_combo_phy_init(dev_priv);
> > > > +		if (INTEL_GEN(dev_priv) >= 11)
> > > > +			/*
> > > > +			 * DMC retains HW context only for port A, the other
> > > > +			 * combo PHY's HW context for port B is lost after
> > > > +			 * DC transitions, so we need to restore it manually.
> > > > +			 */
> > > > +			intel_combo_phy_init(dev_priv);
> > > > +	}
> > > >  }
> > > >  
> > > >  static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
> > > > @@ -987,10 +1019,48 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
> > > >  	if (!dev_priv->csr.dmc_payload)
> > > >  		return;
> > > >  
> > > > -	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> > > > -		skl_enable_dc6(dev_priv);
> > > > -	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> > > > -		gen9_enable_dc5(dev_priv);
> > > > +	if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {
> > > 
> > > target_dc_state would be a better name and it shold be an exact state
> > > instead of a mask.
> > > 
> > > > +		tgl_allow_dc3co(dev_priv);
> > > > +	} else if (dev_priv->csr.max_dc_state & DC_STATE_EN_UPTO_DC5_DC6_MASK) {
> > > > +		if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> > > 
> > > We should make these checks more uniform, by checking here only
> > > target_dc_state.
> > If i understand correctly this comment is about to have a uniform 
> > condition target_dc_state in order to decide which DC state we 
> > interested. Then we may require to check allowed_dc_mask accordingly in 
> > tgl_set_target_dc_state(),
> 
> Yes, we could check here only target_dc_state, setting that one only to
> values permitted by allowed_dc_mask.
For older platform setting target_dc_state value permitted by allowed_dc_mask
i am planning to have a allowed_dc_mask_to_target_dc_state() func, which
will be called from intel_power_domains_init, and initilizes target_dc_state.
for tgl onwards tgl_set_target_dc_state() will do it.
Hope it will be ok ?  
> 
> > but that would not be fitting for 
> > older platforms.
> > What is your input on this? please correct me if i am wrong here.
> > > 
> > > > +			skl_enable_dc6(dev_priv);
> > > > +		else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> > > > +			gen9_enable_dc5(dev_priv);
> > > > +	}
> > > > +}
> > > > +
> > > > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
> > > > +			     bool psr2_deep_sleep)
> > > 
> > > psr2_deep_sleep is a PSR internal stuff, while PSR is using the power
> > > domains framework, so let's keep the deep sleep programming in the PSR
> > > code.
> > I wanted not to enable/disable psr2 deep sleep for each flip flush request.
> > Though PSR2 deep sleep is PSR internal stuff but it is H/W has tied with DC3CO.
> > That was intention to keep psr2_deep sleep enable/diable here (Doing in Patch 6).
> > In order to switch between DC3CO and DC5 we need to call 
> > tgl_psr2_deep_sleep_disable/enable() from outside of PSR code.
> > please correct me if i am wrong here.
> 
> I think it still belongs to the PSR code with other steps needed for
> DC3co setup, like the idle frame programming. PSR deep sleep could also
> be enabled/disabled independently of the DC state we enable (for
> instance enable/disable deep sleep when both DC3co and DC5 is
> disabled). It also needs the PSR lock, which again is internal to the
> PSR code.
> 
> > Thanks ,
> > Anshuman Gupta.  
> > > 
> > > > +{
> > > > +	struct i915_power_well *power_well;
> > > > +	bool dc_off_enabled;
> > > > +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
> > > > +
> > > > +	mutex_lock(&power_domains->lock);
> > > > +	power_well = lookup_power_well(dev_priv, TGL_DISP_DC_OFF);
> > > > +
> > > > +	if (!power_well)
> > > > +		goto unlock;
> > > > +
> > > > +	dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv,
> > > > +							   power_well);
> > > > +	if (state == dev_priv->csr.max_dc_state)
> > > > +		goto unlock;
> > > > +
> > > > +	if (!dc_off_enabled) {
> > > > +		/*
> > > > +		 * Need to disable the DC off power well to
> > > > +		 * effect target DC state.
> > > > +		 */
> > > > +		power_well->desc->ops->enable(dev_priv, power_well);
> > > > +		dev_priv->csr.max_dc_state = state;
> > > > +		power_well->desc->ops->disable(dev_priv, power_well);
> > > > +		goto unlock;
> > > > +	}
> > > > +		dev_priv->csr.max_dc_state = state;
> > > > +
> > > > +unlock:
> > > > +	mutex_unlock(&power_domains->lock);
> > > >  }
> > > >  
> > > >  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> > > > @@ -3610,7 +3680,7 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
> > > >  		.name = "DC off",
> > > >  		.domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS,
> > > >  		.ops = &gen9_dc_off_power_well_ops,
> > > > -		.id = DISP_PW_ID_NONE,
> > > > +		.id = TGL_DISP_DC_OFF,
> > > 
> > > Let's assign this ID on all platforms for consistency.
> > IMO that should be done in different patch, as tgl_set_target_dc_state() is 
> > only consumer of this id ?
> > What is your opinion on this ?
> > > 
> > > >  	},
> > > >  	{
> > > >  		.name = "power well 2",
> > > > @@ -4039,6 +4109,7 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
> > > >  	dev_priv->csr.allowed_dc_mask =
> > > >  		get_allowed_dc_mask(dev_priv, i915_modparams.enable_dc);
> > > >  
> > > > +	dev_priv->csr.max_dc_state = DC_STATE_EN_UPTO_DC5_DC6_MASK;
> > > >  	BUILD_BUG_ON(POWER_DOMAIN_NUM > 64);
> > > >  
> > > >  	mutex_init(&power_domains->lock);
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> > > > index 737b5def7fc6..69ebde992342 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> > > > @@ -100,6 +100,7 @@ enum i915_power_well_id {
> > > >  	SKL_DISP_PW_MISC_IO,
> > > >  	SKL_DISP_PW_1,
> > > >  	SKL_DISP_PW_2,
> > > > +	TGL_DISP_DC_OFF,
> > > >  };
> > > >  
> > > >  #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
> > > > @@ -256,6 +257,8 @@ void intel_display_power_suspend_late(struct drm_i915_private *i915);
> > > >  void intel_display_power_resume_early(struct drm_i915_private *i915);
> > > >  void intel_display_power_suspend(struct drm_i915_private *i915);
> > > >  void intel_display_power_resume(struct drm_i915_private *i915);
> > > > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
> > > > +			     bool psr2_deep_sleep);
> > > >  
> > > >  const char *
> > > >  intel_display_power_domain_str(enum intel_display_power_domain domain);
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index db7480831e52..999da5d2da0b 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -336,6 +336,7 @@ struct intel_csr {
> > > >  	i915_reg_t mmioaddr[20];
> > > >  	u32 mmiodata[20];
> > > >  	u32 dc_state;
> > > > +	u32 max_dc_state;
> > > >  	u32 allowed_dc_mask;
> > > >  	intel_wakeref_t wakeref;
> > > >  };
> > > > -- 
> > > > 2.21.0
> > > >