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

[Michel Dänzer <michel.daenzer@mailbox.org>]

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