[3/3] drm/i915: Define HSW/BDW display power domains the right way up

Submitted by Ville Syrjala on April 18, 2016, 11:02 a.m.

Details

Message ID 1460977348-32260-4-git-send-email-ville.syrjala@linux.intel.com
State Accepted
Commit 9d0996b5903fec7bff64e169b860146a923a6abe
Headers show
Series "drm/i915: Power domain fixes" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ville Syrjala April 18, 2016, 11:02 a.m. 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we're trying to define HSW/BDW power wells by what's not
included. Let's do it the other way around, so that you can actually
tell when the power well would get enabled. This will also allow us to
add new power domains without accidentally adding it to the HSW/BDW
display power domains.

The current set of domains looks rather buggy even:
- POWER_DOMAIN_MODESET is included in the display power well needlessly
- DDI-B to DDI-E were not part of the display power well when they
  should be

So let's fix that up while at it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 45 +++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 19 deletions(-)

Patch hide | download patch | download mbox 

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 585bbe7cefa6..2cead6cb95a1 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1589,30 +1589,37 @@  void intel_display_power_put(struct drm_i915_private *dev_priv,
 	intel_runtime_pm_put(dev_priv);
 }
 
-#define HSW_ALWAYS_ON_POWER_DOMAINS (			\
-	BIT(POWER_DOMAIN_PIPE_A) |			\
-	BIT(POWER_DOMAIN_TRANSCODER_EDP) |		\
-	BIT(POWER_DOMAIN_PORT_DDI_A_LANES) |		\
+#define HSW_DISPLAY_POWER_DOMAINS (			\
+	BIT(POWER_DOMAIN_PIPE_B) |			\
+	BIT(POWER_DOMAIN_PIPE_C) |			\
+	BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER) |		\
+	BIT(POWER_DOMAIN_PIPE_B_PANEL_FITTER) |		\
+	BIT(POWER_DOMAIN_PIPE_C_PANEL_FITTER) |		\
+	BIT(POWER_DOMAIN_TRANSCODER_A) |		\
+	BIT(POWER_DOMAIN_TRANSCODER_B) |		\
+	BIT(POWER_DOMAIN_TRANSCODER_C) |		\
 	BIT(POWER_DOMAIN_PORT_DDI_B_LANES) |		\
 	BIT(POWER_DOMAIN_PORT_DDI_C_LANES) |		\
 	BIT(POWER_DOMAIN_PORT_DDI_D_LANES) |		\
-	BIT(POWER_DOMAIN_PORT_CRT) |			\
-	BIT(POWER_DOMAIN_PLLS) |			\
-	BIT(POWER_DOMAIN_AUX_A) |			\
-	BIT(POWER_DOMAIN_AUX_B) |			\
-	BIT(POWER_DOMAIN_AUX_C) |			\
-	BIT(POWER_DOMAIN_AUX_D) |			\
-	BIT(POWER_DOMAIN_GMBUS) |			\
-	BIT(POWER_DOMAIN_INIT))
-#define HSW_DISPLAY_POWER_DOMAINS (				\
-	(POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS) |	\
+	BIT(POWER_DOMAIN_PORT_CRT) | /* DDI E */	\
+	BIT(POWER_DOMAIN_VGA) |				\
+	BIT(POWER_DOMAIN_AUDIO) |			\
 	BIT(POWER_DOMAIN_INIT))
 
-#define BDW_ALWAYS_ON_POWER_DOMAINS (			\
-	HSW_ALWAYS_ON_POWER_DOMAINS |			\
-	BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER))
-#define BDW_DISPLAY_POWER_DOMAINS (				\
-	(POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS) |	\
+#define BDW_DISPLAY_POWER_DOMAINS (			\
+	BIT(POWER_DOMAIN_PIPE_B) |			\
+	BIT(POWER_DOMAIN_PIPE_C) |			\
+	BIT(POWER_DOMAIN_PIPE_B_PANEL_FITTER) |		\
+	BIT(POWER_DOMAIN_PIPE_C_PANEL_FITTER) |		\
+	BIT(POWER_DOMAIN_TRANSCODER_A) |		\
+	BIT(POWER_DOMAIN_TRANSCODER_B) |		\
+	BIT(POWER_DOMAIN_TRANSCODER_C) |		\
+	BIT(POWER_DOMAIN_PORT_DDI_B_LANES) |		\
+	BIT(POWER_DOMAIN_PORT_DDI_C_LANES) |		\
+	BIT(POWER_DOMAIN_PORT_DDI_D_LANES) |		\
+	BIT(POWER_DOMAIN_PORT_CRT) | /* DDI E */	\
+	BIT(POWER_DOMAIN_VGA) |				\
+	BIT(POWER_DOMAIN_AUDIO) |			\
 	BIT(POWER_DOMAIN_INIT))
 
 #define VLV_DISPLAY_POWER_DOMAINS (		\

Comments

Imre Deak April 19, 2016, 6:25 a.m. 
On Mon, 2016-04-18 at 14:02 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we're trying to define HSW/BDW power wells by what's not
> included. Let's do it the other way around, so that you can actually
> tell when the power well would get enabled. This will also allow us
> to
> add new power domains without accidentally adding it to the HSW/BDW
> display power domains.
> 
> The current set of domains looks rather buggy even:
> - POWER_DOMAIN_MODESET is included in the display power well
> needlessly
> - DDI-B to DDI-E were not part of the display power well when they
>   should be
> 
> So let's fix that up while at it.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yes, much clearer without negations and indirections. We discussed this
already in person, but for reference: not including the DDI domains in
the display power well happened not to cause a problem, since they are
always requested along with their respective pipe and transcoder
domains. Nice work,
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 45 +++++++++++++++++++--
> ------------
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 585bbe7cefa6..2cead6cb95a1 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1589,30 +1589,37 @@ void intel_display_power_put(struct
> drm_i915_private *dev_priv,
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> -#define HSW_ALWAYS_ON_POWER_DOMAINS (			\
> -	BIT(POWER_DOMAIN_PIPE_A) |			\
> -	BIT(POWER_DOMAIN_TRANSCODER_EDP) |		\
> -	BIT(POWER_DOMAIN_PORT_DDI_A_LANES) |		\
> +#define HSW_DISPLAY_POWER_DOMAINS (			\
> +	BIT(POWER_DOMAIN_PIPE_B) |			\
> +	BIT(POWER_DOMAIN_PIPE_C) |			\
> +	BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER) |		\
> +	BIT(POWER_DOMAIN_PIPE_B_PANEL_FITTER) |		\
> +	BIT(POWER_DOMAIN_PIPE_C_PANEL_FITTER) |		\
> +	BIT(POWER_DOMAIN_TRANSCODER_A) |		\
> +	BIT(POWER_DOMAIN_TRANSCODER_B) |		\
> +	BIT(POWER_DOMAIN_TRANSCODER_C) |		\
>  	BIT(POWER_DOMAIN_PORT_DDI_B_LANES) |		\
>  	BIT(POWER_DOMAIN_PORT_DDI_C_LANES) |		\
>  	BIT(POWER_DOMAIN_PORT_DDI_D_LANES) |		\
> -	BIT(POWER_DOMAIN_PORT_CRT) |			\
> -	BIT(POWER_DOMAIN_PLLS) |			\
> -	BIT(POWER_DOMAIN_AUX_A) |			\
> -	BIT(POWER_DOMAIN_AUX_B) |			\
> -	BIT(POWER_DOMAIN_AUX_C) |			\
> -	BIT(POWER_DOMAIN_AUX_D) |			\
> -	BIT(POWER_DOMAIN_GMBUS) |			\
> -	BIT(POWER_DOMAIN_INIT))
> -#define HSW_DISPLAY_POWER_DOMAINS (				\
> -	(POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS) |	
> \
> +	BIT(POWER_DOMAIN_PORT_CRT) | /* DDI E */	\
> +	BIT(POWER_DOMAIN_VGA) |				\
> +	BIT(POWER_DOMAIN_AUDIO) |			\
>  	BIT(POWER_DOMAIN_INIT))
>  
> -#define BDW_ALWAYS_ON_POWER_DOMAINS (			\
> -	HSW_ALWAYS_ON_POWER_DOMAINS |			\
> -	BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER))
> -#define BDW_DISPLAY_POWER_DOMAINS (				\
> -	(POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS) |	
> \
> +#define BDW_DISPLAY_POWER_DOMAINS (			\
> +	BIT(POWER_DOMAIN_PIPE_B) |			\
> +	BIT(POWER_DOMAIN_PIPE_C) |			\
> +	BIT(POWER_DOMAIN_PIPE_B_PANEL_FITTER) |		\
> +	BIT(POWER_DOMAIN_PIPE_C_PANEL_FITTER) |		\
> +	BIT(POWER_DOMAIN_TRANSCODER_A) |		\
> +	BIT(POWER_DOMAIN_TRANSCODER_B) |		\
> +	BIT(POWER_DOMAIN_TRANSCODER_C) |		\
> +	BIT(POWER_DOMAIN_PORT_DDI_B_LANES) |		\
> +	BIT(POWER_DOMAIN_PORT_DDI_C_LANES) |		\
> +	BIT(POWER_DOMAIN_PORT_DDI_D_LANES) |		\
> +	BIT(POWER_DOMAIN_PORT_CRT) | /* DDI E */	\
> +	BIT(POWER_DOMAIN_VGA) |				\
> +	BIT(POWER_DOMAIN_AUDIO) |			\
>  	BIT(POWER_DOMAIN_INIT))
>  
>  #define VLV_DISPLAY_POWER_DOMAINS (		\