diff mbox series

drm/i915/cdclk: Rename intel_cdclk_needs_modeset to intel_cdclk_params_changed

Message ID 20240202131208.453298-1-gustavo.sousa@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/cdclk: Rename intel_cdclk_needs_modeset to intel_cdclk_params_changed | expand

Commit Message

Gustavo Sousa Feb. 2, 2024, 1:12 p.m. UTC
Looks like the name and description of intel_cdclk_needs_modeset()
became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for
cdclk changes if possible"), when it became possible to update the cdclk
without requiring disabling the pipes when only changing the cd2x
divider was enough.

Later on we also added the same type of support with squash and crawling
with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash
when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk
squasher to be reconfigured live") and commit d62686ba3b54
("drm/i915/adl_p: CDCLK crawl support for ADL").

As such, update that function's name and documentation to something more
appropriate, since the real checks for requiring modeset are done
elsewhere.

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---

One thing worth noting here is that, with this change, we are left with an
awkward situation where two function names related to checking changes in cdclk:

  intel_cdclk_params_changed() and intel_cdclk_changed()

,

and I find it weird that we have intel_cdclk_changed(), which checks for the
voltage level as well. Shouldn't the voltage level be a function of cdclk and
ddi clock? Why do we need that?

 drivers/gpu/drm/i915/display/intel_cdclk.c        | 15 +++++++--------
 drivers/gpu/drm/i915/display/intel_cdclk.h        |  4 ++--
 .../drm/i915/display/intel_display_power_well.c   |  4 ++--
 3 files changed, 11 insertions(+), 12 deletions(-)

Comments

Ville Syrjala Feb. 2, 2024, 7:58 p.m. UTC | #1
On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote:
> Looks like the name and description of intel_cdclk_needs_modeset()
> became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for
> cdclk changes if possible"), when it became possible to update the cdclk
> without requiring disabling the pipes when only changing the cd2x
> divider was enough.
> 
> Later on we also added the same type of support with squash and crawling
> with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash
> when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk
> squasher to be reconfigured live") and commit d62686ba3b54
> ("drm/i915/adl_p: CDCLK crawl support for ADL").
> 
> As such, update that function's name and documentation to something more
> appropriate, since the real checks for requiring modeset are done
> elsewhere.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
> 
> One thing worth noting here is that, with this change, we are left with an
> awkward situation where two function names related to checking changes in cdclk:
> 
>   intel_cdclk_params_changed() and intel_cdclk_changed()
> 
> ,
> 
> and I find it weird that we have intel_cdclk_changed(), which checks for the
> voltage level as well. Shouldn't the voltage level be a function of cdclk and
> ddi clock? Why do we need that?
> 
>  drivers/gpu/drm/i915/display/intel_cdclk.c        | 15 +++++++--------
>  drivers/gpu/drm/i915/display/intel_cdclk.h        |  4 ++--
>  .../drm/i915/display/intel_display_power_well.c   |  4 ++--
>  3 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 26200ee3e23f..caadd880865f 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
>  }
>  
>  /**
> - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
> - *                             configurations requires a modeset on all pipes
> + * intel_cdclk_params_changed - Check whether CDCLK parameters changed
>   * @a: first CDCLK configuration
>   * @b: second CDCLK configuration
>   *
>   * Returns:
> - * True if changing between the two CDCLK configurations
> - * requires all pipes to be off, false if not.
> + * True if parameters changed in a way that requires programming the CDCLK
> + * and False otherwise.
>   */
> -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
> -			       const struct intel_cdclk_config *b)
> +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
> +				const struct intel_cdclk_config *b)

The new name isn't very descriptive either.

Outside the cd2x/crawl/squash cases we stil have to consider
two cases:
1. cdclk frequency/pll changes (voltage level can change or not)
2. cdclk frequency/pll doesn't change, but voltage level needs to change

And that difference is what intel_cdclk_needs_modeset() is trying
convey. And intel_cdclk_changed() tells us whether anything at all
is changing.
Ville Syrjala Feb. 2, 2024, 8:06 p.m. UTC | #2
On Fri, Feb 02, 2024 at 09:58:37PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote:
> > Looks like the name and description of intel_cdclk_needs_modeset()
> > became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for
> > cdclk changes if possible"), when it became possible to update the cdclk
> > without requiring disabling the pipes when only changing the cd2x
> > divider was enough.
> > 
> > Later on we also added the same type of support with squash and crawling
> > with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash
> > when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk
> > squasher to be reconfigured live") and commit d62686ba3b54
> > ("drm/i915/adl_p: CDCLK crawl support for ADL").
> > 
> > As such, update that function's name and documentation to something more
> > appropriate, since the real checks for requiring modeset are done
> > elsewhere.
> > 
> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > ---
> > 
> > One thing worth noting here is that, with this change, we are left with an
> > awkward situation where two function names related to checking changes in cdclk:
> > 
> >   intel_cdclk_params_changed() and intel_cdclk_changed()
> > 
> > ,
> > 
> > and I find it weird that we have intel_cdclk_changed(), which checks for the
> > voltage level as well. Shouldn't the voltage level be a function of cdclk and
> > ddi clock? Why do we need that?
> > 
> >  drivers/gpu/drm/i915/display/intel_cdclk.c        | 15 +++++++--------
> >  drivers/gpu/drm/i915/display/intel_cdclk.h        |  4 ++--
> >  .../drm/i915/display/intel_display_power_well.c   |  4 ++--
> >  3 files changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 26200ee3e23f..caadd880865f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
> >  }
> >  
> >  /**
> > - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
> > - *                             configurations requires a modeset on all pipes
> > + * intel_cdclk_params_changed - Check whether CDCLK parameters changed
> >   * @a: first CDCLK configuration
> >   * @b: second CDCLK configuration
> >   *
> >   * Returns:
> > - * True if changing between the two CDCLK configurations
> > - * requires all pipes to be off, false if not.
> > + * True if parameters changed in a way that requires programming the CDCLK
> > + * and False otherwise.
> >   */
> > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
> > -			       const struct intel_cdclk_config *b)
> > +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
> > +				const struct intel_cdclk_config *b)
> 
> The new name isn't very descriptive either.
> 
> Outside the cd2x/crawl/squash cases we stil have to consider
> two cases:
> 1. cdclk frequency/pll changes (voltage level can change or not)
> 2. cdclk frequency/pll doesn't change, but voltage level needs to change
> 
> And that difference is what intel_cdclk_needs_modeset() is trying
> convey. And intel_cdclk_changed() tells us whether anything at all
> is changing.

