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@xxxxxxxxx>
> ---
>
> 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
