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 <[email protected]>
---
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)
