From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH] drm/i915: Don't set min_cdclk in the initial crtc_state Date: Mon, 25 May 2026 19:40:27 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness: Good** The commit message's claim that `intel_compute_global_watermarks` is only c= alled from `intel_atomic_check` is verified =E2=80=94 there is exactly one = call site at `intel_display.c:6517`, and it is preceded by the min_cdclk co= mputation at lines 6514-6515: ```c for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) new_crtc_state->min_cdclk =3D intel_crtc_min_cdclk(new_crtc_state); ret =3D intel_compute_global_watermarks(state); ``` So removing the early computation in `intel_modeset_readout_hw_state()` doe= s not leave any consumer without a valid `min_cdclk` value. **Potential concern =E2=80=94 `min_voltage_level` has the same pattern**: L= ooking 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_s= tate->min_voltage_level` via the same old-vs-new equality check. If `min_vo= ltage_level` is similarly set during readout (I see `cdclk_state->min_volta= ge_level[pipe] =3D crtc_state->min_voltage_level` at `intel_cdclk.c:3661`),= it may suffer from the same "no change detected" problem. This is worth in= vestigating as a follow-up, though it may not manifest as visibly since vol= tage 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 is= sues. The `intel_cdclk_update_crtc_min_cdclk()` function does log the chang= e at `intel_cdclk.c:2967-2970`, but only when a change is detected. Since a= fter this patch `min_cdclk` will be 0 during readout and the actual compute= d value won't appear until the first atomic check, there's a small observab= ility gap. Not a blocker, but the author might consider whether the debug m= essage 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: Compu= te per-crtc min_cdclk earlier")` and CC's stable. The commit message also c= orrectly notes that the regression was exposed by `ba91b9eecb47`, which pre= sumably 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 whethe= r `min_voltage_level` has the same latent bug. --- Generated by Claude Code Patch Reviewer