[PATCH 0/7] drm/vblank: timer: Fix timestamps and improve reliabilty

[PATCH 0/7] drm/vblank: timer: Fix timestamps and improve reliabilty

[Thomas Zimmermann]

It appears that the DRM's vblank timers have always been somewhat buggy
in their timestamp calculations. Fix the generic implementation and
improve reliability.

Patch 1 returns success/failure to the caller, as expected by DRM.

Patch 2 fixes the timestamp calculation to return the time of the first
visible scanline in the current vblank phase.

Patch 3 switches the hrtimer to absolute values since boot, so that it's
easier to work with and we can predict future timestamps reliably.

Patches 4 and 5 allow for estimating timestamps even with the timer
being disabled.

Patches 6 and 7 improve the retrieval of the next vblank timeout's
timestamp.

Tested under heavy CPU-load on bochs and virtio. No ersors or warnings
showed up.

There have been reports about vblank timeouts being handled so late,
that they trigger DRM's internal error checks. Maybe these fixes and
adjustments can help to further avoid hick ups from delayed vblank
timers.

Thomas Zimmermann (7):
  drm/vblank: timer: Return success status from get_vblank_timeout
  drm/vblank: timer: Fix timestamp calculation
  drm/vblank: timer: Use absolute timer since boot
  drm/vblank: timer: Reorganize get_vblank_timeout
  drm/vblank: timer: Estimate vblank timeout if timer is disabled
  drm/vblank: timer: Verify that expiry time is in the future
  drm/vblank: timer: Avoid reading the vblank time unnecessarily

 drivers/gpu/drm/drm_vblank.c        | 105 ++++++++++++++++++++--------
 drivers/gpu/drm/drm_vblank_helper.c |   4 +-
 include/drm/drm_vblank.h            |   2 +-
 3 files changed, 79 insertions(+), 32 deletions(-)


base-commit: 4f554688dffcacf48630c14f9fb77a9f60394c1c
-- 
2.54.0

[PATCH 1/7] drm/vblank: timer: Return success status from get_vblank_timeout

[Thomas Zimmermann]

Return true/false from drm_crtc_vblank_get_vblank_timeout(), depending
on the success of the calculation. Let caller handle failure by itself.

Until now the helper tried to return a vblank time even in the case of
an error. Letting the caller handle the failure is the preferred behavior.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_vblank.c        | 15 +++++++++------
 drivers/gpu/drm/drm_vblank_helper.c |  4 +---
 include/drm/drm_vblank.h            |  2 +-
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f90fb2d13e42..96d70c3d4522 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -2287,18 +2287,19 @@ EXPORT_SYMBOL(drm_crtc_vblank_cancel_timer);
  * The helper drm_crtc_vblank_get_vblank_timeout() returns the next vblank
  * timestamp of the CRTC's vblank timer according to the timer's expiry
  * time.
+ *
+ * Returns:
+ * True on success, or false otherwise.
  */
-void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time)
+bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time)
 {
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
 	struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
 	u64 cur_count;
 	ktime_t cur_time;
 
-	if (!READ_ONCE(vblank->enabled)) {
-		*vblank_time = ktime_get();
-		return;
-	}
+	if (!READ_ONCE(vblank->enabled))
+		return false;
 
 	/*
 	 * A concurrent vblank timeout could update the expires field before
@@ -2312,7 +2313,7 @@ void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
 	} while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
 
 	if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
-		return; /* Already expired */
+		return false; /* Already expired */
 
 	/*
 	 * To prevent races we roll the hrtimer forward before we do any
@@ -2322,5 +2323,7 @@ void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
 	 * correct the timestamp by one frame.
 	 */
 	*vblank_time = ktime_sub(*vblank_time, vtimer->interval);
+
+	return true;
 }
 EXPORT_SYMBOL(drm_crtc_vblank_get_vblank_timeout);
diff --git a/drivers/gpu/drm/drm_vblank_helper.c b/drivers/gpu/drm/drm_vblank_helper.c
index d3f8147ecdc1..aa8df047b2aa 100644
--- a/drivers/gpu/drm/drm_vblank_helper.c
+++ b/drivers/gpu/drm/drm_vblank_helper.c
@@ -169,8 +169,6 @@ bool drm_crtc_vblank_helper_get_vblank_timestamp_from_timer(struct drm_crtc *crt
 							    ktime_t *vblank_time,
 							    bool in_vblank_irq)
 {
-	drm_crtc_vblank_get_vblank_timeout(crtc, vblank_time);
-
-	return true;
+	return drm_crtc_vblank_get_vblank_timeout(crtc, vblank_time);
 }
 EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp_from_timer);
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 2fcef9c0f5b1..1c06e4499dae 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -319,7 +319,7 @@ void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
 
 int drm_crtc_vblank_start_timer(struct drm_crtc *crtc);
 void drm_crtc_vblank_cancel_timer(struct drm_crtc *crtc);
