Merge pull request #14540 from jordan-woyak/hsp-cleanups

HW:HSP: Change read/write interface to use std::span and other cleanups.

Source/Core/Core/HW/DSP.cpp

@@ -505,12 +505,20 @@ void DSPManager::Do_ARAM_DMA()
     }
     else if (!m_aram.wii_mode)
     {
-      while (m_aram_dma.Cnt.count)
+      auto* mm_ptr = memory.GetPointerForRange(m_aram_dma.MMAddr, m_aram_dma.Cnt.count);
+      if (mm_ptr != nullptr)
       {
-        memory.Write_U64(m_system.GetHSP().Read(m_aram_dma.ARAddr), m_aram_dma.MMAddr);
-        m_aram_dma.MMAddr += 8;
-        m_aram_dma.ARAddr += 8;
-        m_aram_dma.Cnt.count -= 8;
+        auto& hsp = m_system.GetHSP();
+        while (m_aram_dma.Cnt.count)
+        {
+          hsp.Read(m_aram_dma.ARAddr,
+                   std::span<u8, HSP::TRANSFER_SIZE>(mm_ptr, HSP::TRANSFER_SIZE));
+
+          m_aram_dma.MMAddr += HSP::TRANSFER_SIZE;
+          m_aram_dma.ARAddr += HSP::TRANSFER_SIZE;
+          m_aram_dma.Cnt.count -= HSP::TRANSFER_SIZE;
+          mm_ptr += HSP::TRANSFER_SIZE;
+        }
       }
     }
   }
@@ -557,13 +565,20 @@ void DSPManager::Do_ARAM_DMA()
     }
     else if (!m_aram.wii_mode)
     {
-      while (m_aram_dma.Cnt.count)
+      auto* mm_ptr = memory.GetPointerForRange(m_aram_dma.MMAddr, m_aram_dma.Cnt.count);
+      if (mm_ptr != nullptr)
       {
-        m_system.GetHSP().Write(m_aram_dma.ARAddr, memory.Read_U64(m_aram_dma.MMAddr));
+        auto& hsp = m_system.GetHSP();
+        while (m_aram_dma.Cnt.count)
+        {
+          hsp.Write(m_aram_dma.ARAddr,
+                    std::span<const u8, HSP::TRANSFER_SIZE>(mm_ptr, HSP::TRANSFER_SIZE));
 
-        m_aram_dma.MMAddr += 8;
-        m_aram_dma.ARAddr += 8;
-        m_aram_dma.Cnt.count -= 8;
+          m_aram_dma.MMAddr += HSP::TRANSFER_SIZE;
+          m_aram_dma.ARAddr += HSP::TRANSFER_SIZE;
+          m_aram_dma.Cnt.count -= HSP::TRANSFER_SIZE;
+          mm_ptr += HSP::TRANSFER_SIZE;
+        }
       }
     }
   }

Source/Core/Core/HW/HSP/HSP.cpp

@@ -5,6 +5,8 @@
 
 #include <memory>
 
+#include <fmt/ranges.h>
+
 #include "Common/ChunkFile.h"
 #include "Core/Config/MainSettings.h"
 #include "Core/HW/HSP/HSP_Device.h"
@@ -16,54 +18,49 @@ HSPManager::~HSPManager() = default;
 
 void HSPManager::Init()
 {
-  AddDevice(Config::Get(Config::MAIN_HSP_DEVICE));
+  SetDevice(Config::Get(Config::MAIN_HSP_DEVICE));
 }
 
 void HSPManager::Shutdown()
 {
-  RemoveDevice();
+  m_device.reset();
 }
 
-u64 HSPManager::Read(u32 address)
+void HSPManager::Read(u32 address, std::span<u8, TRANSFER_SIZE> data)
 {
   DEBUG_LOG_FMT(HSP, "HSP read from 0x{:08x}", address);
-  if (m_device)
-    return m_device->Read(address);
-  return 0;
+
+  m_device->Read(address, data);
 }
 
-void HSPManager::Write(u32 address, u64 value)
+void HSPManager::Write(u32 address, std::span<const u8, TRANSFER_SIZE> data)
 {
-  DEBUG_LOG_FMT(HSP, "HSP write to 0x{:08x}: 0x{:016x}", address, value);
-  if (m_device)
-    m_device->Write(address, value);
+  DEBUG_LOG_FMT(HSP, "HSP write to 0x{:08x}: {:02x}", address, fmt::join(data, " "));
+
+  m_device->Write(address, data);
 }
 
 void HSPManager::DoState(PointerWrap& p)
 {
-  HSPDeviceType type = m_device->GetDeviceType();
-  p.Do(type);
+  const HSPDeviceType current_type = m_device->GetDeviceType();
+  auto state_type = current_type;
+  p.Do(state_type);
 
   // If the type doesn't match, switch to the right device type
-  if (type != m_device->GetDeviceType())
-    AddDevice(type);
+  if (state_type != current_type)
+    SetDevice(state_type);
 
   m_device->DoState(p);
 }
 
-void HSPManager::AddDevice(std::unique_ptr<IHSPDevice> device)
+void HSPManager::SetDevice(std::unique_ptr<IHSPDevice> device)
 {
-  // Set the new one
   m_device = std::move(device);
 }
 
-void HSPManager::AddDevice(const HSPDeviceType device)
+void HSPManager::SetDevice(const HSPDeviceType device)
 {
-  AddDevice(HSPDevice_Create(device));
+  SetDevice(HSPDevice_Create(device));
 }
 
-void HSPManager::RemoveDevice()
-{
-  m_device.reset();
-}
 }  // namespace HSP

Source/Core/Core/HW/HSP/HSP.h

@@ -4,16 +4,15 @@
 #pragma once
 
 #include <memory>
+#include <span>
 
 #include "Common/CommonTypes.h"
+#include "Core/HW/HSP/HSP_Device.h"
 
 class PointerWrap;
 
 namespace HSP
 {
-class IHSPDevice;
-enum class HSPDeviceType : int;
-
 class HSPManager
 {
 public:
@@ -27,14 +26,13 @@ public:
   void Init();
   void Shutdown();
 
-  u64 Read(u32 address);
-  void Write(u32 address, u64 value);
+  void Read(u32 address, std::span<u8, TRANSFER_SIZE> data);
+  void Write(u32 address, std::span<const u8, TRANSFER_SIZE> data);
 
   void DoState(PointerWrap& p);
 
-  void RemoveDevice();
-  void AddDevice(std::unique_ptr<IHSPDevice> device);
-  void AddDevice(HSPDeviceType device);
+  void SetDevice(std::unique_ptr<IHSPDevice> device);
+  void SetDevice(HSPDeviceType device);
 
 private:
   std::unique_ptr<IHSPDevice> m_device;

Source/Core/Core/HW/HSP/HSP_Device.cpp

@@ -12,20 +12,12 @@
 
 namespace HSP
 {
-IHSPDevice::IHSPDevice(HSPDeviceType device_type) : m_device_type(device_type)
-{
-}
-
-HSPDeviceType IHSPDevice::GetDeviceType() const
-{
-  return m_device_type;
-}
+IHSPDevice::~IHSPDevice() = default;
 
 void IHSPDevice::DoState(PointerWrap& p)
 {
 }
 
-// F A C T O R Y
 std::unique_ptr<IHSPDevice> HSPDevice_Create(const HSPDeviceType device)
 {
   auto& system = Core::System::GetInstance();
@@ -33,12 +25,12 @@ std::unique_ptr<IHSPDevice> HSPDevice_Create(const HSPDeviceType device)
   switch (device)
   {
   case HSPDeviceType::ARAMExpansion:
-    return std::make_unique<CHSPDevice_ARAMExpansion>(device);
+    return std::make_unique<CHSPDevice_ARAMExpansion>();
   case HSPDeviceType::GBPlayer:
-    return std::make_unique<CHSPDevice_GBPlayer>(system, device);
+    return std::make_unique<CHSPDevice_GBPlayer>(system);
   case HSPDeviceType::None:
   default:
-    return std::make_unique<CHSPDevice_Null>(device);
+    return std::make_unique<CHSPDevice_Null>();
   }
 }
 }  // namespace HSP

Source/Core/Core/HW/HSP/HSP_Device.h

@@ -4,6 +4,7 @@
 #pragma once
 
 #include <memory>
+#include <span>
 
 #include "Common/CommonTypes.h"
 
@@ -11,6 +12,9 @@ class PointerWrap;
 
 namespace HSP
 {
+
+static constexpr std::size_t TRANSFER_SIZE = 32;
+
 enum class HSPDeviceType : int
 {
   None,
@@ -21,19 +25,17 @@ enum class HSPDeviceType : int
 class IHSPDevice
 {
 public:
-  explicit IHSPDevice(HSPDeviceType device_type);
-  virtual ~IHSPDevice() = default;
+  virtual ~IHSPDevice();
 
-  HSPDeviceType GetDeviceType() const;
+  virtual HSPDeviceType GetDeviceType() const = 0;
 
-  virtual void Write(u32 address, u64 value) = 0;
-  virtual u64 Read(u32 address) = 0;
+  // Note: DSPManager::RegisterMMIO ensures addresses are 32 byte aligned.
+
+  virtual void Read(u32 address, std::span<u8, TRANSFER_SIZE> data) = 0;
+  virtual void Write(u32 address, std::span<const u8, TRANSFER_SIZE> data) = 0;
 
   // Savestate support
   virtual void DoState(PointerWrap& p);
-
-protected:
-  HSPDeviceType m_device_type;
 };
 
 std::unique_ptr<IHSPDevice> HSPDevice_Create(HSPDeviceType device);

Source/Core/Core/HW/HSP/HSP_DeviceARAMExpansion.cpp

@@ -8,36 +8,30 @@
 #include "Common/ChunkFile.h"
 #include "Common/MathUtil.h"
 #include "Common/MemoryUtil.h"
-#include "Common/Swap.h"
 
 #include "Core/Config/MainSettings.h"
 
 namespace HSP
 {
-CHSPDevice_ARAMExpansion::CHSPDevice_ARAMExpansion(HSPDeviceType device) : IHSPDevice(device)
+CHSPDevice_ARAMExpansion::CHSPDevice_ARAMExpansion()
+    : m_size{MathUtil::NextPowerOf2(Config::Get(Config::MAIN_ARAM_EXPANSION_SIZE))},
+      m_mask{m_size - 1}, m_ptr{static_cast<u8*>(Common::AllocateMemoryPages(m_size))}
 {
-  m_size = MathUtil::NextPowerOf2(Config::Get(Config::MAIN_ARAM_EXPANSION_SIZE));
-  m_mask = m_size - 1;
-  m_ptr = static_cast<u8*>(Common::AllocateMemoryPages(m_size));
 }
 
 CHSPDevice_ARAMExpansion::~CHSPDevice_ARAMExpansion()
 {
   Common::FreeMemoryPages(m_ptr, m_size);
-  m_ptr = nullptr;
 }
 
-u64 CHSPDevice_ARAMExpansion::Read(u32 address)
+void CHSPDevice_ARAMExpansion::Read(u32 address, std::span<u8, TRANSFER_SIZE> data)
 {
-  u64 value;
-  std::memcpy(&value, &m_ptr[address & m_mask], sizeof(value));
-  return Common::swap64(value);
+  std::memcpy(data.data(), m_ptr + (address & m_mask), data.size());
 }
 
-void CHSPDevice_ARAMExpansion::Write(u32 address, u64 value)
+void CHSPDevice_ARAMExpansion::Write(u32 address, std::span<const u8, TRANSFER_SIZE> data)
 {
-  value = Common::swap64(value);
-  std::memcpy(&m_ptr[address & m_mask], &value, sizeof(value));
+  std::memcpy(m_ptr + (address & m_mask), data.data(), data.size());
 }
 
 void CHSPDevice_ARAMExpansion::DoState(PointerWrap& p)

Source/Core/Core/HW/HSP/HSP_DeviceARAMExpansion.h

@@ -7,20 +7,22 @@
 
 namespace HSP
 {
-class CHSPDevice_ARAMExpansion : public IHSPDevice
+class CHSPDevice_ARAMExpansion final : public IHSPDevice
 {
 public:
-  explicit CHSPDevice_ARAMExpansion(HSPDeviceType device);
+  explicit CHSPDevice_ARAMExpansion();
   ~CHSPDevice_ARAMExpansion() override;
 
-  void Write(u32 address, u64 value) override;
-  u64 Read(u32 address) override;
+  HSPDeviceType GetDeviceType() const override { return HSPDeviceType::ARAMExpansion; }
+
+  void Read(u32 address, std::span<u8, TRANSFER_SIZE> data) override;
+  void Write(u32 address, std::span<const u8, TRANSFER_SIZE> data) override;
 
   void DoState(PointerWrap&) override;
 
 private:
-  u32 m_size;
-  u32 m_mask;
-  u8* m_ptr = nullptr;
+  const u32 m_size;
+  const u32 m_mask;
+  u8* const m_ptr = nullptr;
 };
 }  // namespace HSP

Source/Core/Core/HW/HSP/HSP_DeviceGBPlayer.cpp

@@ -17,6 +17,7 @@
 
 #include "Common/ChunkFile.h"
 #include "Common/Logging/Log.h"
+#include "Common/Swap.h"
 
 #include "Core/Config/MainSettings.h"
 #include "Core/CoreTiming.h"
@@ -50,10 +51,9 @@ constexpr u8 CONTROL_MASK_IRQ = 0x10;
 // constexpr u8 CONTROL_LINK_CABLE = 0x40;
 // constexpr u8 CONTROL_LINK_ENABLE = 0x80;
 
-constexpr u32 COMMAND_ADDRESS_MASK = 0x1f;
-constexpr u32 AV_ADDRESS_MASK = 0xff8;
+constexpr u32 AV_ADDRESS_MASK = 0xfe0;
 
-class CGBPlayer_Dummy : public HSP::IGBPlayer
+class CGBPlayer_Dummy final : public HSP::IGBPlayer
 {
 public:
   using IGBPlayer::IGBPlayer;
@@ -89,7 +89,7 @@ enum class CHSPDevice_GBPlayer::IRQ : int
 
 #if defined(HAS_LIBMGBA)
 
-class CGBPlayer_mGBA : public IGBPlayer
+class CGBPlayer_mGBA final : public IGBPlayer
 {
 public:
   CGBPlayer_mGBA(Core::System&, CHSPDevice_GBPlayer*);
@@ -232,13 +232,13 @@ void CGBPlayer_mGBA::ReadScanlines(std::span<u32, AV_REGION_SIZE> scanlines)
   {
     const u32 color =
         M_RGB8_TO_RGB5(video_buffer[(m_current_scanline_index * GBA_VIDEO_HORIZONTAL_PIXELS) + i]);
-    u32 c = color & 0xFF;
-    c |= (color & 0xFF00u) << 8;
+    u32 c = color >> 8u;
+    c |= (color & 0xffu) << 16u;
     scanlines[i] = c | (c << 8);  // Parity
   }
 
   if (m_current_scanline_index == 0)
-    scanlines[0] |= 0x80800000;
+    scanlines[0] |= 0x00008080;
 
   m_current_scanline_index += 4;
 
@@ -389,8 +389,7 @@ void CGBPlayer_mGBA::UpdateVideo(u32 scanline_index, s64 cycles_late)
 
 #endif
 
-CHSPDevice_GBPlayer::CHSPDevice_GBPlayer(Core::System& system, HSPDeviceType device)
-    : IHSPDevice(device), m_system{system}
+CHSPDevice_GBPlayer::CHSPDevice_GBPlayer(Core::System& system) : m_system{system}
 {
 #if defined(HAS_LIBMGBA)
   m_gbp = std::make_unique<CGBPlayer_mGBA>(m_system, this);
@@ -399,14 +398,13 @@ CHSPDevice_GBPlayer::CHSPDevice_GBPlayer(Core::System& system, HSPDeviceType dev
 #endif
 }
 
-u64 CHSPDevice_GBPlayer::Read(u32 address)
+void CHSPDevice_GBPlayer::Read(u32 address, std::span<u8, TRANSFER_SIZE> data)
 {
-  u64 value = 0;
   switch (GBPRegister(address >> 20))
   {
   case GBPRegister::Test:
   {
-    std::memcpy(&value, &m_test[address & COMMAND_ADDRESS_MASK], sizeof(value));
+    std::ranges::copy(m_test, data.data());
     break;
   }
   case GBPRegister::Control:
@@ -424,14 +422,19 @@ u64 CHSPDevice_GBPlayer::Read(u32 address)
       m_control &= ~(CONTROL_CART_DETECTED | CONTROL_CART_INSERTED);
     }
 
-    value = m_control * 0x0101010101010101ULL;
+    std::ranges::fill(data, m_control);
     break;
   }
   case GBPRegister::IRQ:
   {
-    value = m_irq * 0x0000000100000001ULL;
-    value |= (m_irq & 0xFF00) * 0x0001010000010100ULL;
-    value &= 0xFFFF7FFFFFFF7FFFULL;
+    u32 value = m_irq;
+    value |= (m_irq & 0xff00) * 0x0100'0001u;
+    value &= 0x7fff'ffffu;
+
+    for (std::size_t i = 0; i != data.size(); i += sizeof(value))
+    {
+      std::memcpy(data.data() + i, &value, sizeof(value));
+    }
     break;
   }
   case GBPRegister::Video:
@@ -442,8 +445,7 @@ u64 CHSPDevice_GBPlayer::Read(u32 address)
 
     const u32 scanlines_pos = offset / 4;
 
-    value = u64(m_scanlines[scanlines_pos]) << 32;
-    value |= m_scanlines[scanlines_pos + 1];
+    std::memcpy(data.data(), m_scanlines.data() + scanlines_pos, data.size());
     break;
   }
   case GBPRegister::Audio:
@@ -452,10 +454,13 @@ u64 CHSPDevice_GBPlayer::Read(u32 address)
     if (offset == 0)
       m_gbp->ReadAudio(m_audio);
 
-    const u32 audio_pos = offset / 4;
+    u32 audio_pos = offset / 4;
 
-    value = 0x0101010100000000ULL * m_audio[audio_pos];
-    value |= 0x01010101ULL * m_audio[audio_pos + 1];
+    for (std::size_t i = 0; i != data.size(); i += sizeof(u32))
+    {
+      std::memset(data.data() + i, m_audio[audio_pos], sizeof(u32));
+      ++audio_pos;
+    }
     break;
   }
   default:
@@ -464,27 +469,22 @@ u64 CHSPDevice_GBPlayer::Read(u32 address)
     break;
   }
   }
-  return value;
 }
 
-void CHSPDevice_GBPlayer::Write(u32 address, u64 value)
+void CHSPDevice_GBPlayer::Write(u32 address, std::span<const u8, TRANSFER_SIZE> data)
 {
-  // This seems to compensate for DSP.cpp turning 32byte writes into 4 x 8byte writes.
-  // TODO: Rectify that and adjust this.
-  constexpr u32 last_chunk = 0x18;
-
   switch (GBPRegister(address >> 20))
   {
   case GBPRegister::Test:
   {
-    value = ~value;
-    std::memcpy(&m_test[address & COMMAND_ADDRESS_MASK], &value, sizeof(value));
+    u8* dest = m_test.data();
+    for (u8 value : data)
+      *(dest++) = value ^ 0xffu;
     break;
   }
   case GBPRegister::Control:
   {
-    if ((address & COMMAND_ADDRESS_MASK) != last_chunk)
-      break;
+    const u8 value = data[0x1f];
 
     if (!(m_control & (CONTROL_3V | CONTROL_5V)) && (value & (CONTROL_3V | CONTROL_5V)))
     {
@@ -507,24 +507,20 @@ void CHSPDevice_GBPlayer::Write(u32 address, u64 value)
   }
   case GBPRegister::IRQ:
   {
-    if ((address & COMMAND_ADDRESS_MASK) != last_chunk)
-      break;
-
-    m_irq &= ~value;
+    const u16 value = Common::swap16(data.data() + 0x1e);
+    m_irq &= u16(~value);
     UpdateInterrupts();
     break;
   }
   case GBPRegister::Keypad:
   {
-    if ((address & COMMAND_ADDRESS_MASK) != last_chunk)
-      break;
-
+    const u16 value = Common::swap16(data.data() + 0x1e);
     m_gbp->SetKeys(value & 0x3FF);
     break;
   }
   default:
   {
-    WARN_LOG_FMT(HSP, "GBPlayer: Unknown write to 0x{:08x}: 0x{:016x}", address, value);
+    WARN_LOG_FMT(HSP, "GBPlayer: Unknown write to 0x{:08x}", address);
     break;
   }
   }

Source/Core/Core/HW/HSP/HSP_DeviceGBPlayer.h

@@ -50,15 +50,17 @@ protected:
   CHSPDevice_GBPlayer* const m_player;
 };
 
-class CHSPDevice_GBPlayer : public IHSPDevice
+class CHSPDevice_GBPlayer final : public IHSPDevice
 {
 public:
   enum class IRQ : int;
 
-  CHSPDevice_GBPlayer(Core::System& system, HSPDeviceType device);
+  explicit CHSPDevice_GBPlayer(Core::System& system);
 
-  void Write(u32 address, u64 value) override;
-  u64 Read(u32 address) override;
+  HSPDeviceType GetDeviceType() const override { return HSPDeviceType::GBPlayer; }
+
+  void Read(u32 address, std::span<u8, TRANSFER_SIZE> data) override;
+  void Write(u32 address, std::span<const u8, TRANSFER_SIZE> data) override;
 
   void DoState(PointerWrap& p) override;
 

Source/Core/Core/HW/HSP/HSP_DeviceNull.cpp

@@ -3,20 +3,14 @@
 
 #include "Core/HW/HSP/HSP_DeviceNull.h"
 
-#include "Core/HW/HSP/HSP.h"
-
 namespace HSP
 {
-CHSPDevice_Null::CHSPDevice_Null(HSPDeviceType device) : IHSPDevice(device)
+
+void CHSPDevice_Null::Read(u32 address, std::span<u8, TRANSFER_SIZE> data)
 {
 }
 
-u64 CHSPDevice_Null::Read(u32 address)
-{
-  return 0;
-}
-
-void CHSPDevice_Null::Write(u32 address, u64 value)
+void CHSPDevice_Null::Write(u32 address, std::span<const u8, TRANSFER_SIZE> data)
 {
 }
 

Source/Core/Core/HW/HSP/HSP_DeviceNull.h

@@ -8,12 +8,12 @@
 
 namespace HSP
 {
-class CHSPDevice_Null : public IHSPDevice
+class CHSPDevice_Null final : public IHSPDevice
 {
 public:
-  explicit CHSPDevice_Null(HSPDeviceType device);
+  HSPDeviceType GetDeviceType() const override { return HSPDeviceType::None; }
 
-  void Write(u32 address, u64 value) override;
-  u64 Read(u32 address) override;
+  void Read(u32 address, std::span<u8, TRANSFER_SIZE> data) override;
+  void Write(u32 address, std::span<const u8, TRANSFER_SIZE> data) override;
 };
 }  // namespace HSP