intel_cdclk_clock_changed() is perhaps the best name I can
think of, off the top of my head. But I'm notoriously bad at
naming things so take that with a pinch of salt.
Gustavo Sousa Feb. 3, 2024, 1:25 p.m. UTC | #3
Quoting Ville Syrjälä (2024-02-02 16:58:37-03:00)
>On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote:
>> Looks like the name and description of intel_cdclk_needs_modeset()
>> became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for
>> cdclk changes if possible"), when it became possible to update the cdclk
>> without requiring disabling the pipes when only changing the cd2x
>> divider was enough.
>> 
>> Later on we also added the same type of support with squash and crawling
>> with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash
>> when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk
>> squasher to be reconfigured live") and commit d62686ba3b54
>> ("drm/i915/adl_p: CDCLK crawl support for ADL").
>> 
>> As such, update that function's name and documentation to something more
>> appropriate, since the real checks for requiring modeset are done
>> elsewhere.
>> 
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>> 
>> One thing worth noting here is that, with this change, we are left with an
>> awkward situation where two function names related to checking changes in cdclk:
>> 
>>   intel_cdclk_params_changed() and intel_cdclk_changed()
>> 
>> ,
>> 
>> and I find it weird that we have intel_cdclk_changed(), which checks for the
>> voltage level as well. Shouldn't the voltage level be a function of cdclk and
>> ddi clock? Why do we need that?
>> 
>>  drivers/gpu/drm/i915/display/intel_cdclk.c        | 15 +++++++--------
>>  drivers/gpu/drm/i915/display/intel_cdclk.h        |  4 ++--
>>  .../drm/i915/display/intel_display_power_well.c   |  4 ++--
>>  3 files changed, 11 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> index 26200ee3e23f..caadd880865f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
>>  }
>>  
>>  /**
>> - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
>> - *                             configurations requires a modeset on all pipes
>> + * intel_cdclk_params_changed - Check whether CDCLK parameters changed
>>   * @a: first CDCLK configuration
>>   * @b: second CDCLK configuration
>>   *
>>   * Returns:
>> - * True if changing between the two CDCLK configurations
>> - * requires all pipes to be off, false if not.
>> + * True if parameters changed in a way that requires programming the CDCLK
>> + * and False otherwise.
>>   */
>> -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
>> -                               const struct intel_cdclk_config *b)
>> +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
>> +                                const struct intel_cdclk_config *b)
>
>The new name isn't very descriptive either.

Yeah... I would much rather use intel_cdclk_changed(), but that one is
already taken.

>
>Outside the cd2x/crawl/squash cases we stil have to consider
>two cases:
>1. cdclk frequency/pll changes (voltage level can change or not)
>2. cdclk frequency/pll doesn't change, but voltage level needs to change
>
>And that difference is what intel_cdclk_needs_modeset() is trying
>convey. And intel_cdclk_changed() tells us whether anything at all
>is changing.

I might be missing something, but, by going through the specs, it looked
to me that voltage level was dependent on cdclk (as well as on ddi
clock) and not the other way around. That's why I find it odd that we
need an intel_cdclk_changed() that, besides looking for changes in
cdclk, also checks for the voltage level.

In intel_set_cdclk(), we check intel_cdclk_changed() before continuing.
If, for example, there is a change in ddi clock that requires a change
in voltage level but no changes in cdclk, intel_cdclk_changed() would
return true, right? Wouldn't that make us unnecessarily go through
intel_set_cdclk()?

