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

Tue Jun 14 07:25:28 UTC 2016

On 14.06.2016 14:53, Daniel Vetter wrote:
> On Tue, Jun 14, 2016 at 11:09:10AM +0900, Michel Dänzer wrote:
>> 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.
> 
> Is there really anything using it like that outside of -ati?

Yes. With DRI3/Present, it's driver independent code in
xserver/present/. With DRI2, it's theoretically up to the DDX driver,
but all drivers seem to have basically the same logic; the modesetting
driver certainly does.

> I didn't know that we pass vblank waits to X clients. Either way annoying,
> since it means you need to keep things working like this for amd drivers
> forever.

Please don't try to single out our drivers, it seems like rather your
driver is the odd man out in this case.

> Afaik others don't use it like that, at least not on intel.

UXA has the same DRI2 logic, not sure about SNA.

> Anyway, I still don't like adding hacks to drm core like this for
> single-use in just one driver.

The driver changes will be ported to radeon of course, and why couldn't
it be used by other drivers for the same purpose?

> drm_irq.c is already really complex and suffering badly from this, and
> we're pretty close to always accidentally breaking something when touching it.

I just don't see how my change makes this any worse.

> How bad would it really be to just always delay the page_flip past vblank?

In the variable refresh rate case it could be pretty bad, basically
dropping to the minimum refresh rate.

> If that's not good enough I'd say we should add a
> faster-than-vblank-but-still-synced page_flip flag. Then userspace could
> tell you exactly whether you should always wait (no flags), or never wait
> (with this new flag).

That would be an inferior solution compared to my series, e.g.: If
userspace calls DRM_IOCTL_MODE_PAGE_FLIP before the target vblank seqno
is reached, it cannot use the new flag, otherwise the flip might take
effect too early. However, if we are then already in the target vblank
period when the fences have signalled and we are ready to program the
flip, we have to wait for the end of vblank first, and the flip will be
delayed by one frame.

If we're going to change the userspace interface, it would be better to
re-purpose the reserved field of struct drm_mode_crtc_page_flip for
explicitly specifying the target vblank seqno (via a new cap and flag).
Then the kernel and userspace would no longer need to second-guess each
other.

But even if we take that route, this series would be desirable for
getting us most of the way there for existing userspace.

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