Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup
> -Original Message-
> From: Jani Nikula
> Sent: Thursday, September 10, 2020 6:31 AM
> To: Srivatsa, Anusha ; intel-
> g...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register
> lookup
>
> On Tue, 08 Sep 2020, 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)
> >
> > v3: Clean up combo_pll_disable() (Rodrigo)
> >
> > Suggested-by: Matt Roper
> > Cc: Ville Syrjälä
> > Cc: Matt Roper
> > Signed-off-by: Anusha Srivatsa
> > Reviewed-by: Rodrigo Vivi
> > ---
> > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 29
> > +++
> > 1 file changed, 17 insertions(+), 12 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..441b6f52e808 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,
>
> Please keep the static keyword and the return type on the same line with
> each other.
>
> And since you're touching this, please rename dev_priv to i915 in all new
> code adding it.
Sure. Thanks for the feedback Jani.
Anusha
> BR,
> Jani.
>
>
> > + 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) {
> > - enable_reg = MG_PLL_ENABLE(0);
> >
> > /*
> > * We need to disable DC states when this DPLL is enabled.
> > @@ -4157,19 +4163,18 @@ 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);
> > +
> > + icl_pll_disable(dev_priv, pll, enable_reg);
> >
> > 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);
> >
> > intel_display_power_put(dev_priv,
> POWER_DOMAIN_DPLL_DC_OFF,
> > pll->wakeref);
> > return;
> > }
> >
> > - icl_pll_disable(dev_priv, pll, enable_reg);
> > }
> >
> > static void tbt_pll_disable(struct drm_i915_private *dev_priv,
>
> --
> Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup
On Tue, 08 Sep 2020, 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)
>
> v3: Clean up combo_pll_disable() (Rodrigo)
>
> Suggested-by: Matt Roper
> Cc: Ville Syrjälä
> Cc: Matt Roper
> Signed-off-by: Anusha Srivatsa
> Reviewed-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 29 +++
> 1 file changed, 17 insertions(+), 12 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..441b6f52e808 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,
Please keep the static keyword and the return type on the same line with
each other.
And since you're touching this, please rename dev_priv to i915 in all
new code adding it.
BR,
Jani.
> + 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) {
> - enable_reg = MG_PLL_ENABLE(0);
>
> /*
>* We need to disable DC states when this DPLL is enabled.
> @@ -4157,19 +4163,18 @@ 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);
> +
> + icl_pll_disable(dev_priv, pll, enable_reg);
>
> 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);
>
> intel_display_power_put(dev_priv, POWER_DOMAIN_DPLL_DC_OFF,
> pll->wakeref);
> return;
> }
>
> - icl_pll_disable(dev_priv, pll, enable_reg);
> }
>
> static void tbt_pll_disable(struct drm_i915_private *dev_priv,
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup
> On Sep 8, 2020, at 4:39 PM, Srivatsa, Anusha
> 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)
>
> v3: Clean up combo_pll_disable() (Rodrigo)
>
> Suggested-by: Matt Roper
> Cc: Ville Syrjälä
> Cc: Matt Roper
> Signed-off-by: Anusha Srivatsa
> Reviewed-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 29 +++
> 1 file changed, 17 insertions(+), 12 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..441b6f52e808 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) {
> - enable_reg = MG_PLL_ENABLE(0);
>
> /*
>* We need to disable DC states when this DPLL is enabled.
> @@ -4157,19 +4163,18 @@ 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);
> +
> + icl_pll_disable(dev_priv, pll, enable_reg);
>
> 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);
>
> intel_display_power_put(dev_priv, POWER_DOMAIN_DPLL_DC_OFF,
> pll->wakeref);
> return;
this return can also be removed
> }
>
> - icl_pll_disable(dev_priv, pll, enable_reg);
> }
>
> static void tbt_pll_disable(struct drm_i915_private *dev_priv,
> --
> 2.25.0
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup
> On Sep 3, 2020, at 10:04 AM, Srivatsa, Anusha
> wrote:
>
>
>
>> -Original Message-
>> From: Vivi, Rodrigo
>> Sent: Wednesday, September 2, 2020 2:32 PM
>> To: Srivatsa, Anusha
>> Cc: intel-gfx@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
>> wrote:
>>>
>>>
>>>
>>>> -Original Message-
>>>> From: Rodrigo Vivi
>>>> Sent: Tuesday, September 1, 2020 12:30 PM
>>>> To: Srivatsa, Anusha
>>>> 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
>>>>> Cc: Ville Syrjälä
>>>>> Cc: Matt Roper
>>>>> Signed-off-by: Anusha Srivatsa
>>>>> ---
>>>>> 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
Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup
> -Original Message-
> From: Vivi, Rodrigo
> Sent: Wednesday, September 2, 2020 2:32 PM
> To: Srivatsa, Anusha
> Cc: intel-gfx@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
> wrote:
> >
> >
> >
> >> -Original Message-
> >> From: Rodrigo Vivi
> >> Sent: Tuesday, September 1, 2020 12:30 PM
> >> To: Srivatsa, Anusha
> >> 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
> >>> Cc: Ville Syrjälä
> >>> Cc: Matt Roper
> >>> Signed-off-by: Anusha Srivatsa
> >>> ---
> >>> 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 infr
Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup
> On Sep 2, 2020, at 12:30 PM, Srivatsa, Anusha
> wrote:
>
>
>
>> -Original Message-
>> From: Rodrigo Vivi
>> Sent: Tuesday, September 1, 2020 12:30 PM
>> To: Srivatsa, Anusha
>> 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
>>> Cc: Ville Syrjälä
>>> Cc: Matt Roper
>>> Signed-off-by: Anusha Srivatsa
>>> ---
>>> 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
Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup
> -Original Message-
> From: Rodrigo Vivi
> Sent: Tuesday, September 1, 2020 12:30 PM
> To: Srivatsa, Anusha
> 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
> > Cc: Ville Syrjälä
> > Cc: Matt Roper
> > Signed-off-by: Anusha Srivatsa
> > ---
> > 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. Are you suggesting using power_{put/get} to
somehow get rid of doubled if?
> > - 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
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
> Cc: Ville Syrjälä
> Cc: Matt Roper
> Signed-off-by: Anusha Srivatsa
> ---
> 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...
> - 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...
>
> 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
Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup
On Mon, Aug 31, 2020 at 07:03:47PM +, Srivatsa, Anusha wrote:
>
>
> > -Original Message-
> > From: Ville Syrjälä
> > Sent: Monday, August 31, 2020 6:42 AM
> > To: Srivatsa, Anusha
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE
> > register
> > lookup
> >
> > On Fri, Aug 28, 2020 at 02:58:32PM -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.
> > >
> > > Suggested-by: Matt Roper
> > > Cc: Matt Roper
> > > Signed-off-by: Anusha Srivatsa
> > > ---
> > > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 27
> > > ---
> > > 1 file changed, 17 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 81ab975fe4f0..388136618bb7 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > > @@ -147,6 +147,20 @@ void assert_shared_dpll(struct drm_i915_private
> > *dev_priv,
> > > pll->info->name, onoff(state), onoff(cur_state)); }
> > >
> > > +static
> > > +i915_reg_t intel_get_pll_enable_reg(struct drm_i915_private *dev_priv,
> > > + struct intel_shared_dpll *pll)
> >
> > combo_pll_enable_reg() ?
> Actually want to avoid mentioning combo in the name. We might have platforms
> that do not have combo phys. We still want this function to be one place
> where platforms gets the PLL_ENABLE register.
There's no point in mixing up different PHY types in a single function.
>
> >
> > > +{
> > > +
> > > + if (IS_ELKHARTLAKE(dev_priv)) {
> > > + if (pll->info->id == DPLL_ID_EHL_DPLL4)
> > > + return MG_PLL_ENABLE(0);
> > > + }
> >
> > Ugly nested if.
> Will change it.
>
> Anusha
> > > +
> > > + 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 +3856,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_get_pll_enable_reg(dev_priv, pll);
> > >
> > > return icl_pll_get_hw_state(dev_priv, pll, hw_state, enable_reg); }
> > > @@ -4045,11 +4054,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_get_pll_enable_reg(dev_priv, pll);
> > >
> > > if (IS_ELKHARTLAKE(dev_priv) &&
> > > pll->info->id == DPLL_ID_EHL_DPLL4) {
> > > - enable_reg = MG_PLL_ENABLE(0);
> > >
> > > /*
> > >* We need to disable DC states when this DPLL is enabled.
> > > @@ -4157,11 +4165,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_get_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);
> > >
> > > 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
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup
> -Original Message-
> From: Ville Syrjälä
> Sent: Monday, August 31, 2020 6:42 AM
> To: Srivatsa, Anusha
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register
> lookup
>
> On Fri, Aug 28, 2020 at 02:58:32PM -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.
> >
> > Suggested-by: Matt Roper
> > Cc: Matt Roper
> > Signed-off-by: Anusha Srivatsa
> > ---
> > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 27
> > ---
> > 1 file changed, 17 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 81ab975fe4f0..388136618bb7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -147,6 +147,20 @@ void assert_shared_dpll(struct drm_i915_private
> *dev_priv,
> > pll->info->name, onoff(state), onoff(cur_state)); }
> >
> > +static
> > +i915_reg_t intel_get_pll_enable_reg(struct drm_i915_private *dev_priv,
> > + struct intel_shared_dpll *pll)
>
> combo_pll_enable_reg() ?
Actually want to avoid mentioning combo in the name. We might have platforms
that do not have combo phys. We still want this function to be one place where
platforms gets the PLL_ENABLE register.
>
> > +{
> > +
> > + if (IS_ELKHARTLAKE(dev_priv)) {
> > + if (pll->info->id == DPLL_ID_EHL_DPLL4)
> > + return MG_PLL_ENABLE(0);
> > + }
>
> Ugly nested if.
Will change it.
Anusha
> > +
> > + 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 +3856,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_get_pll_enable_reg(dev_priv, pll);
> >
> > return icl_pll_get_hw_state(dev_priv, pll, hw_state, enable_reg); }
> > @@ -4045,11 +4054,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_get_pll_enable_reg(dev_priv, pll);
> >
> > if (IS_ELKHARTLAKE(dev_priv) &&
> > pll->info->id == DPLL_ID_EHL_DPLL4) {
> > - enable_reg = MG_PLL_ENABLE(0);
> >
> > /*
> > * We need to disable DC states when this DPLL is enabled.
> > @@ -4157,11 +4165,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_get_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);
> >
> > 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
>
> --
> Ville Syrjälä
> Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/pll: Centralize PLL_ENABLE register lookup
On Fri, Aug 28, 2020 at 02:58:32PM -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.
>
> Suggested-by: Matt Roper
> Cc: Matt Roper
> Signed-off-by: Anusha Srivatsa
> ---
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 27 ---
> 1 file changed, 17 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 81ab975fe4f0..388136618bb7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -147,6 +147,20 @@ void assert_shared_dpll(struct drm_i915_private
> *dev_priv,
> pll->info->name, onoff(state), onoff(cur_state));
> }
>
> +static
> +i915_reg_t intel_get_pll_enable_reg(struct drm_i915_private *dev_priv,
> + struct intel_shared_dpll *pll)
combo_pll_enable_reg() ?
> +{
> +
> + if (IS_ELKHARTLAKE(dev_priv)) {
> + if (pll->info->id == DPLL_ID_EHL_DPLL4)
> + return MG_PLL_ENABLE(0);
> + }
Ugly nested if.
> +
> + 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 +3856,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_get_pll_enable_reg(dev_priv, pll);
>
> return icl_pll_get_hw_state(dev_priv, pll, hw_state, enable_reg);
> }
> @@ -4045,11 +4054,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_get_pll_enable_reg(dev_priv, pll);
>
> if (IS_ELKHARTLAKE(dev_priv) &&
> pll->info->id == DPLL_ID_EHL_DPLL4) {
> - enable_reg = MG_PLL_ENABLE(0);
>
> /*
>* We need to disable DC states when this DPLL is enabled.
> @@ -4157,11 +4165,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_get_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);
>
> 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
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