--
Gustavo Sousa
Gustavo Sousa Feb. 3, 2024, 1:32 p.m. UTC | #4
Quoting Ville Syrjälä (2024-02-02 17:06:02-03:00)
>On Fri, Feb 02, 2024 at 09:58:37PM +0200, Ville Syrjälä wrote:
>> On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote:
>> > Looks like the name and description of intel_cdclk_needs_modeset()
>> > became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for
>> > cdclk changes if possible"), when it became possible to update the cdclk
>> > without requiring disabling the pipes when only changing the cd2x
>> > divider was enough.
>> > 
>> > Later on we also added the same type of support with squash and crawling
>> > with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash
>> > when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk
>> > squasher to be reconfigured live") and commit d62686ba3b54
>> > ("drm/i915/adl_p: CDCLK crawl support for ADL").
>> > 
>> > As such, update that function's name and documentation to something more
>> > appropriate, since the real checks for requiring modeset are done
>> > elsewhere.
>> > 
>> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> > ---
>> > 
>> > One thing worth noting here is that, with this change, we are left with an
>> > awkward situation where two function names related to checking changes in cdclk:
>> > 
>> >   intel_cdclk_params_changed() and intel_cdclk_changed()
>> > 
>> > ,
>> > 
>> > and I find it weird that we have intel_cdclk_changed(), which checks for the
>> > voltage level as well. Shouldn't the voltage level be a function of cdclk and
>> > ddi clock? Why do we need that?
>> > 
>> >  drivers/gpu/drm/i915/display/intel_cdclk.c        | 15 +++++++--------
>> >  drivers/gpu/drm/i915/display/intel_cdclk.h        |  4 ++--
>> >  .../drm/i915/display/intel_display_power_well.c   |  4 ++--
>> >  3 files changed, 11 insertions(+), 12 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > index 26200ee3e23f..caadd880865f 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
>> >  }
>> >  
>> >  /**
>> > - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
>> > - *                             configurations requires a modeset on all pipes
>> > + * intel_cdclk_params_changed - Check whether CDCLK parameters changed
>> >   * @a: first CDCLK configuration
>> >   * @b: second CDCLK configuration
>> >   *
>> >   * Returns:
>> > - * True if changing between the two CDCLK configurations
>> > - * requires all pipes to be off, false if not.
>> > + * True if parameters changed in a way that requires programming the CDCLK
>> > + * and False otherwise.
>> >   */
>> > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
>> > -                               const struct intel_cdclk_config *b)
>> > +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
>> > +                                const struct intel_cdclk_config *b)
>> 
>> The new name isn't very descriptive either.
>> 
>> Outside the cd2x/crawl/squash cases we stil have to consider
>> two cases:
>> 1. cdclk frequency/pll changes (voltage level can change or not)
>> 2. cdclk frequency/pll doesn't change, but voltage level needs to change
>> 
>> And that difference is what intel_cdclk_needs_modeset() is trying
>> convey. And intel_cdclk_changed() tells us whether anything at all
>> is changing.
>
>intel_cdclk_clock_changed() is perhaps the best name I can
>think of, off the top of my head. But I'm notoriously bad at
>naming things so take that with a pinch of salt.

Sounds better indeed. And I think intel_cdclk_changed() would be more
concise, but that one is already taken internally.

--
Gustavo Sousa
Ville Syrjala Feb. 5, 2024, 3:34 p.m. UTC | #5
On Sat, Feb 03, 2024 at 10:25:18AM -0300, Gustavo Sousa wrote:
> Quoting Ville Syrjälä (2024-02-02 16:58:37-03:00)
> >On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote:
> >> Looks like the name and description of intel_cdclk_needs_modeset()
> >> became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for
> >> cdclk changes if possible"), when it became possible to update the cdclk
> >> without requiring disabling the pipes when only changing the cd2x
> >> divider was enough.
> >> 
> >> Later on we also added the same type of support with squash and crawling
> >> with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash
> >> when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk
> >> squasher to be reconfigured live") and commit d62686ba3b54
> >> ("drm/i915/adl_p: CDCLK crawl support for ADL").
> >> 
> >> As such, update that function's name and documentation to something more
> >> appropriate, since the real checks for requiring modeset are done
> >> elsewhere.
> >> 
> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> >> ---
> >> 
> >> One thing worth noting here is that, with this change, we are left with an
> >> awkward situation where two function names related to checking changes in cdclk:
> >> 
> >>   intel_cdclk_params_changed() and intel_cdclk_changed()
> >> 
> >> ,
> >> 
> >> and I find it weird that we have intel_cdclk_changed(), which checks for the
> >> voltage level as well. Shouldn't the voltage level be a function of cdclk and
> >> ddi clock? Why do we need that?
> >> 
> >>  drivers/gpu/drm/i915/display/intel_cdclk.c        | 15 +++++++--------
> >>  drivers/gpu/drm/i915/display/intel_cdclk.h        |  4 ++--
> >>  .../drm/i915/display/intel_display_power_well.c   |  4 ++--
> >>  3 files changed, 11 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> index 26200ee3e23f..caadd880865f 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
> >>  }
> >>  
> >>  /**
> >> - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
> >> - *                             configurations requires a modeset on all pipes
> >> + * intel_cdclk_params_changed - Check whether CDCLK parameters changed
> >>   * @a: first CDCLK configuration
> >>   * @b: second CDCLK configuration
> >>   *
> >>   * Returns:
> >> - * True if changing between the two CDCLK configurations
> >> - * requires all pipes to be off, false if not.
> >> + * True if parameters changed in a way that requires programming the CDCLK
> >> + * and False otherwise.
> >>   */
> >> -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
> >> -                               const struct intel_cdclk_config *b)
> >> +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
> >> +                                const struct intel_cdclk_config *b)
> >
> >The new name isn't very descriptive either.
> 
> Yeah... I would much rather use intel_cdclk_changed(), but that one is
> already taken.
> 
> >
> >Outside the cd2x/crawl/squash cases we stil have to consider
> >two cases:
> >1. cdclk frequency/pll changes (voltage level can change or not)
> >2. cdclk frequency/pll doesn't change, but voltage level needs to change
> >
> >And that difference is what intel_cdclk_needs_modeset() is trying
> >convey. And intel_cdclk_changed() tells us whether anything at all
> >is changing.
> 
> I might be missing something, but, by going through the specs, it looked
> to me that voltage level was dependent on cdclk (as well as on ddi
> clock) and not the other way around. That's why I find it odd that we
> need an intel_cdclk_changed() that, besides looking for changes in
> cdclk, also checks for the voltage level.
> 
> In intel_set_cdclk(), we check intel_cdclk_changed() before continuing.
> If, for example, there is a change in ddi clock that requires a change
> in voltage level but no changes in cdclk, intel_cdclk_changed() would
> return true, right? Wouldn't that make us unnecessarily go through
> intel_set_cdclk()?

intel_set_cdclk() is the thing that does the voltage change.
Gustavo Sousa Feb. 14, 2024, 7:56 p.m. UTC | #6
Hi, Ville.

Sorry for taking long to get back to this.

Quoting Ville Syrjälä (2024-02-05 12:34:57-03:00)
>On Sat, Feb 03, 2024 at 10:25:18AM -0300, Gustavo Sousa wrote:
>> Quoting Ville Syrjälä (2024-02-02 16:58:37-03:00)
>> >On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote:
>> >> Looks like the name and description of intel_cdclk_needs_modeset()
>> >> became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for
>> >> cdclk changes if possible"), when it became possible to update the cdclk
>> >> without requiring disabling the pipes when only changing the cd2x
>> >> divider was enough.
>> >> 
>> >> Later on we also added the same type of support with squash and crawling
>> >> with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash
>> >> when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk
>> >> squasher to be reconfigured live") and commit d62686ba3b54
>> >> ("drm/i915/adl_p: CDCLK crawl support for ADL").
>> >> 
>> >> As such, update that function's name and documentation to something more
>> >> appropriate, since the real checks for requiring modeset are done
>> >> elsewhere.
>> >> 
>> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> >> ---
>> >> 
>> >> One thing worth noting here is that, with this change, we are left with an
>> >> awkward situation where two function names related to checking changes in cdclk:
>> >> 
>> >>   intel_cdclk_params_changed() and intel_cdclk_changed()
>> >> 
>> >> ,
>> >> 
>> >> and I find it weird that we have intel_cdclk_changed(), which checks for the
>> >> voltage level as well. Shouldn't the voltage level be a function of cdclk and
>> >> ddi clock? Why do we need that?
>> >> 
>> >>  drivers/gpu/drm/i915/display/intel_cdclk.c        | 15 +++++++--------
>> >>  drivers/gpu/drm/i915/display/intel_cdclk.h        |  4 ++--
>> >>  .../drm/i915/display/intel_display_power_well.c   |  4 ++--
>> >>  3 files changed, 11 insertions(+), 12 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >> index 26200ee3e23f..caadd880865f 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >> @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
>> >>  }
>> >>  
>> >>  /**
>> >> - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
>> >> - *                             configurations requires a modeset on all pipes
>> >> + * intel_cdclk_params_changed - Check whether CDCLK parameters changed
>> >>   * @a: first CDCLK configuration
>> >>   * @b: second CDCLK configuration
>> >>   *
>> >>   * Returns:
>> >> - * True if changing between the two CDCLK configurations
>> >> - * requires all pipes to be off, false if not.
>> >> + * True if parameters changed in a way that requires programming the CDCLK
>> >> + * and False otherwise.
>> >>   */
>> >> -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
>> >> -                               const struct intel_cdclk_config *b)
>> >> +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
>> >> +                                const struct intel_cdclk_config *b)
>> >
>> >The new name isn't very descriptive either.
>> 
>> Yeah... I would much rather use intel_cdclk_changed(), but that one is
>> already taken.
>> 
>> >
>> >Outside the cd2x/crawl/squash cases we stil have to consider
>> >two cases:
>> >1. cdclk frequency/pll changes (voltage level can change or not)
>> >2. cdclk frequency/pll doesn't change, but voltage level needs to change
>> >
>> >And that difference is what intel_cdclk_needs_modeset() is trying
>> >convey. And intel_cdclk_changed() tells us whether anything at all
>> >is changing.
>> 
>> I might be missing something, but, by going through the specs, it looked
>> to me that voltage level was dependent on cdclk (as well as on ddi
>> clock) and not the other way around. That's why I find it odd that we
>> need an intel_cdclk_changed() that, besides looking for changes in
>> cdclk, also checks for the voltage level.
>> 
>> In intel_set_cdclk(), we check intel_cdclk_changed() before continuing.
>> If, for example, there is a change in ddi clock that requires a change
>> in voltage level but no changes in cdclk, intel_cdclk_changed() would
>> return true, right? Wouldn't that make us unnecessarily go through
>> intel_set_cdclk()?
>
>intel_set_cdclk() is the thing that does the voltage change.

Yep and perhaps I provided an incomplete response above. Sorry.

I was wondering if handling voltage level should really be
intel_set_cdclk()'s responsibility.

I might be missing the big picture here, but, at least for the recent
platforms, I get the understanding that voltage level handling should be
a separate step in the hardware commit process.

Would it be possible to have a commit containing (i) update(s) to ddi
clk and (ii) no update to cdclk such that (i) require an update to
voltage level, right?

