From 2376aec135d957ee74bd2be1d587539dea4d5dab Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:06:12 +0300 Subject: [PATCH] ControllerInterface: Refactor -Fix Add/Remove/Refresh device safety, devices could be added and removed at the same time, causing missing or duplicated devices (rare but possible) -Fix other devices population race conditions in ControllerInterface -Avoid re-creating all devices when dolphin is being shut down -Avoid re-creating devices when the render window handle has changed (just the relevantr ones now) -Avoid sending Devices Changed events if devices haven't actually changed -Made most devices populations will be made async, to increase performance and avoid hanging the host or CPU thread on manual devices refresh --- .../ControllerInterface.cpp | 164 +++++++++++++----- .../ControllerInterface/ControllerInterface.h | 38 +++- 2 files changed, 155 insertions(+), 47 deletions(-) diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp index c4ef52a2a9..54974aed7a 100644 --- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp +++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp @@ -6,6 +6,7 @@ #include +#include "Common/Assert.h" #include "Common/Logging/Log.h" #include "Core/HW/WiimoteReal/WiimoteReal.h" @@ -48,12 +49,13 @@ void ControllerInterface::Initialize(const WindowSystemInfo& wsi) if (m_is_init) return; + std::lock_guard lk_population(m_devices_population_mutex); + m_wsi = wsi; - // Allow backends to add devices as soon as they are initialized. - m_is_init = true; + m_populating_devices_counter = 1; - m_is_populating_devices = true; + m_devices_mutex.lock(); #ifdef CIFACE_USE_WIN32 ciface::Win32::Init(wsi.render_window); @@ -82,33 +84,63 @@ void ControllerInterface::Initialize(const WindowSystemInfo& wsi) ciface::DualShockUDPClient::Init(); #endif + // Don't allow backends to add devices before the first RefreshDevices() as they will be cleaned + // there. Or they'd end up waiting on the devices mutex if populated from another thread. + m_is_init = true; + RefreshDevices(); + + const bool devices_empty = m_devices.empty(); + + m_devices_mutex.unlock(); + + if (m_populating_devices_counter.fetch_sub(1) == 1 && !devices_empty) + InvokeDevicesChangedCallbacks(); } -void ControllerInterface::ChangeWindow(void* hwnd) +void ControllerInterface::ChangeWindow(void* hwnd, WindowChangeReason reason) { if (!m_is_init) return; // This shouldn't use render_surface so no need to update it. m_wsi.render_window = hwnd; - RefreshDevices(); + + // No need to re-add devices if this is an application exit request + if (reason == WindowChangeReason::Exit) + ClearDevices(); + else + RefreshDevices(RefreshReason::WindowChangeOnly); } -void ControllerInterface::RefreshDevices() +void ControllerInterface::RefreshDevices(RefreshReason reason) { if (!m_is_init) return; - { - std::lock_guard lk(m_devices_mutex); - m_devices.clear(); - } + // This lock has two main functions: + // -Avoid a deadlock between m_devices_mutex and ControllerEmu::s_state_mutex when + // InvokeDevicesChangedCallbacks() is called concurrently by two different threads. + // -Avoid devices being destroyed while others of the same type are being created. + // This wasn't thread safe in multiple device sources. + std::lock_guard lk_population(m_devices_population_mutex); - m_is_populating_devices = true; + m_populating_devices_counter.fetch_add(1); + + // We lock m_devices_mutex here to make everything simpler. + // Multiple devices classes have their own "hotplug" thread, and can add/remove devices at any + // time, while actual writes to "m_devices" are safe, the order in which they happen is not. That + // means a thread could be adding devices while we are removing them, or removing them as we are + // populating them (causing missing or duplicate devices). + m_devices_mutex.lock(); // Make sure shared_ptr objects are released before repopulating. - InvokeDevicesChangedCallbacks(); + ClearDevices(); + + // Some of these calls won't immediately populate devices, but will do it async + // with their own PlatformPopulateDevices(). + // This means that devices might end up in different order, unless we override their priority. + // It also means they might appear as "disconnected" in the Qt UI for a tiny bit of time. #ifdef CIFACE_USE_WIN32 ciface::Win32::PopulateDevices(m_wsi.render_window); @@ -142,8 +174,10 @@ void ControllerInterface::RefreshDevices() WiimoteReal::ProcessWiimotePool(); - m_is_populating_devices = false; - InvokeDevicesChangedCallbacks(); + m_devices_mutex.unlock(); + + if (m_populating_devices_counter.fetch_sub(1) == 1) + InvokeDevicesChangedCallbacks(); } void ControllerInterface::PlatformPopulateDevices(std::function callback) @@ -151,12 +185,18 @@ void ControllerInterface::PlatformPopulateDevices(std::function callback if (!m_is_init) return; - m_is_populating_devices = true; + std::lock_guard lk_population(m_devices_population_mutex); - callback(); + m_populating_devices_counter.fetch_add(1); - m_is_populating_devices = false; - InvokeDevicesChangedCallbacks(); + { + std::lock_guard lk(m_devices_mutex); + + callback(); + } + + if (m_populating_devices_counter.fetch_sub(1) == 1) + InvokeDevicesChangedCallbacks(); } // Remove all devices and call library cleanup functions @@ -167,23 +207,11 @@ void ControllerInterface::Shutdown() // Prevent additional devices from being added during shutdown. m_is_init = false; + // Additional safety measure to avoid InvokeDevicesChangedCallbacks() + m_populating_devices_counter = 1; - { - std::lock_guard lk(m_devices_mutex); - - for (const auto& d : m_devices) - { - // Set outputs to ZERO before destroying device - for (ciface::Core::Device::Output* o : d->Outputs()) - o->SetState(0); - } - - m_devices.clear(); - } - - // This will update control references so shared_ptrs are freed up - // BEFORE we shutdown the backends. - InvokeDevicesChangedCallbacks(); + // Update control references so shared_ptrs are freed up BEFORE we shutdown the backends. + ClearDevices(); #ifdef CIFACE_USE_WIN32 ciface::Win32::DeInit(); @@ -207,13 +235,48 @@ void ControllerInterface::Shutdown() #ifdef CIFACE_USE_DUALSHOCKUDPCLIENT ciface::DualShockUDPClient::DeInit(); #endif + + // Make sure no devices had been added within Shutdown() in the time + // between checking they checked atomic m_is_init bool and we changed it. + // We couldn't have locked m_devices_mutex nor m_devices_population_mutex for the whole Shutdown() + // as they could cause deadlocks. Note that this is still not 100% safe as some backends are + // shut down in other places, possibly adding devices after we have shut down, but the chances of + // that happening are basically zero. + ClearDevices(); } -void ControllerInterface::AddDevice(std::shared_ptr device) +void ControllerInterface::ClearDevices() +{ + std::lock_guard lk_population(m_devices_population_mutex); + + { + std::lock_guard lk(m_devices_mutex); + + if (m_devices.empty()) + return; + + for (const auto& d : m_devices) + { + // Set outputs to ZERO before destroying device + for (ciface::Core::Device::Output* o : d->Outputs()) + o->SetState(0); + } + + // Devices will still be alive after this: there are shared ptrs around the code holding them, + // but InvokeDevicesChangedCallbacks() will clean all of them. + m_devices.clear(); + } + + InvokeDevicesChangedCallbacks(); +} + +bool ControllerInterface::AddDevice(std::shared_ptr device) { // If we are shutdown (or in process of shutting down) ignore this request: if (!m_is_init) - return; + return false; + + std::lock_guard lk_population(m_devices_population_mutex); { std::lock_guard lk(m_devices_mutex); @@ -245,12 +308,21 @@ void ControllerInterface::AddDevice(std::shared_ptr device m_devices.emplace_back(std::move(device)); } - if (!m_is_populating_devices) + if (!m_populating_devices_counter) InvokeDevicesChangedCallbacks(); + return true; } -void ControllerInterface::RemoveDevice(std::function callback) +void ControllerInterface::RemoveDevice(std::function callback, + bool force_devices_release) { + // If we are shutdown (or in process of shutting down) ignore this request: + if (!m_is_init) + return; + + std::lock_guard lk_population(m_devices_population_mutex); + + bool any_removed; { std::lock_guard lk(m_devices_mutex); auto it = std::remove_if(m_devices.begin(), m_devices.end(), [&callback](const auto& dev) { @@ -261,17 +333,25 @@ void ControllerInterface::RemoveDevice(std::function callback) { - std::lock_guard lk(m_callbacks_mutex); + std::lock_guard lk(m_callbacks_mutex); m_devices_changed_callbacks.emplace_back(std::move(callback)); return std::prev(m_devices_changed_callbacks.end()); } @@ -323,7 +403,7 @@ ControllerInterface::RegisterDevicesChangedCallback(std::function callba // Unregister a device callback. void ControllerInterface::UnregisterDevicesChangedCallback(const HotplugCallbackHandle& handle) { - std::lock_guard lk(m_callbacks_mutex); + std::lock_guard lk(m_callbacks_mutex); m_devices_changed_callbacks.erase(handle); } diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h index 024548b713..826659941d 100644 --- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h +++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h @@ -60,13 +60,35 @@ class ControllerInterface : public ciface::Core::DeviceContainer public: using HotplugCallbackHandle = std::list>::iterator; + enum class WindowChangeReason + { + // Application is shutting down + Exit, + Other + }; + + enum class RefreshReason + { + // Only the window changed. + WindowChangeOnly, + // User requested, or any other internal reason (e.g. init). + // The window might have changed anyway. + Other + }; + ControllerInterface() : m_is_init(false) {} void Initialize(const WindowSystemInfo& wsi); - void ChangeWindow(void* hwnd); - void RefreshDevices(); + // Only call from one thread at a time. + void ChangeWindow(void* hwnd, WindowChangeReason reason = WindowChangeReason::Other); + // Can be called by any thread at any time (when initialized). + void RefreshDevices(RefreshReason reason = RefreshReason::Other); void Shutdown(); - void AddDevice(std::shared_ptr device); - void RemoveDevice(std::function callback); + bool AddDevice(std::shared_ptr device); + // Removes all the devices the function returns true to. + // If all the devices shared ptrs need to be destroyed immediately, + // set force_devices_release to true. + void RemoveDevice(std::function callback, + bool force_devices_release = false); // This is mandatory to use on device populations functions that can be called concurrently by // more than one thread, or that are called by a single other thread. // Without this, our devices list might end up in a mixed state. @@ -90,10 +112,16 @@ public: static ciface::InputChannel GetCurrentInputChannel(); private: + void ClearDevices(); + std::list> m_devices_changed_callbacks; + mutable std::recursive_mutex m_devices_population_mutex; mutable std::mutex m_callbacks_mutex; std::atomic m_is_init; - std::atomic m_is_populating_devices{false}; + // This is now always protected by m_devices_population_mutex, so + // it doesn't really need to be a counter or atomic anymore (it could be a raw bool), + // but we keep it so for simplicity, in case we changed the design. + std::atomic m_populating_devices_counter; WindowSystemInfo m_wsi; std::atomic m_aspect_ratio_adjustment = 1; };