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/vmwgfx: Convert to DRM vblank timers Date: Tue, 28 Apr 2026 14:31:51 +1000 Message-ID: In-Reply-To: <20260427150250.699768-4-tzimmermann@suse.de> References: <20260427150250.699768-1-tzimmermann@suse.de> <20260427150250.699768-4-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 **Purpose:** The actual conversion =E2=80=94 removes custom hrtimer code an= d wires up the DRM vblank timer infrastructure. **Assessment: Looks good overall, with one style nit.** **Key changes reviewed:** 1. **`vmwgfx_kms.h`**: Removal of `hrtimer timer` and `ktime_t period_ns` f= rom the `vkms` struct in `vmw_display_unit` =E2=80=94 correct, these are no= w managed by the DRM vblank timer infrastructure inside `drm_vblank_crtc`. 2. **LDU/SCRN/STDU helper_funcs**: All three display backends wire up `.han= dle_vblank_timeout =3D vmw_vkms_handle_vblank_timeout`. This is the DRM cal= lback that gets invoked by the shared timer. 3. **`vmw_vkms_get_vblank_timestamp()`**: Replaced the manual timestamp cal= culation with `drm_crtc_vblank_get_vblank_timeout(crtc, vblank_time)`. The = shared implementation in `drm_vblank.c:2285` handles the `!vblank->enabled`= case, the race-safe count+time read loop, and the one-frame correction =E2= =80=94 all of which were done (less robustly) inline before. This is a clea= r improvement. 4. **`vmw_vkms_enable_vblank()`**: Replaced `hrtimer_setup()` + `hrtimer_st= art()` with `drm_crtc_vblank_start_timer(crtc)`. The shared function calls = `drm_calc_timestamping_constants()` internally, so the explicit call is cor= rectly removed. 5. **`vmw_vkms_disable_vblank()`**: Replaced `hrtimer_cancel()` with `drm_c= rtc_vblank_cancel_timer()`. Note: the shared cancel uses `hrtimer_try_to_ca= ncel()` (non-blocking) plus an interval-zeroing approach to avoid deadlocks= , which is actually better than the old `hrtimer_cancel()` (blocking). The = `du->vkms.period_ns =3D ktime_set(0, 0)` clear is correctly removed since t= he period is now tracked internally. 6. **`vmw_vkms_crtc_cleanup()`**: The v2 fix: only cancels the timer when `= vmw->vkms_enabled` is true. This prevents calling into the vblank timer inf= rastructure when it was never initialized. Good fix addressing Zack's revie= w feedback. 7. **`vmwgfx_vkms.h`**: Removal of `#include ` =E2= =80=94 correct, no longer needed. **Style nit:** The `.handle_vblank_timeout` lines in ldu/scrn/stdu use doub= le space before `=3D`: ```c .handle_vblank_timeout =3D vmw_vkms_handle_vblank_timeout, ``` This appears to be intentional alignment with the other fields, but the sur= rounding fields use single-space (e.g., `.atomic_flush =3D ...`). This is v= ery minor and not worth a respin. **No functional issues found.** --- **Summary:** Series is well-structured, the conversion is correct, and the = v2 fixes address the previous review feedback. The shared DRM vblank timer = provides better race handling in the timestamp calculation than the old ope= n-coded version. Reviewed-by worthy. --- Generated by Claude Code Patch Reviewer