public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] drm/vmwgfx: Use DRM's vblank timer
@ 2026-04-27 15:00 Thomas Zimmermann
  2026-04-27 15:00 ` [PATCH v2 1/3] drm/vmwgfx: Determine lock-waiting timeout from vblank state Thomas Zimmermann
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Thomas Zimmermann @ 2026-04-27 15:00 UTC (permalink / raw)
  To: zack.rusin, bcm-kernel-feedback-list, maarten.lankhorst, mripard
  Cc: dri-devel, Thomas Zimmermann

Replace vmwgfx's vblank timer with DRM's shared implementation. The
common code is mostly equivalent, but provides additional protection
against concurrent changes to the vblank counter.

Patches 1 and 2 align vmwgfx with the shared code, so that it can be
converted easily.

The conversion is in patch 3. It replaces vmwgfx's custom timer with
calls to DRM's vblank interfaces. Initialization happens internally.

This series should get some more testing.

v2:
- patch 3: fix vblank cancalling in CRTC cleanup (Zack)
- patch 2: mention timer restart in commit message (Zack)

Thomas Zimmermann (3):
  drm/vmwgfx: Determine lock-waiting timeout from vblank state
  drm/vmwgfx: Move vblank handling into separate helper
  drm/vmwgfx: Convert to DRM vblank timers

 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |  2 -
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c  |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 70 ++++++++--------------------
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h |  2 +-
 6 files changed, 24 insertions(+), 53 deletions(-)

-- 
2.54.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/3] drm/vmwgfx: Determine lock-waiting timeout from vblank state
  2026-04-27 15:00 [PATCH v2 0/3] drm/vmwgfx: Use DRM's vblank timer Thomas Zimmermann
@ 2026-04-27 15:00 ` Thomas Zimmermann
  2026-04-28  4:31   ` Claude review: " Claude Code Review Bot
  2026-04-27 15:00 ` [PATCH v2 2/3] drm/vmwgfx: Move vblank handling into separate helper Thomas Zimmermann
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Thomas Zimmermann @ 2026-04-27 15:00 UTC (permalink / raw)
  To: zack.rusin, bcm-kernel-feedback-list, maarten.lankhorst, mripard
  Cc: dri-devel, Thomas Zimmermann

Use the calculated duration of a frame as stored in the vblank state
for the lock-waiting timeout. Decouples the waiting from the details
of the vblank implementation. Both values should be equal.

This will be helpful for replacing vmwgfx's vblank timer with DRM's
common implementation.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
index 5abd7f5ad2db..7862f6972512 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
@@ -516,9 +516,13 @@ vmw_vkms_set_crc_surface(struct drm_crtc *crtc,
 static inline u64
 vmw_vkms_lock_max_wait_ns(struct vmw_display_unit *du)
 {
-	s64 nsecs = ktime_to_ns(du->vkms.period_ns);
+	struct drm_crtc *crtc = &du->crtc;
+	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
+
+	if (!vblank || !vblank->framedur_ns)
+		return NSEC_PER_SEC / 60; /* disabled; assume 60 Hz */
 
-	return  (nsecs > 0) ? nsecs : 16666666;
+	return vblank->framedur_ns;
 }
 
 /**
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/3] drm/vmwgfx: Move vblank handling into separate helper
  2026-04-27 15:00 [PATCH v2 0/3] drm/vmwgfx: Use DRM's vblank timer Thomas Zimmermann
  2026-04-27 15:00 ` [PATCH v2 1/3] drm/vmwgfx: Determine lock-waiting timeout from vblank state Thomas Zimmermann
@ 2026-04-27 15:00 ` Thomas Zimmermann
  2026-04-28  4:31   ` Claude review: " Claude Code Review Bot
  2026-04-27 15:00 ` [PATCH v2 3/3] drm/vmwgfx: Convert to DRM vblank timers Thomas Zimmermann
  2026-04-28  4:31 ` Claude review: drm/vmwgfx: Use DRM's vblank timer Claude Code Review Bot
  3 siblings, 1 reply; 8+ messages in thread
From: Thomas Zimmermann @ 2026-04-27 15:00 UTC (permalink / raw)
  To: zack.rusin, bcm-kernel-feedback-list, maarten.lankhorst, mripard
  Cc: dri-devel, Thomas Zimmermann

Decouple vblank handling from the underlying hrtimer. This will be
helpful for replacing vmwgfx's vblank timer with DRM's common
implementation.

The new helper vmw_vkms_handle_vblank_timeout() can later be used as
callback for DRM's handle_vblank call as-is. The helper also keeps the
current semantics for restarting the timer. It returns true to restart
the next vblank timeout even if it could not acquire vmwgfx's vblank
lock.

The remaining code in vmw_vkms_vblank_simulate() will be replaced by
the DRM implementation in a later patch.

v2:
- clarify return-value semantics in commit message (Zack)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 37 +++++++++++++++++++---------
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h |  1 +
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
index 7862f6972512..15439ddd4f22 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
@@ -157,27 +157,19 @@ crc_generate_worker(struct work_struct *work)
 		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
 }
 
-static enum hrtimer_restart
-vmw_vkms_vblank_simulate(struct hrtimer *timer)
+bool
+vmw_vkms_handle_vblank_timeout(struct drm_crtc *crtc)
 {
-	struct vmw_display_unit *du = container_of(timer, struct vmw_display_unit, vkms.timer);
-	struct drm_crtc *crtc = &du->crtc;
+	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
 	struct vmw_private *vmw = vmw_priv(crtc->dev);
 	bool has_surface = false;
-	u64 ret_overrun;
 	bool locked, ret;
 
-	ret_overrun = hrtimer_forward_now(&du->vkms.timer,
-					  du->vkms.period_ns);
-	if (ret_overrun != 1)
-		drm_dbg_driver(crtc->dev, "vblank timer missed %lld frames.\n",
-			       ret_overrun - 1);
-
 	locked = vmw_vkms_vblank_trylock(crtc);
 	ret = drm_crtc_handle_vblank(crtc);
 	WARN_ON(!ret);
 	if (!locked)
-		return HRTIMER_RESTART;
+		return true;
 	has_surface = du->vkms.surface != NULL;
 	vmw_vkms_unlock(crtc);
 
@@ -200,6 +192,27 @@ vmw_vkms_vblank_simulate(struct hrtimer *timer)
 			drm_dbg_driver(crtc->dev, "Composer worker already queued\n");
 	}
 
+	return true;
+}
+
+static enum hrtimer_restart
+vmw_vkms_vblank_simulate(struct hrtimer *timer)
+{
+	struct vmw_display_unit *du = container_of(timer, struct vmw_display_unit, vkms.timer);
+	struct drm_crtc *crtc = &du->crtc;
+	u64 ret_overrun;
+	bool success;
+
+	ret_overrun = hrtimer_forward_now(&du->vkms.timer,
+					  du->vkms.period_ns);
+	if (ret_overrun != 1)
+		drm_dbg_driver(crtc->dev, "vblank timer missed %lld frames.\n",
+			       ret_overrun - 1);
+
+	success = vmw_vkms_handle_vblank_timeout(crtc);
+	if (!success)
+		return HRTIMER_NORESTART;
+
 	return HRTIMER_RESTART;
 }
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
index 69ddd33a8444..0b6bbf7c4487 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
@@ -45,6 +45,7 @@ bool vmw_vkms_modeset_lock_relaxed(struct drm_crtc *crtc);
 bool vmw_vkms_vblank_trylock(struct drm_crtc *crtc);
 void vmw_vkms_unlock(struct drm_crtc *crtc);
 
+bool vmw_vkms_handle_vblank_timeout(struct drm_crtc *crtc);
 bool vmw_vkms_get_vblank_timestamp(struct drm_crtc *crtc,
 				   int *max_error,
 				   ktime_t *vblank_time,
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/3] drm/vmwgfx: Convert to DRM vblank timers
  2026-04-27 15:00 [PATCH v2 0/3] drm/vmwgfx: Use DRM's vblank timer Thomas Zimmermann
  2026-04-27 15:00 ` [PATCH v2 1/3] drm/vmwgfx: Determine lock-waiting timeout from vblank state Thomas Zimmermann
  2026-04-27 15:00 ` [PATCH v2 2/3] drm/vmwgfx: Move vblank handling into separate helper Thomas Zimmermann
@ 2026-04-27 15:00 ` Thomas Zimmermann
  2026-04-28  4:31   ` Claude review: " Claude Code Review Bot
  2026-04-28  4:31 ` Claude review: drm/vmwgfx: Use DRM's vblank timer Claude Code Review Bot
  3 siblings, 1 reply; 8+ messages in thread
From: Thomas Zimmermann @ 2026-04-27 15:00 UTC (permalink / raw)
  To: zack.rusin, bcm-kernel-feedback-list, maarten.lankhorst, mripard
  Cc: dri-devel, Thomas Zimmermann

Replace vmwgfx's vblank timer with DRM's common implementation. The
timer handling is almost identical with a few additional bug fixes in
the common code.

Replace most of vmwgfx's vmw_vkms_get_vblank_timestamp() with the
shared helper drm_crtc_vblank_get_vblank_timeout(). The common helper
also works in the presence of delayed vblank timeouts that modify the
vblank counter concurrently.

Set the timeout handler to vmw_vkms_handle_vblank_timeout(). In addition
to handling vblank events, this function also controls CRC generation.

Remove all the hrtimer-related code from vmwgfx. DRM vblank timers
provides this.

v2:
- only cancel vblank timer in CRTC cleanup if vkms_enabled (Zack)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |  2 -
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c  |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 65 ++++------------------------
 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h |  1 -
 6 files changed, 12 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 445471fe9be6..b5670ece90a9 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -322,8 +322,6 @@ struct vmw_display_unit {
 
 	struct {
 		struct work_struct crc_generator_work;
-		struct hrtimer timer;
-		ktime_t period_ns;
 
 		/* protects concurrent access to the vblank handler */
 		atomic_t atomic_lock;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
index f07669b27fba..6f22d5f5c379 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
@@ -402,6 +402,7 @@ static const struct drm_crtc_helper_funcs vmw_ldu_crtc_helper_funcs = {
 	.atomic_flush = vmw_vkms_crtc_atomic_flush,
 	.atomic_enable = vmw_vkms_crtc_atomic_enable,
 	.atomic_disable = vmw_vkms_crtc_atomic_disable,
+	.handle_vblank_timeout  = vmw_vkms_handle_vblank_timeout,
 };
 
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index 605d037d18c6..cfb8ad3ebe43 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -797,6 +797,7 @@ static const struct drm_crtc_helper_funcs vmw_sou_crtc_helper_funcs = {
 	.atomic_flush = vmw_vkms_crtc_atomic_flush,
 	.atomic_enable = vmw_vkms_crtc_atomic_enable,
 	.atomic_disable = vmw_sou_crtc_atomic_disable,
+	.handle_vblank_timeout  = vmw_vkms_handle_vblank_timeout,
 };
 
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index dcbacee97f61..1f5497e2638a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -1515,6 +1515,7 @@ static const struct drm_crtc_helper_funcs vmw_stdu_crtc_helper_funcs = {
 	.atomic_flush = vmw_stdu_crtc_atomic_flush,
 	.atomic_enable = vmw_vkms_crtc_atomic_enable,
 	.atomic_disable = vmw_stdu_crtc_atomic_disable,
+	.handle_vblank_timeout  = vmw_vkms_handle_vblank_timeout,
 };
 
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
index 15439ddd4f22..1f98aa04c9f3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
@@ -195,27 +195,6 @@ vmw_vkms_handle_vblank_timeout(struct drm_crtc *crtc)
 	return true;
 }
 
