[PATCH] drm/i915/cdclk: Rename intel_cdclk_needs_modeset to intel_cdclk_params_changed

[Gustavo Sousa]

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 at 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