[Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup

[Srivatsa, Anusha]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Sent: Wednesday, September 2, 2020 2:32 PM
> To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register
> lookup
> 
> 
> 
> > On Sep 2, 2020, at 12:30 PM, Srivatsa, Anusha
> <anusha.srivatsa at intel.com> wrote:
> >
> >
> >
> >> -----Original Message-----
> >> From: Rodrigo Vivi <rodrigo.vivi at intel.com>
> >> Sent: Tuesday, September 1, 2020 12:30 PM
> >> To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
> >> Cc: intel-gfx at lists.freedesktop.org
> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE
> >> register lookup
> >>
> >> On Tue, Sep 01, 2020 at 11:27:58AM -0700, Anusha Srivatsa wrote:
> >>> We currenty check for platform at multiple parts in the driver to
> >>> grab the correct PLL. Let us begin to centralize it through a helper
> >>> function.
> >>>
> >>> v2: s/intel_get_pll_enable_reg()/intel_combo_pll_enable_reg()
> >>> (Ville)
> >>>
> >>> Suggested-by: Matt Roper <matthew.d.roper at intel.com>
> >>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >>> Cc: Matt Roper <matthew.d.roper at intel.com>
> >>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
> >>> ---
> >>> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 25
> >>> +++++++++++--------
> >>> 1 file changed, 15 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >>> b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >>> index c9013f8f766f..7440836c5e44 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >>> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> >>> @@ -147,6 +147,18 @@ void assert_shared_dpll(struct
> drm_i915_private
> >> *dev_priv,
> >>> 			pll->info->name, onoff(state), onoff(cur_state));  }
> >>>
> >>> +static
> >>> +i915_reg_t intel_combo_pll_enable_reg(struct drm_i915_private
> >> *dev_priv,
> >>> +				    struct intel_shared_dpll *pll) {
> >>> +
> >>> +	if (IS_ELKHARTLAKE(dev_priv) && (pll->info->id ==
> >> DPLL_ID_EHL_DPLL4))
> >>> +			return MG_PLL_ENABLE(0);
> >>> +
> >>> +	return CNL_DPLL_ENABLE(pll->info->id);
> >>> +
> >>> +
> >>> +}
> >>> /**
> >>>  * intel_prepare_shared_dpll - call a dpll's prepare hook
> >>>  * @crtc_state: CRTC, and its state, which has a shared dpll @@
> >>> -3842,12 +3854,7 @@ static bool combo_pll_get_hw_state(struct
> >> drm_i915_private *dev_priv,
> >>> 				   struct intel_shared_dpll *pll,
> >>> 				   struct intel_dpll_hw_state *hw_state)  {
> >>> -	i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
> >>> -
> >>> -	if (IS_ELKHARTLAKE(dev_priv) &&
> >>> -	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> >>> -		enable_reg = MG_PLL_ENABLE(0);
> >>> -	}
> >>> +	i915_reg_t enable_reg = intel_combo_pll_enable_reg(dev_priv, pll);
> >>>
> >>> 	return icl_pll_get_hw_state(dev_priv, pll, hw_state, enable_reg);
> >>> } @@ -4045,11 +4052,10 @@ static void icl_pll_enable(struct
> >>> drm_i915_private *dev_priv,  static void combo_pll_enable(struct
> >> drm_i915_private *dev_priv,
> >>> 			     struct intel_shared_dpll *pll)  {
> >>> -	i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
> >>> +	i915_reg_t enable_reg = intel_combo_pll_enable_reg(dev_priv, pll);
> >>>
> >>> 	if (IS_ELKHARTLAKE(dev_priv) &&
> >>> 	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> >>
> >> there's probably something else that we can do now with the
> >> power_{put,get} to get rid of the, now, doubled if checks...
> >
> > Don't follow you here Rodrigo.
> 
> me neither ;)
> I'm just brainstorming... thinking out lout.
> 
> > Are you suggesting using power_{put/get} to somehow get rid of doubled
> if?
> 
> after this patch, on this path we will do this if check twice.
> not a big deal, but we can probably do something better.
> 
> However I don't understand why we had this get/put here at first place.
> Only for this platform and only for this pll4. So, what I am wondering is that
> we have something better to do with the power_well infrastructure in
> general that would allow us to avoid the if (platform && pll4) in favor of
> something more generic.
> 
> but definitely not a blocker for this patch itself.
Ok. So maybe the power well infrastructure change can be part a later patch?

> 
> >
> >>> -		enable_reg = MG_PLL_ENABLE(0);
> >>>
> >>> 		/*
> >>> 		 * We need to disable DC states when this DPLL is enabled.
> >>> @@ -4157,11 +4163,10 @@ static void icl_pll_disable(struct
> >>> drm_i915_private *dev_priv,  static void combo_pll_disable(struct
> >> drm_i915_private *dev_priv,
> >>> 			      struct intel_shared_dpll *pll)  {
> >>> -	i915_reg_t enable_reg = CNL_DPLL_ENABLE(pll->info->id);
> >>> +	i915_reg_t enable_reg = intel_combo_pll_enable_reg(dev_priv, pll);
> >>>
> >>> 	if (IS_ELKHARTLAKE(dev_priv) &&
> >>> 	    pll->info->id == DPLL_ID_EHL_DPLL4) {
> >>> -		enable_reg = MG_PLL_ENABLE(0);
> >>> 		icl_pll_disable(dev_priv, pll, enable_reg);
> >>
> >> but here, at least, let's clean this function now...
> >> move this call above and out of the if and delete the one below and
> >> keep just the power_put inside the if...
> >
> > Good change. Thanks!
> > Will change that.
 With the above code movement, do I have your reviewed-by?

Anusha 
> > Anusha
> >
> >>>
> >>> 		intel_display_power_put(dev_priv,
> >> POWER_DOMAIN_DPLL_DC_OFF,
> >>> --
> >>> 2.25.0
> >>>
> >>> _______________________________________________
> >>> Intel-gfx mailing list
> >>> Intel-gfx at lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx