> On Sep 3, 2020, at 10:04 AM, Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx> wrote:
>
>
>
>> -----Original Message-----
>> From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
>> Sent: Wednesday, September 2, 2020 2:32 PM
>> To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>
>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH] drm/i915/pll: Centralize PLL_ENABLE register
>> lookup
>>
>>
>>
>>> On Sep 2, 2020, at 12:30 PM, Srivatsa, Anusha
>> <anusha.srivatsa@xxxxxxxxx> wrote:
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>>>> Sent: Tuesday, September 1, 2020 12:30 PM
>>>> To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>
>>>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>>>> Subject: Re: [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@xxxxxxxxx>
>>>>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>>>> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
>>>>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
>>>>> ---
>>>>> 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?
sure
>
>>
>>>
>>>>> - 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?
yes
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
> Anusha
>>> Anusha
>>>
>>>>>
>>>>> intel_display_power_put(dev_priv,
>>>> POWER_DOMAIN_DPLL_DC_OFF,
>>>>> --
>>>>> 2.25.0
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
