From 82223e451a7efc8e8885f27e5a375c6bb2f72ba6 Mon Sep 17 00:00:00 2001 From: Alexander Orzechowski Date: Sun, 26 Jan 2025 17:35:47 -0500 Subject: [PATCH] render/drm_syncobj: Add a callback when ready The old approach of using a signal is fundamentally broken for a common usecase: When the waiter is ready, it's common to immediately finish and free any resources associated with it. Because of the semantics of wl_signal_emit_mutable() this is UB. wl_signal_emit_mutable() always excepts that the waiter hasn't been freed until the signal has finished being emitted. Instead of over engineering the solution, let's just add a callback required by wlr_drm_syncobj_timeline_waiter_init(). In this callback, the implementation is free to finish() or free() any resource it likes. --- backend/wayland/output.c | 12 ++++-------- include/backend/wayland.h | 1 - include/wlr/render/drm_syncobj.h | 10 +++++++++- render/drm_syncobj.c | 6 +++++- types/wlr_linux_drm_syncobj_v1.c | 11 +++-------- 5 files changed, 21 insertions(+), 19 deletions(-) diff --git a/backend/wayland/output.c b/backend/wayland/output.c index 53f4d7772..bbe0230f4 100644 --- a/backend/wayland/output.c +++ b/backend/wayland/output.c @@ -135,7 +135,6 @@ static const struct wp_presentation_feedback_listener }; static void buffer_remove_drm_syncobj_waiter(struct wlr_wl_buffer *buffer) { - wl_list_remove(&buffer->drm_syncobj_ready.link); wlr_drm_syncobj_timeline_waiter_finish(&buffer->drm_syncobj_waiter); buffer->has_drm_syncobj_waiter = false; } @@ -177,8 +176,8 @@ static const struct wl_buffer_listener buffer_listener = { .release = buffer_handle_release, }; -static void buffer_handle_drm_syncobj_ready(struct wl_listener *listener, void *data) { - struct wlr_wl_buffer *buffer = wl_container_of(listener, buffer, drm_syncobj_ready); +static void buffer_handle_drm_syncobj_ready(struct wlr_drm_syncobj_timeline_waiter *waiter) { + struct wlr_wl_buffer *buffer = wl_container_of(waiter, buffer, drm_syncobj_waiter); buffer_remove_drm_syncobj_waiter(buffer); buffer_release(buffer); } @@ -819,14 +818,11 @@ static bool output_commit(struct wlr_output *wlr_output, signal_timeline->wl, signal_point_hi, signal_point_lo); if (!wlr_drm_syncobj_timeline_waiter_init(&buffer->drm_syncobj_waiter, - signal_timeline->base, signal_point, 0, output->backend->event_loop)) { + signal_timeline->base, signal_point, 0, output->backend->event_loop, + buffer_handle_drm_syncobj_ready)) { return false; } buffer->has_drm_syncobj_waiter = true; - - buffer->drm_syncobj_ready.notify = buffer_handle_drm_syncobj_ready; - wl_signal_add(&buffer->drm_syncobj_waiter.events.ready, - &buffer->drm_syncobj_ready); } else { if (output->drm_syncobj_surface_v1 != NULL) { wp_linux_drm_syncobj_surface_v1_destroy(output->drm_syncobj_surface_v1); diff --git a/include/backend/wayland.h b/include/backend/wayland.h index 46e8df95c..daa98ef5e 100644 --- a/include/backend/wayland.h +++ b/include/backend/wayland.h @@ -64,7 +64,6 @@ struct wlr_wl_buffer { bool has_drm_syncobj_waiter; struct wlr_drm_syncobj_timeline_waiter drm_syncobj_waiter; - struct wl_listener drm_syncobj_ready; struct wlr_drm_syncobj_timeline *fallback_signal_timeline; uint64_t fallback_signal_point; diff --git a/include/wlr/render/drm_syncobj.h b/include/wlr/render/drm_syncobj.h index bea86d191..30b4ddc35 100644 --- a/include/wlr/render/drm_syncobj.h +++ b/include/wlr/render/drm_syncobj.h @@ -37,6 +37,11 @@ struct wlr_drm_syncobj_timeline { } WLR_PRIVATE; }; +struct wlr_drm_syncobj_timeline_waiter; + +typedef void (*wlr_drm_syncobj_timeline_ready_callback)( + struct wlr_drm_syncobj_timeline_waiter *waiter); + struct wlr_drm_syncobj_timeline_waiter { struct { struct wl_signal ready; @@ -45,6 +50,7 @@ struct wlr_drm_syncobj_timeline_waiter { struct { int ev_fd; struct wl_event_source *event_source; + wlr_drm_syncobj_timeline_ready_callback callback; } WLR_PRIVATE; }; @@ -92,10 +98,12 @@ bool wlr_drm_syncobj_timeline_check(struct wlr_drm_syncobj_timeline *timeline, * Asynchronously wait for a timeline point. * * See wlr_drm_syncobj_timeline_check() for a definition of flags. + * + * A callback must be provided that will be invoked when the waiter has finished. */ bool wlr_drm_syncobj_timeline_waiter_init(struct wlr_drm_syncobj_timeline_waiter *waiter, struct wlr_drm_syncobj_timeline *timeline, uint64_t point, uint32_t flags, - struct wl_event_loop *loop); + struct wl_event_loop *loop, wlr_drm_syncobj_timeline_ready_callback callback); /** * Cancel a timeline waiter. */ diff --git a/render/drm_syncobj.c b/render/drm_syncobj.c index af93876d4..e0493b462 100644 --- a/render/drm_syncobj.c +++ b/render/drm_syncobj.c @@ -193,12 +193,15 @@ static int handle_eventfd_ready(int ev_fd, uint32_t mask, void *data) { } wl_signal_emit_mutable(&waiter->events.ready, NULL); + waiter->callback(waiter); return 0; } bool wlr_drm_syncobj_timeline_waiter_init(struct wlr_drm_syncobj_timeline_waiter *waiter, struct wlr_drm_syncobj_timeline *timeline, uint64_t point, uint32_t flags, - struct wl_event_loop *loop) { + struct wl_event_loop *loop, wlr_drm_syncobj_timeline_ready_callback callback) { + assert(callback); + int ev_fd; #if HAVE_EVENTFD ev_fd = eventfd(0, EFD_CLOEXEC); @@ -235,6 +238,7 @@ bool wlr_drm_syncobj_timeline_waiter_init(struct wlr_drm_syncobj_timeline_waiter *waiter = (struct wlr_drm_syncobj_timeline_waiter){ .ev_fd = ev_fd, .event_source = source, + .callback = callback, }; wl_signal_init(&waiter->events.ready); return true; diff --git a/types/wlr_linux_drm_syncobj_v1.c b/types/wlr_linux_drm_syncobj_v1.c index 0713bcce6..a10e6a450 100644 --- a/types/wlr_linux_drm_syncobj_v1.c +++ b/types/wlr_linux_drm_syncobj_v1.c @@ -29,7 +29,6 @@ struct wlr_linux_drm_syncobj_surface_v1_commit { struct wlr_drm_syncobj_timeline_waiter waiter; uint32_t cached_seq; - struct wl_listener waiter_ready; struct wl_listener surface_destroy; }; @@ -194,14 +193,13 @@ static struct wlr_linux_drm_syncobj_surface_v1 *surface_from_wlr_surface( static void surface_commit_destroy(struct wlr_linux_drm_syncobj_surface_v1_commit *commit) { wlr_surface_unlock_cached(commit->surface->surface, commit->cached_seq); wl_list_remove(&commit->surface_destroy.link); - wl_list_remove(&commit->waiter_ready.link); wlr_drm_syncobj_timeline_waiter_finish(&commit->waiter); free(commit); } -static void surface_commit_handle_waiter_ready(struct wl_listener *listener, void *data) { +static void surface_commit_handle_waiter_ready(struct wlr_drm_syncobj_timeline_waiter *waiter) { struct wlr_linux_drm_syncobj_surface_v1_commit *commit = - wl_container_of(listener, commit, waiter_ready); + wl_container_of(waiter, commit, waiter); surface_commit_destroy(commit); } @@ -233,7 +231,7 @@ static bool lock_surface_commit(struct wlr_linux_drm_syncobj_surface_v1 *surface struct wl_display *display = wl_client_get_display(client); struct wl_event_loop *loop = wl_display_get_event_loop(display); if (!wlr_drm_syncobj_timeline_waiter_init(&commit->waiter, timeline, point, - flags, loop)) { + flags, loop, surface_commit_handle_waiter_ready)) { free(commit); return false; } @@ -241,9 +239,6 @@ static bool lock_surface_commit(struct wlr_linux_drm_syncobj_surface_v1 *surface commit->surface = surface; commit->cached_seq = wlr_surface_lock_pending(surface->surface); - commit->waiter_ready.notify = surface_commit_handle_waiter_ready; - wl_signal_add(&commit->waiter.events.ready, &commit->waiter_ready); - commit->surface_destroy.notify = surface_commit_handle_surface_destroy; wl_signal_add(&surface->surface->events.destroy, &commit->surface_destroy);