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/display: Refactor amdgpu_dm_crtc_set_vblank Date: Wed, 25 Mar 2026 07:22:02 +1000 Message-ID: In-Reply-To: <20260323202755.315929-4-sunpeng.li@amd.com> References: <20260323202755.315929-1-sunpeng.li@amd.com> <20260323202755.315929-4-sunpeng.li@amd.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 Clean refactoring. The split is logical: `amdgpu_dm_crtc_set_vblank()` does= the HW interrupt work, `amdgpu_dm_crtc_queue_vblank_work()` handles the PS= R/panel optimization work, and the enable/disable wrappers tie them togethe= r. One minor issue: in `amdgpu_dm_crtc_set_vblank()`, the condition changed fr= om `if (enable)` to `if (enable && crtc->enabled)`, but the `crtc->enabled`= check was moved to `amdgpu_dm_crtc_enable_vblank()` which returns `-EINVAL= ` early. So in the non-deferred path, `amdgpu_dm_crtc_set_vblank()` should = never be called with `enable=3Dtrue` and `!crtc->enabled`. But patch 4 call= s `amdgpu_dm_crtc_set_vblank(crtc, true)` from `amdgpu_dm_crtc_deferred_ena= ble_vblank()` *without* the `crtc->enabled` guard. That means the `enable &= & crtc->enabled` check inside `set_vblank` becomes the only guard for the d= eferred path, which is fine =E2=80=94 but it means a deferred enable on an = unconfigured CRTC silently succeeds (returns 0) without actually enabling a= nything. That seems fragile; it might be better to return an error. --- Generated by Claude Code Patch Reviewer