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: Fri, 27 Feb 2026 14:40:53 +1000 Message-ID: In-Reply-To: <20260224212639.390768-4-sunpeng.li@amd.com> References: <20260224212639.390768-1-sunpeng.li@amd.com> <20260224212639.390768-4-sunpeng.li@amd.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review This is a reasonable cleanup separating the vblank workqueue management from the actual interrupt enable/disable logic. **Observation:** The refactored `amdgpu_dm_crtc_set_vblank()` now uses `enable && crtc->enabled` as its guard: ```c if (enable && crtc->enabled) { // do enable work } ``` When `enable=true` but `crtc->enabled=false`, this silently returns 0 (success) without actually enabling anything. The caller `amdgpu_dm_crtc_enable_vblank()` checks `!crtc->enabled` and rejects early, so for the non-deferred path this is fine. But in patch 4, `amdgpu_dm_crtc_deferred_enable_vblank()` does **not** check `crtc->enabled` and calls `set_vblank(crtc, true)` directly. If the CRTC is disabled, `set_vblank` returns 0, and `active_vblank_irq_count` gets incremented without actually enabling the interrupt. The count will be balanced on disable, but the vblank core now believes the interrupt was successfully enabled when it wasn't. **Also:** `amdgpu_dm_crtc_queue_vblank_work()` still uses `GFP_ATOMIC` for the allocation. Since the callers (`enable_vblank`/`disable_vblank` callbacks) can be called from atomic context in the non-deferred path this is needed. But for future clarity, a comment would help. --- Generated by Claude Code Patch Reviewer