Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

[WIP] linux-explicit-synchronization-v1: new protocol implementation #2070

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

emersion
Copy link
Member

@emersion emersion commented Mar 15, 2020

This patch adds support for the linux-explicit-synchronization-unstable-v1 protocol. No support has been added for sync fences yet. The protocol is already useful as-is because it offers per-commit release events (wl_buffer.release is global state). We still need to actually use the fences provided by clients before exposing the protocol. We also need to send our fences to clients if we use the GL renderer, otherwise clients might start re-using buffers before our rendering is complete.

More information about explicit sync: https://lwn.net/Articles/814587/

To test, run weston-simple-dmabuf-egl -m.


Sway PR: swaywm/sway#5113

Since wlr_client_buffer are created at wl_surface.commit time, there's no need for any additional per-commit wlr_surface state. We'll need that once we try waiting for client buffer fences before starting rendering.

Here's the new sync fence API:

void wlr_buffer_set_in_fence(struct wlr_buffer *buffer, int fd);
void wlr_buffer_add_out_fence(struct wlr_buffer *buffer, int fd);

int wlr_renderer_dup_out_fence(struct wlr_renderer *renderer);
bool wlr_renderer_wait_in_fence(struct wlr_renderer *renderer, int fd);

int wlr_output_get_out_fence(struct wlr_output *output);
  • zwp_linux_surface_synchronization_v1.set_acquire_fence calls wlr_buffer_set_in_fence
  • At render time, the renderer waits for wlr_buffer.in_fence_fd. After rendering, it creates an EGLSyncKHR and exposes it via wlr_renderer_get_out_fence. wlr_buffer_add_out_fence is called with this FD.
  • The DRM backend sets the IN_FENCE_FD property to the FD provided via wlr_renderer_get_out_fence (if rendering) or via wlr_buffer.in_fence_fd (if direct scan-out). After commit, it exposes the FD retrieved from OUT_FENCE_PTR via wlr_output_get_out_fence. If direct scan-out is used, wlr_buffer_add_out_fence is called with this FD.
  • On buffer release, the protocol implementation sends zwp_linux_buffer_release.fenced_release with wlr_buffer.out_fence_fd.

TODO:

  • Renderer support
  • DRM support
    • attach_render
    • attach_buffer
    • Cursor
    • Multi-GPU
    • get_out_fence
  • Only expose the extension if both the renderer and the backend support explicit sync
  • Fix FreeBSD build

Follow-up issues:

  • Make the backend release buffers early with a fence (e.g. directly after a commit on DRM)
  • Don't use a buffer if it isn't ready (see "The good, but complicated way" in DRM Explicit fencing #894)

@ddevault ddevault requested review from ascent12 and ddevault March 15, 2020 15:51
@emersion emersion changed the title linux-explicit-synchronization-v1: new protocol implementation [WIP] linux-explicit-synchronization-v1: new protocol implementation Mar 16, 2020
Copy link
Member

@ascent12 ascent12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be hooked up with EGL or otherwise paying attention to the fence status in order to be a correct implementation.
#894

@emersion
Copy link
Member Author

Yeah, see the updated PR description.

@emersion emersion force-pushed the explicit-sync branch 8 times, most recently from 972cadf to aa22e40 Compare March 20, 2020 17:43
@emersion emersion requested a review from ascent12 March 20, 2020 17:50
@emersion
Copy link
Member Author

This PR has been updated and is now functional. I'd like to hear some feedback before tackling the last remaining TODOs.

Copy link
Member

@ascent12 ascent12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks fine to me. About what I expect from this type of implementation.
I would prefer to go with the "better" way of implementing this, but that's probably not going to happen in the near future.

@@ -129,6 +133,12 @@ static bool atomic_crtc_pageflip(struct wlr_drm_backend *drm,
}
atomic_add(&atom, crtc->id, crtc->props.mode_id, crtc->mode_id);
atomic_add(&atom, crtc->id, crtc->props.active, 1);
if (crtc->props.out_fence_ptr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this property is guranteed for atomic drivers.

@emersion emersion force-pushed the explicit-sync branch 2 times, most recently from a19f603 to 4c90509 Compare March 23, 2020 10:48
@emersion emersion marked this pull request as draft April 28, 2020 09:27
@emersion
Copy link
Member Author

I would prefer to go with the "better" way of implementing this, but that's probably not going to happen in the near future.

The goal of this PR is to implement the "easy" way first. Then we can build on top of it to implement the "better" way. Doing all in one go would result in a massive PR.

emersion added 4 commits June 22, 2020 18:27
This commit adds support for IN_FENCE_FD and OUT_FENCE_PTR.
This patch adds support for the
linux-explicit-synchronization-unstable-v1 protocol.

To test, run weston-simple-dmabuf-egl.
@emersion
Copy link
Member Author

A possible issue with the approach taken in this PR is that we bolt the sync fences onto the wlr_buffer. Maybe would be better to require compositors to explicitly wire up the sync fences.

@ddevault ddevault removed their request for review January 21, 2021 16:43
@emersion
Copy link
Member Author

Apparently sync_file is old already and drm_syncobj is the new hotness. We'll probably want to wait for linux-explicit-synchronization-v2 before resuming this work.

@emersion
Copy link
Member Author

emersion commented Nov 1, 2021

wlroots has migrated to gitlab.freedesktop.org. This pull request has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/2070

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants