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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel