[PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Michel Dänzer]

On 06/13/16 17:06, Daniel Vetter wrote:
> On Mon, Jun 13, 2016 at 10:54:37AM +0900, Michel Dänzer wrote:
>> On 10.06.2016 23:43, Daniel Vetter wrote:
>>> On Fri, Jun 10, 2016 at 05:57:12PM +0900, Michel Dänzer wrote:
>>>> From: Michel Dänzer <michel.daenzer at amd.com>
>>>>
>>>> If userspace wants a page flip to take effect during vblank sequence n,
>>>> it has to wait for vblank seqno n-1 before calling the
>>>> DRM_IOCTL_MODE_PAGE_FLIP ioctl.
>>>>
>>>> This change makes sure that we do not program the flip to the hardware
>>>> before the end of vblank seqno n-1 in this case, to prevent the flip
>>>> from taking effect too early.
>>>>
>>>> On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
>>>> during vblank, but userspace didn't wait for the current vblank seqno
>>>> before, this change would still allow the flip to be programmed during
>>>> the current vblank seqno.
>>>
>>> This just sounds like you're sending vblank events out a bit too early.
>>> And watching vblank waits that userspace does works, but it's fragile,
>>> add-hoc and I don't really jump in joy about adding that to the vblank
>>> core. Is there no way you can adjust sending out the vblank events
>>> similarly, to make sure userspace can never sneak in a pageflip too early?
>>
>> What you call "too early" is actually "during the vertical blank period
>> waited for". IMHO only notifying userspace of a vertical blank period
>> when it's already over would defeat the purpose.
> 
> Afaiui the rules are:
> - The timestamp for vblank event needs to agree with whatever oml_sync
>   requries.
> - The event delivery itself needs to be consistent with what page_flip
>   takes, i.e. if userspace sees an event and immediately issues a
>   page_flip then it should not be able to hit the same vblank with that
>   pageflip.
> - The event needs to be after the old buffers are not longer used and can
>   be reused for rendering.

That's only relevant for DRM_IOCTL_MODE_PAGE_FLIP, not
DRM_IOCTL_WAIT_VBLANK.

> - There's no requirement at all that the event gets delivered at a
>   specific point in the vblank, hardware is too different for that to work

As the name implies, the purpose of DRM_IOCTL_WAIT_VBLANK is to wait for
a vertical blank period. If that doesn't work as intended with some
hardware, that's tough luck but not really my problem. :)

>   - that kind of precision is why we have a separate timestamp.

I'm afraid this last item gives away that you're relatively new to this
code. ;) The timestamp was originally literally just the current
gettimeofday when the wait finished (the original DRM_IOCTL_WAIT_VBLANK
ioctl didn't have any asynchronous notification functionality). It was
relatively recently that Mario changed the timestamp to correspond to
the end of the vertical blank period / start of scanout of the next
frame, presumably due to your first rule above.


> I assume you're goal is to not delay page_flips unecessarily, without
> breaking requirement 2 here. Imo a simpler fix would be to delay the
> vblank handling to end of vblank. Fixes everything without hacks, [...]

Except it breaks the original purpose of the wait for vblank
functionality, which is to wait for the beginning of a vertical blank
period. [0] You're focusing too much on page flips and suggesting to
throw out the vblank baby with the bathwater. I really don't see the big
issue which would justify that.


[0] As an analogy, how useful would e.g. calendar notifications be if
they arrived at the end of the events they're about? "Hey, that meeting
you were supposed to attend? It just finished!"


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer