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

From: Daniel Vetter <daniel@ffwll.ch>
To: "Michel Dänzer" <michel@daenzer.net>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip
Date: Mon, 13 Jun 2016 10:06:18 +0200	[thread overview]
Message-ID: <20160613080618.GR3363@phenom.ffwll.local> (raw)
In-Reply-To: <ffdb59d0-a2ed-06e8-f88c-0b37b6645151@daenzer.net>

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@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.
- 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
  - that kind of precision is why we have a separate timestamp.

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, no
single-use driver oddity in the core, and you still can page_flip as late
as you want without forcing a delay to the next vblank if your already
past the start of vblank. And since you have high-precision timestamp
support the vblank core will fudge the timestamp as needed.

Ofc this assumes that amd hw has a vblank_end irq, but most desktop hw
seems to have that (didn't check tbh whether amd has it). So all that's
needed I think is to enable the vblank_end irq (in lockstep with the start
one) and move drm_handle_vblank() from its current place to that new irq.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel