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

Submitted by Anshuamn Gupta on Sept. 26, 2019, 2:56 p.m.

Details

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

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

Commit Message

Anshuamn Gupta Sept. 26, 2019, 2:56 p.m.
Add target_dc_state and tgl_set_target_dc_state() API
in order to enable DC3CO state with existing DC states.
target_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.
v8: Uniform checks by using only target_dc_state instead of allowed_dc_mask
    in "DC off" power well callback. [Imre]
    Adding "DC off" power well id to older platforms. [Imre]
    Removed psr2_deep_sleep flag from tgl_set_target_dc_state. [Imre]
v9: Used switch case for target DC state in
    gen9_dc_off_power_well_disable(), checking DC3CO state against
    allowed DC mask, using WARN_ON() in
    tgl_set_target_dc_state(). [Imre]

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    | 110 ++++++++++++++++--
 .../drm/i915/display/intel_display_power.h    |   2 +
 drivers/gpu/drm/i915/i915_drv.h               |   1 +
 3 files changed, 104 insertions(+), 9 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 0b685c517bcb..9f787556f80d 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -785,6 +785,38 @@  static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
 	dev_priv->csr.dc_state = val & mask;
 }
 
+static void
+allowed_dc_mask_to_target_dc_state(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
+		dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC6;
+	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
+		dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC5;
+	else
+		dev_priv->csr.target_dc_state = DC_STATE_DISABLE;
+}
+
+static void tgl_enable_dc3co(struct drm_i915_private *dev_priv)
+{
+	DRM_DEBUG_KMS("Enabling DC3CO\n");
+	gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO);
+}
+
+static void tgl_disable_dc3co(struct drm_i915_private *dev_priv)
+{
+	u32 val;
+
+	DRM_DEBUG_KMS("Disabling DC3CO\n");
+	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
+	 */
+	usleep_range(200, 210);
+}
+
 static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
 {
 	assert_can_enable_dc9(dev_priv);
@@ -952,7 +984,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)
@@ -968,6 +1001,11 @@  static void gen9_disable_dc_states(struct drm_i915_private *dev_priv)
 {
 	struct intel_cdclk_state cdclk_state = {};
 
+	if (dev_priv->csr.target_dc_state == DC_STATE_EN_DC3CO) {
+		tgl_disable_dc3co(dev_priv);
+		return;
+	}
+
 	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 
 	dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
@@ -1000,10 +1038,63 @@  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)
+	switch (dev_priv->csr.target_dc_state) {
+	case DC_STATE_EN_DC3CO:
+		tgl_enable_dc3co(dev_priv);
+		break;
+	case DC_STATE_EN_UPTO_DC6:
 		skl_enable_dc6(dev_priv);
-	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
+		break;
+	case DC_STATE_EN_UPTO_DC5:
 		gen9_enable_dc5(dev_priv);
+		break;
+	}
+}
+
+void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state)
+{
+	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, SKL_DISP_DC_OFF);
+
+	if (WARN_ON(!power_well))
+		goto unlock;
+
+	/*
+	 *  Compute the adjusted state wrt to the permisisble allowed dc mask.
+	 */
+	if (state != DC_STATE_EN_DC3CO ||
+	    !(dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)) {
+		if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
+			state = DC_STATE_EN_UPTO_DC6;
+		else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
+			state = DC_STATE_EN_UPTO_DC5;
+		else
+			state = DC_STATE_DISABLE;
+	}
+
+	if (state == dev_priv->csr.target_dc_state)
+		goto unlock;
+
+	dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv,
+							   power_well);
+	/*
+	 * If DC off power well is disabled, need to enable and disable the
+	 * DC off power well to effect target DC state.
+	 */
+	if (!dc_off_enabled)
+		power_well->desc->ops->enable(dev_priv, power_well);
+
+	dev_priv->csr.target_dc_state = state;
+
+	if (!dc_off_enabled)
+		power_well->desc->ops->disable(dev_priv, power_well);
+
+unlock:
+	mutex_unlock(&power_domains->lock);
 }
 
 static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
