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 <[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?
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
