modesetting: Fix inaccurate PresentCompleteNotify timing for TearFree
TearFree with the modesetting driver suffers from inaccurate PresentCompleteNotify event timing, which leads to A/V de-synchronization with clients utilizing Present, among other problems.
This turns out to be caused by the faulty assumption in Present that a TearFree flip always completes at the next MSC, relative to the reference CRTC's MSC at the time that Present happens to read it.
This series fixes the inaccurate PresentCompleteNotify timing for TearFree by introducing a way for Present to tack on a presentation's completion notification to the completion of said presentation's corresponding TearFree flip. This ensures that the PresentCompleteNotify timing is always accurate when TearFree is used.
Merge request reports
Activity
added 11 commits
-
c32ddd53...2ef5ef57 - 2 commits from branch
xorg:master - d1952f96 - modesetting: Remove redundant GLAMOR_HAS_GBM #ifdef from ms_do_pageflip
- 4071e0c8 - modesetting: Pass reference CRTC pointer to ms_do_pageflip
- eead861b - modesetting: Pass CRTC pointer to TearFree flip handlers
- 0ad5edf8 - modesetting: Fix memory leak on ms_do_pageflip error
- 7964d1a5 - modesetting: Improve TearFree state check in ms_present_check_flip
- 69ae9b8a - modesetting: Ensure vblank events always run in sequential order
- 7d2ba094 - modesetting: Support accurate DRI presentation timing with TearFree
- 44a68a89 - present: Prevent double vblank enqueue on error when TearFree is used
- cbeee438 - present: Fix inaccurate PresentCompleteNotify timing for TearFree
Toggle commit list-
c32ddd53...2ef5ef57 - 2 commits from branch
165 165 { 166 166 ScreenPtr screen = crtc->pScreen; 167 167 ScrnInfoPtr scrn = xf86ScreenToScrn(screen); 168 #ifdef GLAMOR_HAS_GBM 169 xf86CrtcPtr xf86_crtc = crtc->devPrivate; 170 171 /* Check if this is a fake flip routed through TearFree and abort it */ 172 if (ms_tearfree_dri_abort(xf86_crtc, ms_present_event_match, &event_id)) 173 return; 174 #endif 668 vblank->exec_msc = vblank->target_msc; 669 vblank->queued = TRUE; 670 } 671 default: 672 break; 680 681 /* Try the fake flip first and then fall back to a vblank event */ 682 if (present_flip(vblank->crtc, vblank->event_id, 0, NULL, TRUE) || 683 Success == screen_priv->queue_vblank(screen, 684 window, 685 vblank->crtc, 686 vblank->event_id, 687 completion_msc)) { 688 /* Ensure present_execute_post() runs at the next execution */ 689 vblank->exec_msc = vblank->target_msc; 690 vblank->queued = TRUE; - Comment on lines +682 to +690
Isn't this gonna always queue a vblank event, even if
present_flipsucceeds? Shouldn't it have been the following?682 if (present_flip(vblank->crtc, vblank->event_id, 0, NULL, TRUE) || 683 Success == screen_priv->queue_vblank(screen, 684 window, 685 vblank->crtc, 686 vblank->event_id, 687 completion_msc)) { 688 /* Ensure present_execute_post() runs at the next execution */ 689 vblank->exec_msc = vblank->target_msc; 690 vblank->queued = TRUE; 682 if (!present_flip(vblank->crtc, vblank->event_id, 0, NULL, TRUE) { 683 /* We failed to queue a fake flip, fallback to creating a vblank event */ 684 if (Success == screen_priv->queue_vblank(screen, 685 window, 686 vblank->crtc, 687 vblank->event_id, 688 completion_msc)) { 689 /* Ensure present_execute_post() runs at the next execution */ 690 vblank->exec_msc = vblank->target_msc; 691 vblank->queued = TRUE; 692 }
@kerneltoast: Damn, this is some quality commit messages and comments you wrote! You really took the
be a great collaboratorto heart! Thanks a lot for that, and coming back to fix the presentation timings, I really appreciate it
Anyway, the following commits are
Reviewed-by: Martin Roukala <martin.roukala@mupuf.org>:- d1952f96
- 4071e0c8
- eead861b
- 0ad5edf8 <-- Thanks for clarifying the ownership model
- 7964d1a5 <-- With the comment clarified, or a new commit added introducing a function to check if tearfree is active on the CRTC
- 69ae9b8a
The last 3 commits are not something I feel comfortable reviewing right now. If someone with more knowledge of the timing infrastructure could have a look, it would be appreciated. If noone has the time, I'll try to dedicate some time for this during the week.
Thanks for the review! The A/V desync in video players and stutters when watching YouTube in Firefox were getting to me, so I was compelled to come back and fix presentation timings
The last 3 commits are not something I feel comfortable reviewing right now.
FWIW, the trick for the last 3 commits is that the TearFree timing just mimics what PageFlip does, since PageFlip's timing was always correct. What started out as being conceptually quite simple ended up requiring a lot of code though
@mupuf I've pushed a new version with your feedback implemented. Please let me know how it looks. Thank you!
Thank you @kerneltoast! I will check one commit a day starting from tomorrow, so we should be able to merge by the end of the week if all goes well!
@mupuf Had a chance to grok any of the remaining commits?
added 32 commits
-
cbeee438...098fcedf - 21 commits from branch
xorg:master - 37bc2840 - modesetting: Remove redundant GLAMOR_HAS_GBM #ifdef from ms_do_pageflip
- 141da9d6 - modesetting: Pass reference CRTC pointer to ms_do_pageflip
- c8280689 - modesetting: Pass CRTC pointer to TearFree flip handlers
- 04ee87d8 - modesetting: Fix memory leak on ms_do_pageflip error
- 9237538b - modesetting: Improve TearFree state check in ms_present_check_flip
- 51c0486a - modesetting: Introduce ms_tearfree_is_active_on_crtc helper
- d16be083 - modesetting: Ensure vblank events always run in sequential order
- 7c244a5b - modesetting: Support accurate DRI presentation timing with TearFree
- 413db007 - present: Prevent double vblank enqueue on error when TearFree is used
- f279e1f8 - present: Fix inaccurate PresentCompleteNotify timing for TearFree
- 82975044 - present: Document the TearFree flip reasons in PresentFlipReason
Toggle commit list-
cbeee438...098fcedf - 21 commits from branch
- Resolved by Sultan Alsawaf
51c0486a and 82975044 are
Reviewed-by: Martin Roukala <martin.roukala@mupuf.org>@kerneltoast: Mind updating the commit, adding my R-b to the ones I already reviewed?
Edited by Martin Roukala - Last reply by Sultan Alsawaf
added 12 commits
-
462b0603 - 1 commit from branch
xorg:master - 12ebdafc - modesetting: Remove redundant GLAMOR_HAS_GBM #ifdef from ms_do_pageflip
- d7802903 - modesetting: Pass reference CRTC pointer to ms_do_pageflip
- c2a0490c - modesetting: Pass CRTC pointer to TearFree flip handlers
- 33f3f302 - modesetting: Fix memory leak on ms_do_pageflip error
- a2fe24d0 - modesetting: Improve TearFree state check in ms_present_check_flip
- 4b26d1d1 - modesetting: Introduce ms_tearfree_is_active_on_crtc helper
- 9168f455 - modesetting: Ensure vblank events always run in sequential order
- 81b12161 - modesetting: Support accurate DRI presentation timing with TearFree
- 623a31df - present: Prevent double vblank enqueue on error when TearFree is used
- fdc5af9e - present: Fix inaccurate PresentCompleteNotify timing for TearFree
- 456bb21c - present: Document the TearFree flip reasons in PresentFlipReason
Toggle commit list-
462b0603 - 1 commit from branch
-
81b12161: The commit message is cut. Once fixed, you can add my
Acked-by: Martin Roukala <martin.roukala@mupuf.org> -
623a31df:
Acked-by: Martin Roukala <martin.roukala@mupuf.org> -
fdc5af9e:
Acked-by: Martin Roukala <martin.roukala@mupuf.org>
I am still not hyper confident about these last three commits. Have you tested that this does not regress the tearfree-disabled mode? In the end, this is all I am concerned for, as the tearfree path is well documented :)
-
81b12161: The commit message is cut. Once fixed, you can add my
81b12161: The commit message is cut.
That's a weird GitLab bug; the commit message isn't actually cut. The raw view shows the full message: kerneltoast/xserver@81b12161 (patch)
Have you tested that this does not regress the tearfree-disabled mode?
Yep, everything still works fine for me with TearFree disabled, and the presentation changes are neatly scoped under a TearFree check so I think that aspect is good.
Thanks for the acks! I'll upload a rebased version with them.
added 15 commits
-
456bb21c...7ce57e17 - 4 commits from branch
xorg:master - e40acc4f - modesetting: Remove redundant GLAMOR_HAS_GBM #ifdef from ms_do_pageflip
- 5288e92e - modesetting: Pass reference CRTC pointer to ms_do_pageflip
- aec8148d - modesetting: Pass CRTC pointer to TearFree flip handlers
- 53a4e420 - modesetting: Fix memory leak on ms_do_pageflip error
- 8bc302d0 - modesetting: Improve TearFree state check in ms_present_check_flip
- 284478e6 - modesetting: Introduce ms_tearfree_is_active_on_crtc helper
- 968cf3af - modesetting: Ensure vblank events always run in sequential order
- 12eea378 - modesetting: Support accurate DRI presentation timing with TearFree
- 9655d9f4 - present: Prevent double vblank enqueue on error when TearFree is used
- 41b509ff - present: Fix inaccurate PresentCompleteNotify timing for TearFree
- 31af0122 - present: Document the TearFree flip reasons in PresentFlipReason
Toggle commit list-
456bb21c...7ce57e17 - 4 commits from branch
@mupuf updated!
added 13 commits
-
31af0122...d6b20f5e - 2 commits from branch
xorg:master - 7288b4d1 - modesetting: Remove redundant GLAMOR_HAS_GBM #ifdef from ms_do_pageflip
- 1fd9d79a - modesetting: Pass reference CRTC pointer to ms_do_pageflip
- be864d8e - modesetting: Pass CRTC pointer to TearFree flip handlers
- 35975d90 - modesetting: Fix memory leak on ms_do_pageflip error
- 8b5fd556 - modesetting: Improve TearFree state check in ms_present_check_flip
- 18b14ea1 - modesetting: Introduce ms_tearfree_is_active_on_crtc helper
- 9d1997f7 - modesetting: Ensure vblank events always run in sequential order
- 53b02054 - modesetting: Support accurate DRI presentation timing with TearFree
- d4bd39f1 - present: Prevent double vblank enqueue on error when TearFree is used
- 1fab978a - present: Fix inaccurate PresentCompleteNotify timing for TearFree
- f490622f - present: Document the TearFree flip reasons in PresentFlipReason
Toggle commit list-
31af0122...d6b20f5e - 2 commits from branch
@mupuf It's been a week so I've gone ahead and rebased. Let me know if there's anything else needed on my part. Thanks!
mentioned in issue #928
Any chance that this MR would fix the #928?
@karlson2k Try the master branch of mpv. I submitted some timing fixes to mpv a little while back:
https://github.com/mpv-player/mpv/commit/4a2aa36674a3989d6567ebe1d3c3f8c6c889d906
https://github.com/mpv-player/mpv/commit/0d44ae319dfd5209eb0733caafdcbcf67d228cf3
Thanks, @kerneltoast, but I have this issue with Kodi.
@karlson2k I did a quick grep in Kodi and it looks like Kodi doesn't utilize Present timing events. Kodi would need to listen for PresentComplete events coming from mesa's under-the-hood usage of DRI3/Present when
eglSwapBuffers()is used.Basically, I suspect this could be fixed in Kodi if Kodi were to just add a listener for Present events from the X-server in order to know when its video frames actually become visible on the display. Kodi already uses
eglSwapBuffers()so it's already receiving Present timing events, but it's just not doing anything with them.This is how mpv implemented this sort of thing: https://github.com/mpv-player/mpv/commit/3d459832a88a9bd2835b339cf6ca98f84aad0115#diff-ce7f7bf9a3af824d860a7fd9cc5ff45a10cd5b901162f76c91e68f0acd73f190R1283-R1297
As for why the Intel DDX works fine when the modesetting DDX doesn't, the only thing I can think of is that maybe you're using DRI2 with the Intel DDX instead of DRI3?
It'd also be interesting if this still occurs with mpv after it got the timing fixes.
