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 <[email protected]> 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/[email protected]?part=1
