Re: [PATCH] drm/i915/cdclk: Don't trust boot readout for per-pipe cdclk/voltage tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Zeroing `min_cdclk` and `min_voltage_level` during boot readout corrupts the global state tracking for inherited pipes, leading to FIFO underruns on partial commits.
- [High] The boot-time invalidation of cdclk/voltage tracking is fragile and gets silently undone during hardware sanitization, breaking the fix for the remaining active pipes.
- [Medium] Moving the early return in the CDCLK update paths forces unnecessary global state duplication and memory allocation on every page flip.
- [Low] The `old_min_cdclk` and `old_min_voltage_level` parameters are now ignored and immediately overwritten.
--

commit edef4e8a0b14b779ca2831158248f1bc907d372c
Author: Eduardo Diaz <iamedu@gmail.com>

drm/i915/cdclk: Don't trust boot readout for per-pipe cdclk/voltage tracking

This commit addresses a bug where cold boot corrupts the internal eDP panel
due to cdclk/voltage tracking assuming firmware-programmed state as its own.
It invalidates the tracking state during readout and forces the cdclk and
voltage update functions to skip early returns.

> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index a53d887271778..209a2373d17aa 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2981,11 +2981,14 @@ static int intel_cdclk_update_crtc_min_cdclk(struct intel_atomic_state *state,
>  	bool allow_cdclk_decrease = intel_any_crtc_needs_modeset(state);
>  	int ret;
>  
> -	if (new_min_cdclk == old_min_cdclk)
> -		return 0;
> -
> -	if (!allow_cdclk_decrease && new_min_cdclk < old_min_cdclk)
> -		return 0;
> +	/*
> +	 * old_min_cdclk comes from the previous crtc_state, which after
> +	 * boot-time readout reflects whatever firmware/GOP left running,
> +	 * not what this driver has programmed. For an inherited pipe it
> +	 * equals new_min_cdclk by construction (same mode, same formula),
> +	 * so an early return here would skip the recalculation that
> +	 * matters. Always continue on to the cdclk_state check below.
> +	 */
>  
>  	cdclk_state = intel_atomic_get_cdclk_state(state);
>  	if (IS_ERR(cdclk_state))

[Severity: Medium]
Moving this early return causes the state duplication and memory allocations
in intel_atomic_get_cdclk_state() to occur unconditionally on every commit,
including every page flip. Does this introduce a performance regression or
latency spike in this hot path?

[Severity: Low]
Additionally, since the old_min_cdclk value passed into the function is now
overwritten right after intel_atomic_get_cdclk_state() returns:

    old_min_cdclk = cdclk_state->min_cdclk[crtc->pipe];

Does this make the old_min_cdclk parameter passed by the caller dead code
that is never actually read?

[ ... ]

> @@ -3026,12 +3029,10 @@ static int intel_cdclk_update_crtc_min_voltage_level(struct intel_atomic_state *
>  	bool allow_voltage_level_decrease = intel_any_crtc_needs_modeset(state);
>  	int ret;
>  
> -	if (new_min_voltage_level == old_min_voltage_level)
> -		return 0;
> -
> -	if (!allow_voltage_level_decrease &&
> -	    new_min_voltage_level < old_min_voltage_level)
> -		return 0;
> +	/*
> +	 * old_min_voltage_level is unreliable for the same reason; see
> +	 * intel_cdclk_update_crtc_min_cdclk().
> +	 */
>  
>  	cdclk_state = intel_atomic_get_cdclk_state(state);
>  	if (IS_ERR(cdclk_state))

[Severity: Low]
Similarly to old_min_cdclk, is the old_min_voltage_level parameter now ignored
and overwritten before it is ever used?

[ ... ]

> @@ -3705,6 +3706,28 @@ void intel_cdclk_update_hw_state(struct intel_display *display)
>  	cdclk_state->dbuf_bw_min_cdclk = intel_dbuf_bw_min_cdclk(display, dbuf_bw_state);
>  }
>  
> +/*
> + * intel_cdclk_update_hw_state() seeds min_cdclk[]/min_voltage_level[]
> + * from readout's crtc_state, i.e. from whatever firmware/GOP left
> + * running, not from anything this driver has programmed. A pipe's
> + * first real modeset usually targets the same native mode, so the
> + * freshly computed value matches this seeded baseline and
> + * intel_cdclk_update_crtc_min_cdclk()/_min_voltage_level() conclude
> + * nothing changed, skipping the recalculation that matters. Call
> + * this after readout so the first real commit sees a difference.
> + */
> +void intel_cdclk_invalidate_min_tracking(struct intel_display *display)
> +{
> +	struct intel_cdclk_state *cdclk_state =
> +		to_intel_cdclk_state(display->cdclk.obj.state);
> +	enum pipe pipe;
> +
> +	for_each_pipe(display, pipe) {
> +		cdclk_state->min_cdclk[pipe] = 0;
> +		cdclk_state->min_voltage_level[pipe] = 0;
> +	}
> +}

[Severity: High]
Does zeroing min_cdclk for all pipes corrupt the state for inherited pipes
that are not included in the first atomic commit?

If the OS performs a partial commit (e.g., a cursor update on a single pipe)
on a multi-display setup, intel_cdclk_update_crtc_min_cdclk() will only
update the tracking for the pipe in the commit.

Because intel_compute_min_cdclk() takes the maximum of all pipes' requirements,
will the untouched active pipes with 0 tracked requirement be starved of their
necessary clock speed, leading to a FIFO underrun?

[Severity: High]
Can this boot-time invalidation be silently undone during hardware sanitization?

If a CRTC requires sanitization during boot, intel_crtc_disable_noatomic()
calls intel_cdclk_crtc_disable_noatomic(), which in turn calls
intel_cdclk_update_hw_state().

intel_cdclk_update_hw_state() loops over all pipes and does:

    cdclk_state->min_cdclk[pipe] = crtc_state->min_cdclk;

Since crtc_state->min_cdclk still contains the readout value, does this
re-seed the tracking values for the remaining active pipes, circumventing
the fix entirely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830161703.11570-1-iamedu@gmail.com?part=1