Commit: 7de1a4d1d81ffd4cd2e75d911426edc847267244 Author: Campbell Barton Date: Mon Feb 6 11:38:04 2023 +1100 Branches: master https://developer.blender.org/rB7de1a4d1d81ffd4cd2e75d911426edc847267244
Fix GHOST/Wayland thread-unsafe timer-manager manipulation Mutex locks for manipulating GHOST_System::m_timerManager from GHOST_SystemWayland relied on WAYLAND being the only user of the timer-manager. This isn't the case as timers are fired from `GHOST_System::dispatchEvents`. Resolve by using a separate timer-manager for wayland key-repeat timers. =================================================================== M intern/ghost/intern/GHOST_SystemWayland.cpp M intern/ghost/intern/GHOST_SystemWayland.h =================================================================== diff --git a/intern/ghost/intern/GHOST_SystemWayland.cpp b/intern/ghost/intern/GHOST_SystemWayland.cpp index 8587438a34b..014f3d24bae 100644 --- a/intern/ghost/intern/GHOST_SystemWayland.cpp +++ b/intern/ghost/intern/GHOST_SystemWayland.cpp @@ -82,6 +82,8 @@ #include "CLG_log.h" #ifdef USE_EVENT_BACKGROUND_THREAD +# include "GHOST_TimerTask.h" + # include <pthread.h> #endif @@ -848,7 +850,14 @@ static void gwl_seat_key_repeat_timer_add(GWL_Seat *seat, GHOST_SystemWayland *system = seat->system; const uint64_t time_step = 1000 / seat->key_repeat.rate; const uint64_t time_start = use_delay ? seat->key_repeat.delay : time_step; +#ifdef USE_EVENT_BACKGROUND_THREAD + GHOST_TimerTask *timer = new GHOST_TimerTask( + system->getMilliSeconds() + time_start, time_step, key_repeat_fn, payload); + seat->key_repeat.timer = timer; + system->ghost_timer_manager()->addTimer(timer); +#else seat->key_repeat.timer = system->installTimer(time_start, time_step, key_repeat_fn, payload); +#endif } /** @@ -857,7 +866,12 @@ static void gwl_seat_key_repeat_timer_add(GWL_Seat *seat, static void gwl_seat_key_repeat_timer_remove(GWL_Seat *seat) { GHOST_SystemWayland *system = seat->system; +#ifdef USE_EVENT_BACKGROUND_THREAD + system->ghost_timer_manager()->removeTimer( + static_cast<GHOST_TimerTask *>(seat->key_repeat.timer)); +#else system->removeTimer(seat->key_repeat.timer); +#endif seat->key_repeat.timer = nullptr; } @@ -935,6 +949,16 @@ struct GWL_Display { /** Guard against multiple threads accessing `events_pending` at once. */ std::mutex events_pending_mutex; + /** + * A separate timer queue, needed so the WAYLAND thread can lock access. + * Using the system's #GHOST_Sysem::getTimerManager is not thread safe because + * access to the timer outside of WAYLAND specific logic will not lock. + * + * Needed because #GHOST_System::dispatchEvents fires timers + * outside of WAYLAND (without locking the `timer_mutex`). + */ + GHOST_TimerManager *ghost_timer_manager; + #endif /* USE_EVENT_BACKGROUND_THREAD */ }; @@ -951,6 +975,9 @@ static void gwl_display_destroy(GWL_Display *display) ghost_wl_display_lock_without_input(display->wl_display, display->system->server_mutex); display->events_pthread_is_active = false; } + + delete display->ghost_timer_manager; + display->ghost_timer_manager = nullptr; #endif /* For typical WAYLAND use this will always be set. @@ -5453,6 +5480,8 @@ GHOST_SystemWayland::GHOST_SystemWayland(bool background) #ifdef USE_EVENT_BACKGROUND_THREAD gwl_display_event_thread_create(display_); + + display_->ghost_timer_manager = new GHOST_TimerManager(); #endif } @@ -5533,10 +5562,16 @@ bool GHOST_SystemWayland::processEvents(bool waitForEvent) #endif /* USE_EVENT_BACKGROUND_THREAD */ { + const uint64_t now = getMilliSeconds(); #ifdef USE_EVENT_BACKGROUND_THREAD - std::lock_guard lock_timer_guard{*display_->system->timer_mutex}; + { + std::lock_guard lock_timer_guard{*display_->system->timer_mutex}; + if (ghost_timer_manager()->fireTimers(now)) { + any_processed = true; + } + } #endif - if (getTimerManager()->fireTimers(getMilliSeconds())) { + if (getTimerManager()->fireTimers(now)) { any_processed = true; } } @@ -6759,6 +6794,13 @@ struct wl_shm *GHOST_SystemWayland::wl_shm() const return display_->wl_shm; } +#ifdef USE_EVENT_BACKGROUND_THREAD +GHOST_TimerManager *GHOST_SystemWayland::ghost_timer_manager() +{ + return display_->ghost_timer_manager; +} +#endif + /** \} */ /* -------------------------------------------------------------------- */ diff --git a/intern/ghost/intern/GHOST_SystemWayland.h b/intern/ghost/intern/GHOST_SystemWayland.h index 44026d5efad..153931a0a39 100644 --- a/intern/ghost/intern/GHOST_SystemWayland.h +++ b/intern/ghost/intern/GHOST_SystemWayland.h @@ -165,6 +165,16 @@ class GHOST_SystemWayland : public GHOST_System { bool cursor_grab_use_software_display_get(const GHOST_TGrabCursorMode mode); +#ifdef USE_EVENT_BACKGROUND_THREAD + /** + * Return a separate WAYLAND local timer manager to #GHOST_System::getTimerManager + * Manipulation & access must lock with #GHOST_WaylandSystem::server_mutex. + * + * See #GWL_Display::ghost_timer_manager doc-string for details on why this is needed. + */ + GHOST_TimerManager *ghost_timer_manager(); +#endif + /* WAYLAND direct-data access. */ struct wl_display *wl_display(); @@ -233,7 +243,14 @@ class GHOST_SystemWayland : public GHOST_System { * from running at the same time. */ std::mutex *server_mutex = nullptr; - /** Threads must lock this before manipulating timers. */ + /** + * Threads must lock this before manipulating #GWL_Display::ghost_timer_manager. + * + * \note Using a separate lock to `server_mutex` is necessary because the + * server lock is already held when calling `ghost_wl_display_event_pump`. + * If manipulating the timer used the `server_mutex`, event pump can indirectly + * handle key up/down events which would lock `server_mutex` causing a dead-lock. + */ std::mutex *timer_mutex = nullptr; std::thread::id main_thread_id; _______________________________________________ Bf-blender-cvs mailing list Bf-blender-cvs@blender.org List details, subscription details or unsubscribe: https://lists.blender.org/mailman/listinfo/bf-blender-cvs