@@ -2951,7 +3042,7 @@  static const struct i915_power_well_desc skl_power_wells[] = {
 		.name = "DC off",
 		.domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
 		.ops = &gen9_dc_off_power_well_ops,
-		.id = DISP_PW_ID_NONE,
+		.id = SKL_DISP_DC_OFF,
 	},
 	{
 		.name = "power well 2",
@@ -3033,7 +3124,7 @@  static const struct i915_power_well_desc bxt_power_wells[] = {
 		.name = "DC off",
 		.domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
 		.ops = &gen9_dc_off_power_well_ops,
-		.id = DISP_PW_ID_NONE,
+		.id = SKL_DISP_DC_OFF,
 	},
 	{
 		.name = "power well 2",
@@ -3093,7 +3184,7 @@  static const struct i915_power_well_desc glk_power_wells[] = {
 		.name = "DC off",
 		.domains = GLK_DISPLAY_DC_OFF_POWER_DOMAINS,
 		.ops = &gen9_dc_off_power_well_ops,
-		.id = DISP_PW_ID_NONE,
+		.id = SKL_DISP_DC_OFF,
 	},
 	{
 		.name = "power well 2",
@@ -3262,7 +3353,7 @@  static const struct i915_power_well_desc cnl_power_wells[] = {
 		.name = "DC off",
 		.domains = CNL_DISPLAY_DC_OFF_POWER_DOMAINS,
 		.ops = &gen9_dc_off_power_well_ops,
-		.id = DISP_PW_ID_NONE,
+		.id = SKL_DISP_DC_OFF,
 	},
 	{
 		.name = "power well 2",
@@ -3390,7 +3481,7 @@  static const struct i915_power_well_desc icl_power_wells[] = {
 		.name = "DC off",
 		.domains = ICL_DISPLAY_DC_OFF_POWER_DOMAINS,
 		.ops = &gen9_dc_off_power_well_ops,
-		.id = DISP_PW_ID_NONE,
+		.id = SKL_DISP_DC_OFF,
 	},
 	{
 		.name = "power well 2",
@@ -3623,7 +3714,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 = SKL_DISP_DC_OFF,
 	},
 	{
 		.name = "power well 2",
@@ -4056,6 +4147,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);
 
+	allowed_dc_mask_to_target_dc_state(dev_priv);
 	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..13fc705799fd 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,
+	SKL_DISP_DC_OFF,
 };
 
 #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
@@ -256,6 +257,7 @@  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);
 
 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 fcf7423075ef..cddc98ea9965 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -338,6 +338,7 @@  struct intel_csr {
 	i915_reg_t mmioaddr[20];
 	u32 mmiodata[20];
 	u32 dc_state;
+	u32 target_dc_state;
 	u32 allowed_dc_mask;
 	intel_wakeref_t wakeref;
 };

Comments

On Thu, Sep 26, 2019 at 08:26:17PM +0530, Anshuman Gupta wrote:
> Add target_dc_state and tgl_set_target_dc_state() API
> in order to enable DC3CO state with existing DC states.
> target_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.
> v8: Uniform checks by using only target_dc_state instead of allowed_dc_mask
>     in "DC off" power well callback. [Imre]
>     Adding "DC off" power well id to older platforms. [Imre]
>     Removed psr2_deep_sleep flag from tgl_set_target_dc_state. [Imre]
> v9: Used switch case for target DC state in
>     gen9_dc_off_power_well_disable(), checking DC3CO state against
>     allowed DC mask, using WARN_ON() in
>     tgl_set_target_dc_state(). [Imre]
> 
> 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    | 110 ++++++++++++++++--
>  .../drm/i915/display/intel_display_power.h    |   2 +
>  drivers/gpu/drm/i915/i915_drv.h               |   1 +
>  3 files changed, 104 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 0b685c517bcb..9f787556f80d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -785,6 +785,38 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
>  	dev_priv->csr.dc_state = val & mask;
>  }
>  
> +static void
> +allowed_dc_mask_to_target_dc_state(struct drm_i915_private *dev_priv)
> +{
> +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> +		dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC6;
> +	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> +		dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC5;
> +	else
> +		dev_priv->csr.target_dc_state = DC_STATE_DISABLE;
> +}

The following can be used in tgl_set_target_dc_state() as well:

static u32
sanitize_target_dc_state(struct drm_i915_private *dev_priv,
                         u32 target_dc_state)
{
        u32 states[] = {
                DC_STATE_EN_UPTO_DC6,
                DC_STATE_EN_UPTO_DC5,
                DC_STATE_EN_DC3CO,
        };
        int i;

        for (i = 0; i < ARRAY_SIZE(states); i++)
                if (target_dc_state == states[i] &&
                    (dev_priv->csr.allowed_dc_mask & states[i]))
                        return states[i];

        return DC_STATE_DISABLE;
}

> +
> +static void tgl_enable_dc3co(struct drm_i915_private *dev_priv)
> +{
> +	DRM_DEBUG_KMS("Enabling DC3CO\n");
> +	gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO);
> +}
> +
> +static void tgl_disable_dc3co(struct drm_i915_private *dev_priv)
> +{
> +	u32 val;
> +
> +	DRM_DEBUG_KMS("Disabling DC3CO\n");
> +	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
> +	 */
> +	usleep_range(200, 210);
> +}
> +
>  static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
>  {
>  	assert_can_enable_dc9(dev_priv);
> @@ -952,7 +984,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)
> @@ -968,6 +1001,11 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_cdclk_state cdclk_state = {};
>  
> +	if (dev_priv->csr.target_dc_state == DC_STATE_EN_DC3CO) {
> +		tgl_disable_dc3co(dev_priv);
> +		return;
> +	}
> +
>  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>  
>  	dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> @@ -1000,10 +1038,63 @@ 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)
> +	switch (dev_priv->csr.target_dc_state) {
> +	case DC_STATE_EN_DC3CO:
> +		tgl_enable_dc3co(dev_priv);
> +		break;
> +	case DC_STATE_EN_UPTO_DC6:
>  		skl_enable_dc6(dev_priv);
> -	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> +		break;
> +	case DC_STATE_EN_UPTO_DC5:
>  		gen9_enable_dc5(dev_priv);
> +		break;
> +	}
> +}
> +
> +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state)
> +{
> +	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, SKL_DISP_DC_OFF);
> +
> +	if (WARN_ON(!power_well))
> +		goto unlock;
> +
> +	/*
> +	 *  Compute the adjusted state wrt to the permisisble allowed dc mask.
> +	 */
> +	if (state != DC_STATE_EN_DC3CO ||
> +	    !(dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)) {
> +		if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> +			state = DC_STATE_EN_UPTO_DC6;
> +		else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> +			state = DC_STATE_EN_UPTO_DC5;
> +		else
> +			state = DC_STATE_DISABLE;
> +	}

Instead of the above:
	state = sanitize_dc_target_state(dev_priv, state);

> +
> +	if (state == dev_priv->csr.target_dc_state)
> +		goto unlock;
> +
> +	dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv,
> +							   power_well);
> +	/*
> +	 * If DC off power well is disabled, need to enable and disable the
> +	 * DC off power well to effect target DC state.
> +	 */
> +	if (!dc_off_enabled)
> +		power_well->desc->ops->enable(dev_priv, power_well);
> +
> +	dev_priv->csr.target_dc_state = state;
> +
> +	if (!dc_off_enabled)
> +		power_well->desc->ops->disable(dev_priv, power_well);
> +
> +unlock:
> +	mutex_unlock(&power_domains->lock);
>  }
>  
>  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> @@ -2951,7 +3042,7 @@ static const struct i915_power_well_desc skl_power_wells[] = {
>  		.name = "DC off",
>  		.domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
>  		.ops = &gen9_dc_off_power_well_ops,
> -		.id = DISP_PW_ID_NONE,
> +		.id = SKL_DISP_DC_OFF,
>  	},
>  	{
>  		.name = "power well 2",
> @@ -3033,7 +3124,7 @@ static const struct i915_power_well_desc bxt_power_wells[] = {
>  		.name = "DC off",
>  		.domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
>  		.ops = &gen9_dc_off_power_well_ops,
> -		.id = DISP_PW_ID_NONE,
> +		.id = SKL_DISP_DC_OFF,
>  	},
>  	{
>  		.name = "power well 2",
> @@ -3093,7 +3184,7 @@ static const struct i915_power_well_desc glk_power_wells[] = {
>  		.name = "DC off",
>  		.domains = GLK_DISPLAY_DC_OFF_POWER_DOMAINS,
>  		.ops = &gen9_dc_off_power_well_ops,
> -		.id = DISP_PW_ID_NONE,
> +		.id = SKL_DISP_DC_OFF,
>  	},
>  	{
>  		.name = "power well 2",
> @@ -3262,7 +3353,7 @@ static const struct i915_power_well_desc cnl_power_wells[] = {
>  		.name = "DC off",
>  		.domains = CNL_DISPLAY_DC_OFF_POWER_DOMAINS,
>  		.ops = &gen9_dc_off_power_well_ops,
> -		.id = DISP_PW_ID_NONE,
> +		.id = SKL_DISP_DC_OFF,
>  	},
>  	{
>  		.name = "power well 2",
> @@ -3390,7 +3481,7 @@ static const struct i915_power_well_desc icl_power_wells[] = {
>  		.name = "DC off",
>  		.domains = ICL_DISPLAY_DC_OFF_POWER_DOMAINS,
>  		.ops = &gen9_dc_off_power_well_ops,
> -		.id = DISP_PW_ID_NONE,
> +		.id = SKL_DISP_DC_OFF,
>  	},
>  	{
>  		.name = "power well 2",
> @@ -3623,7 +3714,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 = SKL_DISP_DC_OFF,
>  	},
>  	{
>  		.name = "power well 2",
> @@ -4056,6 +4147,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);
>  
> +	allowed_dc_mask_to_target_dc_state(dev_priv);

and this would become:

	dev_priv->csr.target_dc_state =
		sanitize_dc_target_state(dev_priv, DC_STATE_EN_UPTO_DC6);

>  	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..13fc705799fd 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,
> +	SKL_DISP_DC_OFF,
>  };
>  
>  #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
> @@ -256,6 +257,7 @@ 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);
>  
>  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 fcf7423075ef..cddc98ea9965 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -338,6 +338,7 @@ struct intel_csr {
>  	i915_reg_t mmioaddr[20];
>  	u32 mmiodata[20];
>  	u32 dc_state;
> +	u32 target_dc_state;
>  	u32 allowed_dc_mask;
>  	intel_wakeref_t wakeref;
>  };
> -- 
> 2.21.0
>
On Fri, Sep 27, 2019 at 02:07:43PM +0300, Imre Deak wrote:
> On Thu, Sep 26, 2019 at 08:26:17PM +0530, Anshuman Gupta wrote:
> > Add target_dc_state and tgl_set_target_dc_state() API
> > in order to enable DC3CO state with existing DC states.
> > target_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.
> > v8: Uniform checks by using only target_dc_state instead of allowed_dc_mask
> >     in "DC off" power well callback. [Imre]
> >     Adding "DC off" power well id to older platforms. [Imre]
> >     Removed psr2_deep_sleep flag from tgl_set_target_dc_state. [Imre]
> > v9: Used switch case for target DC state in
> >     gen9_dc_off_power_well_disable(), checking DC3CO state against
> >     allowed DC mask, using WARN_ON() in
> >     tgl_set_target_dc_state(). [Imre]
> > 
> > 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    | 110 ++++++++++++++++--
> >  .../drm/i915/display/intel_display_power.h    |   2 +
> >  drivers/gpu/drm/i915/i915_drv.h               |   1 +
> >  3 files changed, 104 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 0b685c517bcb..9f787556f80d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -785,6 +785,38 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
> >  	dev_priv->csr.dc_state = val & mask;
> >  }
> >  
> > +static void
> > +allowed_dc_mask_to_target_dc_state(struct drm_i915_private *dev_priv)
> > +{
> > +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> > +		dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC6;
> > +	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> > +		dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC5;
> > +	else
> > +		dev_priv->csr.target_dc_state = DC_STATE_DISABLE;
> > +}
> 
> The following can be used in tgl_set_target_dc_state() as well:
> 
> static u32
> sanitize_target_dc_state(struct drm_i915_private *dev_priv,
>                          u32 target_dc_state)
> {
>         u32 states[] = {
>                 DC_STATE_EN_UPTO_DC6,
>                 DC_STATE_EN_UPTO_DC5,
>                 DC_STATE_EN_DC3CO,
>         };
>         int i;
> 
>         for (i = 0; i < ARRAY_SIZE(states); i++)
>                 if (target_dc_state == states[i] &&
>                     (dev_priv->csr.allowed_dc_mask & states[i]))
>                         return states[i];
> 
>         return DC_STATE_DISABLE;
> }
> 
> > +
> > +static void tgl_enable_dc3co(struct drm_i915_private *dev_priv)
> > +{
> > +	DRM_DEBUG_KMS("Enabling DC3CO\n");
> > +	gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO);
> > +}
> > +
> > +static void tgl_disable_dc3co(struct drm_i915_private *dev_priv)
> > +{
> > +	u32 val;
> > +
> > +	DRM_DEBUG_KMS("Disabling DC3CO\n");
> > +	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
> > +	 */
> > +	usleep_range(200, 210);
> > +}
> > +
> >  static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
> >  {
> >  	assert_can_enable_dc9(dev_priv);
> > @@ -952,7 +984,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)
> > @@ -968,6 +1001,11 @@ static void gen9_disable_dc_states(struct drm_i915_private *dev_priv)
> >  {
> >  	struct intel_cdclk_state cdclk_state = {};
> >  
> > +	if (dev_priv->csr.target_dc_state == DC_STATE_EN_DC3CO) {
> > +		tgl_disable_dc3co(dev_priv);
> > +		return;
> > +	}
> > +
> >  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> >  
> >  	dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> > @@ -1000,10 +1038,63 @@ 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)
> > +	switch (dev_priv->csr.target_dc_state) {
> > +	case DC_STATE_EN_DC3CO:
> > +		tgl_enable_dc3co(dev_priv);
> > +		break;
> > +	case DC_STATE_EN_UPTO_DC6:
> >  		skl_enable_dc6(dev_priv);
> > -	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> > +		break;
> > +	case DC_STATE_EN_UPTO_DC5:
> >  		gen9_enable_dc5(dev_priv);
> > +		break;
> > +	}
> > +}
> > +
> > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state)

