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

["Kazlauskas, Nicholas" <Nicholas.Kazlauskas-5C7GfCeVMHo@public.gmane.org>]

On 2/11/19 10:01 AM, 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@gmail.com>
>> Cc: <stable@vger.kernel.org>
>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Michel Dänzer <michel@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.
> 
> 

I'm not sure if this has a good userspace solution.

I think the problem with all of this is the difference between the 
hardware vblank and the DRM vblank.

The hardware vblank counter wraps around at the front porch, DRM wraps 
around at the back porch. The behavior in amdgpu_get_vblank_counter_kms 
is to return the hardware count if vpos < 0 and return the 
hardware_count + 1 if vpos >= 0.

The value for vpos being used here is from 
amdgpu_display_vblank_get_counter which returns a value < 0 if we're in 
vblank and >= 0 if we're in scanout. The value returned in the extended 
front porch will always be considered -vbl_end so it'll never be 
considered within scanout.

The target vblank for the "wait for vblank" logic is 
amdgpu_get_vblank_counter_kms(...) + 1.

The "wait for vblank" logic waits while we're within vblank and if the 
target - amdgpu_get_vblank_counter_kms(...) > 0.

There are 3 cases that can happen here:
(1) The wait starts in the same vblank interval as the last flip
(2) The wait starts in scanout
(3) The wait starts in the next vblank interval as the last flip

In case (1) the wait breaks as soon as we enter scanout.
In case (2) the wait breaks immediately.
In case (3) we wait for the next scanout.

By DRM logic the programming should be happening at the start of the 
back porch, but it doesn't really matter since it's both targeting the 
right scanout for cases (1) and (2).

However, for case (3) we have an issue. If the wait starts within vblank 
front porch then it's not going to be programmed for the upcoming 
scanout, but the one after that.

This is the case that's trying to be addressed with the patch - with a 
caveat.

If the last_flip_vblank != drm_crtc_vblank_count then the target_vblank 
should be < amdgpu_get_vblank_counter_kms which would cause the wait to 
exit immediately. If we're in the back porch of the vblank in this 
instance then we should be waiting until scanout to begin programming 
(and target the scanout after next).

Nicholas Kazlauskas
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx