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/vblank: timer: Avoid reading the vblank time unnecessarily Date: Thu, 04 Jun 2026 14:04:00 +1000 Message-ID: In-Reply-To: <20260601141922.91498-8-tzimmermann@suse.de> References: <20260601141922.91498-1-tzimmermann@suse.de> <20260601141922.91498-8-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 **Optimization and robustness improvement.** The change from double `drm_cr= tc_vblank_count_and_time()` to initial `drm_crtc_vblank_count()` + single `= drm_crtc_vblank_count_and_time()` per iteration avoids one seqlock read per= iteration. **The retry limit of 10 with `drm_WARN_ON` is reasonable** as a backstop. I= f the vblank counter changes 10+ times during this read loop, something is = fundamentally broken. **Subtle loop behavior:** The condition `cur_count !=3D lst_count && retrie= s--` means the `retries--` side effect only happens when the count actually= changed. If the count is stable on the first try, `retries` is never decre= mented and `cur_time` is set correctly. This is correct. **Minor:** `cur_count` is read initially with `drm_crtc_vblank_count()` (wh= ich goes through `drm_vblank_count` =E2=86=92 atomic64_read), while subsequ= ent reads use `drm_crtc_vblank_count_and_time()` (which goes through the se= qlock). The `drm_crtc_vblank_count` path doesn't have the seqlock barrier t= hat `drm_crtc_vblank_count_and_time` has. This means the initial `cur_count= ` could theoretically be stale relative to the following `*vblank_time =3D = READ_ONCE(vtimer->timer.node.expires)` read. However, since we're comparing= with the post-read count, this is caught by the loop =E2=80=94 the loop si= mply runs one extra iteration in that case. No correctness issue, just a th= eoretical efficiency consideration. No functional issues in this patch. --- **Summary of key issues requiring attention:** 1. **Clock domain mismatch (Patches 3, 5):** Replace `ktime_get_ns()` with = `ktime_get_boottime_ns()` in `drm_crtc_vblank_start_timer()` and the disabl= ed-timer estimation path, or keep `CLOCK_MONOTONIC` throughout. The compari= son between timer expiry and vblank core timestamps also needs reconciliati= on. 2. **Missing vmwgfx update (Patch 1):** `vmw_vkms_get_vblank_timestamp()` m= ust propagate the new return value. 3. **Division by zero risk (Patch 3):** Guard `roundup(..., vblank->framedu= r_ns)` against zero denominator. --- Generated by Claude Code Patch Reviewer