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.
--
Earthling Michel Dänzer \ GNOME / Xwayland / Mesa developer
https://redhat.com \ Libre software enthusiast
