From c285ae57fb0027a3e6da12b7097e15e0485ee526 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 11:25:20 +0300 Subject: [PATCH 01/14] 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. --- .../InputCommon/ControllerInterface/ControllerInterface.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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(); } From f90d851e2519d46f62e2037896053bc546e7133b Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 11:32:00 +0300 Subject: [PATCH 02/14] ControllerInterface: mixed comments --- .../ControllerInterface/ControllerInterface.cpp | 8 ++++++++ .../InputCommon/ControllerInterface/ControllerInterface.h | 3 +++ Source/Core/InputCommon/ControllerInterface/CoreDevice.h | 1 + .../ControllerInterface/DInput/DInputKeyboardMouse.cpp | 5 +++++ .../ControllerInterface/DInput/DInputKeyboardMouse.h | 4 ++++ 5 files changed, 21 insertions(+) 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(); + } } } diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h index 12d17614da..024548b713 100644 --- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h +++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h @@ -67,6 +67,9 @@ public: void Shutdown(); void AddDevice(std::shared_ptr device); void RemoveDevice(std::function callback); + // 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. void PlatformPopulateDevices(std::function callback); bool IsInit() const { return m_is_init; } void UpdateInput(); diff --git a/Source/Core/InputCommon/ControllerInterface/CoreDevice.h b/Source/Core/InputCommon/ControllerInterface/CoreDevice.h index c368578949..1994cb402c 100644 --- a/Source/Core/InputCommon/ControllerInterface/CoreDevice.h +++ b/Source/Core/InputCommon/ControllerInterface/CoreDevice.h @@ -227,6 +227,7 @@ public: std::chrono::milliseconds maximum_wait) const; protected: + // Exclusively needed when reading/writing "m_devices" mutable std::recursive_mutex m_devices_mutex; std::vector> m_devices; }; diff --git a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp index ebe2e84286..bc1820b8ad 100644 --- a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp +++ b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp @@ -91,6 +91,11 @@ KeyboardMouse::~KeyboardMouse() { s_keyboard_mouse_exists = false; + // Independently of the order in which we do these, if we put a breakpoint on Unacquire() (or in + // any place in the call stack before this), when refreshing devices from the UI, on the second + // attempt, it will get stuck in an infinite (while) loop inside dinput8.dll. Given that it can't + // be otherwise be reproduced (not even with sleeps), we can just ignore the problem. + // kb m_kb_device->Unacquire(); m_kb_device->Release(); diff --git a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h index ceda59d02c..3bd073a6f1 100644 --- a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h +++ b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h @@ -34,6 +34,7 @@ private: RelativeMouseState relative_mouse; }; + // Keyboard key class Key : public Input { public: @@ -46,6 +47,7 @@ private: const u8 m_index; }; + // Mouse button class Button : public Input { public: @@ -58,6 +60,7 @@ private: const u8 m_index; }; + // Mouse movement offset axis. Includes mouse wheel class Axis : public Input { public: @@ -72,6 +75,7 @@ private: const u8 m_index; }; + // Mouse from window center class Cursor : public Input { public: From 2376aec135d957ee74bd2be1d587539dea4d5dab Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:06:12 +0300 Subject: [PATCH 03/14] 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; }; From c238e4911916e2a90e2e324873e0da4907d243c3 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:08:38 +0300 Subject: [PATCH 04/14] 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.cpp | 21 ++++++++++++--- .../ControllerInterface/ControllerInterface.h | 1 + .../InputCommon/ControllerInterface/OSX/OSX.h | 3 +-- .../ControllerInterface/OSX/OSX.mm | 26 +++++++------------ 4 files changed, 29 insertions(+), 22 deletions(-) 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 diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h index 826659941d..9c91d853e7 100644 --- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h +++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.h @@ -116,6 +116,7 @@ private: std::list> m_devices_changed_callbacks; mutable std::recursive_mutex m_devices_population_mutex; + mutable std::mutex m_pre_population_mutex; mutable std::mutex m_callbacks_mutex; std::atomic m_is_init; // This is now always protected by m_devices_population_mutex, so diff --git a/Source/Core/InputCommon/ControllerInterface/OSX/OSX.h b/Source/Core/InputCommon/ControllerInterface/OSX/OSX.h index 955b426f5e..f4ebc1a911 100644 --- a/Source/Core/InputCommon/ControllerInterface/OSX/OSX.h +++ b/Source/Core/InputCommon/ControllerInterface/OSX/OSX.h @@ -6,8 +6,7 @@ namespace ciface::OSX { -void Init(void* window); -void PopulateDevices(void* window); +void Init(); void DeInit(); void DeviceElementDebugPrint(const void*, void*); diff --git a/Source/Core/InputCommon/ControllerInterface/OSX/OSX.mm b/Source/Core/InputCommon/ControllerInterface/OSX/OSX.mm index 63bddf499e..8c80e9f529 100644 --- a/Source/Core/InputCommon/ControllerInterface/OSX/OSX.mm +++ b/Source/Core/InputCommon/ControllerInterface/OSX/OSX.mm @@ -135,8 +135,6 @@ static void DeviceDebugPrint(IOHIDDeviceRef device) #endif } -static void* g_window; - static std::string GetDeviceRefName(IOHIDDeviceRef inIOHIDDeviceRef) { const NSString* name = reinterpret_cast( @@ -172,10 +170,8 @@ static void DeviceMatchingCallback(void* inContext, IOReturn inResult, void* inS } } -void Init(void* window) +void Init() { - g_window = window; - HIDManager = IOHIDManagerCreate(kCFAllocatorDefault, kIOHIDOptionsTypeNone); if (!HIDManager) ERROR_LOG_FMT(CONTROLLERINTERFACE, "Failed to create HID Manager reference"); @@ -210,19 +206,17 @@ void Init(void* window) }); } -void PopulateDevices(void* window) -{ - DeInit(); - Init(window); -} - void DeInit() { - s_stopper.Signal(); - s_hotplug_thread.join(); + if (HIDManager) + { + s_stopper.Signal(); + s_hotplug_thread.join(); - // This closes all devices as well - IOHIDManagerClose(HIDManager, kIOHIDOptionsTypeNone); - CFRelease(HIDManager); + // This closes all devices as well + IOHIDManagerClose(HIDManager, kIOHIDOptionsTypeNone); + CFRelease(HIDManager); + HIDManager = nullptr; + } } } // namespace ciface::OSX From 1d816f8f267ff468478a4b8ff423b2dd0c734654 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:10:00 +0300 Subject: [PATCH 05/14] ControllerInterface: make real Wiimote use PlatformPopulateDevices() --- .../Core/Core/HW/WiimoteReal/WiimoteReal.cpp | 28 +++++++++++++++---- Source/Core/Core/HW/WiimoteReal/WiimoteReal.h | 5 +++- .../ControllerInterface.cpp | 2 +- .../Wiimote/WiimoteController.cpp | 16 +++++++---- 4 files changed, 38 insertions(+), 13 deletions(-) diff --git a/Source/Core/Core/HW/WiimoteReal/WiimoteReal.cpp b/Source/Core/Core/HW/WiimoteReal/WiimoteReal.cpp index 680e85d61c..83d262ebeb 100644 --- a/Source/Core/Core/HW/WiimoteReal/WiimoteReal.cpp +++ b/Source/Core/Core/HW/WiimoteReal/WiimoteReal.cpp @@ -96,8 +96,15 @@ static void TryToFillWiimoteSlot(u32 index) s_wiimote_pool.erase(s_wiimote_pool.begin()); } +void PopulateDevices() +{ + // There is a very small chance of deadlocks if we didn't do it in another thread + s_wiimote_scanner.PopulateDevices(); +} + // Attempts to fill enabled real wiimote slots. // Push/pull wiimotes to/from ControllerInterface as needed. +// Should be called PopulateDevices() to be in line with other implementations. void ProcessWiimotePool() { std::lock_guard lk(g_wiimotes_mutex); @@ -545,7 +552,7 @@ void WiimoteScanner::StopThread() void WiimoteScanner::SetScanMode(WiimoteScanMode scan_mode) { m_scan_mode.store(scan_mode); - m_scan_mode_changed_event.Set(); + m_scan_mode_changed_or_population_event.Set(); } bool WiimoteScanner::IsReady() const @@ -610,6 +617,12 @@ void WiimoteScanner::PoolThreadFunc() } } +void WiimoteScanner::PopulateDevices() +{ + m_populate_devices.Set(); + m_scan_mode_changed_or_population_event.Set(); +} + void WiimoteScanner::ThreadFunc() { std::thread pool_thread(&WiimoteScanner::PoolThreadFunc, this); @@ -634,7 +647,12 @@ void WiimoteScanner::ThreadFunc() while (m_scan_thread_running.IsSet()) { - m_scan_mode_changed_event.WaitFor(std::chrono::milliseconds(500)); + m_scan_mode_changed_or_population_event.WaitFor(std::chrono::milliseconds(500)); + + if (m_populate_devices.TestAndClear()) + { + g_controller_interface.PlatformPopulateDevices([] { ProcessWiimotePool(); }); + } // Does stuff needed to detect disconnects on Windows for (const auto& backend : m_backends) @@ -675,7 +693,7 @@ void WiimoteScanner::ThreadFunc() } AddWiimoteToPool(std::unique_ptr(wiimote)); - ProcessWiimotePool(); + g_controller_interface.PlatformPopulateDevices([] { ProcessWiimotePool(); }); } if (found_board) @@ -956,12 +974,12 @@ void HandleWiimoteSourceChange(unsigned int index) if (auto removed_wiimote = std::move(g_wiimotes[index])) AddWiimoteToPool(std::move(removed_wiimote)); }); - ProcessWiimotePool(); + g_controller_interface.PlatformPopulateDevices([] { ProcessWiimotePool(); }); } void HandleWiimotesInControllerInterfaceSettingChange() { - ProcessWiimotePool(); + g_controller_interface.PlatformPopulateDevices([] { ProcessWiimotePool(); }); } } // namespace WiimoteReal diff --git a/Source/Core/Core/HW/WiimoteReal/WiimoteReal.h b/Source/Core/Core/HW/WiimoteReal/WiimoteReal.h index 44d6a22af4..701371d4bd 100644 --- a/Source/Core/Core/HW/WiimoteReal/WiimoteReal.h +++ b/Source/Core/Core/HW/WiimoteReal/WiimoteReal.h @@ -171,6 +171,7 @@ public: void StartThread(); void StopThread(); void SetScanMode(WiimoteScanMode scan_mode); + void PopulateDevices(); bool IsReady() const; @@ -183,7 +184,8 @@ private: std::thread m_scan_thread; Common::Flag m_scan_thread_running; - Common::Event m_scan_mode_changed_event; + Common::Flag m_populate_devices; + Common::Event m_scan_mode_changed_or_population_event; std::atomic m_scan_mode{WiimoteScanMode::DO_NOT_SCAN}; }; @@ -204,6 +206,7 @@ void InitAdapterClass(); #endif void HandleWiimotesInControllerInterfaceSettingChange(); +void PopulateDevices(); void ProcessWiimotePool(); } // namespace WiimoteReal 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(); diff --git a/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp b/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp index eaa56ef449..dfd329f5e0 100644 --- a/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp +++ b/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp @@ -128,13 +128,17 @@ void ReleaseDevices(std::optional count) // Remove up to "count" remotes (or all of them if nullopt). // Real wiimotes will be added to the pool. - g_controller_interface.RemoveDevice([&](const Core::Device* device) { - if (device->GetSource() != SOURCE_NAME || count == removed_devices) - return false; + // Make sure to force the device removal immediately (as they are shared ptrs and + // they could be kept alive, preventing us from re-creating the device) + g_controller_interface.RemoveDevice( + [&](const Core::Device* device) { + if (device->GetSource() != SOURCE_NAME || count == removed_devices) + return false; - ++removed_devices; - return true; - }); + ++removed_devices; + return true; + }, + true); } Device::Device(std::unique_ptr wiimote) : m_wiimote(std::move(wiimote)) From 2aa941081e7d07d11500ea6080c9f65af005caeb Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:10:39 +0300 Subject: [PATCH 06/14] ControllerInterface: make SDL use PlatformPopulateDevices() and avoid waiting on SDL async population being finished for no reason --- .../InputCommon/ControllerInterface/SDL/SDL.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/Source/Core/InputCommon/ControllerInterface/SDL/SDL.cpp b/Source/Core/InputCommon/ControllerInterface/SDL/SDL.cpp index f908181c8d..71d75f786c 100644 --- a/Source/Core/InputCommon/ControllerInterface/SDL/SDL.cpp +++ b/Source/Core/InputCommon/ControllerInterface/SDL/SDL.cpp @@ -46,7 +46,6 @@ static void OpenAndAddDevice(int index) static Common::Event s_init_event; static Uint32 s_stop_event_type; static Uint32 s_populate_event_type; -static Common::Event s_populated_event; static std::thread s_hotplug_thread; static bool HandleEventAndContinue(const SDL_Event& e) @@ -64,9 +63,10 @@ static bool HandleEventAndContinue(const SDL_Event& e) } else if (e.type == s_populate_event_type) { - for (int i = 0; i < SDL_NumJoysticks(); ++i) - OpenAndAddDevice(i); - s_populated_event.Set(); + g_controller_interface.PlatformPopulateDevices([] { + for (int i = 0; i < SDL_NumJoysticks(); ++i) + OpenAndAddDevice(i); + }); } else if (e.type == s_stop_event_type) { @@ -111,7 +111,8 @@ void Init() // Drain all of the events and add the initial joysticks before returning. Otherwise, the // individual joystick events as well as the custom populate event will be handled _after_ // ControllerInterface::Init/RefreshDevices has cleared its list of devices, resulting in - // duplicate devices. + // duplicate devices. Adding devices will actually "fail" here, as the ControllerInterface + // hasn't finished initializing yet. SDL_Event e; while (SDL_PollEvent(&e) != 0) { @@ -161,8 +162,6 @@ void PopulateDevices() SDL_Event populate_event{s_populate_event_type}; SDL_PushEvent(&populate_event); - - s_populated_event.Wait(); #endif } From 0718cfd7d781952eff53f052466faaea6b5f3a2f Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:11:27 +0300 Subject: [PATCH 07/14] ControllerInterface: make evdev use PlatformPopulateDevices Also fix evdev non thread safe acces to static variables --- .../ControllerInterface/evdev/evdev.cpp | 45 ++++++++++++++----- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/Source/Core/InputCommon/ControllerInterface/evdev/evdev.cpp b/Source/Core/InputCommon/ControllerInterface/evdev/evdev.cpp index 265d81ad40..cb13c65a1e 100644 --- a/Source/Core/InputCommon/ControllerInterface/evdev/evdev.cpp +++ b/Source/Core/InputCommon/ControllerInterface/evdev/evdev.cpp @@ -202,6 +202,9 @@ static int s_wakeup_eventfd; // There is no easy way to get the device name from only a dev node // during a device removed event, since libevdev can't work on removed devices; // sysfs is not stable, so this is probably the easiest way to get a name for a node. +// This can, and will be modified by different thread, possibly concurrently, +// as devices can be destroyed by any thread at any time. As of now it's protected +// by ControllerInterface::m_devices_population_mutex. static std::map> s_devnode_objects; static std::shared_ptr @@ -260,9 +263,13 @@ static void AddDeviceNode(const char* devnode) // Remove and re-add device as naming and inputs may have changed. // This will also give it the correct index and invoke device change callbacks. - g_controller_interface.RemoveDevice([&evdev_device](const auto* device) { - return static_cast(device) == evdev_device.get(); - }); + // Make sure to force the device removal immediately (as they are shared ptrs and + // they could be kept alive, preventing us from re-creating the device) + g_controller_interface.RemoveDevice( + [&evdev_device](const auto* device) { + return static_cast(device) == evdev_device.get(); + }, + true); g_controller_interface.AddDevice(evdev_device); } @@ -276,6 +283,8 @@ static void AddDeviceNode(const char* devnode) g_controller_interface.AddDevice(evdev_device); } + // If the devices failed to be added to g_controller_interface, it will be added here but then + // immediately removed in its destructor due to the shared ptr not having any references left s_devnode_objects.emplace(devnode, std::move(evdev_device)); } @@ -318,23 +327,30 @@ static void HotplugThreadFunc() if (!devnode) continue; + // Use g_controller_interface.PlatformPopulateDevices() to protect access around + // s_devnode_objects. Note that even if we get these events at the same time as a + // a PopulateDevices() request (e.g. on start up, we might get all the add events + // for connected devices), this won't ever cause duplicate devices as AddDeviceNode() + // automatically removes the old one if it already existed if (strcmp(action, "remove") == 0) { - std::shared_ptr ptr; + g_controller_interface.PlatformPopulateDevices([&devnode] { + std::shared_ptr ptr; - const auto it = s_devnode_objects.find(devnode); - if (it != s_devnode_objects.end()) - ptr = it->second.lock(); + const auto it = s_devnode_objects.find(devnode); + if (it != s_devnode_objects.end()) + ptr = it->second.lock(); - // If we don't recognize this device, ptr will be null and no device will be removed. + // If we don't recognize this device, ptr will be null and no device will be removed. - g_controller_interface.RemoveDevice([&ptr](const auto* device) { - return static_cast(device) == ptr.get(); + g_controller_interface.RemoveDevice([&ptr](const auto* device) { + return static_cast(device) == ptr.get(); + }); }); } else if (strcmp(action, "add") == 0) { - AddDeviceNode(devnode); + g_controller_interface.PlatformPopulateDevices([&devnode] { AddDeviceNode(devnode); }); } } NOTICE_LOG_FMT(CONTROLLERINTERFACE, "evdev hotplug thread stopped"); @@ -376,8 +392,15 @@ void Init() StartHotplugThread(); } +// Only call this when ControllerInterface::m_devices_population_mutex is locked void PopulateDevices() { + // Don't run if we are not initialized + if (!s_hotplug_thread_running.IsSet()) + { + return; + } + // We use udev to iterate over all /dev/input/event* devices. // Note: the Linux kernel is currently limited to just 32 event devices. If // this ever changes, hopefully udev will take care of this. From dcc345400e38e54ac84819e5cbcdf65c9cd95f96 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:14:11 +0300 Subject: [PATCH 08/14] 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. --- .../ControllerInterface/ControllerInterface.cpp | 13 +++++++++++++ .../InputCommon/ControllerInterface/CoreDevice.h | 5 +++++ .../DInput/DInputKeyboardMouse.cpp | 6 ++++++ .../DInput/DInputKeyboardMouse.h | 1 + .../DualShockUDPClient/DualShockUDPClient.cpp | 2 ++ .../Wiimote/WiimoteController.cpp | 6 ++++++ .../ControllerInterface/Wiimote/WiimoteController.h | 1 + 7 files changed, 34 insertions(+) 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) diff --git a/Source/Core/InputCommon/ControllerInterface/CoreDevice.h b/Source/Core/InputCommon/ControllerInterface/CoreDevice.h index 1994cb402c..e094d2477d 100644 --- a/Source/Core/InputCommon/ControllerInterface/CoreDevice.h +++ b/Source/Core/InputCommon/ControllerInterface/CoreDevice.h @@ -128,6 +128,11 @@ public: // (e.g. Xbox 360 controllers have controller number LEDs which should match the ID we use.) virtual std::optional GetPreferredId() const; + // Use this to change the order in which devices are sorted in their list. + // A higher priority means it will be one of the first ones (smaller index), making it more + // likely to be index 0, which is automatically set as the default device when there isn't one. + virtual int GetSortPriority() const { return 0; } + const std::vector& Inputs() const { return m_inputs; } const std::vector& Outputs() const { return m_outputs; } diff --git a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp index bc1820b8ad..0da09ad6e0 100644 --- a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp +++ b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp @@ -230,6 +230,12 @@ std::string KeyboardMouse::GetSource() const return DINPUT_SOURCE_NAME; } +// Give this device a higher priority to make sure it shows first +int KeyboardMouse::GetSortPriority() const +{ + return 5; +} + // names std::string KeyboardMouse::Key::GetName() const { diff --git a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h index 3bd073a6f1..28c6407471 100644 --- a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h +++ b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h @@ -102,6 +102,7 @@ public: std::string GetName() const override; std::string GetSource() const override; + int GetSortPriority() const override; private: void UpdateCursorInput(); diff --git a/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp b/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp index 6511af49c1..73eb0e515a 100644 --- a/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp +++ b/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp @@ -136,6 +136,8 @@ public: std::string GetName() const final override; std::string GetSource() const final override; std::optional GetPreferredId() const final override; + // Always add these at the end, given their hotplug nature + int GetSortPriority() const override { return -2; } private: void ResetPadData(); diff --git a/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp b/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp index dfd329f5e0..d68b0441a6 100644 --- a/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp +++ b/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp @@ -321,6 +321,12 @@ std::string Device::GetSource() const return SOURCE_NAME; } +// Always add these at the end, given their hotplug nature +int Device::GetSortPriority() const +{ + return -1; +} + void Device::RunTasks() { if (IsPerformingTask()) diff --git a/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.h b/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.h index 4864e0abb3..b6c96d0238 100644 --- a/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.h +++ b/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.h @@ -33,6 +33,7 @@ public: std::string GetName() const override; std::string GetSource() const override; + int GetSortPriority() const override; void UpdateInput() override; From a0ecca1a84afb61809aa848405f119a9a7fff965 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:20:20 +0300 Subject: [PATCH 09/14] 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.cpp | 20 +++++++ .../ControllerInterface/CoreDevice.h | 6 +++ .../ControllerInterface/DInput/DInput.cpp | 54 ++++++++++++++----- .../ControllerInterface/DInput/DInput.h | 2 + .../DInput/DInputKeyboardMouse.cpp | 39 ++++++++++---- .../DInput/DInputKeyboardMouse.h | 7 ++- .../ControllerInterface/Win32/Win32.cpp | 48 +++++++++++++---- .../ControllerInterface/Win32/Win32.h | 1 + 8 files changed, 140 insertions(+), 37 deletions(-) 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. diff --git a/Source/Core/InputCommon/ControllerInterface/CoreDevice.h b/Source/Core/InputCommon/ControllerInterface/CoreDevice.h index e094d2477d..d3312568b2 100644 --- a/Source/Core/InputCommon/ControllerInterface/CoreDevice.h +++ b/Source/Core/InputCommon/ControllerInterface/CoreDevice.h @@ -125,6 +125,12 @@ public: // Currently handled on a per-backend basis but this could change. virtual bool IsValid() const { return true; } + // Returns true whether this device is "virtual/emulated", not linked + // to any actual physical device. Mostly used by keyboard and mouse devices, + // and to avoid uselessly recreating the device unless really necessary. + // Doesn't necessarily need to be set to true if the device is virtual. + virtual bool IsVirtualDevice() const { return false; } + // (e.g. Xbox 360 controllers have controller number LEDs which should match the ID we use.) virtual std::optional GetPreferredId() const; diff --git a/Source/Core/InputCommon/ControllerInterface/DInput/DInput.cpp b/Source/Core/InputCommon/ControllerInterface/DInput/DInput.cpp index d508de613a..f5e105dc45 100644 --- a/Source/Core/InputCommon/ControllerInterface/DInput/DInput.cpp +++ b/Source/Core/InputCommon/ControllerInterface/DInput/DInput.cpp @@ -16,6 +16,8 @@ namespace ciface::DInput { +static IDirectInput8* s_idi8 = nullptr; + BOOL CALLBACK DIEnumDeviceObjectsCallback(LPCDIDEVICEOBJECTINSTANCE lpddoi, LPVOID pvRef) { ((std::list*)pvRef)->push_back(*lpddoi); @@ -42,29 +44,57 @@ std::string GetDeviceName(const LPDIRECTINPUTDEVICE8 device) } else { - ERROR_LOG_FMT(PAD, "GetProperty(DIPROP_PRODUCTNAME) failed."); + ERROR_LOG_FMT(CONTROLLERINTERFACE, "GetProperty(DIPROP_PRODUCTNAME) failed."); } return result; } +// Assumes hwnd had not changed from the previous call void PopulateDevices(HWND hwnd) { - // Remove unplugged devices. - g_controller_interface.RemoveDevice( - [](const auto* dev) { return dev->GetSource() == DINPUT_SOURCE_NAME && !dev->IsValid(); }); - - IDirectInput8* idi8; - if (FAILED(DirectInput8Create(GetModuleHandle(nullptr), DIRECTINPUT_VERSION, IID_IDirectInput8, - (LPVOID*)&idi8, nullptr))) + if (!s_idi8 && FAILED(DirectInput8Create(GetModuleHandle(nullptr), DIRECTINPUT_VERSION, + IID_IDirectInput8, (LPVOID*)&s_idi8, nullptr))) { - ERROR_LOG_FMT(PAD, "DirectInput8Create failed."); + ERROR_LOG_FMT(CONTROLLERINTERFACE, "DirectInput8Create failed."); return; } - InitKeyboardMouse(idi8, hwnd); - InitJoystick(idi8, hwnd); + // Remove old (invalid) devices. No need to ever remove the KeyboardMouse device. + // Note that if we have 2+ DInput controllers, not fully repopulating devices + // will mean that a device with index "2" could persist while there is no device with index "0". + // This is slightly inconsistent as when we refresh all devices, they will instead reset, and + // that happens a lot (for uncontrolled reasons, like starting/stopping the emulation). + g_controller_interface.RemoveDevice( + [](const auto* dev) { return dev->GetSource() == DINPUT_SOURCE_NAME && !dev->IsValid(); }); - idi8->Release(); + InitKeyboardMouse(s_idi8, hwnd); + InitJoystick(s_idi8, hwnd); +} + +void ChangeWindow(HWND hwnd) +{ + if (s_idi8) // Has init? Ignore if called before the first PopulateDevices() + { + // The KeyboardMouse device is marked as virtual device, so we avoid removing it. + // We need to force all the DInput joysticks to be destroyed now, or recreation would fail. + g_controller_interface.RemoveDevice( + [](const auto* dev) { + return dev->GetSource() == DINPUT_SOURCE_NAME && !dev->IsVirtualDevice(); + }, + true); + + SetKeyboardMouseWindow(hwnd); + InitJoystick(s_idi8, hwnd); + } +} + +void DeInit() +{ + if (s_idi8) + { + s_idi8->Release(); + s_idi8 = nullptr; + } } } // namespace ciface::DInput diff --git a/Source/Core/InputCommon/ControllerInterface/DInput/DInput.h b/Source/Core/InputCommon/ControllerInterface/DInput/DInput.h index ed159089e0..8be064a1f1 100644 --- a/Source/Core/InputCommon/ControllerInterface/DInput/DInput.h +++ b/Source/Core/InputCommon/ControllerInterface/DInput/DInput.h @@ -20,4 +20,6 @@ BOOL CALLBACK DIEnumDevicesCallback(LPCDIDEVICEINSTANCE lpddi, LPVOID pvRef); std::string GetDeviceName(const LPDIRECTINPUTDEVICE8 device); void PopulateDevices(HWND hwnd); +void ChangeWindow(HWND hwnd); +void DeInit(); } // namespace ciface::DInput diff --git a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp index 0da09ad6e0..e952da6660 100644 --- a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp +++ b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp @@ -6,7 +6,8 @@ #include -#include +#include "Common/Logging/Log.h" +#include "Core/Core.h" #include "InputCommon/ControllerInterface/ControllerInterface.h" #include "InputCommon/ControllerInterface/DInput/DInput.h" @@ -54,15 +55,18 @@ static const struct #include "InputCommon/ControllerInterface/DInput/NamedKeys.h" // NOLINT }; -// Prevent duplicate keyboard/mouse devices. -static bool s_keyboard_mouse_exists = false; +// Prevent duplicate keyboard/mouse devices. Modified by more threads. +static bool s_keyboard_mouse_exists; +static HWND s_hwnd; void InitKeyboardMouse(IDirectInput8* const idi8, HWND hwnd) { if (s_keyboard_mouse_exists) return; - // mouse and keyboard are a combined device, to allow shift+click and stuff + s_hwnd = hwnd; + + // Mouse and keyboard are a combined device, to allow shift+click and stuff // if that's dumb, I will make a VirtualDevice class that just uses ranges of inputs/outputs from // other devices // so there can be a separated Keyboard and mouse, as well as combined KeyboardMouse @@ -70,6 +74,8 @@ void InitKeyboardMouse(IDirectInput8* const idi8, HWND hwnd) LPDIRECTINPUTDEVICE8 kb_device = nullptr; LPDIRECTINPUTDEVICE8 mo_device = nullptr; + // These are "virtual" system devices, so they are always there even if we have no physical + // mouse and keyboard plugged into the computer if (SUCCEEDED(idi8->CreateDevice(GUID_SysKeyboard, &kb_device, nullptr)) && SUCCEEDED(kb_device->SetDataFormat(&c_dfDIKeyboard)) && SUCCEEDED(kb_device->SetCooperativeLevel(nullptr, DISCL_BACKGROUND | DISCL_NONEXCLUSIVE)) && @@ -77,16 +83,23 @@ void InitKeyboardMouse(IDirectInput8* const idi8, HWND hwnd) SUCCEEDED(mo_device->SetDataFormat(&c_dfDIMouse2)) && SUCCEEDED(mo_device->SetCooperativeLevel(nullptr, DISCL_BACKGROUND | DISCL_NONEXCLUSIVE))) { - g_controller_interface.AddDevice(std::make_shared(kb_device, mo_device, hwnd)); + g_controller_interface.AddDevice(std::make_shared(kb_device, mo_device)); return; } + ERROR_LOG_FMT(CONTROLLERINTERFACE, "KeyboardMouse device failed to be created"); + if (kb_device) kb_device->Release(); if (mo_device) mo_device->Release(); } +void SetKeyboardMouseWindow(HWND hwnd) +{ + s_hwnd = hwnd; +} + KeyboardMouse::~KeyboardMouse() { s_keyboard_mouse_exists = false; @@ -105,9 +118,8 @@ KeyboardMouse::~KeyboardMouse() } KeyboardMouse::KeyboardMouse(const LPDIRECTINPUTDEVICE8 kb_device, - const LPDIRECTINPUTDEVICE8 mo_device, HWND hwnd) - : m_kb_device(kb_device), m_mo_device(mo_device), m_hwnd(hwnd), m_last_update(GetTickCount()), - m_state_in() + const LPDIRECTINPUTDEVICE8 mo_device) + : m_kb_device(kb_device), m_mo_device(mo_device), m_last_update(GetTickCount()), m_state_in() { s_keyboard_mouse_exists = true; @@ -160,11 +172,11 @@ void KeyboardMouse::UpdateCursorInput() // Get the cursor position relative to the upper left corner of the current window // (separate or render to main) - ScreenToClient(m_hwnd, &point); + ScreenToClient(s_hwnd, &point); - // Get the size of the current window. (In my case Rect.top and Rect.left was zero.) + // Get the size of the current window (in my case Rect.top and Rect.left was zero). RECT rect; - GetClientRect(m_hwnd, &rect); + GetClientRect(s_hwnd, &rect); // Width and height are the size of the rendering window. They could be 0 const auto win_width = std::max(rect.right - rect.left, 1l); @@ -236,6 +248,11 @@ int KeyboardMouse::GetSortPriority() const return 5; } +bool KeyboardMouse::IsVirtualDevice() const +{ + return true; +} + // names std::string KeyboardMouse::Key::GetName() const { diff --git a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h index 28c6407471..b0fd39c00d 100644 --- a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h +++ b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h @@ -16,6 +16,7 @@ namespace ciface::DInput void InitKeyboardMouse(IDirectInput8* const idi8, HWND hwnd); using RelativeMouseState = RelativeInputState>; +void SetKeyboardMouseWindow(HWND hwnd); class KeyboardMouse : public Core::Device { @@ -96,13 +97,13 @@ private: public: void UpdateInput() override; - KeyboardMouse(const LPDIRECTINPUTDEVICE8 kb_device, const LPDIRECTINPUTDEVICE8 mo_device, - HWND hwnd); + KeyboardMouse(const LPDIRECTINPUTDEVICE8 kb_device, const LPDIRECTINPUTDEVICE8 mo_device); ~KeyboardMouse(); std::string GetName() const override; std::string GetSource() const override; int GetSortPriority() const override; + bool IsVirtualDevice() const override; private: void UpdateCursorInput(); @@ -110,8 +111,6 @@ private: const LPDIRECTINPUTDEVICE8 m_kb_device; const LPDIRECTINPUTDEVICE8 m_mo_device; - const HWND m_hwnd; - DWORD m_last_update; State m_state_in; }; diff --git a/Source/Core/InputCommon/ControllerInterface/Win32/Win32.cpp b/Source/Core/InputCommon/ControllerInterface/Win32/Win32.cpp index b7cf19a13d..066c50a693 100644 --- a/Source/Core/InputCommon/ControllerInterface/Win32/Win32.cpp +++ b/Source/Core/InputCommon/ControllerInterface/Win32/Win32.cpp @@ -11,6 +11,7 @@ #include #include "Common/Event.h" +#include "Common/Flag.h" #include "Common/Logging/Log.h" #include "Common/ScopeGuard.h" #include "Common/Thread.h" @@ -19,20 +20,30 @@ constexpr UINT WM_DOLPHIN_STOP = WM_USER; -static Common::Event s_done_populating; -static std::atomic s_hwnd; +static Common::Event s_received_device_change_event; +// Dolphin's render window +static HWND s_hwnd; +// Windows messaging window (hidden) static HWND s_message_window; static std::thread s_thread; +static Common::Flag s_first_populate_devices_asked; static LRESULT CALLBACK WindowProc(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) { if (message == WM_INPUT_DEVICE_CHANGE) { - g_controller_interface.PlatformPopulateDevices([] { - ciface::DInput::PopulateDevices(s_hwnd); - ciface::XInput::PopulateDevices(); - }); - s_done_populating.Set(); + // Windows automatically sends this message before we ask for it and before we are "ready" to + // listen for it. + if (s_first_populate_devices_asked.IsSet()) + { + s_received_device_change_event.Set(); + // TODO: we could easily use the message passed alongside this event, which tells + // whether a device was added or removed, to avoid removing old, still connected, devices + g_controller_interface.PlatformPopulateDevices([] { + ciface::DInput::PopulateDevices(s_hwnd); + ciface::XInput::PopulateDevices(); + }); + } } return DefWindowProc(hwnd, message, wparam, lparam); @@ -125,10 +136,14 @@ void ciface::Win32::PopulateDevices(void* hwnd) if (s_thread.joinable()) { s_hwnd = static_cast(hwnd); - s_done_populating.Reset(); + s_first_populate_devices_asked.Set(); + s_received_device_change_event.Reset(); + // Do this forced devices refresh in the messaging thread so it won't cause any race conditions PostMessage(s_message_window, WM_INPUT_DEVICE_CHANGE, 0, 0); - if (!s_done_populating.WaitFor(std::chrono::seconds(10))) - ERROR_LOG_FMT(CONTROLLERINTERFACE, "win32 timed out when trying to populate devices"); + std::thread([] { + if (!s_received_device_change_event.WaitFor(std::chrono::seconds(5))) + ERROR_LOG_FMT(CONTROLLERINTERFACE, "win32 timed out when trying to populate devices"); + }).detach(); } else { @@ -137,6 +152,15 @@ void ciface::Win32::PopulateDevices(void* hwnd) } } +void ciface::Win32::ChangeWindow(void* hwnd) +{ + if (s_thread.joinable()) // "Has init?" + { + s_hwnd = static_cast(hwnd); + ciface::DInput::ChangeWindow(s_hwnd); + } +} + void ciface::Win32::DeInit() { NOTICE_LOG_FMT(CONTROLLERINTERFACE, "win32 DeInit"); @@ -145,7 +169,11 @@ void ciface::Win32::DeInit() PostMessage(s_message_window, WM_DOLPHIN_STOP, 0, 0); s_thread.join(); s_message_window = nullptr; + s_received_device_change_event.Reset(); + s_first_populate_devices_asked.Clear(); + DInput::DeInit(); } + s_hwnd = nullptr; XInput::DeInit(); } diff --git a/Source/Core/InputCommon/ControllerInterface/Win32/Win32.h b/Source/Core/InputCommon/ControllerInterface/Win32/Win32.h index acf72d76a0..8d390022a4 100644 --- a/Source/Core/InputCommon/ControllerInterface/Win32/Win32.h +++ b/Source/Core/InputCommon/ControllerInterface/Win32/Win32.h @@ -8,5 +8,6 @@ namespace ciface::Win32 { void Init(void* hwnd); void PopulateDevices(void* hwnd); +void ChangeWindow(void* hwnd); void DeInit(); } // namespace ciface::Win32 From 038b57feccb098a338342f6542bb97d74f9cf7e1 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:21:22 +0300 Subject: [PATCH 10/14] ControllerInterface: DInput Joystick fix non thread safe static variable also fix devices being added to its own custom list of devices even when rejected by the CI --- Source/Core/Common/Logging/Log.h | 1 - Source/Core/Common/Logging/LogManager.cpp | 1 - .../DInput/DInputJoystick.cpp | 36 ++++++++++++------- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/Source/Core/Common/Logging/Log.h b/Source/Core/Common/Logging/Log.h index 9e45881b1c..282b7ac8c4 100644 --- a/Source/Core/Common/Logging/Log.h +++ b/Source/Core/Common/Logging/Log.h @@ -54,7 +54,6 @@ enum LOG_TYPE OSHLE, OSREPORT, OSREPORT_HLE, - PAD, PIXELENGINE, PROCESSORINTERFACE, POWERPC, diff --git a/Source/Core/Common/Logging/LogManager.cpp b/Source/Core/Common/Logging/LogManager.cpp index 2c58148fb2..9cc984998e 100644 --- a/Source/Core/Common/Logging/LogManager.cpp +++ b/Source/Core/Common/Logging/LogManager.cpp @@ -157,7 +157,6 @@ LogManager::LogManager() m_log[OSHLE] = {"HLE", "OSHLE"}; m_log[OSREPORT] = {"OSREPORT", "OSReport EXI"}; m_log[OSREPORT_HLE] = {"OSREPORT_HLE", "OSReport HLE"}; - m_log[PAD] = {"PAD", "Pad"}; m_log[PIXELENGINE] = {"PE", "Pixel Engine"}; m_log[PROCESSORINTERFACE] = {"PI", "Processor Interface"}; m_log[POWERPC] = {"PowerPC", "PowerPC IBM CPU"}; diff --git a/Source/Core/InputCommon/ControllerInterface/DInput/DInputJoystick.cpp b/Source/Core/InputCommon/ControllerInterface/DInput/DInputJoystick.cpp index 67fd33186b..3d51d9c043 100644 --- a/Source/Core/InputCommon/ControllerInterface/DInput/DInputJoystick.cpp +++ b/Source/Core/InputCommon/ControllerInterface/DInput/DInputJoystick.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -29,6 +30,7 @@ struct GUIDComparator }; static std::set s_guids_in_use; +static std::mutex s_guids_mutex; void InitJoystick(IDirectInput8* const idi8, HWND hwnd) { @@ -46,12 +48,16 @@ void InitJoystick(IDirectInput8* const idi8, HWND hwnd) } // Skip devices we are already using. - if (s_guids_in_use.count(joystick.guidInstance)) { - continue; + std::lock_guard lk(s_guids_mutex); + if (s_guids_in_use.count(joystick.guidInstance)) + { + continue; + } } LPDIRECTINPUTDEVICE8 js_device; + // Don't print any warnings on failure if (SUCCEEDED(idi8->CreateDevice(joystick.guidInstance, &js_device, nullptr))) { if (SUCCEEDED(js_device->SetDataFormat(&c_dfDIJoystick))) @@ -60,37 +66,40 @@ void InitJoystick(IDirectInput8* const idi8, HWND hwnd) DISCL_BACKGROUND | DISCL_EXCLUSIVE))) { WARN_LOG_FMT( - PAD, + CONTROLLERINTERFACE, "DInput: Failed to acquire device exclusively. Force feedback will be unavailable."); // Fall back to non-exclusive mode, with no rumble if (FAILED( js_device->SetCooperativeLevel(nullptr, DISCL_BACKGROUND | DISCL_NONEXCLUSIVE))) { - // PanicAlert("SetCooperativeLevel failed!"); js_device->Release(); continue; } } - s_guids_in_use.insert(joystick.guidInstance); auto js = std::make_shared(js_device); - - // only add if it has some inputs/outputs + // only add if it has some inputs/outputs. + // Don't even add it to our static list in case we first created it without a window handle, + // failing to get exclusive mode, and then later managed to obtain it, which mean it + // could now have some outputs if it didn't before. if (js->Inputs().size() || js->Outputs().size()) - g_controller_interface.AddDevice(std::move(js)); + { + if (g_controller_interface.AddDevice(std::move(js))) + { + std::lock_guard lk(s_guids_mutex); + s_guids_in_use.insert(joystick.guidInstance); + } + } } else { - // PanicAlert("SetDataFormat failed!"); js_device->Release(); } } } } -Joystick::Joystick(/*const LPCDIDEVICEINSTANCE lpddi, */ const LPDIRECTINPUTDEVICE8 device) - : m_device(device) -//, m_name(TStringToString(lpddi->tszInstanceName)) +Joystick::Joystick(const LPDIRECTINPUTDEVICE8 device) : m_device(device) { // seems this needs to be done before GetCapabilities // polled or buffered data @@ -183,11 +192,12 @@ Joystick::~Joystick() info.dwSize = sizeof(info); if (SUCCEEDED(m_device->GetDeviceInfo(&info))) { + std::lock_guard lk(s_guids_mutex); s_guids_in_use.erase(info.guidInstance); } else { - ERROR_LOG_FMT(PAD, "DInputJoystick: GetDeviceInfo failed."); + ERROR_LOG_FMT(CONTROLLERINTERFACE, "DInputJoystick: GetDeviceInfo failed."); } DeInitForceFeedback(); From 8b53af9cbc8aabdd5db349c17a8912c0fec723a8 Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:21:43 +0300 Subject: [PATCH 11/14] ControllerInterface: polish DInput Keyboard and Mouse (add comments and logs) Also fix the cursor axis not being updated when the mouse device had failed aquiring, despite them being completely unrelated --- .../DInput/DInputKeyboardMouse.cpp | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp index e952da6660..aaa723849e 100644 --- a/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp +++ b/Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp @@ -123,8 +123,10 @@ KeyboardMouse::KeyboardMouse(const LPDIRECTINPUTDEVICE8 kb_device, { s_keyboard_mouse_exists = true; - m_kb_device->Acquire(); - m_mo_device->Acquire(); + if (FAILED(m_kb_device->Acquire())) + WARN_LOG_FMT(CONTROLLERINTERFACE, "Keyboard device failed to acquire. We'll retry later"); + if (FAILED(m_mo_device->Acquire())) + WARN_LOG_FMT(CONTROLLERINTERFACE, "Mouse device failed to acquire. We'll retry later"); // KEYBOARD // add keys @@ -191,6 +193,8 @@ void KeyboardMouse::UpdateCursorInput() void KeyboardMouse::UpdateInput() { + UpdateCursorInput(); + DIMOUSESTATE2 tmp_mouse; // if mouse position hasn't been updated in a short while, skip a dev state @@ -207,16 +211,14 @@ void KeyboardMouse::UpdateInput() m_last_update = cur_time; - HRESULT kb_hr = m_kb_device->GetDeviceState(sizeof(m_state_in.keyboard), &m_state_in.keyboard); HRESULT mo_hr = m_mo_device->GetDeviceState(sizeof(tmp_mouse), &tmp_mouse); - - if (DIERR_INPUTLOST == kb_hr || DIERR_NOTACQUIRED == kb_hr) - m_kb_device->Acquire(); - if (DIERR_INPUTLOST == mo_hr || DIERR_NOTACQUIRED == mo_hr) - m_mo_device->Acquire(); - - if (SUCCEEDED(mo_hr)) + { + INFO_LOG_FMT(CONTROLLERINTERFACE, "Mouse device failed to get state"); + if (FAILED(m_mo_device->Acquire())) + INFO_LOG_FMT(CONTROLLERINTERFACE, "Mouse device failed to re-acquire, we'll retry later"); + } + else if (SUCCEEDED(mo_hr)) { m_state_in.relative_mouse.Move({tmp_mouse.lX, tmp_mouse.lY, tmp_mouse.lZ}); m_state_in.relative_mouse.Update(); @@ -227,8 +229,16 @@ void KeyboardMouse::UpdateInput() // copy over the buttons std::copy_n(tmp_mouse.rgbButtons, std::size(tmp_mouse.rgbButtons), m_state_in.mouse.rgbButtons); + } - UpdateCursorInput(); + HRESULT kb_hr = m_kb_device->GetDeviceState(sizeof(m_state_in.keyboard), &m_state_in.keyboard); + if (kb_hr == DIERR_INPUTLOST || kb_hr == DIERR_NOTACQUIRED) + { + INFO_LOG_FMT(CONTROLLERINTERFACE, "Keyboard device failed to get state"); + if (SUCCEEDED(m_kb_device->Acquire())) + m_kb_device->GetDeviceState(sizeof(m_state_in.keyboard), &m_state_in.keyboard); + else + INFO_LOG_FMT(CONTROLLERINTERFACE, "Keyboard device failed to re-acquire, we'll retry later"); } } From 08f8c279274455ecf16a3a447378f37062408c9b Mon Sep 17 00:00:00 2001 From: Filoppi Date: Sat, 15 May 2021 12:28:30 +0300 Subject: [PATCH 12/14] ControllerInterface: fix DSU thread safety and use PlatformPopulateDevices() --- .../DualShockUDPClient/DualShockUDPClient.cpp | 37 +++++++++++++++---- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp b/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp index 73eb0e515a..f8347aa51a 100644 --- a/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp +++ b/Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp @@ -7,7 +7,6 @@ #include #include #include -#include #include #include @@ -19,6 +18,7 @@ #include "Common/Logging/Log.h" #include "Common/MathUtil.h" #include "Common/Random.h" +#include "Common/ScopeGuard.h" #include "Common/StringUtil.h" #include "Common/Thread.h" #include "Core/CoreTiming.h" @@ -192,12 +192,12 @@ struct Server std::string m_description; std::string m_address; u16 m_port; - std::mutex m_port_info_mutex; std::array m_port_info; sf::UdpSocket m_socket; SteadyClock::time_point m_disconnect_time = SteadyClock::now(); }; +static bool s_has_init; static bool s_servers_enabled; static std::vector s_servers; static u32 s_client_uid; @@ -217,6 +217,8 @@ static void HotplugThreadFunc() { Common::SetCurrentThreadName("DualShockUDPClient Hotplug Thread"); INFO_LOG_FMT(CONTROLLERINTERFACE, "DualShockUDPClient hotplug thread started"); + Common::ScopeGuard thread_stop_guard{ + [] { INFO_LOG_FMT(CONTROLLERINTERFACE, "DualShockUDPClient hotplug thread stopped"); }}; std::vector timed_out_servers(s_servers.size(), false); @@ -331,7 +333,6 @@ static void HotplugThreadFunc() } } } - INFO_LOG_FMT(CONTROLLERINTERFACE, "DualShockUDPClient hotplug thread stopped"); } static void StartHotplugThread() @@ -355,13 +356,15 @@ static void StopHotplugThread() return; } + s_hotplug_thread.join(); + for (auto& server : s_servers) { server.m_socket.unbind(); // interrupt blocking socket } - s_hotplug_thread.join(); } +// Also just start static void Restart() { INFO_LOG_FMT(CONTROLLERINTERFACE, "DualShockUDPClient Restart"); @@ -377,7 +380,8 @@ static void Restart() } } - PopulateDevices(); // Only removes devices + // Only removes devices as servers have been cleaned + g_controller_interface.PlatformPopulateDevices([] { PopulateDevices(); }); s_client_uid = Common::Random::GenerateValue(); s_next_listports_time = SteadyClock::now(); @@ -388,6 +392,9 @@ static void Restart() static void ConfigChanged() { + if (!s_has_init) + return; + const bool servers_enabled = Config::Get(Settings::SERVERS_ENABLED); const std::string servers_setting = Config::Get(Settings::SERVERS); @@ -400,6 +407,9 @@ static void ConfigChanged() if (servers_enabled != s_servers_enabled || servers_setting != new_servers_setting) { + // Stop the thread before writing to s_servers + StopHotplugThread(); + s_servers_enabled = servers_enabled; s_servers.clear(); @@ -427,6 +437,9 @@ static void ConfigChanged() void Init() { + // Does not support multiple init calls + s_has_init = true; + // The following is added for backwards compatibility const auto server_address_setting = Config::Get(Settings::SERVER_ADDRESS); const auto server_port_setting = Config::Get(Settings::SERVER_PORT); @@ -447,6 +460,10 @@ void Init() ConfigChanged(); // Call it immediately to load settings } +// This can be called by the host thread as well as the hotplug thread, concurrently. +// So use PlatformPopulateDevices(). +// s_servers is already safe because it can only be modified when the DSU thread is not running, +// from the main thread void PopulateDevices() { INFO_LOG_FMT(CONTROLLERINTERFACE, "DualShockUDPClient PopulateDevices"); @@ -457,9 +474,11 @@ void PopulateDevices() g_controller_interface.RemoveDevice( [](const auto* dev) { return dev->GetSource() == DUALSHOCKUDP_SOURCE_NAME; }); - for (auto& server : s_servers) + // Users might have created more than one server on the same IP/Port. + // Devices might end up being duplicated (if the server responds two all requests) + // but they won't conflict. + for (const auto& server : s_servers) { - std::lock_guard lock{server.m_port_info_mutex}; for (size_t port_index = 0; port_index < server.m_port_info.size(); port_index++) { const Proto::MessageType::PortInfo& port_info = server.m_port_info[port_index]; @@ -475,6 +494,10 @@ void PopulateDevices() void DeInit() { StopHotplugThread(); + + s_has_init = false; + s_servers_enabled = false; + s_servers.clear(); } Device::Device(std::string name, int index, std::string server_address, u16 server_port) From a77e3b4a9bc557ac00b63c13abdb4074069e9a7d Mon Sep 17 00:00:00 2001 From: Filoppi Date: Mon, 10 May 2021 22:44:17 +0300 Subject: [PATCH 13/14] InputCommon: Make Wiimote rumble variable thread safe --- .../ControllerInterface/Wiimote/WiimoteController.cpp | 7 ++++--- .../ControllerInterface/Wiimote/WiimoteController.h | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp b/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp index d68b0441a6..82e2cfedda 100644 --- a/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp +++ b/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp @@ -82,14 +82,14 @@ using UndetectableSignedAnalogInput = SignedInput; class Motor final : public Core::Device::Output { public: - Motor(ControlState* value) : m_value(*value) {} + Motor(std::atomic* value) : m_value(*value) {} std::string GetName() const override { return "Motor"; } void SetState(ControlState state) override { m_value = state; } private: - ControlState& m_value; + std::atomic& m_value; }; template @@ -1377,7 +1377,8 @@ void Device::UpdateRumble() { static constexpr auto rumble_period = std::chrono::milliseconds(100); - const auto on_time = std::chrono::duration_cast(rumble_period * m_rumble_level); + const auto on_time = + std::chrono::duration_cast(rumble_period * m_rumble_level.load()); const auto off_time = rumble_period - on_time; const auto now = Clock::now(); diff --git a/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.h b/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.h index b6c96d0238..4d93584a67 100644 --- a/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.h +++ b/Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.h @@ -5,6 +5,7 @@ #pragma once #include +#include #include #include #include @@ -262,7 +263,7 @@ private: bool m_rumble = false; // For pulse of rumble motor to simulate multiple levels. - ControlState m_rumble_level = 0; + std::atomic m_rumble_level; Clock::time_point m_last_rumble_change = Clock::now(); // Assume mode is disabled so one gets set. From 83ea16f40238fa82981221ba65061a7094b2a64b Mon Sep 17 00:00:00 2001 From: Filoppi Date: Fri, 21 May 2021 01:33:38 +0300 Subject: [PATCH 14/14] Qt: Fix IOWindow keeping a shared ptr to devices even after them being removed by the ControllerInterface this prevented some devices from being recreated correctly, as they were exclusive (e.g. DInput Joysticks) This is achieved by calling Settings::ReleaseDevices(), which releases all the UI devices shared ptrs. If we are the host (Qt) thread, DevicesChanged() is now called in line, to avoid devices being hanged onto by the UI. For this, I had to add a method to check whether we are the Host Thread to Qt. Avoid calling ControllerInterface::RefreshDevices() from the CPU thread if the emulation is running and we manually refresh devices from Qt, as that is not necessary anymore. Refactored the way IOWindow lists devices to make it clearer and hold onto disconnected devices. There were so many issues with the previous code: -Devices changes would not be reflected until the window was re-opened -If there was no default device, it would fail to select the device at index 0 -It could have crashed if we had 0 devices -The default device was not highlighted as such --- .../DolphinQt/Config/Mapping/IOWindow.cpp | 94 ++++++++++++++++--- .../Core/DolphinQt/Config/Mapping/IOWindow.h | 9 +- .../Config/Mapping/MappingWindow.cpp | 2 +- Source/Core/DolphinQt/Host.cpp | 15 ++- Source/Core/DolphinQt/Host.h | 3 + Source/Core/DolphinQt/Main.cpp | 2 + Source/Core/DolphinQt/MainWindow.cpp | 8 +- Source/Core/DolphinQt/MainWindow.h | 2 +- Source/Core/DolphinQt/Settings.cpp | 21 ++++- Source/Core/DolphinQt/Settings.h | 1 + 10 files changed, 130 insertions(+), 27 deletions(-) diff --git a/Source/Core/DolphinQt/Config/Mapping/IOWindow.cpp b/Source/Core/DolphinQt/Config/Mapping/IOWindow.cpp index 27c17ce5fb..6d5c149024 100644 --- a/Source/Core/DolphinQt/Config/Mapping/IOWindow.cpp +++ b/Source/Core/DolphinQt/Config/Mapping/IOWindow.cpp @@ -9,8 +9,6 @@ #include #include -#include -#include #include #include #include @@ -31,6 +29,7 @@ #include "DolphinQt/Config/Mapping/MappingWindow.h" #include "DolphinQt/QtUtils/BlockUserInputFilter.h" #include "DolphinQt/QtUtils/ModalMessageBox.h" +#include "DolphinQt/Settings.h" #include "InputCommon/ControlReference/ControlReference.h" #include "InputCommon/ControlReference/ExpressionParser.h" @@ -220,6 +219,8 @@ IOWindow::IOWindow(MappingWidget* parent, ControllerEmu::EmulatedController* con CreateMainLayout(); connect(parent, &MappingWidget::Update, this, &IOWindow::Update); + connect(parent->GetParent(), &MappingWindow::ConfigChanged, this, &IOWindow::ConfigChanged); + connect(&Settings::Instance(), &Settings::ConfigChanged, this, &IOWindow::ConfigChanged); setWindowTitle(type == IOWindow::Type::Input ? tr("Configure Input") : tr("Configure Output")); setWindowFlags(windowFlags() & ~Qt::WindowContextHelpButtonHint); @@ -229,7 +230,7 @@ IOWindow::IOWindow(MappingWidget* parent, ControllerEmu::EmulatedController* con ConnectWidgets(); } -std::shared_ptr IOWindow::GetSelectedDevice() +std::shared_ptr IOWindow::GetSelectedDevice() const { return m_selected_device; } @@ -258,7 +259,7 @@ void IOWindow::CreateMainLayout() m_expression_text->setFont(QFontDatabase::systemFont(QFontDatabase::FixedFont)); new ControlExpressionSyntaxHighlighter(m_expression_text->document()); - m_operators_combo = new QComboBoxWithMouseWheelDisabled(); + m_operators_combo = new QComboBoxWithMouseWheelDisabled(this); m_operators_combo->addItem(tr("Operators")); m_operators_combo->insertSeparator(1); if (m_type == Type::Input) @@ -340,6 +341,7 @@ void IOWindow::CreateMainLayout() m_option_list->horizontalHeader()->setSectionResizeMode(1, QHeaderView::Fixed); m_option_list->setItemDelegate(new InputStateDelegate(this, 1, [&](int row) { + std::lock_guard lock(m_selected_device_mutex); // Clamp off negative values but allow greater than one in the text display. return std::max(GetSelectedDevice()->Inputs()[row]->GetState(), 0.0); })); @@ -400,7 +402,7 @@ void IOWindow::CreateMainLayout() void IOWindow::ConfigChanged() { const QSignalBlocker blocker(this); - const auto lock = m_controller->GetStateLock(); + const auto lock = ControllerEmu::EmulatedController::GetStateLock(); // ensure m_parse_text is in the right state UpdateExpression(m_reference->GetExpression(), UpdateMode::Force); @@ -410,10 +412,10 @@ void IOWindow::ConfigChanged() m_range_spinbox->setValue(m_reference->range * SLIDER_TICK_COUNT); m_range_slider->setValue(m_reference->range * SLIDER_TICK_COUNT); - m_devq = m_controller->GetDefaultDevice(); + if (m_devq.ToString().empty()) + m_devq = m_controller->GetDefaultDevice(); UpdateDeviceList(); - UpdateOptionList(); } void IOWindow::Update() @@ -425,6 +427,8 @@ void IOWindow::Update() void IOWindow::ConnectWidgets() { connect(m_select_button, &QPushButton::clicked, [this] { AppendSelectedOption(); }); + connect(&Settings::Instance(), &Settings::ReleaseDevices, this, &IOWindow::ReleaseDevices); + connect(&Settings::Instance(), &Settings::DevicesChanged, this, &IOWindow::UpdateDeviceList); connect(m_detect_button, &QPushButton::clicked, this, &IOWindow::OnDetectButtonPressed); connect(m_test_button, &QPushButton::clicked, this, &IOWindow::OnTestButtonPressed); @@ -479,16 +483,19 @@ void IOWindow::ConnectWidgets() void IOWindow::AppendSelectedOption() { - if (m_option_list->currentItem() == nullptr) + if (m_option_list->currentRow() < 0) return; m_expression_text->insertPlainText(MappingCommon::GetExpressionForControl( - m_option_list->currentItem()->text(), m_devq, m_controller->GetDefaultDevice())); + m_option_list->item(m_option_list->currentRow(), 0)->text(), m_devq, + m_controller->GetDefaultDevice())); } -void IOWindow::OnDeviceChanged(const QString& device) +void IOWindow::OnDeviceChanged() { - m_devq.FromString(device.toStdString()); + const std::string device_name = + m_devices_combo->count() > 0 ? m_devices_combo->currentData().toString().toStdString() : ""; + m_devq.FromString(device_name); UpdateOptionList(); } @@ -500,7 +507,7 @@ void IOWindow::OnDialogButtonPressed(QAbstractButton* button) return; } - const auto lock = m_controller->GetStateLock(); + const auto lock = ControllerEmu::EmulatedController::GetStateLock(); UpdateExpression(m_expression_text->toPlainText().toStdString()); m_original_expression = m_reference->GetExpression(); @@ -525,6 +532,7 @@ void IOWindow::OnDetectButtonPressed() const auto list = m_option_list->findItems(expression, Qt::MatchFixedString); + // Try to select the first. If this fails, the last selected item would still appear as such if (!list.empty()) m_option_list->setCurrentItem(list[0]); } @@ -541,8 +549,15 @@ void IOWindow::OnRangeChanged(int value) m_range_slider->setValue(m_reference->range * SLIDER_TICK_COUNT); } +void IOWindow::ReleaseDevices() +{ + std::lock_guard lock(m_selected_device_mutex); + m_selected_device = nullptr; +} + void IOWindow::UpdateOptionList() { + std::lock_guard lock(m_selected_device_mutex); m_selected_device = g_controller_interface.FindDevice(m_devq); m_option_list->setRowCount(0); @@ -575,13 +590,62 @@ void IOWindow::UpdateOptionList() void IOWindow::UpdateDeviceList() { + const QSignalBlocker blocker(m_devices_combo); + + const auto previous_device_name = m_devices_combo->currentData().toString().toStdString(); + m_devices_combo->clear(); + // Default to the default device or to the first device if there isn't a default. + // Try to the keep the previous selected device, mark it as disconnected if it's gone, as it could + // reconnect soon after if this is a devices refresh and it would be annoying to lose the value. + const auto default_device_name = m_controller->GetDefaultDevice().ToString(); + int default_device_index = -1; + int previous_device_index = -1; for (const auto& name : g_controller_interface.GetAllDeviceStrings()) - m_devices_combo->addItem(QString::fromStdString(name)); + { + QString qname = QString(); + if (name == default_device_name) + { + default_device_index = m_devices_combo->count(); + // Specify "default" even if we only have one device + qname.append(QLatin1Char{'['} + tr("default") + QStringLiteral("] ")); + } + if (name == previous_device_name) + { + previous_device_index = m_devices_combo->count(); + } + qname.append(QString::fromStdString(name)); + m_devices_combo->addItem(qname, QString::fromStdString(name)); + } - m_devices_combo->setCurrentText( - QString::fromStdString(m_controller->GetDefaultDevice().ToString())); + if (previous_device_index >= 0) + { + m_devices_combo->setCurrentIndex(previous_device_index); + } + else if (!previous_device_name.empty()) + { + const QString qname = QString::fromStdString(previous_device_name); + QString adjusted_qname; + if (previous_device_name == default_device_name) + { + adjusted_qname.append(QLatin1Char{'['} + tr("default") + QStringLiteral("] ")); + } + adjusted_qname.append(QLatin1Char{'['} + tr("disconnected") + QStringLiteral("] ")) + .append(qname); + m_devices_combo->addItem(adjusted_qname, qname); + m_devices_combo->setCurrentIndex(m_devices_combo->count() - 1); + } + else if (default_device_index >= 0) + { + m_devices_combo->setCurrentIndex(default_device_index); + } + else if (m_devices_combo->count() > 0) + { + m_devices_combo->setCurrentIndex(0); + } + // The device object might have changed so we need to always refresh it + OnDeviceChanged(); } void IOWindow::UpdateExpression(std::string new_expression, UpdateMode mode) diff --git a/Source/Core/DolphinQt/Config/Mapping/IOWindow.h b/Source/Core/DolphinQt/Config/Mapping/IOWindow.h index 320ebefde4..7d22ddbc0d 100644 --- a/Source/Core/DolphinQt/Config/Mapping/IOWindow.h +++ b/Source/Core/DolphinQt/Config/Mapping/IOWindow.h @@ -5,6 +5,7 @@ #pragma once #include +#include #include #include @@ -69,16 +70,16 @@ public: explicit IOWindow(MappingWidget* parent, ControllerEmu::EmulatedController* m_controller, ControlReference* ref, Type type); - std::shared_ptr GetSelectedDevice(); - private: + std::shared_ptr GetSelectedDevice() const; + void CreateMainLayout(); void ConnectWidgets(); void ConfigChanged(); void Update(); void OnDialogButtonPressed(QAbstractButton* button); - void OnDeviceChanged(const QString& device); + void OnDeviceChanged(); void OnDetectButtonPressed(); void OnTestButtonPressed(); void OnRangeChanged(int range); @@ -86,6 +87,7 @@ private: void AppendSelectedOption(); void UpdateOptionList(); void UpdateDeviceList(); + void ReleaseDevices(); enum class UpdateMode { @@ -135,4 +137,5 @@ private: ciface::Core::DeviceQualifier m_devq; Type m_type; std::shared_ptr m_selected_device; + std::mutex m_selected_device_mutex; }; diff --git a/Source/Core/DolphinQt/Config/Mapping/MappingWindow.cpp b/Source/Core/DolphinQt/Config/Mapping/MappingWindow.cpp index ac6230cec4..f3aed4eea0 100644 --- a/Source/Core/DolphinQt/Config/Mapping/MappingWindow.cpp +++ b/Source/Core/DolphinQt/Config/Mapping/MappingWindow.cpp @@ -328,7 +328,7 @@ bool MappingWindow::IsMappingAllDevices() const void MappingWindow::RefreshDevices() { - Core::RunAsCPUThread([&] { g_controller_interface.RefreshDevices(); }); + g_controller_interface.RefreshDevices(); } void MappingWindow::OnGlobalDevicesChanged() diff --git a/Source/Core/DolphinQt/Host.cpp b/Source/Core/DolphinQt/Host.cpp index b0cfb507bd..f85b0b7262 100644 --- a/Source/Core/DolphinQt/Host.cpp +++ b/Source/Core/DolphinQt/Host.cpp @@ -35,6 +35,8 @@ #include "VideoCommon/RenderBase.h" #include "VideoCommon/VideoConfig.h" +static thread_local bool tls_is_host_thread = false; + Host::Host() { State::SetOnAfterLoadCallback([] { Host_UpdateDisasmDialog(); }); @@ -51,6 +53,16 @@ Host* Host::GetInstance() return s_instance; } +void Host::DeclareAsHostThread() +{ + tls_is_host_thread = true; +} + +bool Host::IsHostThread() +{ + return tls_is_host_thread; +} + void Host::SetRenderHandle(void* handle) { m_render_to_main = Config::Get(Config::MAIN_RENDER_TO_MAIN); @@ -62,8 +74,7 @@ void Host::SetRenderHandle(void* handle) if (g_renderer) { g_renderer->ChangeSurface(handle); - if (g_controller_interface.IsInit()) - g_controller_interface.ChangeWindow(handle); + g_controller_interface.ChangeWindow(handle); } } diff --git a/Source/Core/DolphinQt/Host.h b/Source/Core/DolphinQt/Host.h index 6be28d5464..90c8b7042d 100644 --- a/Source/Core/DolphinQt/Host.h +++ b/Source/Core/DolphinQt/Host.h @@ -22,6 +22,9 @@ public: static Host* GetInstance(); + void DeclareAsHostThread(); + bool IsHostThread(); + bool GetRenderFocus(); bool GetRenderFullFocus(); bool GetRenderFullscreen(); diff --git a/Source/Core/DolphinQt/Main.cpp b/Source/Core/DolphinQt/Main.cpp index 9efad48c35..1399a40391 100644 --- a/Source/Core/DolphinQt/Main.cpp +++ b/Source/Core/DolphinQt/Main.cpp @@ -120,6 +120,8 @@ int WINAPI wWinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, PWSTR pCmdLine } #endif + Host::GetInstance()->DeclareAsHostThread(); + #ifdef __APPLE__ // On macOS, a command line option matching the format "-psn_X_XXXXXX" is passed when // the application is launched for the first time. This is to set the "ProcessSerialNumber", diff --git a/Source/Core/DolphinQt/MainWindow.cpp b/Source/Core/DolphinQt/MainWindow.cpp index 3b01fec11b..265155f2b9 100644 --- a/Source/Core/DolphinQt/MainWindow.cpp +++ b/Source/Core/DolphinQt/MainWindow.cpp @@ -789,7 +789,7 @@ void MainWindow::TogglePause() void MainWindow::OnStopComplete() { m_stop_requested = false; - HideRenderWidget(); + HideRenderWidget(true, m_exit_requested); #ifdef USE_DISCORD_PRESENCE if (!m_netplay_dialog->isVisible()) Discord::UpdateDiscordPresence(); @@ -1099,7 +1099,7 @@ void MainWindow::ShowRenderWidget() } } -void MainWindow::HideRenderWidget(bool reinit) +void MainWindow::HideRenderWidget(bool reinit, bool is_exit) { if (m_rendering_to_main) { @@ -1136,7 +1136,9 @@ void MainWindow::HideRenderWidget(bool reinit) // The controller interface will still be registered to the old render widget, if the core // has booted. Therefore, we should re-bind it to the main window for now. When the core // is next started, it will be swapped back to the new render widget. - g_controller_interface.ChangeWindow(GetWindowSystemInfo(windowHandle()).render_window); + g_controller_interface.ChangeWindow(GetWindowSystemInfo(windowHandle()).render_window, + is_exit ? ControllerInterface::WindowChangeReason::Exit : + ControllerInterface::WindowChangeReason::Other); } } diff --git a/Source/Core/DolphinQt/MainWindow.h b/Source/Core/DolphinQt/MainWindow.h index 79383b3541..7d9e97de28 100644 --- a/Source/Core/DolphinQt/MainWindow.h +++ b/Source/Core/DolphinQt/MainWindow.h @@ -142,7 +142,7 @@ private: const std::optional& savestate_path = {}); void StartGame(std::unique_ptr&& parameters); void ShowRenderWidget(); - void HideRenderWidget(bool reinit = true); + void HideRenderWidget(bool reinit = true, bool is_exit = false); void ShowSettingsWindow(); void ShowGeneralWindow(); diff --git a/Source/Core/DolphinQt/Settings.cpp b/Source/Core/DolphinQt/Settings.cpp index 7541a9087f..3f9fb663f0 100644 --- a/Source/Core/DolphinQt/Settings.cpp +++ b/Source/Core/DolphinQt/Settings.cpp @@ -25,6 +25,7 @@ #include "Core/NetPlayClient.h" #include "Core/NetPlayServer.h" +#include "DolphinQt/Host.h" #include "DolphinQt/QtUtils/QueueOnObject.h" #include "InputCommon/ControllerInterface/ControllerInterface.h" @@ -44,8 +45,24 @@ Settings::Settings() Config::AddConfigChangedCallback( [this] { QueueOnObject(this, [this] { emit ConfigChanged(); }); }); - g_controller_interface.RegisterDevicesChangedCallback( - [this] { QueueOnObject(this, [this] { emit DevicesChanged(); }); }); + g_controller_interface.RegisterDevicesChangedCallback([this] { + if (Host::GetInstance()->IsHostThread()) + { + emit DevicesChanged(); + } + else + { + // Any device shared_ptr in the host thread needs to be released immediately as otherwise + // they'd continue living until the queued event has run, but some devices can't be recreated + // until they are destroyed. + // This is safe from any thread. Devices will be refreshed and re-acquired and in + // DevicesChanged(). Waiting on QueueOnObject() to have finished running was not feasible as + // it would cause deadlocks without heavy workarounds. + emit ReleaseDevices(); + + QueueOnObject(this, [this] { emit DevicesChanged(); }); + } + }); } Settings::~Settings() = default; diff --git a/Source/Core/DolphinQt/Settings.h b/Source/Core/DolphinQt/Settings.h index d5fd90989c..e2c5dbde9e 100644 --- a/Source/Core/DolphinQt/Settings.h +++ b/Source/Core/DolphinQt/Settings.h @@ -192,6 +192,7 @@ signals: void AutoUpdateTrackChanged(const QString& mode); void FallbackRegionChanged(const DiscIO::Region& region); void AnalyticsToggled(bool enabled); + void ReleaseDevices(); void DevicesChanged(); void SDCardInsertionChanged(bool inserted); void USBKeyboardConnectionChanged(bool connected);