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 <[email protected]> > --- > 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ä <[email protected]> > > /* > @@ -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
