Merge pull request #9702 from Filoppi/controller_interface_fixes

Controller Interface refactor

Source/Core/Common/Logging/Log.h

@@ -54,7 +54,6 @@ enum LOG_TYPE
   OSHLE,
   OSREPORT,
   OSREPORT_HLE,
-  PAD,
   PIXELENGINE,
   PROCESSORINTERFACE,
   POWERPC,

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"};

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>(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

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<WiimoteScanMode> m_scan_mode{WiimoteScanMode::DO_NOT_SCAN};
 };
 
@@ -204,6 +206,7 @@ void InitAdapterClass();
 #endif
 
 void HandleWiimotesInControllerInterfaceSettingChange();
+void PopulateDevices();
 void ProcessWiimotePool();
 
 }  // namespace WiimoteReal

Source/Core/DolphinQt/Config/Mapping/IOWindow.cpp

@@ -9,8 +9,6 @@
 
 #include <QComboBox>
 #include <QDialogButtonBox>
-#include <QGroupBox>
-#include <QHBoxLayout>
 #include <QHeaderView>
 #include <QItemDelegate>
 #include <QLabel>
@@ -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<ciface::Core::Device> IOWindow::GetSelectedDevice()
+std::shared_ptr<ciface::Core::Device> 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)

Source/Core/DolphinQt/Config/Mapping/IOWindow.h

@@ -5,6 +5,7 @@
 #pragma once
 
 #include <memory>
+#include <mutex>
 #include <string>
 
 #include <QComboBox>
@@ -69,16 +70,16 @@ public:
   explicit IOWindow(MappingWidget* parent, ControllerEmu::EmulatedController* m_controller,
                     ControlReference* ref, Type type);
 
-  std::shared_ptr<ciface::Core::Device> GetSelectedDevice();
-
 private:
+  std::shared_ptr<ciface::Core::Device> 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<ciface::Core::Device> m_selected_device;
+  std::mutex m_selected_device_mutex;
 };

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()

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);
   }
 }
 

Source/Core/DolphinQt/Host.h

@@ -22,6 +22,9 @@ public:
 
   static Host* GetInstance();
 
+  void DeclareAsHostThread();
+  bool IsHostThread();
+
   bool GetRenderFocus();
   bool GetRenderFullFocus();
   bool GetRenderFullscreen();

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",

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);
   }
 }
 

Source/Core/DolphinQt/MainWindow.h

@@ -142,7 +142,7 @@ private:
                  const std::optional<std::string>& savestate_path = {});
   void StartGame(std::unique_ptr<BootParameters>&& parameters);
   void ShowRenderWidget();
-  void HideRenderWidget(bool reinit = true);
+  void HideRenderWidget(bool reinit = true, bool is_exit = false);
 
   void ShowSettingsWindow();
   void ShowGeneralWindow();

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;

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);

Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "Common/Assert.h"
 #include "Common/Logging/Log.h"
 #include "Core/HW/WiimoteReal/WiimoteReal.h"
 
@@ -37,6 +38,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)
@@ -44,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);
@@ -58,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();
@@ -78,33 +82,95 @@ 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;
 
+#ifdef CIFACE_USE_OSX
+  if (m_wsi.type == WindowSystemType::MacOS)
   {
-    std::lock_guard lk(m_devices_mutex);
-    m_devices.clear();
+    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
 
-  m_is_populating_devices = true;
+  // 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);
+
+#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.
+  // 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<Device> 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);
@@ -116,7 +182,10 @@ void ControllerInterface::RefreshDevices()
 #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
@@ -136,10 +205,12 @@ void ControllerInterface::RefreshDevices()
   ciface::DualShockUDPClient::PopulateDevices();
 #endif
 
-  WiimoteReal::ProcessWiimotePool();
+  WiimoteReal::PopulateDevices();
 
-  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<void()> callback)
@@ -147,12 +218,18 @@ void ControllerInterface::PlatformPopulateDevices(std::function<void()> 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
@@ -163,23 +240,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_ptr<Device>s are freed up
-  // BEFORE we shutdown the backends.
-  InvokeDevicesChangedCallbacks();
+  // Update control references so shared_ptr<Device>s are freed up BEFORE we shutdown the backends.
+  ClearDevices();
 
 #ifdef CIFACE_USE_WIN32
   ciface::Win32::DeInit();
@@ -203,13 +268,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<ciface::Core::Device> 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<ciface::Core::Device> 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);
@@ -239,14 +339,36 @@ void ControllerInterface::AddDevice(std::shared_ptr<ciface::Core::Device> 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<ciface::Core::Device>& a,
+                        const std::shared_ptr<ciface::Core::Device>& 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_is_populating_devices)
+  if (!m_populating_devices_counter)
     InvokeDevicesChangedCallbacks();
+  return true;
 }
 
-void ControllerInterface::RemoveDevice(std::function<bool(const ciface::Core::Device*)> callback)
+void ControllerInterface::RemoveDevice(std::function<bool(const ciface::Core::Device*)> 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) {
@@ -257,22 +379,34 @@ void ControllerInterface::RemoveDevice(std::function<bool(const ciface::Core::De
       }
       return false;
     });
+    const size_t prev_size = m_devices.size();
     m_devices.erase(it, m_devices.end());
+    any_removed = m_devices.size() != prev_size;
   }
 
-  if (!m_is_populating_devices)
+  if (any_removed && (!m_populating_devices_counter || force_devices_release))
     InvokeDevicesChangedCallbacks();
 }
 
 // Update input for all devices if lock can be acquired without waiting.
 void ControllerInterface::UpdateInput()
 {
-  // Don't block the UI or CPU thread (to avoid a short but noticeable frame drop)
+  // This should never happen
+  ASSERT(m_is_init);
+  if (!m_is_init)
+    return;
+
+  // TODO: if we are an emulation input channel, we should probably always lock
+  // Prefer outdated values over blocking UI or CPU thread (avoids short but noticeable frame drop)
   if (m_devices_mutex.try_lock())
   {
     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();
+    }
   }
 }
 
@@ -307,7 +441,7 @@ Common::Vec2 ControllerInterface::GetWindowInputScale() const
 ControllerInterface::HotplugCallbackHandle
 ControllerInterface::RegisterDevicesChangedCallback(std::function<void()> callback)
 {
-  std::lock_guard<std::mutex> 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());
 }
@@ -315,14 +449,16 @@ ControllerInterface::RegisterDevicesChangedCallback(std::function<void()> callba
 // Unregister a device callback.
 void ControllerInterface::UnregisterDevicesChangedCallback(const HotplugCallbackHandle& handle)
 {
-  std::lock_guard<std::mutex> lk(m_callbacks_mutex);
+  std::lock_guard lk(m_callbacks_mutex);
   m_devices_changed_callbacks.erase(handle);
 }
 
 // Invoke all callbacks that were registered
 void ControllerInterface::InvokeDevicesChangedCallbacks() const
 {
-  std::lock_guard<std::mutex> 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();
 }

Source/Core/InputCommon/ControllerInterface/ControllerInterface.h

@@ -60,13 +60,38 @@ class ControllerInterface : public ciface::Core::DeviceContainer
 public:
   using HotplugCallbackHandle = std::list<std::function<void()>>::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<ciface::Core::Device> device);
-  void RemoveDevice(std::function<bool(const ciface::Core::Device*)> callback);
+  bool AddDevice(std::shared_ptr<ciface::Core::Device> 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<bool(const ciface::Core::Device*)> 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.
   void PlatformPopulateDevices(std::function<void()> callback);
   bool IsInit() const { return m_is_init; }
   void UpdateInput();
@@ -87,10 +112,17 @@ public:
   static ciface::InputChannel GetCurrentInputChannel();
 
 private:
+  void ClearDevices();
+
   std::list<std::function<void()>> 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<bool> m_is_init;
-  std::atomic<bool> 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<int> m_populating_devices_counter;
   WindowSystemInfo m_wsi;
   std::atomic<float> m_aspect_ratio_adjustment = 1;
 };

Source/Core/InputCommon/ControllerInterface/CoreDevice.h

@@ -125,9 +125,20 @@ 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<int> 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<Input*>& Inputs() const { return m_inputs; }
   const std::vector<Output*>& Outputs() const { return m_outputs; }
 
@@ -227,6 +238,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<std::shared_ptr<Device>> m_devices;
 };

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<DIDEVICEOBJECTINSTANCE>*)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

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

Source/Core/InputCommon/ControllerInterface/DInput/DInputJoystick.cpp

@@ -4,6 +4,7 @@
 
 #include <algorithm>
 #include <limits>
+#include <mutex>
 #include <set>
 #include <sstream>
 #include <type_traits>
@@ -29,6 +30,7 @@ struct GUIDComparator
 };
 
 static std::set<GUID, GUIDComparator> 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<Joystick>(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();