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

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

> On Sep 2, 2020, at 12:30 PM, Srivatsa, Anusha <anusha.srivatsa@intel.com> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Sent: Tuesday, September 1, 2020 12:30 PM
>> To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
>> Cc: intel-gfx@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@intel.com>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@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.

> 
>>> -		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.
> 
> Anusha
> 
>>> 
>>> 		intel_display_power_put(dev_priv,
>> POWER_DOMAIN_DPLL_DC_OFF,
>>> --
>>> 2.25.0
>>> 
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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