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

[Imre Deak]

On Thu, Oct 12, 2023 at 05:56:34PM +0300, Jani Nikula wrote:
> On Thu, 12 Oct 2023, Ville Syrjala <ville.syrjala at linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala at 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 at intel.com>
> 
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala at 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