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

Tue Jun 14 05:53:41 UTC 2016

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? 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.
Afaik others don't use it like that, at least not on intel. Weston has
some hacks to use vblank waits for plane flips, but that's all disabled
code because it just doesn't work - you need full atomic.

Anyway, I still don't like adding hacks to drm core like this for
single-use in just one driver. 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. How bad would it really be to just
always delay the page_flip past vblank? Userspace can still use async
flips for lower latency.

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). It would also neatly fit into atomic plans, since we
want to implement that (so that everyone has a benchmarking/low-latency
mode, without hw support for async flips on all planes). And it wouldn't
need special tracking code in drm_irq.c, making me happy. Thoughts?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch