Re: [PATCH 1/2] drm/i915/display: Seed skl_cdclk_init_hw() with SKU-max, not zero

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

On Tue, Aug 04, 2026 at 02:58:30PM +0530, Arun R Murthy wrote:
> skl_cdclk_init_hw() has a recovery path that runs when skl_sanitize_cdclk()
> has decided the pre-OS CDCLK state is not usable and zeroes
> display->cdclk.hw.
> 
> Seed with the SKU-permitted maximum instead (as read from SKL_DFSM), so
> the recovery path lands on a cdclk that satisfies every pipe on the SKU.
> The very first atomic commit that follows will dial cdclk down to what
> active pipes actually need via the normal skl_modeset_calc_cdclk() path.
> 
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16595
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> Tested-by: Polo-François Poli <polo.code.dev@gmail.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 65 +++++++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index a53d887271778193fd400b9199ad841d5d9730fe..19f3ef8adfcded378428924539db9336834278ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1288,6 +1288,31 @@ static void skl_sanitize_cdclk(struct intel_display *display)
>  	display->cdclk.hw.vco = ~0;
>  }
>  
> +/*
> + * Seed cdclk from the SKU-permitted maximum. Used by skl_cdclk_init_hw()'s
> + * recovery path when skl_sanitize_cdclk() has decided the pre-OS CDCLK state
> + * is not usable and has zeroed display->cdclk.hw.
> + * Seed with the SKU top instead: it is guaranteed to satisfy every pipe on
> + * the platform, and the very first atomic commit will dial cdclk down via
> + * skl_modeset_calc_cdclk() once per-pipe min_cdclk is known.
> + */
> +static int skl_cdclk_init_hw_seed(struct intel_display *display, int vco)
> +{
> +	u32 limit = intel_de_read(display, SKL_DFSM) & SKL_DFSM_CDCLK_LIMIT_MASK;
> +	int max_cdclk;
> +
> +	if (limit == SKL_DFSM_CDCLK_LIMIT_675)
> +		max_cdclk = 617143;
> +	else if (limit == SKL_DFSM_CDCLK_LIMIT_540)
> +		max_cdclk = 540000;
> +	else if (limit == SKL_DFSM_CDCLK_LIMIT_450)
> +		max_cdclk = 432000;
> +	else
> +		max_cdclk = 308571;
> +
> +	return skl_calc_cdclk(max_cdclk, vco);

That's just the max cdclk. We already have it.

> +}
> +
>  static void skl_cdclk_init_hw(struct intel_display *display)
>  {
>  	struct intel_cdclk_config cdclk_config;
> @@ -1311,9 +1336,14 @@ static void skl_cdclk_init_hw(struct intel_display *display)
>  	cdclk_config.vco = display->cdclk.skl_preferred_vco_freq;
>  	if (cdclk_config.vco == 0)
>  		cdclk_config.vco = 8100000;
> -	cdclk_config.cdclk = skl_calc_cdclk(0, cdclk_config.vco);
> +
> +	cdclk_config.cdclk = skl_cdclk_init_hw_seed(display, cdclk_config.vco);

We sanitize to min cdclk on all platforms. Don't add weird differences
between platforms.

>  	cdclk_config.voltage_level = skl_calc_voltage_level(cdclk_config.cdclk);
>  
> +	drm_dbg_kms(display->drm,
> +		    "cdclk: init seed cdclk=%d vco=%d volt=%d (SKU-max, no per-pipe min_cdclk yet)\n",
> +		    cdclk_config.cdclk, cdclk_config.vco, cdclk_config.voltage_level);

What?

> +
>  	skl_set_cdclk(display, &cdclk_config, INVALID_PIPE);
>  }
>  
> @@ -4231,6 +4261,39 @@ void intel_cdclk_read_hw(struct intel_display *display)
>  	cdclk_state->logical = display->cdclk.hw;
>  }
>  
> +/**
> + * intel_cdclk_sync_hw_state - Adopt the HW-asserted cdclk into SW state.
> + * @display: display instance
> + * @hw_config: the cdclk configuration as currently read back from HW
> + *
> + * This helper reconciles the SW state to match what HW is currently asserting.
> + * It updates display->cdclk.hw and the committed cdclk_state's actual/logical
> + * so that any following atomic commit sees a truthful baseline and will
> + * naturally attach a cdclk_state (via intel_cdclk_atomic_check()) to raise
> + * cdclk when the workload demands it.
> + */
> +void intel_cdclk_sync_hw_state(struct intel_display *display,
> +			       const struct intel_cdclk_config *hw_config)
> +{
> +	struct intel_cdclk_state *cdclk_state =
> +		to_intel_cdclk_state(display->cdclk.obj.state);
> +
> +	if (!intel_cdclk_clock_changed(&display->cdclk.hw, hw_config))
> +		return;
> +
> +	drm_dbg_kms(display->drm,
> +		    "cdclk: syncing SW state to HW after drift (sw %d/vco %d/ref %d -> hw %d/vco %d/ref %d)\n",
> +		    display->cdclk.hw.cdclk, display->cdclk.hw.vco, display->cdclk.hw.ref,
> +		    hw_config->cdclk, hw_config->vco, hw_config->ref);
> +
> +	display->cdclk.hw.cdclk = hw_config->cdclk;
> +	display->cdclk.hw.vco = hw_config->vco;
> +	display->cdclk.hw.ref = hw_config->ref;
> +
> +	cdclk_state->actual = display->cdclk.hw;
> +	cdclk_state->logical = display->cdclk.hw;

I have no idea what's happening here or why? We already copy the hw
state into the initial sw state somewhere.

> +}
> +
>  static int calc_cdclk(const struct intel_crtc_state *crtc_state, int min_cdclk)
>  {
>  	struct intel_display *display = to_intel_display(crtc_state);
> 
> -- 
> 2.25.1

-- 
Ville Syrjälä
Intel