On Mon, Feb 11, 2019 at 7:44 PM Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx> wrote: > > 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. ... > > > > 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. > Small correction: It's actually the other way around. DRM wraps at start of vblank ~ start of front porch, whereas the hw counter wraps at start of back porch. I think that removes the caveat you mention below, if i understand what you meant. One way i tested if this patch does what i expect it to do was do flips (glXSwapbuffers calls) with different durations between calls, and continuously change that wait time in between calls to see if the delta between consecutive flip timestamps follows those wait times, ie. without any big discontinous "jumps" inbetween. > 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. > Another small correction: Afaik we don't actually use the sign of vpos for "in vblank" detection anywhere, but instead the returned status flag. Which is good, given that patch 1/3 of that other vrr fixes series slightly change the meaning of the sign in vrr mode. -mario > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
