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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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>

>  
>  	/*
> @@ -584,8 +583,6 @@ static int vlv_calc_cdclk(struct intel_display *display, int min_cdclk)
>  
>  static u8 vlv_calc_voltage_level(struct intel_display *display, int cdclk)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(display->drm);
> -
>  	if (display->platform.valleyview) {
>  		if (cdclk >= 320000) /* jump to highest voltage for 400MHz too */
>  			return 2;
> @@ -599,7 +596,7 @@ static u8 vlv_calc_voltage_level(struct intel_display *display, int cdclk)
>  		 * hardware has shown that we just need to write the desired
>  		 * CCK divider into the Punit register.
>  		 */
> -		return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
> +		return DIV_ROUND_CLOSEST(vlv_clock_get_hpll_vco(display->drm) << 1, cdclk) - 1;
>  	}
>  }
>  
> @@ -664,7 +661,6 @@ static void vlv_set_cdclk(struct intel_display *display,
>  			  const struct intel_cdclk_config *cdclk_config,
>  			  enum pipe pipe)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(display->drm);
>  	int cdclk = cdclk_config->cdclk;
>  	u32 val, cmd = cdclk_config->voltage_level;
>  	intel_wakeref_t wakeref;
> @@ -709,7 +705,7 @@ static void vlv_set_cdclk(struct intel_display *display,
>  	if (cdclk == 400000) {
>  		u32 divider;
>  
> -		divider = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1,
> +		divider = DIV_ROUND_CLOSEST(vlv_clock_get_hpll_vco(display->drm) << 1,
>  					    cdclk) - 1;
>  
>  		/* adjust cdclk divider */
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 022f32ffd697..7b5379262a37 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -144,17 +144,20 @@ static void bdw_set_pipe_misc(struct intel_dsb *dsb,
>  /* returns HPLL frequency in kHz */
>  int vlv_clock_get_hpll_vco(struct drm_device *drm)
>  {
> +	struct drm_i915_private *i915 = to_i915(drm);
>  	int hpll_freq, vco_freq[] = { 800, 1600, 2000, 2400 };
>  
> -	vlv_cck_get(drm);
> -
> -	/* Obtain SKU information */
> -	hpll_freq = vlv_cck_read(drm, CCK_FUSE_REG) &
> -		CCK_FUSE_HPLL_FREQ_MASK;
> +	if (!i915->hpll_freq) {
> +		vlv_cck_get(drm);
> +		/* Obtain SKU information */
> +		hpll_freq = vlv_cck_read(drm, CCK_FUSE_REG) &
> +			CCK_FUSE_HPLL_FREQ_MASK;
> +		vlv_cck_put(drm);
>  
> -	vlv_cck_put(drm);
> +		i915->hpll_freq = vco_freq[hpll_freq] * 1000;
> +	}
>  
> -	return vco_freq[hpll_freq] * 1000;
> +	return i915->hpll_freq;
>  }
>  
>  static int vlv_get_cck_clock(struct drm_device *drm,
> @@ -179,15 +182,7 @@ static int vlv_get_cck_clock(struct drm_device *drm,
>  static int vlv_get_cck_clock_hpll(struct drm_device *drm,
>  				  const char *name, u32 reg)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(drm);
> -	int hpll;
> -
> -	if (dev_priv->hpll_freq == 0)
> -		dev_priv->hpll_freq = vlv_clock_get_hpll_vco(drm);
> -
> -	hpll = vlv_get_cck_clock(drm, name, reg, dev_priv->hpll_freq);
> -
> -	return hpll;
> +	return vlv_get_cck_clock(drm, name, reg, vlv_clock_get_hpll_vco(drm));
>  }
>  
>  int vlv_clock_get_hrawclk(struct drm_device *drm)
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel