diff mbox series

[2/4] drm/i915: Abstract the extra JSL/EHL DPLL4 power domain better

Message ID 20231012123522.26045-3-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: DPLL code cleanups | expand

Commit Message

Ville Syrjälä Oct. 12, 2023, 12:35 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Just include the JSL/EHL DPLL4 extra power domain in the dpll_info
struct. This way the same approach could be used by other platforms
as well (should the need arise), and we don't have to sprinkle
platform checks all over the place.

Note that I'm perhaps slightly abusing things here as
power_domain==0 (which is actually POWER_DOMAIN_DISPLAY_CORE) now
indicates that no extra power domain is needed. I suppose using
POWER_DOMAIN_INVALID would be more correct, but then we'd have to
sprinkle that to all the other DPLLs.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 30 +++++--------------
 drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  6 ++++
 2 files changed, 14 insertions(+), 22 deletions(-)

Comments

Jani Nikula Oct. 12, 2023, 2:56 p.m. UTC | #1 
On Thu, 12 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Just include the JSL/EHL DPLL4 extra power domain in the dpll_info
> struct. This way the same approach could be used by other platforms
> as well (should the need arise), and we don't have to sprinkle
> platform checks all over the place.
>
> Note that I'm perhaps slightly abusing things here as
> power_domain==0 (which is actually POWER_DOMAIN_DISPLAY_CORE) now
> indicates that no extra power domain is needed. I suppose using
> POWER_DOMAIN_INVALID would be more correct, but then we'd have to
> sprinkle that to all the other DPLLs.

Cc: Imre, how bad do you think that is?

