* Re: [PATCH] drm/i915: Don't set min_cdclk in the initial crtc_state
2026-05-21 21:13 ` Ville Syrjälä
@ 2026-05-22 13:33 ` Marius Hoch
2026-05-25 9:40 ` Claude review: " Claude Code Review Bot
2026-05-25 9:40 ` Claude Code Review Bot
2 siblings, 0 replies; 5+ messages in thread
From: Marius Hoch @ 2026-05-22 13:33 UTC (permalink / raw)
To: Ville Syrjälä
Cc: linux-kernel, stable, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen,
Tvrtko Ursulin, David Airlie, Simona Vetter, Joe Perches,
Mika Kahola, intel-gfx, intel-xe, dri-devel
Thanks for the quick reply!
On 21/05/2026 23:13, Ville Syrjälä wrote:
> On Thu, May 21, 2026 at 08:07:12PM +0200, Marius Hoch wrote:
>> Setting the min_cdclk this early means that intel_cdclk_atomic_check
>> (called via intel_atomic_check) will not pick up the initial min_cdclk, as
>> there is no change between the old and new atomic states.
> If there is no change then there is no need to change the CDCLK.
As far as I see, the problem is that the minimal CDCLK is reflected in
the initial crtc_state, but there is no direct connection to the actual
(hardware) CDCLK, which can be lower at this point. I'll add some more
details below.
> It's hard to say what you're really trying to work around here.
>
> Please a file a new bug at
> https://gitlab.freedesktop.org/drm/intel/issues/new and
> attach the full dmesg from boot with 'log_buf_len=4M drm.debug=0xe'
> passed to he kernel cmdline.
Makes sense, I've filed
https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16209 for that.
As far as I see, the CDCLK is updated based on / for an atomic change in
the following way: intel_atomic_check calls intel_cdclk_atomic_check to
update the CDCLK iff needed. In order to find out if a CDCLK calc is
needed, intel_cdclk_atomic_check in turn (via intel_cdclk_atomic_check
and intel_crtcs_calc_min_cdclk) calls intel_cdclk_update_crtc_min_cdclk.
In intel_cdclk_update_crtc_min_cdclk, the old and the new min_cdclk get
compared, and if there's a change, the CDCLK will be recalculated.
The issue here is that we start out with the min_cdclk set (to 158400 in
the GLK case) in the initial / old atomic state (which is not reflected
in the actual / hardware state). Thus intel_cdclk_update_crtc_min_cdclk
won't be able to detect a change here (as both the old and the new
atomic state have the same min_cdclk), and thus won't indicate that the
CDCLK needs to be recalculated (even though the actual CDCLK might be
lower as min_cdclk, as can be seen on the dmesg attached in the ticket).
If you want to see this addressed in another way, I'm happy to provide
another patch. Also I'm happy to provide more details, if needed.
>
>> This is
>> problematic, especially on Gemini Lake, where the picture gets unstable if
>> the CDCLK is too low (see vlv_dsi_min_cdclk).
See beb29980026f /
https://lore.kernel.org/all/20190430125119.7478-1-stanislav.lisovskiy@intel.com/
and the tickets linked from there for context here. See also
cf696856bc54, which fixed a regression related to this Gemini Lake quirk
before.
>>
>> This was introduced in 7a8d9cfa6db0, which states that the min_cdclk must
>> be set before calling intel_compute_global_watermarks. However, as the
>> only place that calls intel_compute_global_watermarks is
>> intel_atomic_check, right after setting the min_cdclk on new_crtc_state,
>> there is no need to set the min_cdclk initially.
>>
>> This surfaced as a bug on my IdeaPad Duet 3 after ba91b9eecb47, leading
>> to the screen output being completely garbled initially (when asking for
>> the dm-crypt passphrase). It recovers after the passphrase prompt, as this
>> only affects the initial state.
I've bisected this problem to ba91b9eecb47 initially, and naively adding
the intel_modeset_calc_cdclk back after intel_modeset_checks in
intel_atomic_check also fixes this issue
(https://github.com/mariushoch/linux/commit/5d083fe47a9c0c10afcdf5b5cff5683cbfb3fe22).
>>
>> Tested on an IdeaPad Duet 3 10IGL5-LTE (with UHD Graphics 605).
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 7a8d9cfa6db0 ("drm/i915: Compute per-crtc min_cdclk earlier")
>> Signed-off-by: Marius Hoch <mail@mariushoch.de>
>> ---
>> drivers/gpu/drm/i915/display/intel_modeset_setup.c | 5 -----
>> 1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> index 4086f16a12bf..9278856375e9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> @@ -865,11 +865,6 @@ static void intel_modeset_readout_hw_state(struct intel_display *display)
>> crtc_state->plane_min_cdclk[plane->id]);
>> }
>>
>> - crtc_state->min_cdclk = intel_crtc_min_cdclk(crtc_state);
>> -
>> - drm_dbg_kms(display->drm, "[CRTC:%d:%s] min_cdclk %d kHz\n",
>> - crtc->base.base.id, crtc->base.name, crtc_state->min_cdclk);
>> -
>> intel_pmdemand_update_port_clock(display, pmdemand_state, pipe,
>> crtc_state->port_clock);
>> }
>> --
>> 2.54.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Claude review: Re: [PATCH] drm/i915: Don't set min_cdclk in the initial crtc_state
2026-05-21 21:13 ` Ville Syrjälä
2026-05-22 13:33 ` Marius Hoch
@ 2026-05-25 9:40 ` Claude Code Review Bot
2026-05-25 9:40 ` Claude Code Review Bot
2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 9:40 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Correctness: Good**
The commit message's claim that `intel_compute_global_watermarks` is only called from `intel_atomic_check` is verified — there is exactly one call site at `intel_display.c:6517`, and it is preceded by the min_cdclk computation at lines 6514-6515:
```c
for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
new_crtc_state->min_cdclk = intel_crtc_min_cdclk(new_crtc_state);
ret = intel_compute_global_watermarks(state);
```
So removing the early computation in `intel_modeset_readout_hw_state()` does not leave any consumer without a valid `min_cdclk` value.
**Potential concern — `min_voltage_level` has the same pattern**: Looking at `intel_crtcs_calc_min_cdclk()` at `intel_cdclk.c:3438-3441`, the function also compares `old_crtc_state->min_voltage_level` with `new_crtc_state->min_voltage_level` via the same old-vs-new equality check. If `min_voltage_level` is similarly set during readout (I see `cdclk_state->min_voltage_level[pipe] = crtc_state->min_voltage_level` at `intel_cdclk.c:3661`), it may suffer from the same "no change detected" problem. This is worth investigating as a follow-up, though it may not manifest as visibly since voltage level mismatches are less likely to cause garbled output.
**Lost debug logging**: The patch removes:
```c
drm_dbg_kms(display->drm, "[CRTC:%d:%s] min_cdclk %d kHz\n",
crtc->base.base.id, crtc->base.name, crtc_state->min_cdclk);
```
This debug output during initial readout was useful for diagnosing CDCLK issues. The `intel_cdclk_update_crtc_min_cdclk()` function does log the change at `intel_cdclk.c:2967-2970`, but only when a change is detected. Since after this patch `min_cdclk` will be 0 during readout and the actual computed value won't appear until the first atomic check, there's a small observability gap. Not a blocker, but the author might consider whether the debug message is worth preserving (logging the computed value even if not storing it) for boot-time diagnostics.
**Fixes tag and stable CC**: Correctly tags `7a8d9cfa6db0 ("drm/i915: Compute per-crtc min_cdclk earlier")` and CC's stable. The commit message also correctly notes that the regression was exposed by `ba91b9eecb47`, which presumably changed CDCLK behavior enough to make the latent bug visible.
**Overall**: Reviewed-by worthy. The fix directly addresses the root cause with minimal code change. The only actionable suggestion is to check whether `min_voltage_level` has the same latent bug.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 5+ messages in thread