Re: [PATCH v2 10/15] drm/i915: cache the results in vlv_clock_get_hpll_vco() and use it more

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 16 Sep 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Sep 12, 2025 at 05:48:49PM +0300, Jani Nikula wrote:
>> Use vlv_clock_get_hpll_vco() helper more to avoid looking at
>> i915->hpll_freq directly. Cache and return the cached results to avoid
>> repeated lookups.
>> 
>> v2: Rebase
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_cdclk.c   | 10 +++-----
>>  drivers/gpu/drm/i915/display/intel_display.c | 27 ++++++++------------
>>  2 files changed, 14 insertions(+), 23 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> index ea1e6d964764..e77efa0f33ed 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> @@ -563,8 +563,7 @@ static void hsw_get_cdclk(struct intel_display *display,
>>  
>>  static int vlv_calc_cdclk(struct intel_display *display, int min_cdclk)
>>  {
>> -	struct drm_i915_private *dev_priv = to_i915(display->drm);
>> -	int freq_320 = (dev_priv->hpll_freq <<  1) % 320000 != 0 ?
>> +	int freq_320 = (vlv_clock_get_hpll_vco(display->drm) <<  1) % 320000 != 0 ?
>>  		333333 : 320000;
>
> The somewhat dodgy thing here is that the device might not even be
> awake when this is called. So this on-demand caching only works
> correctly if the first call happens to be done at the right time
> by accident.
>
> I suppose we do end up calling most of these at some point during
> the driver initialization when everything is powered on, but the
> whole thing does feel rather fragile. I suppose we can hope that
> the limited CI coverage was enough to catch most of that.
>
> It would probably make sense to introduce an explicit initalization
> function that can be called under controlled circumstances. But we
> could do that as a followup, and in the meantime maybe toss in a
> few remarks somewhere to remind us about this potential trap...
>
> Series is
> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks, pushed the lot.

There wasn't really a point where I could've neatly snug in a comment
about caching while applying, so I sent a follow-up [1].

BR,
Jani.


[1] https://lore.kernel.org/r/20250917135200.1932903-1-jani.nikula@intel.com


-- 
Jani Nikula, Intel