-
Notifications
You must be signed in to change notification settings - Fork 314
Do proper wayland teardown cleaning up listeners and fds #2066
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
91cfce5
1f0b558
b264a71
d3f550a
d94597c
678cc1e
863aaca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| #include "Script/Script.h" | ||
|
|
||
| #include <sys/eventfd.h> | ||
| #include <sys/types.h> | ||
| #include <sys/stat.h> | ||
| #include <fcntl.h> | ||
|
|
@@ -22,6 +23,7 @@ | |
| #include <optional> | ||
| #include <span> | ||
| #include <string> | ||
| #include <thread> | ||
| #include <unordered_map> | ||
| #include <unordered_set> | ||
| #include <utility> | ||
|
|
@@ -169,6 +171,14 @@ using namespace std::literals; | |
|
|
||
| struct drm_t g_DRM = {}; | ||
|
|
||
| // Flip handler thread control. Keep the thread object global so we | ||
| // can join it during shutdown instead of detaching and risking the | ||
| // thread still using the DRM fd while we clean up. | ||
| static std::thread g_page_flip_handler_thread; | ||
| static std::atomic<bool> g_page_flip_handler_thread_should_exit{false}; | ||
|
|
||
| static int g_page_flip_eventfd = -1; | ||
|
|
||
| namespace gamescope | ||
| { | ||
| class CDRMBackend; | ||
|
|
@@ -777,25 +787,52 @@ void flip_handler_thread_run(void) | |
| { | ||
| pthread_setname_np( pthread_self(), "gamescope-kms" ); | ||
|
|
||
| struct pollfd pollfd = { | ||
| .fd = g_DRM.fd, | ||
| .events = POLLIN, | ||
| }; | ||
| // Prepare pollfds: one for DRM fd and for eventfd to | ||
| // wake up the poll immediately when requested to exit. | ||
| struct pollfd fds[2]; | ||
| int nfds = 0; | ||
|
|
||
| fds[nfds].fd = g_DRM.fd; | ||
| fds[nfds].events = POLLIN; | ||
| nfds++; | ||
|
|
||
| if ( g_page_flip_eventfd != -1 ) { | ||
| fds[nfds].fd = g_page_flip_eventfd; | ||
| fds[nfds].events = POLLIN; | ||
| nfds++; | ||
| } | ||
|
|
||
| while ( true ) | ||
| while ( !g_page_flip_handler_thread_should_exit.load( std::memory_order_acquire ) ) | ||
| { | ||
| int ret = poll( &pollfd, 1, -1 ); | ||
| int ret = poll( fds, nfds, -1 ); | ||
| if ( ret < 0 ) { | ||
| if ( errno == EINTR ) | ||
| continue; | ||
| drm_log.errorf_errno( "polling for DRM events failed" ); | ||
| break; | ||
| } | ||
| if ( ret == 0 ) { | ||
| // timeout, check exit condition again | ||
| continue; | ||
| } | ||
|
|
||
| drmEventContext evctx = { | ||
| .version = 3, | ||
| .page_flip_handler2 = page_flip_handler, | ||
| }; | ||
| drmHandleEvent(g_DRM.fd, &evctx); | ||
| // Check if eventfd signaled | ||
| if ( nfds > 1 && (fds[1].revents & POLLIN) ) { | ||
| uint64_t val; | ||
| ssize_t r = read( g_page_flip_eventfd, &val, sizeof( val ) ); | ||
| (void)r; | ||
| } | ||
|
|
||
| if ( (fds[0].revents & POLLIN) ) { | ||
| drmEventContext evctx = { | ||
| .version = 3, | ||
| .page_flip_handler2 = page_flip_handler, | ||
| }; | ||
| drmHandleEvent( g_DRM.fd, &evctx ); | ||
| } | ||
| } | ||
|
|
||
| drm_log.debugf("page_flip_handler_thread exiting"); | ||
| } | ||
|
|
||
| static bool refresh_state( drm_t *drm ) | ||
|
|
@@ -1379,8 +1416,13 @@ bool init_drm(struct drm_t *drm, int width, int height, int refresh) | |
| } | ||
| } | ||
|
|
||
| std::thread flip_handler_thread( flip_handler_thread_run ); | ||
| flip_handler_thread.detach(); | ||
| // Create an eventfd to wake the flip handler poll for immediate exit. | ||
| g_page_flip_eventfd = eventfd( 0, EFD_NONBLOCK | EFD_CLOEXEC ); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need an eventfd here? Wouldn't a pipe be enough if we close the write end to wake up the pageflip thread and detect HANGUP? |
||
| if ( g_page_flip_eventfd == -1 ) { | ||
| drm_log.errorf_errno( "page-flip eventfd creation failed" ); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just fail startup if we can't create the FD? Having to handle missing FD everywhere is painful and will likely just result in a deadlock.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, fair enough |
||
| } | ||
|
|
||
| g_page_flip_handler_thread = std::thread( flip_handler_thread_run ); | ||
|
|
||
| // Set log priority to the max, liftoff_log_scope will filter for us. | ||
| liftoff_log_set_priority(LIFTOFF_DEBUG); | ||
|
|
@@ -1551,9 +1593,30 @@ void finish_drm(struct drm_t *drm) | |
| drm->connectors.clear(); | ||
|
|
||
|
|
||
| // Signal the page-flip handler thread to exit and join it so it won't be | ||
| // using the DRM fd while we clean it up. The thread polls with a | ||
| // timeout, so this join will return shortly. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thread doesn't poll with a timeout AFAIU. |
||
| if ( g_page_flip_handler_thread.joinable() ) { | ||
| g_page_flip_handler_thread_should_exit.store( true, std::memory_order_release ); | ||
|
|
||
| // Wake the eventfd so the poll wakes immediately. | ||
| if ( g_page_flip_eventfd != -1 ) { | ||
| uint64_t one = 1; | ||
| ssize_t w = write( g_page_flip_eventfd, &one, sizeof(one) ); | ||
| (void)w; | ||
|
|
||
| g_page_flip_handler_thread.join(); | ||
|
|
||
| close( g_page_flip_eventfd ); | ||
| g_page_flip_eventfd = -1; | ||
| } else { | ||
| drm_log.errorf( "No eventfd for flip handler thread; waiting for page-flip thread to exit. We may hang here." ); | ||
| g_page_flip_handler_thread.join(); | ||
| } | ||
| } | ||
|
|
||
| // We can't close the DRM FD here, it might still be in use by the | ||
| // page-flip handler thread. | ||
| wlsession_close_kms(); | ||
| g_DRM.fd = -1; | ||
| } | ||
|
|
||
| gamescope::OwningRc<gamescope::IBackendFb> drm_fbid_from_dmabuf( struct drm_t *drm, struct wlr_dmabuf_attributes *dma_buf ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -475,6 +475,29 @@ static void wlserver_handle_touch_motion(struct wl_listener *listener, void *dat | |
| wlserver_touchmotion( event->x, event->y, event->touch_id, event->time_msec, false, touch->connector ); | ||
| } | ||
|
|
||
| static void wlserver_handle_pointer_destroy(struct wl_listener *listener, void *data) | ||
| { | ||
| struct wlserver_pointer *pointer = wl_container_of( listener, pointer, destroy ); | ||
|
|
||
| wl_list_remove( &pointer->motion.link ); | ||
| wl_list_remove( &pointer->button.link ); | ||
| wl_list_remove( &pointer->axis.link ); | ||
| wl_list_remove( &pointer->frame.link ); | ||
| wl_list_remove( &pointer->destroy.link ); | ||
| free( pointer ); | ||
| } | ||
|
|
||
| static void wlserver_handle_touch_destroy(struct wl_listener *listener, void *data) | ||
| { | ||
| struct wlserver_touch *touch = wl_container_of( listener, touch, destroy ); | ||
|
|
||
| wl_list_remove( &touch->down.link ); | ||
| wl_list_remove( &touch->up.link ); | ||
| wl_list_remove( &touch->motion.link ); | ||
| wl_list_remove( &touch->destroy.link ); | ||
| free( touch ); | ||
| } | ||
|
|
||
| static void wlserver_new_input(struct wl_listener *listener, void *data) | ||
| { | ||
| struct wlr_input_device *device = (struct wlr_input_device *) data; | ||
|
|
@@ -504,7 +527,7 @@ static void wlserver_new_input(struct wl_listener *listener, void *data) | |
| { | ||
| struct wlserver_pointer *pointer = (struct wlserver_pointer *) calloc( 1, sizeof( struct wlserver_pointer ) ); | ||
|
|
||
| pointer->wlr = (struct wlr_pointer *)device; | ||
| pointer->wlr = wlr_pointer_from_input_device( device ); | ||
|
|
||
| pointer->motion.notify = wlserver_handle_pointer_motion; | ||
| wl_signal_add( &pointer->wlr->events.motion, &pointer->motion ); | ||
|
|
@@ -514,20 +537,24 @@ static void wlserver_new_input(struct wl_listener *listener, void *data) | |
| wl_signal_add( &pointer->wlr->events.axis, &pointer->axis); | ||
| pointer->frame.notify = wlserver_handle_pointer_frame; | ||
| wl_signal_add( &pointer->wlr->events.frame, &pointer->frame); | ||
| pointer->destroy.notify = wlserver_handle_pointer_destroy; | ||
| wl_signal_add( &device->events.destroy, &pointer->destroy); | ||
| } | ||
| break; | ||
| case WLR_INPUT_DEVICE_TOUCH: | ||
| { | ||
| struct wlserver_touch *touch = (struct wlserver_touch *) calloc( 1, sizeof( struct wlserver_touch ) ); | ||
|
|
||
| touch->wlr = (struct wlr_touch *)device; | ||
| touch->wlr = wlr_touch_from_input_device( device ); | ||
|
|
||
| touch->down.notify = wlserver_handle_touch_down; | ||
| wl_signal_add( &touch->wlr->events.down, &touch->down ); | ||
| touch->up.notify = wlserver_handle_touch_up; | ||
| wl_signal_add( &touch->wlr->events.up, &touch->up ); | ||
| touch->motion.notify = wlserver_handle_touch_motion; | ||
| wl_signal_add( &touch->wlr->events.motion, &touch->motion ); | ||
| touch->destroy.notify = wlserver_handle_touch_destroy; | ||
| wl_signal_add( &device->events.destroy, &touch->destroy); | ||
|
|
||
| wlserver_touch_associate_connector( touch ); | ||
| } | ||
|
|
@@ -610,6 +637,9 @@ static void handle_wl_surface_destroy( struct wl_listener *l, void *data ) | |
| wl_resource_set_user_data( pSwapchain, nullptr ); | ||
| } | ||
|
|
||
| wl_list_remove( &surf->commit.link ); | ||
| wl_list_remove( &surf->destroy.link ); | ||
|
|
||
| delete surf; | ||
| } | ||
|
|
||
|
|
@@ -1702,16 +1732,23 @@ int wlsession_open_kms( const char *device_name ) { | |
| } | ||
| } | ||
|
|
||
| struct wl_listener *listener = new wl_listener(); | ||
| listener->notify = kms_device_handle_change; | ||
| wl_signal_add( &wlserver.wlr.device->events.change, listener ); | ||
| wlserver.wlr.device_change_listener = new wl_listener(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Orthogonal nit: would be slightly nicer to not use |
||
| wlserver.wlr.device_change_listener->notify = kms_device_handle_change; | ||
| wl_signal_add( &wlserver.wlr.device->events.change, wlserver.wlr.device_change_listener ); | ||
|
|
||
| return wlserver.wlr.device->fd; | ||
| } | ||
|
|
||
| void wlsession_close_kms() | ||
| { | ||
| if ( wlserver.wlr.device_change_listener ) | ||
| { | ||
| wl_list_remove( &wlserver.wlr.device_change_listener->link ); | ||
| delete wlserver.wlr.device_change_listener; | ||
| wlserver.wlr.device_change_listener = nullptr; | ||
| } | ||
| wlr_session_close_file( wlserver.wlr.session, wlserver.wlr.device ); | ||
| wlserver.wlr.device = nullptr; | ||
| } | ||
|
|
||
| #endif | ||
|
|
@@ -1773,6 +1810,8 @@ gamescope_xwayland_server_t::~gamescope_xwayland_server_t() | |
| } | ||
| content_overrides.clear(); | ||
|
|
||
| wl_list_remove(&xwayland_ready_listener.link); | ||
|
|
||
| wlr_xwayland_server_destroy(xwayland_server); | ||
| xwayland_server = nullptr; | ||
|
|
||
|
|
@@ -2240,6 +2279,23 @@ void wlserver_run(void) | |
| // wlroots will restart it automatically. | ||
| wlserver_lock(); | ||
| wlserver.wlr.xwayland_servers.clear(); | ||
|
|
||
| wl_list_remove( &new_surface_listener.link ); | ||
| wl_list_remove( &new_input_listener.link ); | ||
| wl_list_remove( &wlserver.new_pointer_constraint.link ); | ||
| wl_list_remove( &wlserver.new_xdg_surface.link ); | ||
| wl_list_remove( &wlserver.new_xdg_toplevel.link ); | ||
| wl_list_remove( &wlserver.new_layer_shell_surface.link ); | ||
|
|
||
| #if HAVE_SESSION | ||
| if ( wlserver.wlr.session ) | ||
| { | ||
| wl_list_remove( &wlserver.session_active.link ); | ||
| wlr_session_destroy( wlserver.wlr.session ); | ||
| wlserver.wlr.session = nullptr; | ||
| } | ||
| #endif | ||
|
|
||
| wl_display_destroy_clients(wlserver.display); | ||
| wl_display_destroy(wlserver.display); | ||
| wlserver.display = NULL; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cannot happen, timeout is -1.