[PATCH] drm/amd/display: Use vrr friendly pageflip throttling in DC.

[Daniel Vetter]

On Wed, Feb 13, 2019 at 11:54 AM Michel Dänzer <michel at daenzer.net> wrote:
>
> On 2019-02-13 10:53 a.m., Daniel Vetter wrote:
> > On Mon, Feb 11, 2019 at 04:01:12PM +0100, Michel Dänzer wrote:
> >> On 2019-02-09 7:52 a.m., Mario Kleiner wrote:
> >>> In VRR mode, keep track of the vblank count of the last
> >>> completed pageflip in amdgpu_crtc->last_flip_vblank, as
> >>> recorded in the pageflip completion handler after each
> >>> completed flip.
> >>>
> >>> Use that count to prevent mmio programming a new pageflip
> >>> within the same vblank in which the last pageflip completed,
> >>> iow. to throttle pageflips to at most one flip per video
> >>> frame, while at the same time allowing to request a flip
> >>> not only before start of vblank, but also anywhere within
> >>> vblank.
> >>>
> >>> The old logic did the same, and made sense for regular fixed
> >>> refresh rate flipping, but in vrr mode it prevents requesting
> >>> a flip anywhere inside the possibly huge vblank, thereby
> >>> reducing framerate in vrr mode instead of improving it, by
> >>> delaying a slightly delayed flip requests up to a maximum
> >>> vblank duration + 1 scanout duration. This would limit VRR
> >>> usefulness to only help applications with a very high GPU
> >>> demand, which can submit the flip request before start of
> >>> vblank, but then have to wait long for fences to complete.
> >>>
> >>> With this method a flip can be both requested and - after
> >>> fences have completed - executed, ie. it doesn't matter if
> >>> the request (amdgpu_dm_do_flip()) gets delayed until deep
> >>> into the extended vblank due to cpu execution delays. This
> >>> also allows clients which want to regulate framerate within
> >>> the vrr range a much more fine-grained control of flip timing,
> >>> a feature that might be useful for video playback, and is
> >>> very useful for neuroscience/vision research applications.
> >>>
> >>> In regular non-VRR mode, retain the old flip submission
> >>> behavior. This to keep flip scheduling for fullscreen X11/GLX
> >>> OpenGL clients intact, if they use the GLX_OML_sync_control
> >>> extensions glXSwapBufferMscOML(, ..., target_msc,...) function
> >>> with a specific target_msc target vblank count.
> >>>
> >>> glXSwapBuffersMscOML() or DRI3/Present PresentPixmap() will
> >>> not flip at the proper target_msc for a non-zero target_msc
> >>> if VRR mode is active with this patch. They'd often flip one
> >>> frame too early. However, this limitation should not matter
> >>> much in VRR mode, as scheduling based on vblank counts is
> >>> pretty futile/unusable under variable refresh duration
> >>> anyway, so no real extra harm is done.
> >>>
> >>> According to some testing already done with this patch by
> >>> Nicholas on top of my tests, IGT tests didn't report any
> >>> problems. If fixes stuttering and flickering when flipping
> >>> at rates below the minimum vrr refresh rate.
> >>>
> >>> Fixes: bb47de736661 ("drm/amdgpu: Set FreeSync state using drm VRR
> >>> properties")
> >>> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
> >>> Cc: <stable at vger.kernel.org>
> >>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> >>> Cc: Harry Wentland <harry.wentland at amd.com>
> >>> Cc: Alex Deucher <alexander.deucher at amd.com>
> >>> Cc: Michel Dänzer <michel at daenzer.net>
> >>
> >> I wonder if this couldn't be solved in a simpler / cleaner way by making
> >> use of the target MSC passed to the page_flip_target hook.
> >
> > Requiring that all compositors that use VRR also have to use page_flip
> > target (which is not yet exposed on the atomic side yet at all) just
> > because amdgpu doesn't sound like a great idea. I think better to handle
> > this in the amdgpu kernel driver, for similar reasons we've originally
> > added this.
>
> We've originally added what?

The pageflip delay right after you receive a vblank. amdgpu did accept
pageflips for the current frame even after the vblank for the same was
sent out already, breaking non-amdgpu specific compositors. This is
why the page_flip target was added, so that amdgpu could still do
this, while delaying the page_flip for everyone else.

Side effect of that code is that the refresh rate goes slow if you
page_flip without a target and happen to issue it right in the vblank
(somewhat unlikely, given that scanout takes longer than blank
period). "Fixing" that by requiring everyone to specificy the target
(which isn't enabled even for atomic) doesn't sound like a good
solution to me. And page_flip target is an amdgpu-only feature.

> Also not sure what you mean by "because amdgpu". Other drivers which
> want to support VRR might also have to deal with this issue one way or
> another, as it's due to page flips submitted during a vertical blank
> period only being expected to take effect during the following vertical
> blank period normally.

Yeah I expect we'll need to do something similar for intel vrr. That's
why I'm in this discussion. Making the vblank and page_flip timestamps
agree, plus issueing page_flips as soon as possible (without violating
the ordering rules generic userspace expects) sounds like the best
option here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch