From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/amd/pm: Use pm_display_cfg in legacy DPM (v2) Date: Fri, 27 Feb 2026 12:46:57 +1000 Message-ID: In-Reply-To: <20260225215013.11224-3-rosenp@gmail.com> References: <20260225215013.11224-1-rosenp@gmail.com> <20260225215013.11224-3-rosenp@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This is the main patch. It adds `amdgpu_dpm_get_display_cfg()` to populate = the `pm_display_cfg` struct from legacy (non-DC) display state, and convert= s all legacy DPM consumers from the old `dpm.new_active_crtcs`/`dpm.new_act= ive_crtc_count` fields to use `pm_display_cfg`. **Observation (pre-existing upstream bug):** In `amdgpu_dpm_get_display_cfg= ()`, the `crtc_index` field is not initialized before the iteration loop: ```c + if (amdgpu_crtc->crtc_id < cfg->crtc_index) { + /* Find first active CRTC and its line time. */ + cfg->crtc_index =3D amdgpu_crtc->crtc_id; + cfg->line_time_in_us =3D amdgpu_crtc->line_time; + } ``` Since `cfg` is `&adev->pm.pm_display_cfg` (zero-initialized from device str= uct allocation), `cfg->crtc_index` starts at 0. No `crtc_id` can be less th= an 0 (uint32_t), so this block will never execute on the first call. As a r= esult, `line_time_in_us` remains 0. Compare with the DC code path in `dce11= 0_clk_mgr.c:130` which correctly initializes `crtc_index` to `num_timing_ge= nerator` before the loop to act as a sentinel value. The non-DC path should= similarly initialize `cfg->crtc_index` to a large value (e.g. `UINT_MAX`) = at the start of the function. The same issue affects `cfg->vrefresh` which = is not reset between calls. However, this matches the upstream code exactly, so it is **not** a valid o= bjection to this stable backport. The consumers in `si_upload_smc_data` gua= rd against `line_time_in_us =3D=3D 0`: ```c + if (cfg->line_time_in_us) { + mclk_change_block_cp_min =3D 200 / cfg->line_time_in_us; + mclk_change_block_cp_max =3D 100 / cfg->line_time_in_us; ``` so it won't cause a division by zero, just suboptimal MCLK switching behavi= or. Worth fixing upstream separately. The conversion of `legacy_dpm.c` to conditionally call the new function onl= y for non-DC: ```c - amdgpu_dpm_get_active_displays(adev); + if (!adev->dc_enabled) + amdgpu_dpm_get_display_cfg(adev); ``` is correct =E2=80=94 when DC is enabled, DC already populates `pm_display_c= fg` through its own path. The changes in `si_dpm.c` and `kv_dpm.c` are mechanical replacements from o= ld fields to new ones and look correct. The `amd_powerplay.c` change consol= idates the previously inline display config population into the shared func= tion, which is also correct. The `NULL` pointer safety for `amdgpu_crtc->connector` in the new function = relies on the `!amdgpu_crtc->enabled` check above, which is the same patter= n used by the old code. Overall: correct stable backport. The crtc_index initialization issue shoul= d be raised as a separate upstream fix. --- Generated by Claude Code Patch Reviewer