-void drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time);
+bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time);
 
 /*
  * Helpers for struct drm_crtc_funcs
-- 
2.54.0

[PATCH 2/7] drm/vblank: timer: Fix timestamp calculation

[Thomas Zimmermann]

In drm_crtc_vblank_get_vblank_timeout(), return the timestamp of the
first visible scanline after the last vblank timeout. This is what the
caller expects.

A vblank phase starts with a vblank timeout. At this point the display
is blanked for several scanlines. Afterwards the display is unblanked
until the next vblank timeout occurs. The display content is only visible
during that second part.

The current implementation of drm_crtc_vblank_get_vblank_timeout()
returns the timestamp of the last vblank timeout that started the current
vblank phase. But the display only unblanks after 20 to 30 percent of
the overall frame duration. The returned timestamp is therefore too early.

The next vblank timeout is already known when calculating the returned
timestamp. Instead of subtracting the duration of a full frame from the
value, only subtract the duration of the active, visible part. The result
is the timestamp of the first visible scanline, as expected by the caller.

This bug was not introduced by the generic vblank timer. It appears that
the get_vblank_timeout logic has always been buggy since it was first
added in commit 3a0709928b17 ("drm/vkms: Add vblank events simulated by
hrtimers").

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_vblank.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 96d70c3d4522..d52df247d04e 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -2293,10 +2293,19 @@ EXPORT_SYMBOL(drm_crtc_vblank_cancel_timer);
  */
 bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_time)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
 	struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
+	const struct drm_display_mode *mode;
 	u64 cur_count;
 	ktime_t cur_time;
+	s64 framedur_ns;
+	s64 activedur_ns;
+
+	if (drm_drv_uses_atomic_modeset(dev))
+		mode = &vblank->hwmode;
+	else
+		mode = &crtc->hwmode;
 
 	if (!READ_ONCE(vblank->enabled))
 		return false;
@@ -2312,17 +2321,26 @@ bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
 		*vblank_time = READ_ONCE(vtimer->timer.node.expires);
 	} while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
 
-	if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
+	if (drm_WARN_ON(dev, !ktime_compare(*vblank_time, cur_time)))
 		return false; /* Already expired */
 
+	framedur_ns = vblank->framedur_ns;
+
 	/*
-	 * 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 timestamp by one frame.
+	 * To prevent races we rolled the hrtimer forward before we did any
+	 * timeout 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 always need to correct the timestamp. The returned
+	 * time should be the time of the first active scanline after the
+	 * previous vblank. Hence subtract the active phase's duration from
+	 * the next expiration time.
 	 */
-	*vblank_time = ktime_sub(*vblank_time, vtimer->interval);
+	if (drm_WARN_ON(dev, !mode->crtc_vtotal))
+		return false;
+	activedur_ns = div_s64(framedur_ns * mode->crtc_vdisplay, mode->crtc_vtotal);
+	*vblank_time = ktime_sub_ns(*vblank_time, activedur_ns);
 
 	return true;
 }
-- 
2.54.0

[PATCH 3/7] drm/vblank: timer: Use absolute timer since boot

[Thomas Zimmermann]

Replace HRTIMER_MODE_REL with HRTIMER_MODE_ABS. Align the vblank
timeouts at multiples of the current mode's frame duration. Use
CLOCK_BOOTTIME to avoid clock gaps from suspends. Allows the timer
code to easily estimate future timeouts even while the timer is
disabled.

Also add a separate error message for cases where the timeout handler
tries to forward an unexpired timer. This would indicate a problem in
how the timer is being set up.

Suggested-by: Michel Dänzer <michel.daenzer@mailbox.org>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_vblank.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index d52df247d04e..03b07e3c2598 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -2192,7 +2192,9 @@ static enum hrtimer_restart drm_vblank_timer_function(struct hrtimer *timer)
 		return HRTIMER_NORESTART;
 
 	ret_overrun = hrtimer_forward_now(&vtimer->timer, interval);
-	if (ret_overrun != 1)
+	if (!ret_overrun)
+		drm_dbg_vbl(dev, "vblank timer underrun\n");
+	else if (ret_overrun != 1)
 		drm_dbg_vbl(dev, "vblank timer overrun\n");
 
 	if (crtc_funcs->handle_vblank_timeout)
@@ -2221,6 +2223,7 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc)
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
 	struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
 	unsigned long flags;
+	s64 vblank_time_ns;
 
 	if (!vtimer->crtc) {
 		/*
@@ -2229,7 +2232,7 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc)
 		vtimer->crtc = crtc;
 		spin_lock_init(&vtimer->interval_lock);
 		hrtimer_setup(&vtimer->timer, drm_vblank_timer_function,
-			      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+			      CLOCK_BOOTTIME, HRTIMER_MODE_ABS);
 	} else {
 		/*
 		 * Timer should not be active. If it is, wait for the
@@ -2245,7 +2248,13 @@ int drm_crtc_vblank_start_timer(struct drm_crtc *crtc)
 	vtimer->interval = ns_to_ktime(vblank->framedur_ns);
 	spin_unlock_irqrestore(&vtimer->interval_lock, flags);
 
-	hrtimer_start(&vtimer->timer, vtimer->interval, HRTIMER_MODE_REL);
+	/*
+	 * Always align the vblank timeout to the frame duration. Allows
+	 * for estimating the next vblank even if the hrtimer has been
+	 * disabled.
+	 */
+	vblank_time_ns = roundup(ktime_get_ns(), vblank->framedur_ns);
+	hrtimer_start(&vtimer->timer, ns_to_ktime(vblank_time_ns), HRTIMER_MODE_ABS);
 
 	return 0;
 }
-- 
2.54.0

[PATCH 4/7] drm/vblank: timer: Reorganize get_vblank_timeout

[Thomas Zimmermann]

Handle vblank->enabled in a separate branch before handling the
opposite case. Prepares the code for estimating the vblank timeout
while vblanking is disabled. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_vblank.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 03b07e3c2598..cecaef98aa52 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -2306,8 +2306,6 @@ bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
 	struct drm_vblank_crtc_timer *vtimer = &vblank->vblank_timer;
 	const struct drm_display_mode *mode;
-	u64 cur_count;
-	ktime_t cur_time;
 	s64 framedur_ns;
 	s64 activedur_ns;
 
@@ -2316,24 +2314,28 @@ bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
 	else
 		mode = &crtc->hwmode;
 
-	if (!READ_ONCE(vblank->enabled))
-		return false;
+	if (READ_ONCE(vblank->enabled)) {
+		ktime_t cur_time;
+		u64 cur_count;
 
-	/*
-	 * A concurrent vblank timeout could update the expires field before
-	 * we compare it with the vblank time. Hence we'd compare the old
-	 * expiry time to the new vblank time; deducing the timer had already
-	 * expired. Reread until we get consistent values from both fields.
-	 */
-	do {
-		cur_count = drm_crtc_vblank_count_and_time(crtc, &cur_time);
-		*vblank_time = READ_ONCE(vtimer->timer.node.expires);
-	} while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
+		/*
+		 * A concurrent vblank timeout could update the expires field before
+		 * we compare it with the vblank time. Hence we'd compare the old
+		 * expiry time to the new vblank time; deducing the timer had already
+		 * expired. Reread until we get consistent values from both fields.
+		 */
+		do {
+			cur_count = drm_crtc_vblank_count_and_time(crtc, &cur_time);
+			*vblank_time = READ_ONCE(vtimer->timer.node.expires);
+		} while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
 
-	if (drm_WARN_ON(dev, !ktime_compare(*vblank_time, cur_time)))
-		return false; /* Already expired */
+		if (drm_WARN_ON(dev, !ktime_compare(*vblank_time, cur_time)))
+			return false; /* already expired */
 
-	framedur_ns = vblank->framedur_ns;
+		framedur_ns = vblank->framedur_ns;
+	} else {
+		return false;
+	}
 
 	/*
 	 * To prevent races we rolled the hrtimer forward before we did any
-- 
2.54.0

[PATCH 5/7] drm/vblank: timer: Estimate vblank timeout if timer is disabled

[Thomas Zimmermann]

Estimate the next vblank timeout from the duration of a frame in
the currently programmed display mode. Timeouts are aligned to
frame duration, so we can round up to the next alignment.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_vblank.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index cecaef98aa52..b5d2fb741b2d 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -2333,6 +2333,17 @@ bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
 			return false; /* already expired */
 
 		framedur_ns = vblank->framedur_ns;
+	} else if (mode->crtc_clock) {
+		u64 framesize = mode->crtc_htotal * mode->crtc_vtotal;
+
+		/*
+		 * With the vblank timer being disabled, we don't have an
+		 * expiry time. As the timeouts are aligned to the display
+		 * mode's clock, we can estimate when the expiry time would
+		 * have been.
+		 */
+		framedur_ns = div_u64(framesize * 1000000llu, mode->crtc_clock);
+		*vblank_time = roundup(ktime_get_ns(), framedur_ns);
 	} else {
 		return false;
 	}
-- 
2.54.0

[PATCH 6/7] drm/vblank: timer: Verify that expiry time is in the future

[Thomas Zimmermann]

The timer expiry must be later than the current vblank timestamp. By
testing with !ktime_compare(), the expiry time could also be before
the vblank timestamp. Use ktime_after() to verify that the timer expires
in the future.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_vblank.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index b5d2fb741b2d..75e2183be0ab 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -2329,7 +2329,7 @@ bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
 			*vblank_time = READ_ONCE(vtimer->timer.node.expires);
 		} while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
 
-		if (drm_WARN_ON(dev, !ktime_compare(*vblank_time, cur_time)))
+		if (drm_WARN_ON(dev, !ktime_after(*vblank_time, cur_time)))
 			return false; /* already expired */
 
 		framedur_ns = vblank->framedur_ns;
-- 
2.54.0

[PATCH 7/7] drm/vblank: timer: Avoid reading the vblank time unnecessarily

[Thomas Zimmermann]

In drm_crtc_vblank_get_vblank_timeout(), there's a loop to read
a consistent vblank count and time. Only read the time once per
iteration and avoid costly locking and an atomic read.

Return an error after 10 retries. This indicates that the vblank
counter is broken or being updated way too often.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_vblank.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 75e2183be0ab..05f28e27cbff 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -2316,7 +2316,8 @@ bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
 
 	if (READ_ONCE(vblank->enabled)) {
 		ktime_t cur_time;
-		u64 cur_count;
+		u64 lst_count, cur_count;
+		unsigned int retries = 10;
 
 		/*
 		 * A concurrent vblank timeout could update the expires field before
@@ -2324,10 +2325,15 @@ bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
 		 * expiry time to the new vblank time; deducing the timer had already
 		 * expired. Reread until we get consistent values from both fields.
 		 */
+		cur_count = drm_crtc_vblank_count(crtc);
 		do {
-			cur_count = drm_crtc_vblank_count_and_time(crtc, &cur_time);
+			lst_count = cur_count;
 			*vblank_time = READ_ONCE(vtimer->timer.node.expires);
-		} while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
+			cur_count = drm_crtc_vblank_count_and_time(crtc, &cur_time);
+		} while (cur_count != lst_count && retries--);
+
+		if (drm_WARN_ON(dev, cur_count != lst_count))
+			return false; /* broken vblank counter */
 
 		if (drm_WARN_ON(dev, !ktime_after(*vblank_time, cur_time)))
 			return false; /* already expired */
-- 
2.54.0

Re: [PATCH 2/7] drm/vblank: timer: Fix timestamp calculation

[Michel Dänzer]

On 6/1/26 16:08, Thomas Zimmermann wrote:
> In drm_crtc_vblank_get_vblank_timeout(), return the timestamp of the
> first visible scanline after the last vblank timeout. This is what the
> caller expects.
> 
> A vblank phase starts with a vblank timeout. At this point the display
> is blanked for several scanlines. Afterwards the display is unblanked
> until the next vblank timeout occurs. The display content is only visible
> during that second part.
> 
> The current implementation of drm_crtc_vblank_get_vblank_timeout()
> returns the timestamp of the last vblank timeout that started the current
> vblank phase. But the display only unblanks after 20 to 30 percent of
> the overall frame duration. The returned timestamp is therefore too early.
> 
> The next vblank timeout is already known when calculating the returned
> timestamp. Instead of subtracting the duration of a full frame from the
> value, only subtract the duration of the active, visible part. The result
> is the timestamp of the first visible scanline, as expected by the caller.
> 
> This bug was not introduced by the generic vblank timer. It appears that
> the get_vblank_timeout logic has always been buggy since it was first
> added in commit 3a0709928b17 ("drm/vkms: Add vblank events simulated by
> hrtimers").
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_vblank.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 96d70c3d4522..d52df247d04e 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> [...]
> @@ -2312,17 +2321,26 @@ bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
>  		*vblank_time = READ_ONCE(vtimer->timer.node.expires);
>  	} while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
>  
> -	if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
> +	if (drm_WARN_ON(dev, !ktime_compare(*vblank_time, cur_time)))
>  		return false; /* Already expired */
>  
> +	framedur_ns = vblank->framedur_ns;
> +
>  	/*
> -	 * 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 timestamp by one frame.
> +	 * To prevent races we rolled the hrtimer forward before we did any
> +	 * timeout 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 always need to correct the timestamp. The returned
> +	 * time should be the time of the first active scanline after the
> +	 * previous vblank. Hence subtract the active phase's duration from
> +	 * the next expiration time.
>  	 */
> -	*vblank_time = ktime_sub(*vblank_time, vtimer->interval);
> +	if (drm_WARN_ON(dev, !mode->crtc_vtotal))
> +		return false;
> +	activedur_ns = div_s64(framedur_ns * mode->crtc_vdisplay, mode->crtc_vtotal);
> +	*vblank_time = ktime_sub_ns(*vblank_time, activedur_ns);