We need a documentation for exported functions.

> > +{
> > +	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, SKL_DISP_DC_OFF);
> > +
> > +	if (WARN_ON(!power_well))
> > +		goto unlock;
> > +
> > +	/*
> > +	 *  Compute the adjusted state wrt to the permisisble allowed dc mask.
> > +	 */
> > +	if (state != DC_STATE_EN_DC3CO ||
> > +	    !(dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)) {
> > +		if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> > +			state = DC_STATE_EN_UPTO_DC6;
> > +		else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> > +			state = DC_STATE_EN_UPTO_DC5;
> > +		else
> > +			state = DC_STATE_DISABLE;
> > +	}
> 
> Instead of the above:
> 	state = sanitize_dc_target_state(dev_priv, state);
> 
> > +
> > +	if (state == dev_priv->csr.target_dc_state)
> > +		goto unlock;
> > +
> > +	dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv,
> > +							   power_well);
> > +	/*
> > +	 * If DC off power well is disabled, need to enable and disable the
> > +	 * DC off power well to effect target DC state.
> > +	 */
> > +	if (!dc_off_enabled)
> > +		power_well->desc->ops->enable(dev_priv, power_well);
> > +
> > +	dev_priv->csr.target_dc_state = state;
> > +
> > +	if (!dc_off_enabled)
> > +		power_well->desc->ops->disable(dev_priv, power_well);
> > +
> > +unlock:
> > +	mutex_unlock(&power_domains->lock);
> >  }
> >  
> >  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> > @@ -2951,7 +3042,7 @@ static const struct i915_power_well_desc skl_power_wells[] = {
> >  		.name = "DC off",
> >  		.domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
> >  		.ops = &gen9_dc_off_power_well_ops,
> > -		.id = DISP_PW_ID_NONE,
> > +		.id = SKL_DISP_DC_OFF,
> >  	},
> >  	{
> >  		.name = "power well 2",
> > @@ -3033,7 +3124,7 @@ static const struct i915_power_well_desc bxt_power_wells[] = {
> >  		.name = "DC off",
> >  		.domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
> >  		.ops = &gen9_dc_off_power_well_ops,
> > -		.id = DISP_PW_ID_NONE,
> > +		.id = SKL_DISP_DC_OFF,
> >  	},
> >  	{
> >  		.name = "power well 2",
> > @@ -3093,7 +3184,7 @@ static const struct i915_power_well_desc glk_power_wells[] = {
> >  		.name = "DC off",
> >  		.domains = GLK_DISPLAY_DC_OFF_POWER_DOMAINS,
> >  		.ops = &gen9_dc_off_power_well_ops,
> > -		.id = DISP_PW_ID_NONE,
> > +		.id = SKL_DISP_DC_OFF,
> >  	},
> >  	{
> >  		.name = "power well 2",
> > @@ -3262,7 +3353,7 @@ static const struct i915_power_well_desc cnl_power_wells[] = {
> >  		.name = "DC off",
> >  		.domains = CNL_DISPLAY_DC_OFF_POWER_DOMAINS,
> >  		.ops = &gen9_dc_off_power_well_ops,
> > -		.id = DISP_PW_ID_NONE,
> > +		.id = SKL_DISP_DC_OFF,
> >  	},
> >  	{
> >  		.name = "power well 2",
> > @@ -3390,7 +3481,7 @@ static const struct i915_power_well_desc icl_power_wells[] = {
> >  		.name = "DC off",
> >  		.domains = ICL_DISPLAY_DC_OFF_POWER_DOMAINS,
> >  		.ops = &gen9_dc_off_power_well_ops,
> > -		.id = DISP_PW_ID_NONE,
> > +		.id = SKL_DISP_DC_OFF,
> >  	},
> >  	{
> >  		.name = "power well 2",
> > @@ -3623,7 +3714,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 = SKL_DISP_DC_OFF,
> >  	},
> >  	{
> >  		.name = "power well 2",
> > @@ -4056,6 +4147,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);
> >  
> > +	allowed_dc_mask_to_target_dc_state(dev_priv);
> 
> and this would become:
> 
> 	dev_priv->csr.target_dc_state =
> 		sanitize_dc_target_state(dev_priv, DC_STATE_EN_UPTO_DC6);
> 
> >  	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..13fc705799fd 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,
> > +	SKL_DISP_DC_OFF,
> >  };
> >  
> >  #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
> > @@ -256,6 +257,7 @@ 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);
> >  
> >  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 fcf7423075ef..cddc98ea9965 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -338,6 +338,7 @@ struct intel_csr {
> >  	i915_reg_t mmioaddr[20];
> >  	u32 mmiodata[20];
> >  	u32 dc_state;
> > +	u32 target_dc_state;
> >  	u32 allowed_dc_mask;
> >  	intel_wakeref_t wakeref;
> >  };
> > -- 
> > 2.21.0
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 27 Sep 2019, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, Sep 27, 2019 at 02:07:43PM +0300, Imre Deak wrote:
>> On Thu, Sep 26, 2019 at 08:26:17PM +0530, Anshuman Gupta wrote:
>> > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state)
>
> We need a documentation for exported functions.

And really you should make an effort to *NOT* expose platform specific
functions from your C modules. Yes, we have some, but the direction
should be the opposite of adding more.

I'll be more strict about this going forward. We need to improve the
interfaces we have.

>> > @@ -256,6 +257,7 @@ 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);

This sticks out like a sore thumb.

And you're not even using the function outside of intel_display_power.h!

BR,
Jani.


>> >  
>> >  const char *
>> >  intel_display_power_domain_str(enum intel_display_power_domain domain);
On Fri, Sep 27, 2019 at 04:40:59PM +0300, Jani Nikula wrote:
> On Fri, 27 Sep 2019, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, Sep 27, 2019 at 02:07:43PM +0300, Imre Deak wrote:
> >> On Thu, Sep 26, 2019 at 08:26:17PM +0530, Anshuman Gupta wrote:
> >> > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state)
> >
> > We need a documentation for exported functions.
> 
> And really you should make an effort to *NOT* expose platform specific
> functions from your C modules. Yes, we have some, but the direction
> should be the opposite of adding more.
> 
> I'll be more strict about this going forward. We need to improve the
> interfaces we have.

Yes, the original idea was to add a new power well for the DC3co power
state, which would allow us to use the existing get/put interface. That
doesn't work too well though, because we can have only one of the states
enabled, so we went with having only one DC power well and a new API to
set its target DC state.

> 
> >> > @@ -256,6 +257,7 @@ 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);
> 
> This sticks out like a sore thumb.

Yep, so the rule is to have a uniform prefix for all exported functions,
so we could rename this to intel_display_power_set_dc_target(),

> And you're not even using the function outside of intel_display_power.h!

It's used, but the use is added only later. I agree that, as commented
elsewhere, new functions should be added in the same patch where they
are used. 

> BR,
> Jani.
> 
> 
> >> >  
> >> >  const char *
> >> >  intel_display_power_domain_str(enum intel_display_power_domain domain);
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center