--
Gustavo Sousa
Ville Syrjala Feb. 14, 2024, 8:08 p.m. UTC | #7
On Wed, Feb 14, 2024 at 04:56:50PM -0300, Gustavo Sousa wrote:
> Hi, Ville.
> 
> Sorry for taking long to get back to this.
> 
> Quoting Ville Syrjälä (2024-02-05 12:34:57-03:00)
> >On Sat, Feb 03, 2024 at 10:25:18AM -0300, Gustavo Sousa wrote:
> >> Quoting Ville Syrjälä (2024-02-02 16:58:37-03:00)
> >> >On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote:
> >> >> Looks like the name and description of intel_cdclk_needs_modeset()
> >> >> became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for
> >> >> cdclk changes if possible"), when it became possible to update the cdclk
> >> >> without requiring disabling the pipes when only changing the cd2x
> >> >> divider was enough.
> >> >> 
> >> >> Later on we also added the same type of support with squash and crawling
> >> >> with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash
> >> >> when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk
> >> >> squasher to be reconfigured live") and commit d62686ba3b54
> >> >> ("drm/i915/adl_p: CDCLK crawl support for ADL").
> >> >> 
> >> >> As such, update that function's name and documentation to something more
> >> >> appropriate, since the real checks for requiring modeset are done
> >> >> elsewhere.
> >> >> 
> >> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> >> >> ---
> >> >> 
> >> >> One thing worth noting here is that, with this change, we are left with an
> >> >> awkward situation where two function names related to checking changes in cdclk:
> >> >> 
> >> >>   intel_cdclk_params_changed() and intel_cdclk_changed()
> >> >> 
> >> >> ,
> >> >> 
> >> >> and I find it weird that we have intel_cdclk_changed(), which checks for the
> >> >> voltage level as well. Shouldn't the voltage level be a function of cdclk and
> >> >> ddi clock? Why do we need that?
> >> >> 
> >> >>  drivers/gpu/drm/i915/display/intel_cdclk.c        | 15 +++++++--------
> >> >>  drivers/gpu/drm/i915/display/intel_cdclk.h        |  4 ++--
> >> >>  .../drm/i915/display/intel_display_power_well.c   |  4 ++--
> >> >>  3 files changed, 11 insertions(+), 12 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> >> index 26200ee3e23f..caadd880865f 100644
> >> >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> >> @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
> >> >>  }
> >> >>  
> >> >>  /**
> >> >> - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
> >> >> - *                             configurations requires a modeset on all pipes
> >> >> + * intel_cdclk_params_changed - Check whether CDCLK parameters changed
> >> >>   * @a: first CDCLK configuration
> >> >>   * @b: second CDCLK configuration
> >> >>   *
> >> >>   * Returns:
> >> >> - * True if changing between the two CDCLK configurations
> >> >> - * requires all pipes to be off, false if not.
> >> >> + * True if parameters changed in a way that requires programming the CDCLK
> >> >> + * and False otherwise.
> >> >>   */
> >> >> -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
> >> >> -                               const struct intel_cdclk_config *b)
> >> >> +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
> >> >> +                                const struct intel_cdclk_config *b)
> >> >
> >> >The new name isn't very descriptive either.
> >> 
> >> Yeah... I would much rather use intel_cdclk_changed(), but that one is
> >> already taken.
> >> 
> >> >
> >> >Outside the cd2x/crawl/squash cases we stil have to consider
> >> >two cases:
> >> >1. cdclk frequency/pll changes (voltage level can change or not)
> >> >2. cdclk frequency/pll doesn't change, but voltage level needs to change
> >> >
> >> >And that difference is what intel_cdclk_needs_modeset() is trying
> >> >convey. And intel_cdclk_changed() tells us whether anything at all
> >> >is changing.
> >> 
> >> I might be missing something, but, by going through the specs, it looked
> >> to me that voltage level was dependent on cdclk (as well as on ddi
> >> clock) and not the other way around. That's why I find it odd that we
> >> need an intel_cdclk_changed() that, besides looking for changes in
> >> cdclk, also checks for the voltage level.
> >> 
> >> In intel_set_cdclk(), we check intel_cdclk_changed() before continuing.
> >> If, for example, there is a change in ddi clock that requires a change
> >> in voltage level but no changes in cdclk, intel_cdclk_changed() would
> >> return true, right? Wouldn't that make us unnecessarily go through
> >> intel_set_cdclk()?
> >
> >intel_set_cdclk() is the thing that does the voltage change.
> 
> Yep and perhaps I provided an incomplete response above. Sorry.
> 
> I was wondering if handling voltage level should really be
> intel_set_cdclk()'s responsibility.
> 
> I might be missing the big picture here, but, at least for the recent
> platforms, I get the understanding that voltage level handling should be
> a separate step in the hardware commit process.
> 
> Would it be possible to have a commit containing (i) update(s) to ddi
> clk and (ii) no update to cdclk such that (i) require an update to
> voltage level, right?

That is possible yes. But I don't think there's much
point in complicating things by splitting the voltage
level stuff into a completely separate thing.

What I think we could do is split .set_cdclk() into more
fine grained steps so that:
- it's easier to reuse individual pieces across platforms without
  ugly if ladders
- perhaps make it a bit easier to skip unnecessary steps
  although the actual cdclk progrmaming in the nop case really
  just ends up being a CDLCK_CTL/etc. register write
  so not a big deal in practice.
Gustavo Sousa Feb. 14, 2024, 8:08 p.m. UTC | #8
Quoting Gustavo Sousa (2024-02-03 10:32:18-03:00)
>Quoting Ville Syrjälä (2024-02-02 17:06:02-03:00)
>>On Fri, Feb 02, 2024 at 09:58:37PM +0200, Ville Syrjälä wrote:
>>> On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote:
>>> > Looks like the name and description of intel_cdclk_needs_modeset()
>>> > became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for
>>> > cdclk changes if possible"), when it became possible to update the cdclk
>>> > without requiring disabling the pipes when only changing the cd2x
>>> > divider was enough.
>>> > 
>>> > Later on we also added the same type of support with squash and crawling
>>> > with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash
>>> > when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk
>>> > squasher to be reconfigured live") and commit d62686ba3b54
>>> > ("drm/i915/adl_p: CDCLK crawl support for ADL").
>>> > 
>>> > As such, update that function's name and documentation to something more
>>> > appropriate, since the real checks for requiring modeset are done
>>> > elsewhere.
>>> > 
>>> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>> > ---
>>> > 
>>> > One thing worth noting here is that, with this change, we are left with an
>>> > awkward situation where two function names related to checking changes in cdclk:
>>> > 
>>> >   intel_cdclk_params_changed() and intel_cdclk_changed()
>>> > 
>>> > ,
>>> > 
>>> > and I find it weird that we have intel_cdclk_changed(), which checks for the
>>> > voltage level as well. Shouldn't the voltage level be a function of cdclk and
>>> > ddi clock? Why do we need that?
>>> > 
>>> >  drivers/gpu/drm/i915/display/intel_cdclk.c        | 15 +++++++--------
>>> >  drivers/gpu/drm/i915/display/intel_cdclk.h        |  4 ++--
>>> >  .../drm/i915/display/intel_display_power_well.c   |  4 ++--
>>> >  3 files changed, 11 insertions(+), 12 deletions(-)
>>> > 
>>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>>> > index 26200ee3e23f..caadd880865f 100644
>>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>>> > @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
>>> >  }
>>> >  
>>> >  /**
>>> > - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
>>> > - *                             configurations requires a modeset on all pipes
>>> > + * intel_cdclk_params_changed - Check whether CDCLK parameters changed
>>> >   * @a: first CDCLK configuration
>>> >   * @b: second CDCLK configuration
>>> >   *
>>> >   * Returns:
>>> > - * True if changing between the two CDCLK configurations
>>> > - * requires all pipes to be off, false if not.
>>> > + * True if parameters changed in a way that requires programming the CDCLK
>>> > + * and False otherwise.
>>> >   */
>>> > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
>>> > -                               const struct intel_cdclk_config *b)
>>> > +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
>>> > +                                const struct intel_cdclk_config *b)
>>> 
>>> The new name isn't very descriptive either.
>>> 
>>> Outside the cd2x/crawl/squash cases we stil have to consider
>>> two cases:
>>> 1. cdclk frequency/pll changes (voltage level can change or not)
>>> 2. cdclk frequency/pll doesn't change, but voltage level needs to change
>>> 
>>> And that difference is what intel_cdclk_needs_modeset() is trying
>>> convey. And intel_cdclk_changed() tells us whether anything at all
>>> is changing.
>>
>>intel_cdclk_clock_changed() is perhaps the best name I can
>>think of, off the top of my head. But I'm notoriously bad at
>>naming things so take that with a pinch of salt.
>
>Sounds better indeed. And I think intel_cdclk_changed() would be more
>concise, but that one is already taken internally.

Hm... On second thought, I'm not sure intel_cdclk_clock_changed() would
be very accurate, at least with the current implementation. We are
checking not only on the resulting cdclk frequency, but also on
ref and vco values.

Although I'm not sure if that could happen in real life, but the current
implementation gives me the interpretation that, in theory, a change in
ref or vco could happen even if the cdclk value did not change. Or are
we safe to say that checking on cdclk should be enough and looking at
vco and ref is actually redundant?

--
Gustavo Sousa
Ville Syrjala Feb. 14, 2024, 8:15 p.m. UTC | #9
On Wed, Feb 14, 2024 at 05:08:48PM -0300, Gustavo Sousa wrote:
> Quoting Gustavo Sousa (2024-02-03 10:32:18-03:00)
> >Quoting Ville Syrjälä (2024-02-02 17:06:02-03:00)
> >>On Fri, Feb 02, 2024 at 09:58:37PM +0200, Ville Syrjälä wrote:
> >>> On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote:
> >>> > Looks like the name and description of intel_cdclk_needs_modeset()
> >>> > became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for
> >>> > cdclk changes if possible"), when it became possible to update the cdclk
> >>> > without requiring disabling the pipes when only changing the cd2x
> >>> > divider was enough.
> >>> > 
> >>> > Later on we also added the same type of support with squash and crawling
> >>> > with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash
> >>> > when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk
> >>> > squasher to be reconfigured live") and commit d62686ba3b54
> >>> > ("drm/i915/adl_p: CDCLK crawl support for ADL").
> >>> > 
> >>> > As such, update that function's name and documentation to something more
> >>> > appropriate, since the real checks for requiring modeset are done
> >>> > elsewhere.
> >>> > 
> >>> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> >>> > ---
> >>> > 
> >>> > One thing worth noting here is that, with this change, we are left with an
> >>> > awkward situation where two function names related to checking changes in cdclk:
> >>> > 
> >>> >   intel_cdclk_params_changed() and intel_cdclk_changed()
> >>> > 
> >>> > ,
> >>> > 
> >>> > and I find it weird that we have intel_cdclk_changed(), which checks for the
> >>> > voltage level as well. Shouldn't the voltage level be a function of cdclk and
> >>> > ddi clock? Why do we need that?
> >>> > 
> >>> >  drivers/gpu/drm/i915/display/intel_cdclk.c        | 15 +++++++--------
> >>> >  drivers/gpu/drm/i915/display/intel_cdclk.h        |  4 ++--
> >>> >  .../drm/i915/display/intel_display_power_well.c   |  4 ++--
> >>> >  3 files changed, 11 insertions(+), 12 deletions(-)
> >>> > 
> >>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >>> > index 26200ee3e23f..caadd880865f 100644
> >>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >>> > @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
> >>> >  }
> >>> >  
> >>> >  /**
> >>> > - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
> >>> > - *                             configurations requires a modeset on all pipes
> >>> > + * intel_cdclk_params_changed - Check whether CDCLK parameters changed
> >>> >   * @a: first CDCLK configuration
> >>> >   * @b: second CDCLK configuration
> >>> >   *
> >>> >   * Returns:
> >>> > - * True if changing between the two CDCLK configurations
> >>> > - * requires all pipes to be off, false if not.
> >>> > + * True if parameters changed in a way that requires programming the CDCLK
> >>> > + * and False otherwise.
> >>> >   */
> >>> > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
> >>> > -                               const struct intel_cdclk_config *b)
> >>> > +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
> >>> > +                                const struct intel_cdclk_config *b)
> >>> 
> >>> The new name isn't very descriptive either.
> >>> 
> >>> Outside the cd2x/crawl/squash cases we stil have to consider
> >>> two cases:
> >>> 1. cdclk frequency/pll changes (voltage level can change or not)
> >>> 2. cdclk frequency/pll doesn't change, but voltage level needs to change
> >>> 
> >>> And that difference is what intel_cdclk_needs_modeset() is trying
> >>> convey. And intel_cdclk_changed() tells us whether anything at all
> >>> is changing.
> >>
> >>intel_cdclk_clock_changed() is perhaps the best name I can
> >>think of, off the top of my head. But I'm notoriously bad at
> >>naming things so take that with a pinch of salt.
> >
> >Sounds better indeed. And I think intel_cdclk_changed() would be more
> >concise, but that one is already taken internally.
> 
> Hm... On second thought, I'm not sure intel_cdclk_clock_changed() would
> be very accurate, at least with the current implementation. We are
> checking not only on the resulting cdclk frequency, but also on
> ref and vco values.

