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

[Jani Nikula]

On Sun, 30 Aug 2026, Eduardo Diaz <iamedu@xxxxxxxxx> wrote:
> On Panther Lake (xe3lpd) laptops, cold boot reliably corrupts the
> internal eDP panel: pipe A gets a "Selective fetch area calculation
> failed in pipe A" warning immediately followed by a CPU pipe A FIFO
> underrun, and the panel stays corrupted for the rest of the session.
> A subsequent suspend/resume cycle (or any other full re-modeset)
> "fixes" it, which pointed at cdclk/voltage-level tracking rather than
> a genuine hardware race.
>
> intel_modeset_readout_hw_state() runs once at driver probe (and again
> on resume) to figure out what firmware/GOP left the display in. For
> each already-active pipe it calls intel_cdclk_update_hw_state(),
> which seeds cdclk_state->min_cdclk[]/min_voltage_level[] directly
> from the freshly read-out crtc_state. That treats "firmware left this
> pipe active with mode X" as proof that this driver's own cdclk,
> voltage-level and DBUF setup for mode X is already established in
> hardware. It isn't -- only firmware's own, entirely separate code
> path has ever touched those registers.
>
> The OS driver's first real modeset for an inherited pipe typically
> targets the same native panel mode, so the freshly computed
> crtc_state->min_cdclk/min_voltage_level trivially match this
> readout-seeded baseline. intel_cdclk_update_crtc_min_cdclk() and
> intel_cdclk_update_crtc_min_voltage_level() then conclude nothing
> changed and skip the recalculation, on the one commit where it
> actually matters: taking a pipe from firmware ownership to being
> correctly configured by this driver.
>
> Fix this in two parts:
>
>  - Invalidate the per-pipe min_cdclk[]/min_voltage_level[] tracking
>    right after boot-time readout, so the first real atomic commit is
>    guaranteed to see a difference.
>
>  - Stop early-returning in intel_cdclk_update_crtc_min_cdclk() and
>    intel_cdclk_update_crtc_min_voltage_level() based on the crtc_state
>    comparison alone. That comparison is unreliable for exactly the
>    same reason (it's derived from the same readout), and skips the
>    real, tracked-state check below it.
>
> Bisected on real hardware (Lenovo Yoga 9i 14IPH11, Panther Lake) down
> to a narrow window between v6.19.10 (clean on every cold boot) and
> v7.1.10 (broken on every cold boot); confirmed via live kernel
> tracing that the skip path fires unconditionally on this platform
> from the very first post-boot atomic commit onward. This fix
> eliminates the FIFO underrun across many consecutive cold boots on
> the same hardware, with no regression observed across suspend/resume.

Do you have a regressing commit? Nearly 600 commits were merged to i915
display alone between 6.19 and 7.1 so it's not really a narrow window.

Have you filed a bug as described at [1]? With debugs, logs, and
everything.

Have you tried with 1786d2688781 ("drm/i915/cdclk: Avoid spurious cdclk
sanitization on PTL+")?


BR,
Jani.


[1] https://drm.pages.freedesktop.org/intel-docs/how-to-file-i915-bugs.html


> The investigation and this fix were developed with the assistance of
> Claude (Anthropic), driven and verified end to end on the affected
> hardware by the Signed-off-by below.
>
> Signed-off-by: Eduardo Diaz <iamedu@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c    | 45 ++++++++++++++-----
>  drivers/gpu/drm/i915/display/intel_cdclk.h    |  1 +
>  .../drm/i915/display/intel_modeset_setup.c    |  1 +
>  3 files changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index a53d887271..209a2373d1 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))
> @@ -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))
> @@ -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;
> +	}
> +}
> +
>  void intel_cdclk_crtc_disable_noatomic(struct intel_crtc *crtc)
>  {
>  	struct intel_display *display = to_intel_display(crtc);
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index a60cbf745e..1517d3605a 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -46,6 +46,7 @@ int intel_cdclk_state_set_joined_mbus(struct intel_atomic_state *state, bool joi
>  struct intel_cdclk_state *
>  intel_atomic_get_cdclk_state(struct intel_atomic_state *state);
>  void intel_cdclk_update_hw_state(struct intel_display *display);
> +void intel_cdclk_invalidate_min_tracking(struct intel_display *display);
>  void intel_cdclk_crtc_disable_noatomic(struct intel_crtc *crtc);
>  int intel_cdclk_update_dbuf_bw_min_cdclk(struct intel_atomic_state *state,
>  					 int old_min_cdclk, int new_min_cdclk,
> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> index 6aed881737..c7be6e63e7 100644
> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
> @@ -880,6 +880,7 @@ static void intel_modeset_readout_hw_state(struct intel_display *display)
>  	intel_bw_update_hw_state(display);
>  	intel_dbuf_bw_update_hw_state(display);
>  	intel_cdclk_update_hw_state(display);
> +	intel_cdclk_invalidate_min_tracking(display);
>  
>  	intel_pmdemand_init_pmdemand_params(display, pmdemand_state);
>  }

-- 
Jani Nikula, Intel