Normally the timestamp returned by drm_crtc_vblank_count_and_time is supposed to correspond to the end of vertical blank / start of active, in which case the new code here looks wrong.

Also, while the current time is inside an active area, it's supposed to return the timestamp corresponding to the start of the current active area, not the next one.


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Re: [PATCH 2/7] drm/vblank: timer: Fix timestamp calculation

[Thomas Zimmermann]

Hi

Am 01.06.26 um 18:24 schrieb Michel Dänzer:
> On 6/1/26 16:08, Thomas Zimmermann wrote:
>> In drm_crtc_vblank_get_vblank_timeout(), return the timestamp of the
>> first visible scanline after the last vblank timeout. This is what the
>> caller expects.
>>
>> A vblank phase starts with a vblank timeout. At this point the display
>> is blanked for several scanlines. Afterwards the display is unblanked
>> until the next vblank timeout occurs. The display content is only visible
>> during that second part.
>>
>> The current implementation of drm_crtc_vblank_get_vblank_timeout()
>> returns the timestamp of the last vblank timeout that started the current
>> vblank phase. But the display only unblanks after 20 to 30 percent of
>> the overall frame duration. The returned timestamp is therefore too early.
>>
>> The next vblank timeout is already known when calculating the returned
>> timestamp. Instead of subtracting the duration of a full frame from the
>> value, only subtract the duration of the active, visible part. The result
>> is the timestamp of the first visible scanline, as expected by the caller.
>>
>> This bug was not introduced by the generic vblank timer. It appears that
>> the get_vblank_timeout logic has always been buggy since it was first
>> added in commit 3a0709928b17 ("drm/vkms: Add vblank events simulated by
>> hrtimers").
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_vblank.c | 32 +++++++++++++++++++++++++-------
>>   1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> index 96d70c3d4522..d52df247d04e 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> [...]
>> @@ -2312,17 +2321,26 @@ bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
>>   		*vblank_time = READ_ONCE(vtimer->timer.node.expires);
>>   	} while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
>>   
>> -	if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
>> +	if (drm_WARN_ON(dev, !ktime_compare(*vblank_time, cur_time)))
>>   		return false; /* Already expired */
>>   
>> +	framedur_ns = vblank->framedur_ns;
>> +
>>   	/*
>> -	 * 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 timestamp by one frame.
>> +	 * To prevent races we rolled the hrtimer forward before we did any
>> +	 * timeout 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 always need to correct the timestamp. The returned
>> +	 * time should be the time of the first active scanline after the
>> +	 * previous vblank. Hence subtract the active phase's duration from
>> +	 * the next expiration time.
>>   	 */
>> -	*vblank_time = ktime_sub(*vblank_time, vtimer->interval);
>> +	if (drm_WARN_ON(dev, !mode->crtc_vtotal))
>> +		return false;
>> +	activedur_ns = div_s64(framedur_ns * mode->crtc_vdisplay, mode->crtc_vtotal);
>> +	*vblank_time = ktime_sub_ns(*vblank_time, activedur_ns);
> Normally the timestamp returned by drm_crtc_vblank_count_and_time is supposed to correspond to the end of vertical blank / start of active, in which case the new code here looks wrong.
>
> Also, while the current time is inside an active area, it's supposed to return the timestamp corresponding to the start of the current active area, not the next one.

The initial value of *vblank_time is when the vblank timer fires _next_ 
and the display blanks. Subtracting the length of the active period 
should give the time of the first active scanline within the current 
vblank phase.

Isn't that exactly what you describe?

Best regards
Thomas

>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

Re: [PATCH 2/7] drm/vblank: timer: Fix timestamp calculation