Anyway,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 30 +++++--------------
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  6 ++++
>  2 files changed, 14 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index b7997b096796..4e524cb8ed83 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -3838,17 +3838,8 @@ static void combo_pll_enable(struct drm_i915_private *i915,
>  {
>  	i915_reg_t enable_reg = intel_combo_pll_enable_reg(i915, pll);
>  
> -	if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) &&
> -	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> -
> -		/*
> -		 * We need to disable DC states when this DPLL is enabled.
> -		 * This can be done by taking a reference on DPLL4 power
> -		 * domain.
> -		 */
> -		pll->wakeref = intel_display_power_get(i915,
> -						       POWER_DOMAIN_DC_OFF);
> -	}
> +	if (pll->info->power_domain)
> +		pll->wakeref = intel_display_power_get(i915, pll->info->power_domain);
>  
>  	icl_pll_power_enable(i915, pll, enable_reg);
>  
> @@ -3946,10 +3937,8 @@ static void combo_pll_disable(struct drm_i915_private *i915,
>  
>  	icl_pll_disable(i915, pll, enable_reg);
>  
> -	if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) &&
> -	    pll->info->id == DPLL_ID_EHL_DPLL4)
> -		intel_display_power_put(i915, POWER_DOMAIN_DC_OFF,
> -					pll->wakeref);
> +	if (pll->info->power_domain)
> +		intel_display_power_put(i915, pll->info->power_domain, pll->wakeref);
>  }
>  
>  static void tbt_pll_disable(struct drm_i915_private *i915,
> @@ -4041,7 +4030,8 @@ static const struct intel_dpll_mgr icl_pll_mgr = {
>  static const struct dpll_info ehl_plls[] = {
>  	{ .name = "DPLL 0", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL0, },
>  	{ .name = "DPLL 1", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL1, },
> -	{ .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4, },
> +	{ .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4,
> +	  .power_domain = POWER_DOMAIN_DC_OFF, },
>  	{}
>  };
>  
> @@ -4369,12 +4359,8 @@ static void readout_dpll_hw_state(struct drm_i915_private *i915,
>  
>  	pll->on = intel_dpll_get_hw_state(i915, pll, &pll->state.hw_state);
>  
> -	if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) &&
> -	    pll->on &&
> -	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> -		pll->wakeref = intel_display_power_get(i915,
> -						       POWER_DOMAIN_DC_OFF);
> -	}
> +	if (pll->on && pll->info->power_domain)
> +		pll->wakeref = intel_display_power_get(i915, pll->info->power_domain);
>  
>  	pll->state.pipe_mask = 0;
>  	for_each_intel_crtc(&i915->drm, crtc) {
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> index dd4796a61751..2e7ea0d8d3ff 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> @@ -27,6 +27,7 @@
>  
>  #include <linux/types.h>
>  
> +#include "intel_display_power.h"
>  #include "intel_wakeref.h"
>  
>  #define for_each_shared_dpll(__i915, __pll, __i) \
> @@ -270,6 +271,11 @@ struct dpll_info {
>  	 */
>  	enum intel_dpll_id id;
>  
> +	/**
> +	 * @power_domain: extra power domain required by the DPLL
> +	 */
> +	enum intel_display_power_domain power_domain;
> +
>  #define INTEL_DPLL_ALWAYS_ON	(1 << 0)
>  	/**
>  	 * @flags:
Imre Deak Oct. 12, 2023, 4:20 p.m. UTC | #2 
On Thu, Oct 12, 2023 at 05:56:34PM +0300, Jani Nikula wrote:
> On Thu, 12 Oct 2023, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Just include the JSL/EHL DPLL4 extra power domain in the dpll_info
> > struct. This way the same approach could be used by other platforms
> > as well (should the need arise), and we don't have to sprinkle
> > platform checks all over the place.
> >
> > Note that I'm perhaps slightly abusing things here as
> > power_domain==0 (which is actually POWER_DOMAIN_DISPLAY_CORE) now
> > indicates that no extra power domain is needed. I suppose using
> > POWER_DOMAIN_INVALID would be more correct, but then we'd have to
> > sprinkle that to all the other DPLLs.
> 
> Cc: Imre, how bad do you think that is?

It looks ok to me.

With the original 64 bit domain mask it made sense to keep
POWER_DOMAIN_INVALID at the end of the enum list, but nothing depends on
the actual value, so for clarity it could be moved to be the first
item.

> 
> Anyway,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 30 +++++--------------
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  6 ++++
> >  2 files changed, 14 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > index b7997b096796..4e524cb8ed83 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -3838,17 +3838,8 @@ static void combo_pll_enable(struct drm_i915_private *i915,
> >  {
> >  	i915_reg_t enable_reg = intel_combo_pll_enable_reg(i915, pll);
> >  
> > -	if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) &&
> > -	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> > -
> > -		/*
> > -		 * We need to disable DC states when this DPLL is enabled.
> > -		 * This can be done by taking a reference on DPLL4 power
> > -		 * domain.
> > -		 */
> > -		pll->wakeref = intel_display_power_get(i915,
> > -						       POWER_DOMAIN_DC_OFF);
> > -	}
> > +	if (pll->info->power_domain)
> > +		pll->wakeref = intel_display_power_get(i915, pll->info->power_domain);
> >  
> >  	icl_pll_power_enable(i915, pll, enable_reg);
> >  
> > @@ -3946,10 +3937,8 @@ static void combo_pll_disable(struct drm_i915_private *i915,
> >  
> >  	icl_pll_disable(i915, pll, enable_reg);
> >  
> > -	if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) &&
> > -	    pll->info->id == DPLL_ID_EHL_DPLL4)
> > -		intel_display_power_put(i915, POWER_DOMAIN_DC_OFF,
> > -					pll->wakeref);
> > +	if (pll->info->power_domain)
> > +		intel_display_power_put(i915, pll->info->power_domain, pll->wakeref);
> >  }
> >  
> >  static void tbt_pll_disable(struct drm_i915_private *i915,
> > @@ -4041,7 +4030,8 @@ static const struct intel_dpll_mgr icl_pll_mgr = {
> >  static const struct dpll_info ehl_plls[] = {
> >  	{ .name = "DPLL 0", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL0, },
> >  	{ .name = "DPLL 1", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL1, },
> > -	{ .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4, },
> > +	{ .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4,
> > +	  .power_domain = POWER_DOMAIN_DC_OFF, },
> >  	{}
> >  };
> >  
> > @@ -4369,12 +4359,8 @@ static void readout_dpll_hw_state(struct drm_i915_private *i915,
> >  
> >  	pll->on = intel_dpll_get_hw_state(i915, pll, &pll->state.hw_state);
> >  
> > -	if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) &&
> > -	    pll->on &&
> > -	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> > -		pll->wakeref = intel_display_power_get(i915,
> > -						       POWER_DOMAIN_DC_OFF);
> > -	}
> > +	if (pll->on && pll->info->power_domain)
> > +		pll->wakeref = intel_display_power_get(i915, pll->info->power_domain);
> >  
> >  	pll->state.pipe_mask = 0;
> >  	for_each_intel_crtc(&i915->drm, crtc) {
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > index dd4796a61751..2e7ea0d8d3ff 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > @@ -27,6 +27,7 @@
> >  
> >  #include <linux/types.h>
> >  
> > +#include "intel_display_power.h"
> >  #include "intel_wakeref.h"
> >  
> >  #define for_each_shared_dpll(__i915, __pll, __i) \
> > @@ -270,6 +271,11 @@ struct dpll_info {
> >  	 */
> >  	enum intel_dpll_id id;
> >  
> > +	/**
> > +	 * @power_domain: extra power domain required by the DPLL
> > +	 */
> > +	enum intel_display_power_domain power_domain;
> > +
> >  #define INTEL_DPLL_ALWAYS_ON	(1 << 0)
> >  	/**
> >  	 * @flags:
> 
> -- 
> Jani Nikula, Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
index b7997b096796..4e524cb8ed83 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -3838,17 +3838,8 @@  static void combo_pll_enable(struct drm_i915_private *i915,
 {
 	i915_reg_t enable_reg = intel_combo_pll_enable_reg(i915, pll);
 
-	if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) &&
-	    pll->info->id == DPLL_ID_EHL_DPLL4) {
-
-		/*
-		 * We need to disable DC states when this DPLL is enabled.
-		 * This can be done by taking a reference on DPLL4 power
-		 * domain.
-		 */
-		pll->wakeref = intel_display_power_get(i915,
-						       POWER_DOMAIN_DC_OFF);
-	}
+	if (pll->info->power_domain)
+		pll->wakeref = intel_display_power_get(i915, pll->info->power_domain);
 
 	icl_pll_power_enable(i915, pll, enable_reg);
 
@@ -3946,10 +3937,8 @@  static void combo_pll_disable(struct drm_i915_private *i915,
 
 	icl_pll_disable(i915, pll, enable_reg);
 
-	if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) &&
-	    pll->info->id == DPLL_ID_EHL_DPLL4)
-		intel_display_power_put(i915, POWER_DOMAIN_DC_OFF,
-					pll->wakeref);
+	if (pll->info->power_domain)
+		intel_display_power_put(i915, pll->info->power_domain, pll->wakeref);
 }
 
 static void tbt_pll_disable(struct drm_i915_private *i915,
@@ -4041,7 +4030,8 @@  static const struct intel_dpll_mgr icl_pll_mgr = {
 static const struct dpll_info ehl_plls[] = {
 	{ .name = "DPLL 0", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL0, },
 	{ .name = "DPLL 1", .funcs = &combo_pll_funcs, .id = DPLL_ID_ICL_DPLL1, },
-	{ .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4, },
+	{ .name = "DPLL 4", .funcs = &combo_pll_funcs, .id = DPLL_ID_EHL_DPLL4,
+	  .power_domain = POWER_DOMAIN_DC_OFF, },
 	{}
 };
 
@@ -4369,12 +4359,8 @@  static void readout_dpll_hw_state(struct drm_i915_private *i915,
 
 	pll->on = intel_dpll_get_hw_state(i915, pll, &pll->state.hw_state);
 
-	if ((IS_JASPERLAKE(i915) || IS_ELKHARTLAKE(i915)) &&
-	    pll->on &&
-	    pll->info->id == DPLL_ID_EHL_DPLL4) {
-		pll->wakeref = intel_display_power_get(i915,
-						       POWER_DOMAIN_DC_OFF);
-	}
+	if (pll->on && pll->info->power_domain)
+		pll->wakeref = intel_display_power_get(i915, pll->info->power_domain);
 
 	pll->state.pipe_mask = 0;
 	for_each_intel_crtc(&i915->drm, crtc) {
diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
index dd4796a61751..2e7ea0d8d3ff 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
@@ -27,6 +27,7 @@ 
 
 #include <linux/types.h>
 
+#include "intel_display_power.h"
 #include "intel_wakeref.h"
 
 #define for_each_shared_dpll(__i915, __pll, __i) \
@@ -270,6 +271,11 @@  struct dpll_info {
 	 */
 	enum intel_dpll_id id;
 
+	/**
+	 * @power_domain: extra power domain required by the DPLL
+	 */
+	enum intel_display_power_domain power_domain;
+
 #define INTEL_DPLL_ALWAYS_ON	(1 << 0)
 	/**
 	 * @flags: