Merge pull request #7742 from jordan-woyak/si-device-change-fix

SI: Device change logic fix.

Source/Core/Core/HW/SI/SI.cpp

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <array>
+#include <atomic>
 #include <cstring>
 #include <memory>
 
@@ -102,6 +103,8 @@ struct SSIChannel
   USIChannelIn_Hi in_hi;
   USIChannelIn_Lo in_lo;
   std::unique_ptr<ISIDevice> device;
+
+  bool has_recent_device_change;
 };
 
 // SI Poll: Controls how often a device is polled
@@ -205,6 +208,9 @@ union USIEXIClockCount
 static CoreTiming::EventType* s_change_device_event;
 static CoreTiming::EventType* s_tranfer_pending_event;
 
+// User-configured device type. possibly overridden by TAS/Netplay
+static std::array<std::atomic<SIDevices>, MAX_SI_CHANNELS> s_desired_device_types;
+
 // STATE_TO_SAVE
 static std::array<SSIChannel, MAX_SI_CHANNELS> s_channel;
 static USIPoll s_poll;
@@ -236,20 +242,8 @@ static void SetNoResponse(u32 channel)
 
 static void ChangeDeviceCallback(u64 user_data, s64 cycles_late)
 {
-  u8 channel = (u8)(user_data >> 32);
-  SIDevices device = (SIDevices)(u32)user_data;
-
-  // Skip redundant (spammed) device changes
-  if (GetDeviceType(channel) != device)
-  {
-    s_channel[channel].out.hex = 0;
-    s_channel[channel].in_hi.hex = 0;
-    s_channel[channel].in_lo.hex = 0;
-
-    SetNoResponse(channel);
-
-    AddDevice(device, channel);
-  }
+  // The purpose of this callback is to simply re-enable device changes.
+  s_channel[user_data].has_recent_device_change = false;
 }
 
 static void UpdateInterrupts()
@@ -329,26 +323,18 @@ void DoState(PointerWrap& p)
     p.Do(s_channel[i].in_hi.hex);
     p.Do(s_channel[i].in_lo.hex);
     p.Do(s_channel[i].out.hex);
+    p.Do(s_channel[i].has_recent_device_change);
 
     std::unique_ptr<ISIDevice>& device = s_channel[i].device;
     SIDevices type = device->GetDeviceType();
     p.Do(type);
 
-    if (type == device->GetDeviceType())
+    if (type != device->GetDeviceType())
     {
-      device->DoState(p);
-    }
-    else
-    {
-      // If no movie is active, we'll assume the user wants to keep their current devices
-      // instead of the ones they had when the savestate was created.
-      // But we need to restore the current devices first just in case.
-      SIDevices original_device = device->GetDeviceType();
-      std::unique_ptr<ISIDevice> save_device = SIDevice_Create(type, i);
-      save_device->DoState(p);
-      AddDevice(std::move(save_device));
-      ChangeDeviceDeterministic(original_device, i);
+      AddDevice(SIDevice_Create(type, i));
     }
+
+    device->DoState(p);
   }
 
   p.Do(s_poll);
@@ -365,27 +351,30 @@ void Init()
     s_channel[i].out.hex = 0;
     s_channel[i].in_hi.hex = 0;
     s_channel[i].in_lo.hex = 0;
+    s_channel[i].has_recent_device_change = false;
 
     if (Movie::IsMovieActive())
     {
+      s_desired_device_types[i] = SIDEVICE_NONE;
+
       if (Movie::IsUsingPad(i))
       {
         SIDevices current = SConfig::GetInstance().m_SIDevice[i];
         // GC pad-compatible devices can be used for both playing and recording
-        if (SIDevice_IsGCController(current))
-          AddDevice(Movie::IsUsingBongo(i) ? SIDEVICE_GC_TARUKONGA : current, i);
+        if (Movie::IsUsingBongo(i))
+          s_desired_device_types[i] = SIDEVICE_GC_TARUKONGA;
+        else if (SIDevice_IsGCController(current))
+          s_desired_device_types[i] = current;
         else
-          AddDevice(Movie::IsUsingBongo(i) ? SIDEVICE_GC_TARUKONGA : SIDEVICE_GC_CONTROLLER, i);
-      }
-      else
-      {
-        AddDevice(SIDEVICE_NONE, i);
+          s_desired_device_types[i] = SIDEVICE_GC_CONTROLLER;
       }
     }
     else if (!NetPlay::IsNetPlayRunning())
     {
-      AddDevice(SConfig::GetInstance().m_SIDevice[i], i);
+      s_desired_device_types[i] = SConfig::GetInstance().m_SIDevice[i];
     }
+
+    AddDevice(s_desired_device_types[i], i);
   }
 
   s_poll.hex = 0;
@@ -594,31 +583,48 @@ void AddDevice(const SIDevices device, int device_number)
 
 void ChangeDevice(SIDevices device, int channel)
 {
-  // Called from GUI, so we need to use FromThread::NON_CPU.
-  // Let the hardware see no device for 1 second
-  // TODO: Calling GetDeviceType here isn't threadsafe.
-  if (GetDeviceType(channel) != device)
-  {
-    CoreTiming::ScheduleEvent(0, s_change_device_event, ((u64)channel << 32) | SIDEVICE_NONE,
-                              CoreTiming::FromThread::NON_CPU);
-    CoreTiming::ScheduleEvent(SystemTimers::GetTicksPerSecond(), s_change_device_event,
-                              ((u64)channel << 32) | device, CoreTiming::FromThread::NON_CPU);
-  }
+  // Actual device change will happen in UpdateDevices.
+  s_desired_device_types[channel] = device;
 }
 
-void ChangeDeviceDeterministic(SIDevices device, int channel)
+static void ChangeDeviceDeterministic(SIDevices device, int channel)
 {
-  // Called from savestates, so we don't use FromThread::NON_CPU.
-  if (GetDeviceType(channel) != device)
+  if (s_channel[channel].has_recent_device_change)
+    return;
+
+  if (GetDeviceType(channel) != SIDEVICE_NONE)
   {
-    CoreTiming::ScheduleEvent(0, s_change_device_event, ((u64)channel << 32) | SIDEVICE_NONE);
-    CoreTiming::ScheduleEvent(SystemTimers::GetTicksPerSecond(), s_change_device_event,
-                              ((u64)channel << 32) | device);
+    // Detach the current device before switching to the new one.
+    device = SIDEVICE_NONE;
   }
+
+  s_channel[channel].out.hex = 0;
+  s_channel[channel].in_hi.hex = 0;
+  s_channel[channel].in_lo.hex = 0;
+
+  SetNoResponse(channel);
+
+  AddDevice(device, channel);
+
+  // Prevent additional device changes on this channel for one second.
+  s_channel[channel].has_recent_device_change = true;
+  CoreTiming::ScheduleEvent(SystemTimers::GetTicksPerSecond(), s_change_device_event, channel);
 }
 
 void UpdateDevices()
 {
+  // Check for device change requests:
+  for (int i = 0; i != MAX_SI_CHANNELS; ++i)
+  {
+    const SIDevices current_type = GetDeviceType(i);
+    const SIDevices desired_type = s_desired_device_types[i];
+
+    if (current_type != desired_type)
+    {
+      ChangeDeviceDeterministic(desired_type, i);
+    }
+  }
+
   // Hinting NetPlay that all controllers will be polled in
   // succession, in order to optimize networking
   NetPlay::SetSIPollBatching(true);

Source/Core/Core/HW/SI/SI.h

@@ -38,7 +38,6 @@ void AddDevice(SIDevices device, int device_number);
 void AddDevice(std::unique_ptr<ISIDevice> device);
 
 void ChangeDevice(SIDevices device, int channel);
-void ChangeDeviceDeterministic(SIDevices device, int channel);
 
 SIDevices GetDeviceType(int channel);
 

Source/Core/Core/Movie.cpp

@@ -409,7 +409,7 @@ bool IsNetPlayRecording()
 }
 
 // NOTE: Host Thread
-void ChangePads(bool instantly)
+void ChangePads()
 {
   if (!Core::IsRunning())
     return;
@@ -422,7 +422,7 @@ void ChangePads(bool instantly)
       controllers |= (1 << i);
   }
 
-  if (instantly && (s_controllers & 0x0F) == controllers)
+  if ((s_controllers & 0x0F) == controllers)
     return;
 
   for (int i = 0; i < SerialInterface::MAX_SI_CHANNELS; ++i)
@@ -441,10 +441,7 @@ void ChangePads(bool instantly)
       }
     }
 
-    if (instantly)  // Changes from savestates need to be instantaneous
-      SerialInterface::AddDevice(device, i);
-    else
-      SerialInterface::ChangeDevice(device, i);
+    SerialInterface::ChangeDevice(device, i);
   }
 }
 
@@ -961,7 +958,7 @@ void LoadInput(const std::string& movie_path)
     t_record.WriteArray(&tmpHeader, 1);
   }
 
-  ChangePads(true);
+  ChangePads();
   if (SConfig::GetInstance().bWii)
     ChangeWiiPads(true);
 

Source/Core/Core/Movie.h

@@ -152,7 +152,7 @@ bool IsNetPlayRecording();
 bool IsUsingPad(int controller);
 bool IsUsingWiimote(int wiimote);
 bool IsUsingBongo(int controller);
-void ChangePads(bool instantly = false);
+void ChangePads();
 void ChangeWiiPads(bool instantly = false);
 
 void SetReadOnly(bool bEnabled);

Source/Core/Core/NetPlayClient.cpp

@@ -1566,21 +1566,21 @@ void NetPlayClient::UpdateDevices()
     {
       if (SerialInterface::SIDevice_IsGCController(SConfig::GetInstance().m_SIDevice[local_pad]))
       {
-        SerialInterface::AddDevice(SConfig::GetInstance().m_SIDevice[local_pad], pad);
+        SerialInterface::ChangeDevice(SConfig::GetInstance().m_SIDevice[local_pad], pad);
       }
       else
       {
-        SerialInterface::AddDevice(SerialInterface::SIDEVICE_GC_CONTROLLER, pad);
+        SerialInterface::ChangeDevice(SerialInterface::SIDEVICE_GC_CONTROLLER, pad);
       }
       local_pad++;
     }
     else if (player_id > 0)
     {
-      SerialInterface::AddDevice(SerialInterface::SIDEVICE_GC_CONTROLLER, pad);
+      SerialInterface::ChangeDevice(SerialInterface::SIDEVICE_GC_CONTROLLER, pad);
     }
     else
     {
-      SerialInterface::AddDevice(SerialInterface::SIDEVICE_NONE, pad);
+      SerialInterface::ChangeDevice(SerialInterface::SIDEVICE_NONE, pad);
     }
     pad++;
   }

Source/Core/Core/State.cpp

@@ -74,7 +74,7 @@ static Common::Event g_compressAndDumpStateSyncEvent;
 static std::thread g_save_thread;
 
 // Don't forget to increase this after doing changes on the savestate system
-static const u32 STATE_VERSION = 101;  // Last changed in PR 7761
+static const u32 STATE_VERSION = 102;  // Last changed in PR 7742
 
 // Maps savestate versions to Dolphin versions.
 // Versions after 42 don't need to be added to this list,