[Michel Dänzer]

On 6/1/26 19:30, Thomas Zimmermann wrote:
> Am 01.06.26 um 18:24 schrieb Michel Dänzer:
>> On 6/1/26 16:08, Thomas Zimmermann wrote:
>>> In drm_crtc_vblank_get_vblank_timeout(), return the timestamp of the
>>> first visible scanline after the last vblank timeout. This is what the
>>> caller expects.
>>>
>>> A vblank phase starts with a vblank timeout. At this point the display
>>> is blanked for several scanlines. Afterwards the display is unblanked
>>> until the next vblank timeout occurs. The display content is only visible
>>> during that second part.
>>>
>>> The current implementation of drm_crtc_vblank_get_vblank_timeout()
>>> returns the timestamp of the last vblank timeout that started the current
>>> vblank phase. But the display only unblanks after 20 to 30 percent of
>>> the overall frame duration. The returned timestamp is therefore too early.
>>>
>>> The next vblank timeout is already known when calculating the returned
>>> timestamp. Instead of subtracting the duration of a full frame from the
>>> value, only subtract the duration of the active, visible part. The result
>>> is the timestamp of the first visible scanline, as expected by the caller.
>>>
>>> This bug was not introduced by the generic vblank timer. It appears that
>>> the get_vblank_timeout logic has always been buggy since it was first
>>> added in commit 3a0709928b17 ("drm/vkms: Add vblank events simulated by
>>> hrtimers").
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/gpu/drm/drm_vblank.c | 32 +++++++++++++++++++++++++-------
>>>   1 file changed, 25 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>>> index 96d70c3d4522..d52df247d04e 100644
>>> --- a/drivers/gpu/drm/drm_vblank.c
>>> +++ b/drivers/gpu/drm/drm_vblank.c
>>> [...]
>>> @@ -2312,17 +2321,26 @@ bool drm_crtc_vblank_get_vblank_timeout(struct drm_crtc *crtc, ktime_t *vblank_t
>>>           *vblank_time = READ_ONCE(vtimer->timer.node.expires);
>>>       } while (cur_count != drm_crtc_vblank_count_and_time(crtc, &cur_time));
>>>   -    if (drm_WARN_ON(crtc->dev, !ktime_compare(*vblank_time, cur_time)))
>>> +    if (drm_WARN_ON(dev, !ktime_compare(*vblank_time, cur_time)))
>>>           return false; /* Already expired */
>>>   +    framedur_ns = vblank->framedur_ns;
>>> +
>>>       /*
>>> -     * 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 timestamp by one frame.
>>> +     * To prevent races we rolled the hrtimer forward before we did any
>>> +     * timeout 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 always need to correct the timestamp. The returned
>>> +     * time should be the time of the first active scanline after the
>>> +     * previous vblank. Hence subtract the active phase's duration from
>>> +     * the next expiration time.
>>>        */
>>> -    *vblank_time = ktime_sub(*vblank_time, vtimer->interval);
>>> +    if (drm_WARN_ON(dev, !mode->crtc_vtotal))
>>> +        return false;
>>> +    activedur_ns = div_s64(framedur_ns * mode->crtc_vdisplay, mode->crtc_vtotal);
>>> +    *vblank_time = ktime_sub_ns(*vblank_time, activedur_ns);
>> Normally the timestamp returned by drm_crtc_vblank_count_and_time is supposed to correspond to the end of vertical blank / start of active, in which case the new code here looks wrong.
>>
>> Also, while the current time is inside an active area, it's supposed to return the timestamp corresponding to the start of the current active area, not the next one.
> 
> The initial value of *vblank_time is when the vblank timer fires _next_ and the display blanks. Subtracting the length of the active period should give the time of the first active scanline within the current vblank phase.
> 
> Isn't that exactly what you describe?

I don't think so.

It means that the timestamp returned by drm_(crtc_)vblank_count_and_time (which also used e.g. in events sent to user space) corresponds to the end of active / start of vblank, not to the end of vblank / start of active as it should (and does when the vblank timer isn't used).


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast

Claude review: drm/vblank: timer: Fix timestamps and improve reliabilty

[Claude Code Review Bot]

Overall Series Review

Subject: drm/vblank: timer: Fix timestamps and improve reliabilty
Author: Thomas Zimmermann <tzimmermann@suse.de>
Patches: 11
Reviewed: 2026-06-04T14:03:58.367915

---

This series fixes timestamps and improves reliability for the DRM vblank timer infrastructure, used by virtual/software GPU drivers (vkms, bochs, virtio, vmwgfx). The intent is sound — the timestamp calculation has long-standing issues, and the improvements are worthwhile.

However, **Patch 3 introduces a clock domain mismatch bug** that affects multiple subsequent patches. The timer is switched to `CLOCK_BOOTTIME`, but the initial timer start and various timestamp reads/comparisons still use `ktime_get()` / `ktime_get_ns()` which return `CLOCK_MONOTONIC` time. On any system that has been suspended, boottime and monotonic time diverge, making the comparisons invalid. This is the most critical issue in the series.

Additionally, **Patch 1 does not update the vmwgfx caller** (`vmw_vkms_get_vblank_timestamp`), which still ignores the new return value and will return `true` with stale/uninitialized `vblank_time` on error.

The refactoring patches (4, 7) are reasonable, and Patch 6 is a correct and clear bugfix. Patch 2's timestamp correction logic is sound in principle.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: timer: Return success status from get_vblank_timeout

[Claude Code Review Bot]

Patch Review

**Concept:** Good. Propagating failure instead of silently returning `ktime_get()` is the right thing.

**Issue: vmwgfx caller not updated.** `vmw_vkms_get_vblank_timestamp()` at `drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c:245` still calls `drm_crtc_vblank_get_vblank_timeout(crtc, vblank_time)` and ignores the return value, unconditionally returning `true`:

```c
drm_crtc_vblank_get_vblank_timeout(crtc, vblank_time);
return true;
```

After this patch, when the function returns `false`, `vblank_time` is uninitialized/stale, yet vmwgfx will report success to the vblank core. This needs the same treatment as `drm_crtc_vblank_helper_get_vblank_timestamp_from_timer`:

```c
return drm_crtc_vblank_get_vblank_timeout(crtc, vblank_time);
```

**Minor:** The existing `drm_WARN_ON(crtc->dev, !ktime_compare(...))` path in the original code returned `void`, so it was a silent "give up." Now it returns `false`, which is correct behavior, but the `drm_WARN_ON` will print a kernel warning for a condition that may not truly be an error (the timer could have legitimately just expired). Consider whether a `drm_dbg_vbl` is more appropriate here.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: timer: Fix timestamp calculation

[Claude Code Review Bot]

Patch Review

**Concept:** Correct. The returned timestamp should be the start of the active display period, not the start of the vblank period. Subtracting `activedur_ns` instead of `framedur_ns` (i.e., `vtimer->interval`) achieves this.

**The math:**
```c
activedur_ns = div_s64(framedur_ns * mode->crtc_vdisplay, mode->crtc_vtotal);
*vblank_time = ktime_sub_ns(*vblank_time, activedur_ns);
```

`next_expiry - active_duration = start_of_active_period`. This is correct geometry.

**Observation on `drm_WARN_ON(dev, !mode->crtc_vtotal)`:** This is a good safety check, but after this returns `false`, the caller (`drm_crtc_vblank_helper_get_vblank_timestamp_from_timer`) now also returns `false`. Make sure callers handle that gracefully — this may trigger DRM's "failed to get vblank timestamp" warnings. A `vtotal` of 0 indicates a fundamentally broken mode, so warning is appropriate.

**No issues with the logic itself.** This is a real bug fix for a long-standing issue.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: timer: Use absolute timer since boot

[Claude Code Review Bot]

Patch Review

**Critical issue: Clock domain mismatch.** The timer is changed from `CLOCK_MONOTONIC` to `CLOCK_BOOTTIME`, but the initial start time uses `ktime_get_ns()`, which returns `CLOCK_MONOTONIC` time:

```c
hrtimer_setup(&vtimer->timer, drm_vblank_timer_function,
              CLOCK_BOOTTIME, HRTIMER_MODE_ABS);
...
vblank_time_ns = roundup(ktime_get_ns(), vblank->framedur_ns);
hrtimer_start(&vtimer->timer, ns_to_ktime(vblank_time_ns), HRTIMER_MODE_ABS);
```

This should be `ktime_get_boottime_ns()`. After any system suspend, `CLOCK_BOOTTIME > CLOCK_MONOTONIC`, so a monotonic timestamp used as a boottime absolute value would be in the past, causing the timer to fire immediately and then forward rapidly.

This mismatch also breaks the comparison in the existing `drm_crtc_vblank_get_vblank_timeout()`: `vtimer->timer.node.expires` is now in the boottime domain, while `cur_time` from `drm_crtc_vblank_count_and_time()` is in the monotonic domain (vblank timestamps are stored via `ktime_get()` in the vblank core). The `!ktime_compare(*vblank_time, cur_time)` check compares values from different clock domains.

**Division by zero risk:** If `vblank->framedur_ns` is 0 (e.g., dotclock is 0), `roundup(ktime_get_ns(), 0)` will trigger a division by zero. The `drm_calc_timestamping_constants` called just before can leave `framedur_ns` at 0 if `dotclock <= 0`. A guard is needed.

**The underrun detection** (`!ret_overrun` case) is a nice addition for debugging.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: timer: Reorganize get_vblank_timeout

[Claude Code Review Bot]

Patch Review

**Concept:** Pure code motion to prepare for Patch 5. No functional changes claimed or observed.

**Minor style note:** The `else { return false; }` at the end of the if/else chain is fine, though using early return for the disabled-with-no-clock case (as done in Patch 5's `else if (mode->crtc_clock)` fallthrough) would be more idiomatic kernel style. But this is a prep patch, so it's fine.

No issues.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: timer: Estimate vblank timeout if timer is disabled

[Claude Code Review Bot]

Patch Review

**Same clock domain issue as Patch 3:**
```c
*vblank_time = roundup(ktime_get_ns(), framedur_ns);
```

This returns a `CLOCK_MONOTONIC` value, while when the timer IS running (the `if (vblank->enabled)` branch), `*vblank_time` comes from `vtimer->timer.node.expires` which is now `CLOCK_BOOTTIME`. The caller receives timestamps from different clock domains depending on the code path. Should be `ktime_get_boottime_ns()` to match Patch 3's timer clock.

**Conceptual question:** The estimation assumes the frame-aligned phase established in `drm_crtc_vblank_start_timer` persists across timer disable/enable cycles. But since the alignment is computed fresh at start time (`roundup(now, framedur_ns)`), the phase will shift each time the timer is restarted. The "estimate" won't match the actual timer phase that was or will be active. This might be acceptable as a best-effort approximation, but the commit message's claim that "timeouts are aligned to frame duration, so we can round up to the next alignment" overstates the precision — the phase is arbitrary.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: timer: Verify that expiry time is in the future

[Claude Code Review Bot]

Patch Review

**Good fix.** The original `!ktime_compare()` only catches the `expiry == cur_time` case. `!ktime_after()` correctly catches `expiry <= cur_time`:

```c
-		if (drm_WARN_ON(dev, !ktime_compare(*vblank_time, cur_time)))
+		if (drm_WARN_ON(dev, !ktime_after(*vblank_time, cur_time)))
```

This is a clear improvement. If the timer expiry is somehow behind the current vblank time, it should be treated as expired.

**However**, after Patch 3's clock domain change, `*vblank_time` is `CLOCK_BOOTTIME` and `cur_time` is `CLOCK_MONOTONIC`. This comparison is fundamentally broken regardless of `ktime_compare` vs `ktime_after`. Fixing the clock domain issue in Patch 3 would make this patch correct.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: timer: Avoid reading the vblank time unnecessarily

[Claude Code Review Bot]

Patch Review

**Optimization and robustness improvement.** The change from double `drm_crtc_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. If the vblank counter changes 10+ times during this read loop, something is fundamentally broken.

**Subtle loop behavior:** The condition `cur_count != lst_count && retries--` means the `retries--` side effect only happens when the count actually changed. If the count is stable on the first try, `retries` is never decremented and `cur_time` is set correctly. This is correct.

**Minor:** `cur_count` is read initially with `drm_crtc_vblank_count()` (which goes through `drm_vblank_count` → atomic64_read), while subsequent reads use `drm_crtc_vblank_count_and_time()` (which goes through the seqlock). The `drm_crtc_vblank_count` path doesn't have the seqlock barrier that `drm_crtc_vblank_count_and_time` has. This means the initial `cur_count` could theoretically be stale relative to the following `*vblank_time = READ_ONCE(vtimer->timer.node.expires)` read. However, since we're comparing with the post-read count, this is caught by the loop — the loop simply runs one extra iteration in that case. No correctness issue, just a theoretical 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 disabled-timer estimation path, or keep `CLOCK_MONOTONIC` throughout. The comparison between timer expiry and vblank core timestamps also needs reconciliation.
2. **Missing vmwgfx update (Patch 1):** `vmw_vkms_get_vblank_timestamp()` must propagate the new return value.
3. **Division by zero risk (Patch 3):** Guard `roundup(..., vblank->framedur_ns)` against zero denominator.

---
Generated by Claude Code Patch Reviewer