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: Implement deferred vblanks on IPS platforms Date: Fri, 27 Feb 2026 14:40:53 +1000 Message-ID: In-Reply-To: <20260224212639.390768-5-sunpeng.li@amd.com> References: <20260224212639.390768-1-sunpeng.li@amd.com> <20260224212639.390768-5-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 This is the driver-side implementation. The pre/post callbacks acquire/rele= ase `dc_lock` to bracket HW access. The design is sound in principle. **Issue 1: `crtc->state` access from worker context** In `amdgpu_dm_crtc_post_enable_vblank()` and `amdgpu_dm_crtc_post_disable_v= blank()`: ```c struct dm_crtc_state *acrtc_state =3D to_dm_crtc_state(crtc->state); ``` These run from the deferred workqueue, meaning there's a window between whe= n the work was queued (from `drm_vblank_get()`) and when it executes. Durin= g that window, an atomic commit could swap `crtc->state`. Accessing `crtc->= state` without holding `drm_modeset_lock` from a worker is unsafe. The stre= am pointer could become stale or freed. Consider either: - Capturing the stream reference at queue time and passing it through to th= e worker, or - Ensuring `dc_lock` serialization is sufficient to prevent state changes (= it might be, since commits also take `dc_lock`, but this is fragile and sho= uld be documented) **Issue 2: dc_lock held across drm_vblank_enable/disable internals** `pre_enable_vblank` takes `dc_lock` and `post_enable_vblank` releases it. T= his means `dc_lock` is held while the DRM core calls `__enable_vblank` =E2= =86=92 `enable_vblank` callback =E2=86=92 `amdgpu_dm_crtc_deferred_enable_v= blank()`. That callback also does `amdgpu_dm_crtc_set_vblank()` which itsel= f touches DC state. Since `dc_lock` is already held, this requires that all= the code in the `set_vblank` path is safe for recursive `dc_lock` holding = (mutex is not recursive by default). Verify that `dc_lock` isn't taken agai= n inside `set_vblank` =E2=86=92 interrupt enable path, or this will deadloc= k. **Issue 3: Missing `crtc->enabled` check in deferred enable** As noted in patch 3 review, `amdgpu_dm_crtc_deferred_enable_vblank()` doesn= 't check `crtc->enabled` before calling `set_vblank`. The `post_enable_vbla= nk` callback does check `crtc->enabled` and bails early, but `active_vblank= _irq_count` has already been incremented by that point. **Issue 4: Duplicate vtable** `amdgpu_dm_crtc_deferred_vblank_funcs` duplicates the entire `amdgpu_dm_crt= c_funcs` structure with only 6 fields differing (enable/disable_vblank + 4 = pre/post callbacks). If future changes are made to `amdgpu_dm_crtc_funcs`, = the deferred variant must be kept in sync manually. Consider documenting th= is coupling prominently, or using a helper to populate shared fields. --- Generated by Claude Code Patch Reviewer