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/amdgpu: Replace VKMS vblank timer with common implementation Date: Thu, 23 Apr 2026 09:48:28 +1000 Message-ID: In-Reply-To: <20260420125223.234974-1-tzimmermann@suse.de> References: <20260420125223.234974-1-tzimmermann@suse.de> <20260420125223.234974-1-tzimmermann@suse.de> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Correctness of the macro substitutions:** The `DRM_CRTC_VBLANK_TIMER_FUNCS` macro expands to: ```c .enable_vblank =3D drm_crtc_vblank_helper_enable_vblank_timer, .disable_vblank =3D drm_crtc_vblank_helper_disable_vblank_timer, .get_vblank_timestamp =3D drm_crtc_vblank_helper_get_vblank_timestamp_from_= timer ``` These replace the removed `amdgpu_vkms_enable_vblank`, `amdgpu_vkms_disable= _vblank`, and `amdgpu_vkms_get_vblank_timestamp` respectively. Functionally= equivalent =E2=80=94 the shared helpers call `drm_crtc_vblank_start_timer(= )`, `drm_crtc_vblank_cancel_timer()`, and `drm_crtc_vblank_get_vblank_timeo= ut()` which implement the same hrtimer logic, but with the timer state stor= ed in `struct drm_vblank_crtc_timer` (inside `drm_vblank_crtc`) rather than= in `amdgpu_crtc`. The `DRM_CRTC_HELPER_VBLANK_FUNCS` macro expands to: ```c .atomic_flush =3D drm_crtc_vblank_atomic_flush, .atomic_enable =3D drm_crtc_vblank_atomic_enable, .atomic_disable =3D drm_crtc_vblank_atomic_disable ``` These replace the three removed `amdgpu_vkms_crtc_atomic_*` functions. The = logic is equivalent =E2=80=94 `drm_crtc_vblank_on()`/`drm_crtc_vblank_off()= ` for enable/disable, and the arm-or-send-event pattern for flush. **Improvements in the shared code over the old amdgpu code:** 1. **Deadlock avoidance in cancel:** The old code used `hrtimer_try_to_canc= el()` in `disable_vblank` and `hrtimer_cancel()` in `sw_fini`. The shared `= drm_crtc_vblank_cancel_timer()` explicitly avoids `hrtimer_cancel()` due to= potential deadlocks between `vblank_time_lock` and `softirq_expiry_lock`, = instead zeroing the interval and using `hrtimer_try_to_cancel()`. This is t= he "deadlocks and race conditions" the commit message references. 2. **Timestamp race protection:** The old `amdgpu_vkms_get_vblank_timestamp= ()` does a single `READ_ONCE()` of the timer expiry, then a simple `WARN_ON= ` comparison against `vblank->time`. The shared `drm_crtc_vblank_get_vblank= _timeout()` uses a retry loop with `drm_crtc_vblank_count_and_time()` to pr= otect against concurrent vblank timeouts updating the expiry between reads. 3. **atomic_flush locking:** The old code used `spin_lock_irqsave`/`spin_un= lock_irqrestore` and cleared `crtc->state->event =3D NULL` outside the lock= . The shared code uses `spin_lock_irq`/`spin_unlock_irq` (appropriate since= atomic_flush runs in process context) and clears `crtc_state->event =3D NU= LL` inside the lock. The shared code also properly fetches `crtc_state` via= `drm_atomic_get_new_crtc_state()` rather than directly accessing `crtc->st= ate`. **hrtimer init/fini removal:** The removal of `hrtimer_setup()` from `amdgpu_vkms_crtc_init` and `hrtimer_= cancel()` loop from `amdgpu_vkms_sw_fini` is correct =E2=80=94 the shared i= mplementation initializes the timer lazily on first `drm_crtc_vblank_start_= timer()` call, and cleanup is handled by the vblank core. **Minor nit =E2=80=94 leftover dead fields in `amdgpu_vkms_output`:** After this patch, the `period_ns` and `event` fields in `struct amdgpu_vkms= _output` (in `amdgpu_vkms.h:21-22`) become completely unused. All three cal= l sites that referenced `period_ns` via `drm_crtc_to_amdgpu_vkms_output()` = are in the removed functions. The `event` field was already unused before t= his patch (the flush function used `crtc->state->event`, not `output->event= `). The `drm_crtc_to_amdgpu_vkms_output` macro in `amdgpu_vkms.h:12-13` als= o loses all callers in `amdgpu_vkms.c` (its three uses at lines 49, 71, 94 = are all in removed functions), though it is still used implicitly by `amdgp= u_vkms_output_init()` which takes the struct by pointer. Consider cleaning = up `period_ns` and `event` from the struct in this patch since they're dire= ctly related to the removed code. **Reviewed-by assessment:** Patch is correct and safe to apply. The dead fi= eld cleanup is a minor nice-to-have, not a blocker. --- Generated by Claude Code Patch Reviewer