diff mbox

[1/2] drm/i915/skl: Remove of the DC5 code

Message ID 1435596299-10058-1-git-send-email-damien.lespiau@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lespiau, Damien June 29, 2015, 4:44 p.m. UTC 
This code is all dead code since we want to go up to DC6, always.

Cc: A.Sunil Kamath <sunil.kamath@intel.com>
Cc: Suketu Shah <suketu.j.shah@intel.com>
Cc Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 99 +++++----------------------------
 1 file changed, 13 insertions(+), 86 deletions(-)

Comments

Imre Deak June 29, 2015, 4:54 p.m. UTC | #1 
On Mon, 2015-06-29 at 17:44 +0100, Damien Lespiau wrote:
> This code is all dead code since we want to go up to DC6, always.

On BXT DC6 is not available, so we can only go to DC5. It's disabled on
BXT atm, since runtime PM isn't enabled either.

> Cc: A.Sunil Kamath <sunil.kamath@intel.com>
> Cc: Suketu Shah <suketu.j.shah@intel.com>
> Cc Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 99 +++++----------------------------
>  1 file changed, 13 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index a472012..ae80ffa 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -49,7 +49,6 @@
>   * present for a given platform.
>   */
>  
> -#define GEN9_ENABLE_DC5(dev) 0
>  #define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev)
>  
>  #define for_each_power_well(i, power_well, domain_mask, power_domains)	\
> @@ -455,71 +454,6 @@ static void gen9_set_dc_state_debugmask_memory_up(
>  	}
>  }
>  
> -static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
> -{
> -	struct drm_device *dev = dev_priv->dev;
> -	bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv,
> -					SKL_DISP_PW_2);
> -
> -	WARN(!IS_SKYLAKE(dev), "Platform doesn't support DC5.\n");
> -	WARN(!HAS_RUNTIME_PM(dev), "Runtime PM not enabled.\n");
> -	WARN(pg2_enabled, "PG2 not disabled to enable DC5.\n");
> -
> -	WARN((I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5),
> -				"DC5 already programmed to be enabled.\n");
> -	WARN(dev_priv->pm.suspended,
> -		"DC5 cannot be enabled, if platform is runtime-suspended.\n");
> -
> -	assert_csr_loaded(dev_priv);
> -}
> -
> -static void assert_can_disable_dc5(struct drm_i915_private *dev_priv)
> -{
> -	bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv,
> -					SKL_DISP_PW_2);
> -	/*
> -	 * During initialization, the firmware may not be loaded yet.
> -	 * We still want to make sure that the DC enabling flag is cleared.
> -	 */
> -	if (dev_priv->power_domains.initializing)
> -		return;
> -
> -	WARN(!pg2_enabled, "PG2 not enabled to disable DC5.\n");
> -	WARN(dev_priv->pm.suspended,
> -		"Disabling of DC5 while platform is runtime-suspended should never happen.\n");
> -}
> -
> -static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
> -{
> -	uint32_t val;
> -
> -	assert_can_enable_dc5(dev_priv);
> -
> -	DRM_DEBUG_KMS("Enabling DC5\n");
> -
> -	gen9_set_dc_state_debugmask_memory_up(dev_priv);
> -
> -	val = I915_READ(DC_STATE_EN);
> -	val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
> -	val |= DC_STATE_EN_UPTO_DC5;
> -	I915_WRITE(DC_STATE_EN, val);
> -	POSTING_READ(DC_STATE_EN);
> -}
> -
> -static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
> -{
> -	uint32_t val;
> -
> -	assert_can_disable_dc5(dev_priv);
> -
> -	DRM_DEBUG_KMS("Disabling DC5\n");
> -
> -	val = I915_READ(DC_STATE_EN);
> -	val &= ~DC_STATE_EN_UPTO_DC5;
> -	I915_WRITE(DC_STATE_EN, val);
> -	POSTING_READ(DC_STATE_EN);
> -}
> -
>  static void assert_can_enable_dc6(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
> @@ -626,20 +560,16 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  				!I915_READ(HSW_PWR_WELL_BIOS),
>  				"Invalid for power well status to be enabled, unless done by the BIOS, \
>  				when request is to disable!\n");
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> -				if (SKL_ENABLE_DC6(dev)) {
> -					skl_disable_dc6(dev_priv);
> -					/*
> -					 * DDI buffer programming unnecessary during driver-load/resume
> -					 * as it's already done during modeset initialization then.
> -					 * It's also invalid here as encoder list is still uninitialized.
> -					 */
> -					if (!dev_priv->power_domains.initializing)
> -						intel_prepare_ddi(dev);
> -				} else {
> -					gen9_disable_dc5(dev_priv);
> -				}
> +			if (SKL_ENABLE_DC6(dev) &&
> +			    power_well->data == SKL_DISP_PW_2) {
> +				skl_disable_dc6(dev_priv);
> +				/*
> +				 * DDI buffer programming unnecessary during driver-load/resume
> +				 * as it's already done during modeset initialization then.
> +				 * It's also invalid here as encoder list is still uninitialized.
> +				 */
> +				if (!dev_priv->power_domains.initializing)
> +					intel_prepare_ddi(dev);
>  			}
>  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
>  		}
> @@ -658,8 +588,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  			POSTING_READ(HSW_PWR_WELL_DRIVER);
>  			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>  
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> +			if (SKL_ENABLE_DC6(dev) &&
> +			    power_well->data == SKL_DISP_PW_2) {
>  				enum csr_state state;
>  				/* TODO: wait for a completion event or
>  				 * similar here instead of busy
> @@ -671,10 +601,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  					DRM_ERROR("CSR firmware not ready (%d)\n",
>  							state);
>  				else
> -					if (SKL_ENABLE_DC6(dev))
> -						skl_enable_dc6(dev_priv);
> -					else
> -						gen9_enable_dc5(dev_priv);
> +					skl_enable_dc6(dev_priv);
>  			}
>  		}
>  	}
Lespiau, Damien June 29, 2015, 4:59 p.m. UTC | #2 
On Mon, Jun 29, 2015 at 07:54:42PM +0300, Imre Deak wrote:
> On Mon, 2015-06-29 at 17:44 +0100, Damien Lespiau wrote:
> > This code is all dead code since we want to go up to DC6, always.
> 
> On BXT DC6 is not available, so we can only go to DC5. It's disabled on
> BXT atm, since runtime PM isn't enabled either.

I knew there was a catch!

That code is pretty convoluted btw, it would make sense to have proper
pre_disable/post_enabled hooks on power wells?
Imre Deak June 29, 2015, 5:08 p.m. UTC | #3 
On Mon, 2015-06-29 at 17:59 +0100, Damien Lespiau wrote:
> On Mon, Jun 29, 2015 at 07:54:42PM +0300, Imre Deak wrote:
> > On Mon, 2015-06-29 at 17:44 +0100, Damien Lespiau wrote:
> > > This code is all dead code since we want to go up to DC6, always.
> > 
> > On BXT DC6 is not available, so we can only go to DC5. It's disabled on
> > BXT atm, since runtime PM isn't enabled either.
> 
> I knew there was a catch!
> 
> That code is pretty convoluted btw, it would make sense to have proper
> pre_disable/post_enabled hooks on power wells?

Yes, it would make sense to simplify the code. I haven't thought about
adding new hooks, not sure how useful they would be on other platforms.
But to me it seems that the enable/disable parts in skl_set_power_well()
could be split out to the existing skl_power_well_{enable,disable}, have
you looked into that already?

--Imre
Kamath, Sunil June 29, 2015, 5:15 p.m. UTC | #4 
But DC5 needed for BXT

- Sunil

>-----Original Message-----
>From: Lespiau, Damien 
>Sent: Monday, June 29, 2015 10:15 PM
>To: intel-gfx@lists.freedesktop.org
>Cc: Kamath, Sunil; Shah, Suketu J
>Subject: [PATCH 1/2] drm/i915/skl: Remove of the DC5 code
>
>This code is all dead code since we want to go up to DC6, always.
>
>Cc: A.Sunil Kamath <sunil.kamath@intel.com>
>Cc: Suketu Shah <suketu.j.shah@intel.com> Cc Animesh Manna <animesh.manna@intel.com>
>Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>
>Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
>---
Lespiau, Damien June 29, 2015, 5:19 p.m. UTC | #5 
On Mon, Jun 29, 2015 at 08:08:59PM +0300, Imre Deak wrote:
> On Mon, 2015-06-29 at 17:59 +0100, Damien Lespiau wrote:
> > On Mon, Jun 29, 2015 at 07:54:42PM +0300, Imre Deak wrote:
> > > On Mon, 2015-06-29 at 17:44 +0100, Damien Lespiau wrote:
> > > > This code is all dead code since we want to go up to DC6, always.
> > > 
> > > On BXT DC6 is not available, so we can only go to DC5. It's disabled on
> > > BXT atm, since runtime PM isn't enabled either.
> > 
> > I knew there was a catch!
> > 
> > That code is pretty convoluted btw, it would make sense to have proper
> > pre_disable/post_enabled hooks on power wells?
> 
> Yes, it would make sense to simplify the code. I haven't thought about
> adding new hooks, not sure how useful they would be on other platforms.
> But to me it seems that the enable/disable parts in skl_set_power_well()
> could be split out to the existing skl_power_well_{enable,disable}, have
> you looked into that already?

Nop, I haven't, but that's indeed a nice middle ground.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index a472012..ae80ffa 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -49,7 +49,6 @@ 
  * present for a given platform.
  */
 
-#define GEN9_ENABLE_DC5(dev) 0
 #define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev)
 
 #define for_each_power_well(i, power_well, domain_mask, power_domains)	\
@@ -455,71 +454,6 @@  static void gen9_set_dc_state_debugmask_memory_up(
 	}
 }
 
-static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
-{
-	struct drm_device *dev = dev_priv->dev;
-	bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv,
-					SKL_DISP_PW_2);
-
-	WARN(!IS_SKYLAKE(dev), "Platform doesn't support DC5.\n");
-	WARN(!HAS_RUNTIME_PM(dev), "Runtime PM not enabled.\n");
-	WARN(pg2_enabled, "PG2 not disabled to enable DC5.\n");
-
-	WARN((I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5),
-				"DC5 already programmed to be enabled.\n");
-	WARN(dev_priv->pm.suspended,
-		"DC5 cannot be enabled, if platform is runtime-suspended.\n");
-
-	assert_csr_loaded(dev_priv);
-}
-
-static void assert_can_disable_dc5(struct drm_i915_private *dev_priv)
-{
-	bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv,
-					SKL_DISP_PW_2);
-	/*
-	 * During initialization, the firmware may not be loaded yet.
-	 * We still want to make sure that the DC enabling flag is cleared.
-	 */
-	if (dev_priv->power_domains.initializing)
-		return;
-
-	WARN(!pg2_enabled, "PG2 not enabled to disable DC5.\n");
-	WARN(dev_priv->pm.suspended,
-		"Disabling of DC5 while platform is runtime-suspended should never happen.\n");
-}
-
-static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
-{
-	uint32_t val;
-
-	assert_can_enable_dc5(dev_priv);
-
-	DRM_DEBUG_KMS("Enabling DC5\n");
-
-	gen9_set_dc_state_debugmask_memory_up(dev_priv);
-
-	val = I915_READ(DC_STATE_EN);
-	val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
-	val |= DC_STATE_EN_UPTO_DC5;
-	I915_WRITE(DC_STATE_EN, val);
-	POSTING_READ(DC_STATE_EN);
-}
-
-static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
-{
-	uint32_t val;
-
-	assert_can_disable_dc5(dev_priv);
-
-	DRM_DEBUG_KMS("Disabling DC5\n");
-
-	val = I915_READ(DC_STATE_EN);
-	val &= ~DC_STATE_EN_UPTO_DC5;
-	I915_WRITE(DC_STATE_EN, val);
-	POSTING_READ(DC_STATE_EN);
-}
-
 static void assert_can_enable_dc6(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -626,20 +560,16 @@  static void skl_set_power_well(struct drm_i915_private *dev_priv,
 				!I915_READ(HSW_PWR_WELL_BIOS),
 				"Invalid for power well status to be enabled, unless done by the BIOS, \
 				when request is to disable!\n");
-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
-				power_well->data == SKL_DISP_PW_2) {
-				if (SKL_ENABLE_DC6(dev)) {
-					skl_disable_dc6(dev_priv);
-					/*
-					 * DDI buffer programming unnecessary during driver-load/resume
-					 * as it's already done during modeset initialization then.
-					 * It's also invalid here as encoder list is still uninitialized.
-					 */
-					if (!dev_priv->power_domains.initializing)
-						intel_prepare_ddi(dev);
-				} else {
-					gen9_disable_dc5(dev_priv);
-				}
+			if (SKL_ENABLE_DC6(dev) &&
+			    power_well->data == SKL_DISP_PW_2) {
+				skl_disable_dc6(dev_priv);
+				/*
+				 * DDI buffer programming unnecessary during driver-load/resume
+				 * as it's already done during modeset initialization then.
+				 * It's also invalid here as encoder list is still uninitialized.
+				 */
+				if (!dev_priv->power_domains.initializing)
+					intel_prepare_ddi(dev);
 			}
 			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
 		}
@@ -658,8 +588,8 @@  static void skl_set_power_well(struct drm_i915_private *dev_priv,
 			POSTING_READ(HSW_PWR_WELL_DRIVER);
 			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
 
-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
-				power_well->data == SKL_DISP_PW_2) {
+			if (SKL_ENABLE_DC6(dev) &&
+			    power_well->data == SKL_DISP_PW_2) {
 				enum csr_state state;
 				/* TODO: wait for a completion event or
 				 * similar here instead of busy
@@ -671,10 +601,7 @@  static void skl_set_power_well(struct drm_i915_private *dev_priv,
 					DRM_ERROR("CSR firmware not ready (%d)\n",
 							state);
 				else
-					if (SKL_ENABLE_DC6(dev))
-						skl_enable_dc6(dev_priv);
-					else
-						gen9_enable_dc5(dev_priv);
+					skl_enable_dc6(dev_priv);
 			}
 		}
 	}