From c285ae57fb0027a3e6da12b7097e15e0485ee526 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 11:25:20 +0300 Subject: ControllerInterface: fix rare deadlock A "devices changed" callback could have ended up waiting on another thread that was also populating devices and waiting on the previous thread to release the callbacks mutex. --- Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp') diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp index 0cae47c741..afdf768435 100644 --- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp +++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp @@ -322,7 +322,9 @@ void ControllerInterface::UnregisterDevicesChangedCallback(const HotplugCallback // Invoke all callbacks that were registered void ControllerInterface::InvokeDevicesChangedCallbacks() const { - std::lock_guard lk(m_callbacks_mutex); - for (const auto& callback : m_devices_changed_callbacks) + m_callbacks_mutex.lock(); + const auto devices_changed_callbacks = m_devices_changed_callbacks; + m_callbacks_mutex.unlock(); + for (const auto& callback : devices_changed_callbacks) callback(); } -- cgit v1.2.3 From f90d851e2519d46f62e2037896053bc546e7133b Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 11:32:00 +0300 Subject: ControllerInterface: mixed comments --- .../Core/InputCommon/ControllerInterface/ControllerInterface.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp') diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp index afdf768435..c4ef52a2a9 100644 --- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp +++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp @@ -37,6 +37,10 @@ ControllerInterface g_controller_interface; +// We need to save which input channel we are in by thread, so we can access the correct input +// update values in different threads by input channel. We start from InputChannel::Host on all +// threads as hotkeys are updated from a worker thread, but UI can read from the main thread. This +// will never interfere with game threads. static thread_local ciface::InputChannel tls_input_channel = ciface::InputChannel::Host; void ControllerInterface::Initialize(const WindowSystemInfo& wsi) @@ -272,7 +276,11 @@ void ControllerInterface::UpdateInput() { std::lock_guard lk(m_devices_mutex, std::adopt_lock); for (const auto& d : m_devices) + { + // Theoretically we could avoid updating input on devices that don't have any references to + // them, but in practice a few devices types could break in different ways, so we don't d->UpdateInput(); + } } } -- cgit v1.2.3 From 2376aec135d957ee74bd2be1d587539dea4d5dab Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:06:12 +0300 Subject: 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/ControllerInterface.cpp | 164 +++++++++++++++------ 1 file changed, 122 insertions(+), 42 deletions(-) (limited to 'Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp') 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::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(); } -void ControllerInterface::AddDevice(std::shared_ptr device) +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); } -- cgit v1.2.3 From c238e4911916e2a90e2e324873e0da4907d243c3 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:08:38 +0300 Subject: ControllerInterface: Remove OSX window handle also make it more thread safe (avoid rare deadlock) and fix it trying to add devices before the CI has init --- .../ControllerInterface/ControllerInterface.cpp | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) (limited to 'Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp') diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp index 54974aed7a..b84e8af571 100644 --- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp +++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp @@ -64,9 +64,7 @@ void ControllerInterface::Initialize(const WindowSystemInfo& wsi) // nothing needed #endif #ifdef CIFACE_USE_OSX - if (m_wsi.type == WindowSystemType::MacOS) - ciface::OSX::Init(wsi.render_window); -// nothing needed for Quartz +// nothing needed for OSX and Quartz #endif #ifdef CIFACE_USE_SDL ciface::SDL::Init(); @@ -118,6 +116,18 @@ void ControllerInterface::RefreshDevices(RefreshReason reason) if (!m_is_init) return; +#ifdef CIFACE_USE_OSX + if (m_wsi.type == WindowSystemType::MacOS) + { + std::lock_guard lk_pre_population(m_pre_population_mutex); + // This is needed to stop its threads before locking our mutexes, to avoid deadlocks + // (in case it tried to add a device after we had locked m_devices_population_mutex). + // There doesn't seem to be an easy to way to repopulate OSX devices without restarting + // its hotplug thread. This will not release its devices, that's still done below. + ciface::OSX::DeInit(); + } +#endif + // 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. @@ -152,7 +162,10 @@ void ControllerInterface::RefreshDevices(RefreshReason reason) #ifdef CIFACE_USE_OSX if (m_wsi.type == WindowSystemType::MacOS) { - ciface::OSX::PopulateDevices(m_wsi.render_window); + { + std::lock_guard lk_pre_population(m_pre_population_mutex); + ciface::OSX::Init(); + } ciface::Quartz::PopulateDevices(m_wsi.render_window); } #endif -- cgit v1.2.3 From 1d816f8f267ff468478a4b8ff423b2dd0c734654 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:10:00 +0300 Subject: ControllerInterface: make real Wiimote use PlatformPopulateDevices() --- Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp') diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp index b84e8af571..cb4eae1472 100644 --- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp +++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp @@ -185,7 +185,7 @@ void ControllerInterface::RefreshDevices(RefreshReason reason) ciface::DualShockUDPClient::PopulateDevices(); #endif - WiimoteReal::ProcessWiimotePool(); + WiimoteReal::PopulateDevices(); m_devices_mutex.unlock(); -- cgit v1.2.3 From dcc345400e38e54ac84819e5cbcdf65c9cd95f96 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:14:11 +0300 Subject: ControllerInterface: devices population is now async so implement devices sorting priority This helps us keeping the most important devices (e.g. Mouse and Keyboard) on the top of the list of devices (they still are on all OSes supported by dolphin and to make hotplug devices like DSU appear at the bottom. --- .../InputCommon/ControllerInterface/ControllerInterface.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp') diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp index cb4eae1472..dd22ffe730 100644 --- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp +++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp @@ -319,6 +319,19 @@ bool ControllerInterface::AddDevice(std::shared_ptr device NOTICE_LOG_FMT(CONTROLLERINTERFACE, "Added device: {}", device->GetQualifiedName()); m_devices.emplace_back(std::move(device)); + + // We can't (and don't want) to control the order in which devices are added, but we + // need their order to be consistent, and we need the same one to always be the first, where + // present (the keyboard and mouse device usually). This is because when defaulting a + // controller profile, it will automatically select the first device in the list as its default. + std::stable_sort(m_devices.begin(), m_devices.end(), + [](const std::shared_ptr& a, + const std::shared_ptr& b) { + // It would be nice to sort devices by Source then Name then ID but it's + // better to leave them sorted by the add order, which also avoids breaking + // the order on other platforms that are less tested. + return a->GetSortPriority() > b->GetSortPriority(); + }); } if (!m_populating_devices_counter) -- cgit v1.2.3 From a0ecca1a84afb61809aa848405f119a9a7fff965 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:20:20 +0300 Subject: ControllerInterface: Implement ChangeWindow on DInput without recreating the devices Also polished DInput code in general to try and mitigate issue 11702. Added a lot of logging and comments. --- .../ControllerInterface/ControllerInterface.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) (limited to 'Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp') diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp index dd22ffe730..7c17a61448 100644 --- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp +++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp @@ -135,6 +135,26 @@ void ControllerInterface::RefreshDevices(RefreshReason reason) // This wasn't thread safe in multiple device sources. std::lock_guard lk_population(m_devices_population_mutex); +#if defined(CIFACE_USE_WIN32) && !defined(CIFACE_USE_XLIB) && !defined(CIFACE_USE_OSX) + // If only the window changed, avoid removing and re-adding all devices. + // Instead only refresh devices that require the window handle. + if (reason == RefreshReason::WindowChangeOnly) + { + m_populating_devices_counter.fetch_add(1); + + { + std::lock_guard lk(m_devices_mutex); + // No need to do anything else in this case. + // Only (Win32) DInput needs the window handle to be updated. + ciface::Win32::ChangeWindow(m_wsi.render_window); + } + + if (m_populating_devices_counter.fetch_sub(1) == 1) + InvokeDevicesChangedCallbacks(); + return; + } +#endif + m_populating_devices_counter.fetch_add(1); // We lock m_devices_mutex here to make everything simpler. -- cgit v1.2.3