That's all about the clock.

> 
> Although I'm not sure if that could happen in real life, but the current
> implementation gives me the interpretation that, in theory, a change in
> ref or vco could happen even if the cdclk value did not change. Or are
> we safe to say that checking on cdclk should be enough and looking at
> vco and ref is actually redundant?

ref never changes, vco could in theory change such that the
resulting cdclk stays the same, but probably no real world
examples of that.

But I don't see any point in putting those kinds of assumptions
into the code given that it wouldn't provide any actual benefits.
Gustavo Sousa Feb. 14, 2024, 8:30 p.m. UTC | #10
Quoting Ville Syrjälä (2024-02-14 17:15:51-03:00)
>On Wed, Feb 14, 2024 at 05:08:48PM -0300, Gustavo Sousa wrote:
>> Quoting Gustavo Sousa (2024-02-03 10:32:18-03:00)
>> >Quoting Ville Syrjälä (2024-02-02 17:06:02-03:00)
>> >>On Fri, Feb 02, 2024 at 09:58:37PM +0200, Ville Syrjälä wrote:
>> >>> On Fri, Feb 02, 2024 at 10:12:08AM -0300, Gustavo Sousa wrote:
>> >>> > Looks like the name and description of intel_cdclk_needs_modeset()
>> >>> > became inacurate as of commit 59f9e9cab3a1 ("drm/i915: Skip modeset for
>> >>> > cdclk changes if possible"), when it became possible to update the cdclk
>> >>> > without requiring disabling the pipes when only changing the cd2x
>> >>> > divider was enough.
>> >>> > 
>> >>> > Later on we also added the same type of support with squash and crawling
>> >>> > with commit 25e0e5ae5610 ("drm/i915/display: Do both crawl and squash
>> >>> > when changing cdclk"), commit d4a23930490d ("drm/i915: Allow cdclk
>> >>> > squasher to be reconfigured live") and commit d62686ba3b54
>> >>> > ("drm/i915/adl_p: CDCLK crawl support for ADL").
>> >>> > 
>> >>> > As such, update that function's name and documentation to something more
>> >>> > appropriate, since the real checks for requiring modeset are done
>> >>> > elsewhere.
>> >>> > 
>> >>> > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> >>> > ---
>> >>> > 
>> >>> > One thing worth noting here is that, with this change, we are left with an
>> >>> > awkward situation where two function names related to checking changes in cdclk:
>> >>> > 
>> >>> >   intel_cdclk_params_changed() and intel_cdclk_changed()
>> >>> > 
>> >>> > ,
>> >>> > 
>> >>> > and I find it weird that we have intel_cdclk_changed(), which checks for the
>> >>> > voltage level as well. Shouldn't the voltage level be a function of cdclk and
>> >>> > ddi clock? Why do we need that?
>> >>> > 
>> >>> >  drivers/gpu/drm/i915/display/intel_cdclk.c        | 15 +++++++--------
>> >>> >  drivers/gpu/drm/i915/display/intel_cdclk.h        |  4 ++--
>> >>> >  .../drm/i915/display/intel_display_power_well.c   |  4 ++--
>> >>> >  3 files changed, 11 insertions(+), 12 deletions(-)
>> >>> > 
>> >>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >>> > index 26200ee3e23f..caadd880865f 100644
>> >>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >>> > @@ -2233,17 +2233,16 @@ static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
>> >>> >  }
>> >>> >  
>> >>> >  /**
>> >>> > - * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
>> >>> > - *                             configurations requires a modeset on all pipes
>> >>> > + * intel_cdclk_params_changed - Check whether CDCLK parameters changed
>> >>> >   * @a: first CDCLK configuration
>> >>> >   * @b: second CDCLK configuration
>> >>> >   *
>> >>> >   * Returns:
>> >>> > - * True if changing between the two CDCLK configurations
>> >>> > - * requires all pipes to be off, false if not.
>> >>> > + * True if parameters changed in a way that requires programming the CDCLK
>> >>> > + * and False otherwise.
>> >>> >   */
>> >>> > -bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
>> >>> > -                               const struct intel_cdclk_config *b)
>> >>> > +bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
>> >>> > +                                const struct intel_cdclk_config *b)
>> >>> 
>> >>> The new name isn't very descriptive either.
>> >>> 
>> >>> Outside the cd2x/crawl/squash cases we stil have to consider
>> >>> two cases:
>> >>> 1. cdclk frequency/pll changes (voltage level can change or not)
>> >>> 2. cdclk frequency/pll doesn't change, but voltage level needs to change
>> >>> 
>> >>> And that difference is what intel_cdclk_needs_modeset() is trying
>> >>> convey. And intel_cdclk_changed() tells us whether anything at all
>> >>> is changing.
>> >>
>> >>intel_cdclk_clock_changed() is perhaps the best name I can
>> >>think of, off the top of my head. But I'm notoriously bad at
>> >>naming things so take that with a pinch of salt.
>> >
>> >Sounds better indeed. And I think intel_cdclk_changed() would be more
>> >concise, but that one is already taken internally.
>> 
>> Hm... On second thought, I'm not sure intel_cdclk_clock_changed() would
>> be very accurate, at least with the current implementation. We are
>> checking not only on the resulting cdclk frequency, but also on
>> ref and vco values.
>
>That's all about the clock.
>
>> 
>> Although I'm not sure if that could happen in real life, but the current
>> implementation gives me the interpretation that, in theory, a change in
>> ref or vco could happen even if the cdclk value did not change. Or are
>> we safe to say that checking on cdclk should be enough and looking at
>> vco and ref is actually redundant?
>
>ref never changes, vco could in theory change such that the
>resulting cdclk stays the same, but probably no real world
>examples of that.
>
>But I don't see any point in putting those kinds of assumptions
>into the code given that it wouldn't provide any actual benefits.

Alright. Thanks!

I've sent a v2 using the proposed name:
https://patchwork.freedesktop.org/series/129908/

--
Gustavo Sousa
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 26200ee3e23f..caadd880865f 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -2233,17 +2233,16 @@  static bool intel_cdclk_can_squash(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_cdclk_needs_modeset - Determine if changong between the CDCLK
- *                             configurations requires a modeset on all pipes
+ * intel_cdclk_params_changed - Check whether CDCLK parameters changed
  * @a: first CDCLK configuration
  * @b: second CDCLK configuration
  *
  * Returns:
- * True if changing between the two CDCLK configurations
- * requires all pipes to be off, false if not.
+ * True if parameters changed in a way that requires programming the CDCLK
+ * and False otherwise.
  */
-bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
-			       const struct intel_cdclk_config *b)
+bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
+				const struct intel_cdclk_config *b)
 {
 	return a->cdclk != b->cdclk ||
 		a->vco != b->vco ||
@@ -2295,7 +2294,7 @@  static bool intel_cdclk_can_cd2x_update(struct drm_i915_private *dev_priv,
 static bool intel_cdclk_changed(const struct intel_cdclk_config *a,
 				const struct intel_cdclk_config *b)
 {
-	return intel_cdclk_needs_modeset(a, b) ||
+	return intel_cdclk_params_changed(a, b) ||
 		a->voltage_level != b->voltage_level;
 }
 
@@ -3202,7 +3201,7 @@  int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
 		drm_dbg_kms(&dev_priv->drm,
 			    "Can change cdclk cd2x divider with pipe %c active\n",
 			    pipe_name(pipe));
-	} else if (intel_cdclk_needs_modeset(&old_cdclk_state->actual,
+	} else if (intel_cdclk_params_changed(&old_cdclk_state->actual,
 					     &new_cdclk_state->actual)) {
 		/* All pipes must be switched off while we change the cdclk. */
 		ret = intel_modeset_all_pipes_late(state, "CDCLK change");
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
index 48fd7d39e0cd..f8d3b2b3be6a 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
@@ -60,8 +60,8 @@  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
 void intel_update_max_cdclk(struct drm_i915_private *dev_priv);
 void intel_update_cdclk(struct drm_i915_private *dev_priv);
 u32 intel_read_rawclk(struct drm_i915_private *dev_priv);
-bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
-			       const struct intel_cdclk_config *b);
+bool intel_cdclk_params_changed(const struct intel_cdclk_config *a,
+				const struct intel_cdclk_config *b);
 void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state);
 void intel_set_cdclk_post_plane_update(struct intel_atomic_state *state);
 void intel_cdclk_dump_config(struct drm_i915_private *i915,
diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
index 47cd6bb04366..7364ca520764 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
@@ -968,8 +968,8 @@  void gen9_disable_dc_states(struct drm_i915_private *dev_priv)
 	intel_cdclk_get_cdclk(dev_priv, &cdclk_config);
 	/* Can't read out voltage_level so can't use intel_cdclk_changed() */
 	drm_WARN_ON(&dev_priv->drm,
-		    intel_cdclk_needs_modeset(&dev_priv->display.cdclk.hw,
-					      &cdclk_config));
+		    intel_cdclk_params_changed(&dev_priv->display.cdclk.hw,
+					       &cdclk_config));
 
 	gen9_assert_dbuf_enabled(dev_priv);