Re: [PATCH 15/16] drm/i915/cdclk: abstract intel_cdclk_actual() and intel_cdclk_actual_voltage_level()

[Imre Deak <imre.deak@intel.com>]

On Thu, Jun 19, 2025 at 01:11:32PM +0300, Jani Nikula wrote:
> On Wed, 18 Jun 2025, Imre Deak <imre.deak@intel.com> wrote:
> > On Thu, Jun 12, 2025 at 03:12:10PM +0300, Jani Nikula wrote:
> >> Add intel_cdclk_actual() and intel_cdclk_actual_voltage_level() helpers
> >> to avoid looking at struct intel_cdclk_state internals outside of
> >> intel_cdclk.c.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_cdclk.c    | 10 ++++++++++
> >>  drivers/gpu/drm/i915/display/intel_cdclk.h    |  2 ++
> >>  drivers/gpu/drm/i915/display/intel_pmdemand.c |  4 ++--
> >>  3 files changed, 14 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> index 994be1d0e20c..2e8abf237bd1 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> @@ -3884,3 +3884,13 @@ void intel_cdclk_read_hw(struct intel_display *display)
> >>  	cdclk_state->actual = display->cdclk.hw;
> >>  	cdclk_state->logical = display->cdclk.hw;
> >>  }
> >> +
> >> +int intel_cdclk_actual(const struct intel_cdclk_state *cdclk_state)
> >> +{
> >> +	return cdclk_state->actual.cdclk;
> >> +}
> >> +
> >> +int intel_cdclk_actual_voltage_level(const struct intel_cdclk_state *cdclk_state)
> >> +{
> >> +	return cdclk_state->actual.voltage_level;
> >> +}
> >
> > These could've been grouped better after intel_cdclk_logical().
> 
> Yes, changing that.
> 
> > I wondered if it'd make sense to use
> > intel_cdclk_{logical,actual}_cdclk() instead of 
> > intel_cdclk_{logical,actual}().
> 
> Mmh. I dislike the repetition, "cdclk logical cdclk"...

Yes, though there's already intel_cdclk_min_cdclk() anyway.

> > Or *_clock() instead of *_cdclk() in the above and other helpers.
> 
> ...so I set out to consistently use "clock", but then it didn't feel
> right for things like "intel_cdclk_min_cdclk" because it's then compared
> against min_cdclk in a number of places.
> 
> I don't know, leave it as it is now in the patches?

I only pointed this out since intel_cdclk_actual() is strange wrt.
intel_cdclk_actual_voltage_level() for instace. But sure, this is not a
big deal.

> 
> BR,
> Jani.
> 
> 
> 
> >
> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> >> index 0d5ee1826168..f38605c6ab72 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> >> @@ -103,5 +103,7 @@ int intel_cdclk_bw_min_cdclk(const struct intel_cdclk_state *cdclk_state);
> >>  bool intel_cdclk_pmdemand_needs_update(struct intel_atomic_state *state);
> >>  void intel_cdclk_force_min_cdclk(struct intel_cdclk_state *cdclk_state, int force_min_cdclk);
> >>  void intel_cdclk_read_hw(struct intel_display *display);
> >> +int intel_cdclk_actual(const struct intel_cdclk_state *cdclk_state);
> >> +int intel_cdclk_actual_voltage_level(const struct intel_cdclk_state *cdclk_state);
> >>  
> >>  #endif /* __INTEL_CDCLK_H__ */
> >> diff --git a/drivers/gpu/drm/i915/display/intel_pmdemand.c b/drivers/gpu/drm/i915/display/intel_pmdemand.c
> >> index 16ef68ef4041..d806c15db7ce 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_pmdemand.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_pmdemand.c
> >> @@ -360,9 +360,9 @@ int intel_pmdemand_atomic_check(struct intel_atomic_state *state)
> >>  		return PTR_ERR(new_cdclk_state);
> >>  
> >>  	new_pmdemand_state->params.voltage_index =
> >> -		new_cdclk_state->actual.voltage_level;
> >> +		intel_cdclk_actual_voltage_level(new_cdclk_state);
> >>  	new_pmdemand_state->params.cdclk_freq_mhz =
> >> -		DIV_ROUND_UP(new_cdclk_state->actual.cdclk, 1000);
> >> +		DIV_ROUND_UP(intel_cdclk_actual(new_cdclk_state), 1000);
> >>  
> >>  	intel_pmdemand_update_max_ddiclk(display, state, new_pmdemand_state);
> >>  
> >> -- 
> >> 2.39.5
> >> 
> 
> -- 
> Jani Nikula, Intel