-static enum hrtimer_restart
-vmw_vkms_vblank_simulate(struct hrtimer *timer)
-{
-	struct vmw_display_unit *du = container_of(timer, struct vmw_display_unit, vkms.timer);
-	struct drm_crtc *crtc = &du->crtc;
-	u64 ret_overrun;
-	bool success;
-
-	ret_overrun = hrtimer_forward_now(&du->vkms.timer,
-					  du->vkms.period_ns);
-	if (ret_overrun != 1)
-		drm_dbg_driver(crtc->dev, "vblank timer missed %lld frames.\n",
-			       ret_overrun - 1);
-
-	success = vmw_vkms_handle_vblank_timeout(crtc);
-	if (!success)
-		return HRTIMER_NORESTART;
-
-	return HRTIMER_RESTART;
-}
-
 void
 vmw_vkms_init(struct vmw_private *vmw)
 {
@@ -258,32 +237,12 @@ vmw_vkms_get_vblank_timestamp(struct drm_crtc *crtc,
 			      ktime_t *vblank_time,
 			      bool in_vblank_irq)
 {
-	struct drm_device *dev = crtc->dev;
-	struct vmw_private *vmw = vmw_priv(dev);
-	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
-	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
+	struct vmw_private *vmw = vmw_priv(crtc->dev);
 
 	if (!vmw->vkms_enabled)
 		return false;
 
-	if (!READ_ONCE(vblank->enabled)) {
-		*vblank_time = ktime_get();
-		return true;
-	}
-
-	*vblank_time = READ_ONCE(du->vkms.timer.node.expires);
-
-	if (WARN_ON(*vblank_time == vblank->time))
-		return true;
-
-	/*
-	 * To prevent races we roll the hrtimer forward before we do any
-	 * interrupt processing - this is how real hw works (the interrupt is
-	 * only generated after all the vblank registers are updated) and what
-	 * the vblank core expects. Therefore we need to always correct the
-	 * timestampe by one frame.
-	 */
-	*vblank_time -= du->vkms.period_ns;
+	drm_crtc_vblank_get_vblank_timeout(crtc, vblank_time);
 
 	return true;
 }
@@ -293,20 +252,11 @@ vmw_vkms_enable_vblank(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct vmw_private *vmw = vmw_priv(dev);
-	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
-	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
 
 	if (!vmw->vkms_enabled)
 		return -EINVAL;
 
-	drm_calc_timestamping_constants(crtc, &crtc->mode);
-
-	hrtimer_setup(&du->vkms.timer, &vmw_vkms_vblank_simulate, CLOCK_MONOTONIC,
-		      HRTIMER_MODE_REL);
-	du->vkms.period_ns = ktime_set(0, vblank->framedur_ns);
-	hrtimer_start(&du->vkms.timer, du->vkms.period_ns, HRTIMER_MODE_REL);
-
-	return 0;
+	return drm_crtc_vblank_start_timer(crtc);
 }
 
 void
@@ -318,9 +268,9 @@ vmw_vkms_disable_vblank(struct drm_crtc *crtc)
 	if (!vmw->vkms_enabled)
 		return;
 
-	hrtimer_cancel(&du->vkms.timer);
+	drm_crtc_vblank_cancel_timer(crtc);
+
 	du->vkms.surface = NULL;
-	du->vkms.period_ns = ktime_set(0, 0);
 }
 
 enum vmw_vkms_lock_state {
@@ -344,12 +294,15 @@ vmw_vkms_crtc_init(struct drm_crtc *crtc)
 void
 vmw_vkms_crtc_cleanup(struct drm_crtc *crtc)
 {
+	struct vmw_private *vmw = vmw_priv(crtc->dev);
 	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
 
+	if (vmw->vkms_enabled)
+		drm_crtc_vblank_cancel_timer(crtc);
+
 	if (du->vkms.surface)
 		vmw_surface_unreference(&du->vkms.surface);
 	WARN_ON(work_pending(&du->vkms.crc_generator_work));
-	hrtimer_cancel(&du->vkms.timer);
 }
 
 void
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
index 0b6bbf7c4487..40c679f59db2 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
@@ -29,7 +29,6 @@
 #ifndef VMWGFX_VKMS_H_
 #define VMWGFX_VKMS_H_
 
-#include <linux/hrtimer_types.h>
 #include <linux/types.h>
 
 struct drm_atomic_state;
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Claude review: drm/vmwgfx: Use DRM's vblank timer
  2026-04-27 15:00 [PATCH v2 0/3] drm/vmwgfx: Use DRM's vblank timer Thomas Zimmermann
                   ` (2 preceding siblings ...)
  2026-04-27 15:00 ` [PATCH v2 3/3] drm/vmwgfx: Convert to DRM vblank timers Thomas Zimmermann
@ 2026-04-28  4:31 ` Claude Code Review Bot
  3 siblings, 0 replies; 8+ messages in thread
From: Claude Code Review Bot @ 2026-04-28  4:31 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/vmwgfx: Use DRM's vblank timer
Author: Thomas Zimmermann <tzimmermann@suse.de>
Patches: 4
Reviewed: 2026-04-28T14:31:50.265693

---

This is a clean, well-structured 3-patch series that replaces vmwgfx's custom hrtimer-based vblank simulation with the shared DRM vblank timer infrastructure. The patches are logically ordered: patch 1 decouples the lock timeout from the timer internals, patch 2 extracts the vblank handling into a callback-compatible helper, and patch 3 performs the actual conversion. The series is a net code reduction (~24 insertions vs ~53 deletions) and the shared DRM implementation provides better race protection in `drm_crtc_vblank_get_vblank_timeout()`.

The series looks correct and ready to merge. I have one minor concern about patch 1 and a minor style nit, but nothing blocking.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Claude review: drm/vmwgfx: Determine lock-waiting timeout from vblank state
  2026-04-27 15:00 ` [PATCH v2 1/3] drm/vmwgfx: Determine lock-waiting timeout from vblank state Thomas Zimmermann
@ 2026-04-28  4:31   ` Claude Code Review Bot
  0 siblings, 0 replies; 8+ messages in thread
From: Claude Code Review Bot @ 2026-04-28  4:31 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Purpose:** Switches `vmw_vkms_lock_max_wait_ns()` from reading `du->vkms.period_ns` (the hrtimer period) to reading `vblank->framedur_ns` from the DRM vblank core. This decouples the lock timeout from the timer implementation details.

**Assessment: Looks good.**

The change from:
```c
s64 nsecs = ktime_to_ns(du->vkms.period_ns);
return (nsecs > 0) ? nsecs : 16666666;
```
to:
```c
struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
if (!vblank || !vblank->framedur_ns)
    return NSEC_PER_SEC / 60;
return vblank->framedur_ns;
```

is semantically equivalent. The fallback value `NSEC_PER_SEC / 60 = 16666666` matches the old hardcoded `16666666`.

**Minor observation:** `drm_crtc_vblank_crtc()` is a simple index into `dev->vblank`, which is allocated by `drm_vblank_init()`. The `!vblank` check is defensive — in practice, if `drm_vblank_init()` succeeded (which is required for `vkms_enabled` to be true), this shouldn't be NULL. But it's harmless and provides safety if the function is called before vblank init, which is reasonable.

No issues.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Claude review: drm/vmwgfx: Move vblank handling into separate helper
  2026-04-27 15:00 ` [PATCH v2 2/3] drm/vmwgfx: Move vblank handling into separate helper Thomas Zimmermann
@ 2026-04-28  4:31   ` Claude Code Review Bot
  0 siblings, 0 replies; 8+ messages in thread
From: Claude Code Review Bot @ 2026-04-28  4:31 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Purpose:** Extracts the vblank event handling and CRC work scheduling from `vmw_vkms_vblank_simulate()` into a new `vmw_vkms_handle_vblank_timeout()` function that matches the `handle_vblank_timeout` callback signature (`bool (*)(struct drm_crtc *)`).

**Assessment: Looks good.**

The split is mechanical and correct. The new `vmw_vkms_handle_vblank_timeout()` contains the vblank handling logic:

```c
bool vmw_vkms_handle_vblank_timeout(struct drm_crtc *crtc)
{
    ...
    locked = vmw_vkms_vblank_trylock(crtc);
    ret = drm_crtc_handle_vblank(crtc);
    WARN_ON(!ret);
    if (!locked)
        return true;
    ...
    return true;
}
```

The function always returns `true` (restart the timer), which preserves the existing behavior: the old code only returned `HRTIMER_NORESTART` when `vmw_vkms_handle_vblank_timeout()` returned false, but it never does. The commit message correctly notes this: "returns true to restart the next vblank timeout even if it could not acquire vmwgfx's vblank lock."

The remaining `vmw_vkms_vblank_simulate()` wrapper is a thin shim that handles the hrtimer-specific forward logic and delegates to the new helper. This wrapper is then cleanly removed in patch 3.

The declaration in `vmwgfx_vkms.h` is correctly added.

No issues.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Claude review: drm/vmwgfx: Convert to DRM vblank timers
  2026-04-27 15:00 ` [PATCH v2 3/3] drm/vmwgfx: Convert to DRM vblank timers Thomas Zimmermann
@ 2026-04-28  4:31   ` Claude Code Review Bot
  0 siblings, 0 replies; 8+ messages in thread
From: Claude Code Review Bot @ 2026-04-28  4:31 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Purpose:** The actual conversion — removes custom hrtimer code and 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` from the `vkms` struct in `vmw_display_unit` — correct, these are now managed by the DRM vblank timer infrastructure inside `drm_vblank_crtc`.

2. **LDU/SCRN/STDU helper_funcs**: All three display backends wire up `.handle_vblank_timeout = vmw_vkms_handle_vblank_timeout`. This is the DRM callback that gets invoked by the shared timer.

3. **`vmw_vkms_get_vblank_timestamp()`**: Replaced the manual timestamp calculation 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 — all of which were done (less robustly) inline before. This is a clear improvement.

4. **`vmw_vkms_enable_vblank()`**: Replaced `hrtimer_setup()` + `hrtimer_start()` with `drm_crtc_vblank_start_timer(crtc)`. The shared function calls `drm_calc_timestamping_constants()` internally, so the explicit call is correctly removed.

5. **`vmw_vkms_disable_vblank()`**: Replaced `hrtimer_cancel()` with `drm_crtc_vblank_cancel_timer()`. Note: the shared cancel uses `hrtimer_try_to_cancel()` (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 = ktime_set(0, 0)` clear is correctly removed since the 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 infrastructure when it was never initialized. Good fix addressing Zack's review feedback.

7. **`vmwgfx_vkms.h`**: Removal of `#include <linux/hrtimer_types.h>` — correct, no longer needed.

**Style nit:** The `.handle_vblank_timeout` lines in ldu/scrn/stdu use double space before `=`:
```c
.handle_vblank_timeout  = vmw_vkms_handle_vblank_timeout,
```
This appears to be intentional alignment with the other fields, but the surrounding fields use single-space (e.g., `.atomic_flush = ...`). This is very 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 open-coded version. Reviewed-by worthy.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-04-28  4:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-27 15:00 [PATCH v2 0/3] drm/vmwgfx: Use DRM's vblank timer Thomas Zimmermann
2026-04-27 15:00 ` [PATCH v2 1/3] drm/vmwgfx: Determine lock-waiting timeout from vblank state Thomas Zimmermann
2026-04-28  4:31   ` Claude review: " Claude Code Review Bot
2026-04-27 15:00 ` [PATCH v2 2/3] drm/vmwgfx: Move vblank handling into separate helper Thomas Zimmermann
2026-04-28  4:31   ` Claude review: " Claude Code Review Bot
2026-04-27 15:00 ` [PATCH v2 3/3] drm/vmwgfx: Convert to DRM vblank timers Thomas Zimmermann
2026-04-28  4:31   ` Claude review: " Claude Code Review Bot
2026-04-28  4:31 ` Claude review: drm/vmwgfx: Use DRM's vblank timer Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox