WIP linux-drm-syncobj-v1

[ids1024]

So far just exposes the protocol but doesn't do anything. https://github.com/Smithay/smithay/pull/1327 does indeed make that part mostly painless instead of having impl bounds to deal with and a delegate macro.

[ids1024]

Reading a bit more about IN_FENCE_FD and OUT_FENCE_PTR, I think we need to use both if we want DRM scanout to not use implicit sync?

https://github.com/ValveSoftware/gamescope/blob/master/src/drm.cpp uses an g_nAlwaysSignalledSyncFile as the IN_FENCE_FD. Which is probably what we want as well (for direct scanout) if we only commit the buffer when we have already waited for it to be ready.

Then we can poll the fence produced by OUT_FENCE_PTR, at not signal release points for buffers used in direct scanout until that.

[Drakulix]

Reading a bit more about IN_FENCE_FD and OUT_FENCE_PTR, I think we need to use both if we want DRM scanout to not use implicit sync?

I am not sure, if you can use OUT_FENCE_PTR without IN_FENCE_FD. It wouldn't really make sense for the driver to support one but not the other. We probably should just set them both all the time.

https://github.com/ValveSoftware/gamescope/blob/master/src/drm.cpp uses an g_nAlwaysSignalledSyncFile as the IN_FENCE_FD. Which is probably what we want as well (for direct scanout) if we only commit the buffer when we have already waited for it to be ready.

Yes, if we do CPU latching (that is polling the fence), then this seems to be the right approach for direct-scanout.

Though I would still just send the fence for composited frames, although we obviously could apply the principle there as well, as I tried here: https://github.com/pop-os/cosmic-comp/pull/291

However I don't see a benefit outside of cases, where the driver supports polling fences, but not IN_FENCE_FD. In those we could skip a CPU blocking glFinish, which also becomes a non-issue once we have a thread per surface.

Perhaps there are some advantages for VRR use cases, but I don't think that is worth the trouble for a first iteration.

Then we can poll the fence produced by OUT_FENCE_PTR, at not signal release points for buffers used in direct scanout until that.

:+1:

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 20.29%. Comparing base (c982040) to head (a17585c). Report is 63 commits behind head on master.

Files	Patch %	Lines
src/backend/renderer/utils/wayland.rs	93.75%	1 Missing :warning:
src/wayland/shell/xdg/handlers/positioner.rs	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1356      +/-   ##
==========================================
+ Coverage   20.27%   20.29%   +0.01%     
==========================================
  Files         161      162       +1     
  Lines       26041    26351     +310     
==========================================
+ Hits         5281     5348      +67     
- Misses      20760    21003     +243     

Flag	Coverage Δ
wlcs-buffer	17.72% <92.30%> (+0.04%)	:arrow_up:
wlcs-core	17.39% <92.30%> (+0.03%)	:arrow_up:
wlcs-output	7.08% <0.00%> (-0.15%)	:arrow_down:
wlcs-pointer-input	19.25% <88.46%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[ids1024]

Perhaps not using a blocker could make sense for a cage-like compositor, or as an optimization when there is only a single fullscreen application. Though I'm not sure what issues there might be if the IN_FENCE_FD isn't signaled for a long time (say, a broken client that never signals it?). At some point we'll want to do testing and see what works and if it can improve things.

Without blockers, a renderer may also need to handle the acquire points. A Vulkan renderer should import the timeline as a VkSemaphore. I think we also would need to use EGL_ANDROID_native_fence_sync in the EGL renderer, and wait on the acquire fence? I don't think we can rely on implicit sync.

(Perhaps it would be best to document that the blocker (or something equivalent) is required for Smithay's syncobj-v1 implementation, and generalize things later?)

Testing this: I see vkgears on cosmic-comp is only using direct scanout to an overlay plane when full-screened. In which case, export_sync_file runs successfully. Though I can't really tell if implicit sync is being avoided, and that this is preventing sync errors, since at least with vkgears I don't see artifacts if I removed the blocker and use a signaled in fence.

Wayland EGL on the Nvidia 555 driver, and Mesa Vulkan on git, both freeze if BufferInner::drop doesn't signal the release fence. So as long as we make sure to keep a reference to Buffer as long as we need the buffer, that seems to be working. Which seems correct for direct-scanout if we hold it until we get a page flip event.

[Drakulix]

Perhaps not using a blocker could make sense for a cage-like compositor, or as an optimization when there is only a single fullscreen application. Though I'm not sure what issues there might be if the IN_FENCE_FD isn't signaled for a long time (say, a broken client that never signals it?). At some point we'll want to do testing and see what works and if it can improve things.

Given commits could be blocking, this might permanently lock up a display without a timeout. It might be reasonable to never send any user-provided fences and instead require adding a Blocker, if one chooses the implement linux-drm-syncobj.

Without blockers, a renderer may also need to handle the acquire points. A Vulkan renderer should import the timeline as a VkSemaphore. I think we also would need to use EGL_ANDROID_native_fence_sync in the EGL renderer, and wait on the acquire fence? I don't think we can rely on implicit sync.

We kinda do that already? SyncPoints can be awaited via the GPU driver in smithay currently and we use that for synchronizing multi-gpu copies and block for compositing before submitting.

(Perhaps it would be best to document that the blocker (or something equivalent) is required for Smithay's syncobj-v1 implementation, and generalize things later?)

Yes, agreed.

So to sum up:

• Never pass user-provided fences to IN_FENCE_FD, only for compositing like we do now (and denylist the nvidia driver here for now).
• Pass a signalled fence for direct-scanout, if downstream signals that it is using blockers (either automagically or with a flag on the DrmCompositor).
• Otherwise just fallback to implicit sync for direct-scanout.

Sounds good?

Testing this: I see vkgears on cosmic-comp is only using direct scanout to an overlay plane when full-screened. In which case, export_sync_file runs successfully. Though I can't really tell if implicit sync is being avoided, and that this is preventing sync errors, since at least with vkgears I don't see artifacts if I removed the blocker and use a signaled in fence.

That is the issue with races, I guess.

Wayland EGL on the Nvidia 555 driver, and Mesa Vulkan on git, both freeze if BufferInner::drop doesn't signal the release fence. So as long as we make sure to keep a reference to Buffer as long as we need the buffer, that seems to be working. Which seems correct for direct-scanout if we hold it until we get a page flip event.

Sounds good!

[ids1024]

Given commits could be blocking, this might permanently lock up a display without a timeout. It might be reasonable to never send any user-provided fences and instead require adding a Blocker, if one chooses the implement linux-drm-syncobj.

Apparently fences have timeouts? Since the kernel wants to prevent an indefinite lock like this: https://docs.kernel.org/driver-api/dma-buf.html#indefinite-dma-fences

Presumably the timeout behavior would be similar to implict sync without a DmabufBlocker? In which case that's just consistent with the expected problems when not using a blocker.

So that does sound like something that could be a viable optimization for things like fullscreen applications, even if a general purpose compositor probably want to use blockers under most circumstances (for implicit or explicit sync). But it's an optimization that can be handled later, with more testing.

We kinda do that already? SyncPoints can be awaited via the GPU driver in smithay currently and we use that for synchronizing multi-gpu copies and block for compositing before submitting.

Ah right, we already have support for that in the renderer. So if we want to use the acquire_fence without a CPU wait, we'll want to pass the fence to EGL there. But that can be handled later.

• Never pass user-provided fences to IN_FENCE_FD, only for compositing like we do now (and denylist the nvidia driver here for now).
• Pass a signalled fence for direct-scanout, if downstream signals that it is using blockers (either automagically or with a flag on the DrmCompositor).
• Otherwise just fallback to implicit sync for direct-scanout.

I'm not sure if we can automagically fallback to implicit sync. We could just not pass an IN_FENCE_FD, and ignore the acquire fence, but I don't think ignoring the explicit fence is guaranteed to work (with all drivers)?

I think we need to either:

1. Document that, at least for now, use of DrmSyncPointBlocker is required with the Smithay syncobj-v1 protocol implementation.
   • If drmSyncobjEventfd isn't supported by the kernel, the protocol shouldn't be exposed.
   • I think this matches what Mutter is doing.
2. If a blocker isn't used, use IN_FENCE_FD, or import the fence into EGL
   • Since IN_FENCE_FD doesn't work on Nvidia, for now, DrmSyncPointBlocker is the only way to get proper synchronization with the Nvidia driver?

[Drakulix]

So that does sound like something that could be a viable optimization for things like fullscreen applications

Right. For fullscreen surfaces, we probably want to just pass through the fence, not for normal desktop usage though. I agree this is a later optimization.

Ah right, we already have support for that in the renderer. So if we want to use the acquire_fence without a CPU wait, we'll want to pass the fence to EGL there. But that can be handled later.

Yes, but we kinda don't want that (except for the fullscreen case again), because then we would block compositing for an undefined amount of time. The goal is to render as late as possible and use the latest ready buffer.

We could just not pass an IN_FENCE_FD, and ignore the acquire fence, but I don't think ignoring the explicit fence is guaranteed to work (with all drivers)?

Yes we can absolutely do that, that is what we are doing right now. We would only block drivers not supporting implicit sync (nvidia). Anyone serious about nvidia support, just needs to implement the protocol and I don't want to mandate that.

So strong preference for 1.

[DemiMarie]

My understanding is that DRM syncobjs are, at least in the future, not guaranteed to ever signal. This is because future drivers may support Userspace Memory Fences (UMFs) and userspace can deadlock themselves with them. Therefore, I think Smithay needs to do a (maybe asynchronous) explicit wait (possibly with a timeout) so that the display cannot be locked up forever.

[ids1024]

I've rebased this (and the cosmic-comp PR) and made a few small improvements to the doc comments.

I believe this reflects how we agreed the implementation/API should work, and as far as I can tell works correctly. Though it's hard to test thoroughly. The latest version of nvidia/egl-wayland should fix the issues that NVIDIA drivers were having on certain clients with compositors that supported explicit sync.

If no one else sees issues with this, it should be good to merge.

[cmeissl]

Thanks for working on this!

I will try to review and also test it on a few different devices during the next few days.

[PolyMeilex]

Tested a few clients that make use of syncobj on my AMD systems, everything works as expected. :ok_hand: