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

[Daniel Vetter]

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. Also there's scheduling
jitter.

> > 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. The above few rules are what
generic userspace (afaik at least) cares about, and what therefore drivers
should implement. Note that at least to my knowledge absolutely nothing
cares when exactly in the vblank the interrupt fires, and the event gets
delivered. We even moved that around in the i915 driver iirc, exactly
because you could squeeze in a page-flip like you can here on radeon.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch