Claude review: Re: [PATCH] drm/i915: Don't set min_cdclk in the initial crtc_state

[Claude Code Review Bot <claude-review@example.com>]

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