[PATCH 0/4] drm/915: Sanitize DC state handling

[PATCH 0/4] drm/915: Sanitize DC state handling

[Imre Deak]

This patchset simplifies the way how we select the target DC state based
on the platform and module options. It also fixes one ordering problem I
noticed while going through the code, see the first patch.

Imre Deak (4):
  drm/i915/skl: Fix power domain suspend sequence
  drm/i915/gen9: Sanitize handling of allowed DC states
  drm/i915/gen9: Disable DC states if power well support is disabled
  drm/i915/gen9: Remove state asserts when disabling DC states

 drivers/gpu/drm/i915/i915_drv.h         |   1 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 122 +++++++++++++++-----------------
 2 files changed, 60 insertions(+), 63 deletions(-)

-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915/skl: Fix power domain suspend sequence

[Imre Deak]

During system suspend we need to first disable power wells then
unitialize the display core. In case power well support is disabled we
did this in the wrong order, so fix this up.

Fixes: d314cd43 ("drm/i915: fix handling of the disable_power_well module option")
CC: stable@vger.kernel.org
CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index e232976..8276dc2 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2319,15 +2319,15 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
  */
 void intel_power_domains_suspend(struct drm_i915_private *dev_priv)
 {
-	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
-		skl_display_core_uninit(dev_priv);
-
 	/*
 	 * Even if power well support was disabled we still want to disable
 	 * power wells while we are system suspended.
 	 */
 	if (!i915.disable_power_well)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
+	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
+		skl_display_core_uninit(dev_priv);
 }
 
 /**
-- 
2.5.0

[PATCH 1/4] drm/i915/skl: Fix power domain suspend sequence

[Imre Deak]

During system suspend we need to first disable power wells then
unitialize the display core. In case power well support is disabled we
did this in the wrong order, so fix this up.

Fixes: d314cd43 ("drm/i915: fix handling of the disable_power_well module option")
CC: stable@vger.kernel.org
CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index e232976..8276dc2 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2319,15 +2319,15 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
  */
 void intel_power_domains_suspend(struct drm_i915_private *dev_priv)
 {
-	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
-		skl_display_core_uninit(dev_priv);
-
 	/*
 	 * Even if power well support was disabled we still want to disable
 	 * power wells while we are system suspended.
 	 */
 	if (!i915.disable_power_well)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
+	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
+		skl_display_core_uninit(dev_priv);
 }
 
 /**
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915/gen9: Sanitize handling of allowed DC states

[Imre Deak]

We can simplify the conditions selecting the target DC state during
runtime by calculating the allowed DC states in advance during driver
loading. This also makes it easier to disable DC states depending on the
i915.disable_power_well module option, added in the next patch.

CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 74 +++++++++++++++++++++++----------
 2 files changed, 54 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9e76bfc..b563de5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -750,6 +750,7 @@ struct intel_csr {
 	i915_reg_t mmioaddr[8];
 	uint32_t mmiodata[8];
 	uint32_t dc_state;
+	uint32_t allowed_dc_mask;
 };
 
 #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 8276dc2..88df99e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -538,12 +538,8 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
 	else
 		mask |= DC_STATE_EN_UPTO_DC6;
 
-	WARN_ON_ONCE(state & ~mask);
-
-	if (i915.enable_dc == 0)
-		state = DC_STATE_DISABLE;
-	else if (i915.enable_dc == 1 && state > DC_STATE_EN_UPTO_DC5)
-		state = DC_STATE_EN_UPTO_DC5;
+	if (WARN_ON_ONCE(state & ~dev_priv->csr.allowed_dc_mask))
+		state &= dev_priv->csr.allowed_dc_mask;
 
 	val = I915_READ(DC_STATE_EN);
 	DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n",
@@ -659,8 +655,7 @@ static void gen9_disable_dc5_dc6(struct drm_i915_private *dev_priv)
 {
 	assert_can_disable_dc5(dev_priv);
 
-	if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
-	    i915.enable_dc != 0 && i915.enable_dc != 1)
+	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
 		assert_can_disable_dc6(dev_priv);
 
 	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
@@ -839,26 +834,19 @@ static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
 static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
 					   struct i915_power_well *power_well)
 {
-	if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
-	    i915.enable_dc != 0 && i915.enable_dc != 1)
+	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
 		skl_enable_dc6(dev_priv);
-	else
+	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
 		gen9_enable_dc5(dev_priv);
 }
 
 static void gen9_dc_off_power_well_sync_hw(struct drm_i915_private *dev_priv,
 					   struct i915_power_well *power_well)
 {
-	if (power_well->count > 0) {
-		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
-	} else {
-		if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
-		    i915.enable_dc != 0 &&
-		    i915.enable_dc != 1)
-			gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
-		else
-			gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
-	}
+	if (power_well->count > 0)
+		gen9_dc_off_power_well_enable(dev_priv, power_well);
+	else
+		gen9_dc_off_power_well_disable(dev_priv, power_well);
 }
 
 static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
@@ -2023,6 +2011,48 @@ sanitize_disable_power_well_option(const struct drm_i915_private *dev_priv,
 	return 1;
 }
 
+static uint32_t get_allowed_dc_mask(const struct drm_i915_private *dev_priv,
+				    int enable_dc)
+{
+	uint32_t mask = 0;
+
+	/*
+	 * DC9 has a separate HW flow from the rest of the DC states, not
+	 * depending on the DMC firmware. It's needed by system
+	 * suspend/resume, so allow it unconditionally.
+	 */
+	if (IS_BROXTON(dev_priv))
+		mask |= DC_STATE_EN_DC9;
+
+	if (!enable_dc)
+		return mask;
+
+	if (!HAS_CSR(dev_priv))
+		return mask;
+
+	mask |= DC_STATE_EN_UPTO_DC5;
+	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
+		mask |= DC_STATE_EN_UPTO_DC6;
+	} else if (!IS_BROXTON(dev_priv)) {
+		MISSING_CASE(INTEL_DEVID(dev_priv));
+		mask = 0;
+	}
+
+	switch (enable_dc) {
+	case 1:
+		mask &= ~DC_STATE_EN_UPTO_DC6;
+		break;
+	case 2:
+	case -1:
+		break;
+	default:
+		DRM_ERROR("Unexpected value for enable_dc (%d)\n", enable_dc);
+		break;
+	}
+
+	return mask;
+}
+
 #define set_power_wells(power_domains, __power_wells) ({		\
 	(power_domains)->power_wells = (__power_wells);			\
 	(power_domains)->power_well_count = ARRAY_SIZE(__power_wells);	\
@@ -2041,6 +2071,8 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
 
 	i915.disable_power_well = sanitize_disable_power_well_option(dev_priv,
 						     i915.disable_power_well);
+	dev_priv->csr.allowed_dc_mask = get_allowed_dc_mask(dev_priv,
+							    i915.enable_dc);
 
 	BUILD_BUG_ON(POWER_DOMAIN_NUM > 31);
 
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915/gen9: Disable DC states if power well support is disabled

[Imre Deak]

If power well support is disabled via the i915.disable_power_well module
option we should never enable DC states. Currently we would enable DC
states even in this case during system suspend, where we need to disable
all power wells regardless of the disable_power_well option.

CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 88df99e..7f65d5f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2030,6 +2030,9 @@ static uint32_t get_allowed_dc_mask(const struct drm_i915_private *dev_priv,
 	if (!HAS_CSR(dev_priv))
 		return mask;
 
+	if (!i915.disable_power_well)
+		return mask;
+
 	mask |= DC_STATE_EN_UPTO_DC5;
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
 		mask |= DC_STATE_EN_UPTO_DC6;
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915/gen9: Remove state asserts when disabling DC states

[Imre Deak]

Disabling the DC states when it's already disabled is a valid scenario,
for example during HW state sanitization during driver loading and
resuming or when DC states are disabled via the i915.enable_dc or
disable_power_well option.

CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 41 +--------------------------------
 1 file changed, 1 insertion(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 7f65d5f..1661c2a 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -458,8 +458,6 @@ static void assert_can_enable_dc9(struct drm_i915_private *dev_priv)
 static void assert_can_disable_dc9(struct drm_i915_private *dev_priv)
 {
 	WARN(intel_irqs_enabled(dev_priv), "Interrupts not disabled yet.\n");
-	WARN(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_DC9),
-		"DC9 already programmed to be disabled.\n");
 	WARN(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5,
 		"DC5 still not disabled.\n");
 
@@ -602,18 +600,6 @@ static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
 	assert_csr_loaded(dev_priv);
 }
 
-static void assert_can_disable_dc5(struct drm_i915_private *dev_priv)
-{
-	/*
-	 * 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;
-
-	assert_rpm_wakelock_held(dev_priv);
-}
-
 static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
 {
 	assert_can_enable_dc5(dev_priv);
@@ -638,29 +624,6 @@ static void assert_can_enable_dc6(struct drm_i915_private *dev_priv)
 	assert_csr_loaded(dev_priv);
 }
 
-static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
-{
-	/*
-	 * 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_ONCE(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6),
-		  "DC6 already programmed to be disabled.\n");
-}
-
-static void gen9_disable_dc5_dc6(struct drm_i915_private *dev_priv)
-{
-	assert_can_disable_dc5(dev_priv);
-
-	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
-		assert_can_disable_dc6(dev_priv);
-
-	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
-}
-
 void skl_enable_dc6(struct drm_i915_private *dev_priv)
 {
 	assert_can_enable_dc6(dev_priv);
@@ -673,8 +636,6 @@ void skl_enable_dc6(struct drm_i915_private *dev_priv)
 
 void skl_disable_dc6(struct drm_i915_private *dev_priv)
 {
-	assert_can_disable_dc6(dev_priv);
-
 	DRM_DEBUG_KMS("Disabling DC6\n");
 
 	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
@@ -828,7 +789,7 @@ static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv,
 static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
 					  struct i915_power_well *power_well)
 {
-	gen9_disable_dc5_dc6(dev_priv);
+	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 }
 
 static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915/skl: Fix power domain suspend sequence

[Patrik Jakobsson]

On Wed, Feb 24, 2016 at 07:57:43PM +0200, Imre Deak wrote:
> During system suspend we need to first disable power wells then
> unitialize the display core. In case power well support is disabled we
> did this in the wrong order, so fix this up.
> 
> Fixes: d314cd43 ("drm/i915: fix handling of the disable_power_well module option")
> CC: stable@vger.kernel.org
> CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index e232976..8276dc2 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2319,15 +2319,15 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
>   */
>  void intel_power_domains_suspend(struct drm_i915_private *dev_priv)
>  {
> -	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
> -		skl_display_core_uninit(dev_priv);
> -
>  	/*
>  	 * Even if power well support was disabled we still want to disable
>  	 * power wells while we are system suspended.
>  	 */
>  	if (!i915.disable_power_well)
>  		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> +
> +	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
> +		skl_display_core_uninit(dev_priv);
>  }
>  
>  /**
> -- 
> 2.5.0
> 

-- 
---
Intel Sweden AB Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027 

Re: [PATCH 2/4] drm/i915/gen9: Sanitize handling of allowed DC states

[Patrik Jakobsson]

On Wed, Feb 24, 2016 at 07:57:44PM +0200, Imre Deak wrote:
> We can simplify the conditions selecting the target DC state during
> runtime by calculating the allowed DC states in advance during driver
> loading. This also makes it easier to disable DC states depending on the
> i915.disable_power_well module option, added in the next patch.
> 
> CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 74 +++++++++++++++++++++++----------
>  2 files changed, 54 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9e76bfc..b563de5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -750,6 +750,7 @@ struct intel_csr {
>  	i915_reg_t mmioaddr[8];
>  	uint32_t mmiodata[8];
>  	uint32_t dc_state;
> +	uint32_t allowed_dc_mask;
>  };
>  
>  #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 8276dc2..88df99e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -538,12 +538,8 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
>  	else
>  		mask |= DC_STATE_EN_UPTO_DC6;
>  
> -	WARN_ON_ONCE(state & ~mask);
> -
> -	if (i915.enable_dc == 0)
> -		state = DC_STATE_DISABLE;
> -	else if (i915.enable_dc == 1 && state > DC_STATE_EN_UPTO_DC5)
> -		state = DC_STATE_EN_UPTO_DC5;
> +	if (WARN_ON_ONCE(state & ~dev_priv->csr.allowed_dc_mask))
> +		state &= dev_priv->csr.allowed_dc_mask;
>  
>  	val = I915_READ(DC_STATE_EN);
>  	DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n",
> @@ -659,8 +655,7 @@ static void gen9_disable_dc5_dc6(struct drm_i915_private *dev_priv)
>  {
>  	assert_can_disable_dc5(dev_priv);
>  
> -	if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> -	    i915.enable_dc != 0 && i915.enable_dc != 1)
> +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
>  		assert_can_disable_dc6(dev_priv);
>  
>  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> @@ -839,26 +834,19 @@ static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
>  static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
>  					   struct i915_power_well *power_well)
>  {
> -	if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> -	    i915.enable_dc != 0 && i915.enable_dc != 1)
> +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
>  		skl_enable_dc6(dev_priv);
> -	else
> +	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
>  		gen9_enable_dc5(dev_priv);
>  }
>  
>  static void gen9_dc_off_power_well_sync_hw(struct drm_i915_private *dev_priv,
>  					   struct i915_power_well *power_well)
>  {
> -	if (power_well->count > 0) {
> -		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> -	} else {
> -		if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> -		    i915.enable_dc != 0 &&
> -		    i915.enable_dc != 1)
> -			gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
> -		else
> -			gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
> -	}
> +	if (power_well->count > 0)
> +		gen9_dc_off_power_well_enable(dev_priv, power_well);
> +	else
> +		gen9_dc_off_power_well_disable(dev_priv, power_well);
>  }
>  
>  static void i9xx_always_on_power_well_noop(struct drm_i915_private *dev_priv,
> @@ -2023,6 +2011,48 @@ sanitize_disable_power_well_option(const struct drm_i915_private *dev_priv,
>  	return 1;
>  }
>  
> +static uint32_t get_allowed_dc_mask(const struct drm_i915_private *dev_priv,
> +				    int enable_dc)
> +{
> +	uint32_t mask = 0;
> +
> +	/*
> +	 * DC9 has a separate HW flow from the rest of the DC states, not
> +	 * depending on the DMC firmware. It's needed by system
> +	 * suspend/resume, so allow it unconditionally.
> +	 */
> +	if (IS_BROXTON(dev_priv))
> +		mask |= DC_STATE_EN_DC9;
> +
> +	if (!enable_dc)
> +		return mask;
> +
> +	if (!HAS_CSR(dev_priv))
> +		return mask;
> +
> +	mask |= DC_STATE_EN_UPTO_DC5;
> +	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> +		mask |= DC_STATE_EN_UPTO_DC6;
> +	} else if (!IS_BROXTON(dev_priv)) {
> +		MISSING_CASE(INTEL_DEVID(dev_priv));
> +		mask = 0;
> +	}
> +
> +	switch (enable_dc) {
> +	case 1:
> +		mask &= ~DC_STATE_EN_UPTO_DC6;
> +		break;
> +	case 2:

Should we make some noise here on BXT? Just a thought, not sure it's
useful.

> +	case -1:
> +		break;
> +	default:
> +		DRM_ERROR("Unexpected value for enable_dc (%d)\n", enable_dc);
> +		break;
> +	}
> +
> +	return mask;
> +}
> +
>  #define set_power_wells(power_domains, __power_wells) ({		\
>  	(power_domains)->power_wells = (__power_wells);			\
>  	(power_domains)->power_well_count = ARRAY_SIZE(__power_wells);	\
> @@ -2041,6 +2071,8 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>  
>  	i915.disable_power_well = sanitize_disable_power_well_option(dev_priv,
>  						     i915.disable_power_well);
> +	dev_priv->csr.allowed_dc_mask = get_allowed_dc_mask(dev_priv,
> +							    i915.enable_dc);
>  
>  	BUILD_BUG_ON(POWER_DOMAIN_NUM > 31);
>  
> -- 
> 2.5.0
> 

-- 
---
Intel Sweden AB Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/gen9: Disable DC states if power well support is disabled

[Patrik Jakobsson]

On Wed, Feb 24, 2016 at 07:57:45PM +0200, Imre Deak wrote:
> If power well support is disabled via the i915.disable_power_well module
> option we should never enable DC states. Currently we would enable DC
> states even in this case during system suspend, where we need to disable
> all power wells regardless of the disable_power_well option.
> 
> CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 88df99e..7f65d5f 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2030,6 +2030,9 @@ static uint32_t get_allowed_dc_mask(const struct drm_i915_private *dev_priv,
>  	if (!HAS_CSR(dev_priv))
>  		return mask;
>  
> +	if (!i915.disable_power_well)
> +		return mask;
> +
>  	mask |= DC_STATE_EN_UPTO_DC5;
>  	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
>  		mask |= DC_STATE_EN_UPTO_DC6;
> -- 
> 2.5.0
> 

-- 
---
Intel Sweden AB Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/gen9: Remove state asserts when disabling DC states

[Patrik Jakobsson]

On Wed, Feb 24, 2016 at 07:57:46PM +0200, Imre Deak wrote:
> Disabling the DC states when it's already disabled is a valid scenario,
> for example during HW state sanitization during driver loading and
> resuming or when DC states are disabled via the i915.enable_dc or
> disable_power_well option.
> 
> CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Nice to see these go.

Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 41 +--------------------------------
>  1 file changed, 1 insertion(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 7f65d5f..1661c2a 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -458,8 +458,6 @@ static void assert_can_enable_dc9(struct drm_i915_private *dev_priv)
>  static void assert_can_disable_dc9(struct drm_i915_private *dev_priv)
>  {
>  	WARN(intel_irqs_enabled(dev_priv), "Interrupts not disabled yet.\n");
> -	WARN(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_DC9),
> -		"DC9 already programmed to be disabled.\n");
>  	WARN(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5,
>  		"DC5 still not disabled.\n");
>  
> @@ -602,18 +600,6 @@ static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
>  	assert_csr_loaded(dev_priv);
>  }
>  
> -static void assert_can_disable_dc5(struct drm_i915_private *dev_priv)
> -{
> -	/*
> -	 * 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;
> -
> -	assert_rpm_wakelock_held(dev_priv);
> -}
> -
>  static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
>  {
>  	assert_can_enable_dc5(dev_priv);
> @@ -638,29 +624,6 @@ static void assert_can_enable_dc6(struct drm_i915_private *dev_priv)
>  	assert_csr_loaded(dev_priv);
>  }
>  
> -static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> -{
> -	/*
> -	 * 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_ONCE(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6),
> -		  "DC6 already programmed to be disabled.\n");
> -}
> -
> -static void gen9_disable_dc5_dc6(struct drm_i915_private *dev_priv)
> -{
> -	assert_can_disable_dc5(dev_priv);
> -
> -	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> -		assert_can_disable_dc6(dev_priv);
> -
> -	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> -}
> -
>  void skl_enable_dc6(struct drm_i915_private *dev_priv)
>  {
>  	assert_can_enable_dc6(dev_priv);
> @@ -673,8 +636,6 @@ void skl_enable_dc6(struct drm_i915_private *dev_priv)
>  
>  void skl_disable_dc6(struct drm_i915_private *dev_priv)
>  {
> -	assert_can_disable_dc6(dev_priv);
> -
>  	DRM_DEBUG_KMS("Disabling DC6\n");
>  
>  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> @@ -828,7 +789,7 @@ static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv,
>  static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
>  					  struct i915_power_well *power_well)
>  {
> -	gen9_disable_dc5_dc6(dev_priv);
> +	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>  }
>  
>  static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
> -- 
> 2.5.0
> 

-- 
---
Intel Sweden AB Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915/gen9: Sanitize handling of allowed DC states

[Imre Deak]

On ma, 2016-02-29 at 14:02 +0100, Patrik Jakobsson wrote:
> On Wed, Feb 24, 2016 at 07:57:44PM +0200, Imre Deak wrote:
> > We can simplify the conditions selecting the target DC state during
> > runtime by calculating the allowed DC states in advance during
> > driver
> > loading. This also makes it easier to disable DC states depending
> > on the
> > i915.disable_power_well module option, added in the next patch.
> > 
> > CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |  1 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 74
> > +++++++++++++++++++++++----------
> >  2 files changed, 54 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 9e76bfc..b563de5 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -750,6 +750,7 @@ struct intel_csr {
> >  	i915_reg_t mmioaddr[8];
> >  	uint32_t mmiodata[8];
> >  	uint32_t dc_state;
> > +	uint32_t allowed_dc_mask;
> >  };
> >  
> >  #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 8276dc2..88df99e 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -538,12 +538,8 @@ static void gen9_set_dc_state(struct
> > drm_i915_private *dev_priv, uint32_t state)
> >  	else
> >  		mask |= DC_STATE_EN_UPTO_DC6;
> >  
> > -	WARN_ON_ONCE(state & ~mask);
> > -
> > -	if (i915.enable_dc == 0)
> > -		state = DC_STATE_DISABLE;
> > -	else if (i915.enable_dc == 1 && state >
> > DC_STATE_EN_UPTO_DC5)
> > -		state = DC_STATE_EN_UPTO_DC5;
> > +	if (WARN_ON_ONCE(state & ~dev_priv->csr.allowed_dc_mask))
> > +		state &= dev_priv->csr.allowed_dc_mask;
> >  
> >  	val = I915_READ(DC_STATE_EN);
> >  	DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n",
> > @@ -659,8 +655,7 @@ static void gen9_disable_dc5_dc6(struct
> > drm_i915_private *dev_priv)
> >  {
> >  	assert_can_disable_dc5(dev_priv);
> >  
> > -	if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> > -	    i915.enable_dc != 0 && i915.enable_dc != 1)
> > +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> >  		assert_can_disable_dc6(dev_priv);
> >  
> >  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > @@ -839,26 +834,19 @@ static void
> > gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
> >  static void gen9_dc_off_power_well_disable(struct drm_i915_private
> > *dev_priv,
> >  					   struct i915_power_well
> > *power_well)
> >  {
> > -	if ((IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) &&
> > -	    i915.enable_dc != 0 && i915.enable_dc != 1)
> > +	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> >  		skl_enable_dc6(dev_priv);
> > -	else
> > +	else if (dev_priv->csr.allowed_dc_mask &
> > DC_STATE_EN_UPTO_DC5)
> >  		gen9_enable_dc5(dev_priv);
> >  }
> >  
> >  static void gen9_dc_off_power_well_sync_hw(struct drm_i915_private
> > *dev_priv,
> >  					   struct i915_power_well
> > *power_well)
> >  {
> > -	if (power_well->count > 0) {
> > -		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > -	} else {
> > -		if ((IS_SKYLAKE(dev_priv) ||
> > IS_KABYLAKE(dev_priv)) &&
> > -		    i915.enable_dc != 0 &&
> > -		    i915.enable_dc != 1)
> > -			gen9_set_dc_state(dev_priv,
> > DC_STATE_EN_UPTO_DC6);
> > -		else
> > -			gen9_set_dc_state(dev_priv,
> > DC_STATE_EN_UPTO_DC5);
> > -	}
> > +	if (power_well->count > 0)
> > +		gen9_dc_off_power_well_enable(dev_priv,
> > power_well);
> > +	else
> > +		gen9_dc_off_power_well_disable(dev_priv,
> > power_well);
> >  }
> >  
> >  static void i9xx_always_on_power_well_noop(struct drm_i915_private
> > *dev_priv,
> > @@ -2023,6 +2011,48 @@ sanitize_disable_power_well_option(const
> > struct drm_i915_private *dev_priv,
> >  	return 1;
> >  }
> >  
> > +static uint32_t get_allowed_dc_mask(const struct drm_i915_private
> > *dev_priv,
> > +				    int enable_dc)
> > +{
> > +	uint32_t mask = 0;
> > +
> > +	/*
> > +	 * DC9 has a separate HW flow from the rest of the DC
> > states, not
> > +	 * depending on the DMC firmware. It's needed by system
> > +	 * suspend/resume, so allow it unconditionally.
> > +	 */
> > +	if (IS_BROXTON(dev_priv))
> > +		mask |= DC_STATE_EN_DC9;
> > +
> > +	if (!enable_dc)
> > +		return mask;
> > +
> > +	if (!HAS_CSR(dev_priv))
> > +		return mask;
> > +
> > +	mask |= DC_STATE_EN_UPTO_DC5;
> > +	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> > +		mask |= DC_STATE_EN_UPTO_DC6;
> > +	} else if (!IS_BROXTON(dev_priv)) {
> > +		MISSING_CASE(INTEL_DEVID(dev_priv));
> > +		mask = 0;
> > +	}
> > +
> > +	switch (enable_dc) {
> > +	case 1:
> > +		mask &= ~DC_STATE_EN_UPTO_DC6;
> > +		break;
> > +	case 2:
> 
> Should we make some noise here on BXT? Just a thought, not sure it's
> useful.

Yes, I added this now. I'll resend the whole patchset with this change
since patchwork didn't pick up the first version for testing.

--Imre

> 
> > +	case -1:
> > +		break;
> > +	default:
> > +		DRM_ERROR("Unexpected value for enable_dc (%d)\n",
> > enable_dc);
> > +		break;
> > +	}
> > +
> > +	return mask;
> > +}
> > +
> >  #define set_power_wells(power_domains, __power_wells) ({		
> > \
> >  	(power_domains)->power_wells = (__power_wells);		
> > 	\
> >  	(power_domains)->power_well_count =
> > ARRAY_SIZE(__power_wells);	\
> > @@ -2041,6 +2071,8 @@ int intel_power_domains_init(struct
> > drm_i915_private *dev_priv)
> >  
> >  	i915.disable_power_well =
> > sanitize_disable_power_well_option(dev_priv,
> >  						     i915.disable_
> > power_well);
> > +	dev_priv->csr.allowed_dc_mask =
> > get_allowed_dc_mask(dev_priv,
> > +							    i915.e
> > nable_dc);
> >  
> >  	BUILD_BUG_ON(POWER_DOMAIN_NUM > 31);
> >  
> > -- 
> > 2.5.0
> > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx