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

[Michel Dänzer]

On 06/13/16 23:06, Daniel Vetter wrote:
> On Mon, Jun 13, 2016 at 05:58:29PM +0900, Michel Dänzer wrote:
>> 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.
> 
> Yup, mixed that up.
> 
>>> - 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.
> 
> Most hw just seems to give you a vblank interrupt somewhere in the vblank
> are, or sometimes even slightly before that.

Our hardware tends to trigger the vblank interrupt early, but it's still
useful in that drawing operations submitted after it cannot affect the
previously scanned out frame.

> Also there's scheduling jitter.

Sure, but there's a big difference between "no guarantee that we're
still in vblank" vs "guarantee that we're no longer in vblank".


>>> 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!"
> 
> Ok, what exactly is the use-case for waiting for vblanks _without_
> scheduling a flip afterwards? At least in drm the rule is that ABI is what
> userspace observes and actually cares about.

E.g.: In cases where page flipping cannot be used, Xorg / the DDX driver
waits for the target vertical blank period before emitting the drawing
commands for a buffer swap operation. If the vblank notification only
arrives when the vertical blank period is already over, this is very
likely to result in tearing.

Some X compositors and AFAIK even applications such as media players can
use DRM_IOCTL_WAIT_VBLANK similarly. Obviously it's not intended to be
used directly like that, but nonetheless it is.


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