CPU: Make interrupts actually edge-triggered

stenzek · Mar 17, 2024 · fa68509 · fa68509
1 parent 3702a53
commit fa68509
Show file tree

Hide file tree

Showing 13 changed files with 147 additions and 100 deletions.
diff --git a/src/core/cdrom.cpp b/src/core/cdrom.cpp
@@ -986,6 +986,7 @@ void CDROM::WriteRegister(u32 offset, u8 value)
       s_interrupt_flag_register &= ~(value & INTERRUPT_REGISTER_MASK);
       if (s_interrupt_flag_register == 0)
       {
+        InterruptController::SetLineState(InterruptController::IRQ::CDROM, false);
         if (HasPendingAsyncInterrupt())
           QueueDeliverAsyncInterrupt();
         else
@@ -1212,10 +1213,8 @@ void CDROM::UpdateStatusRegister()
 
 void CDROM::UpdateInterruptRequest()
 {
-  if ((s_interrupt_flag_register & s_interrupt_enable_register) == 0)
-    return;
-
-  InterruptController::InterruptRequest(InterruptController::IRQ::CDROM);
+  InterruptController::SetLineState(InterruptController::IRQ::CDROM,
+                                   (s_interrupt_flag_register & s_interrupt_enable_register) != 0);
 }
 
 bool CDROM::HasPendingDiscEvent()

diff --git a/src/core/cpu_core.cpp b/src/core/cpu_core.cpp
@@ -398,15 +398,14 @@ void CPU::RaiseBreakException(u32 CAUSE_bits, u32 EPC, u32 instruction_bits)
   RaiseException(CAUSE_bits, EPC, GetExceptionVector());
 }
 
-void CPU::SetExternalInterrupt(u8 bit)
-{
-  g_state.cop0_regs.cause.Ip |= static_cast<u8>(1u << bit);
-  CheckForPendingInterrupt();
-}
-
-void CPU::ClearExternalInterrupt(u8 bit)
-{
-  g_state.cop0_regs.cause.Ip &= static_cast<u8>(~(1u << bit));
+void CPU::SetIRQRequest(bool state)
+{
+  // Only uses bit 10.
+  constexpr u32 bit = (1u << 10);
+  const u32 old_cause = g_state.cop0_regs.cause.bits;
+  g_state.cop0_regs.cause.bits = (g_state.cop0_regs.cause.bits & ~bit) | (state ? bit : 0u);
+  if (old_cause ^ g_state.cop0_regs.cause.bits && state)
+    CheckForPendingInterrupt();
 }
 
 ALWAYS_INLINE_RELEASE void CPU::UpdateLoadDelay()

diff --git a/src/core/cpu_core.h b/src/core/cpu_core.h
@@ -176,8 +176,7 @@ bool SafeWriteMemoryHalfWord(VirtualMemoryAddress addr, u16 value);
 bool SafeWriteMemoryWord(VirtualMemoryAddress addr, u32 value);
 
 // External IRQs
-void SetExternalInterrupt(u8 bit);
-void ClearExternalInterrupt(u8 bit);
+void SetIRQRequest(bool state);
 
 void DisassembleAndPrint(u32 addr);
 void DisassembleAndLog(u32 addr);

diff --git a/src/core/dma.cpp b/src/core/dma.cpp
@@ -1,4 +1,4 @@
-// SPDX-FileCopyrightText: 2019-2022 Connor McLaughlin <[email protected]>
+// SPDX-FileCopyrightText: 2019-2024 Connor McLaughlin <[email protected]>
 // SPDX-License-Identifier: (GPL-3.0 OR CC-BY-NC-ND-4.0)
 
 #include "dma.h"
@@ -22,6 +22,8 @@
 #include "common/log.h"
 #include "common/string_util.h"
 
+#include "fmt/format.h"
+
 #include <array>
 #include <memory>
 #include <vector>
@@ -203,8 +205,20 @@ static constexpr std::array<bool (*)(), NUM_CHANNELS> s_channel_transfer_functio
   &TransferChannel<Channel::OTC>,
 }};
 
+[[maybe_unused]] static constexpr std::array<const char*, NUM_CHANNELS> s_channel_names = {
+  {"MDECin", "MDECout", "GPU", "CDROM", "SPU", "PIO", "OTC"}};
+
 }; // namespace DMA
 
+template<>
+struct fmt::formatter<DMA::Channel> : fmt::formatter<fmt::string_view>
+{
+  auto format(DMA::Channel channel, fmt::format_context& ctx) const
+  {
+    return formatter<fmt::string_view>::format(DMA::s_channel_names[static_cast<u32>(channel)], ctx);
+  }
+};
+
 u32 DMA::GetAddressMask()
 {
   return Bus::g_ram_mask & 0xFFFFFFFCu;
@@ -332,12 +346,13 @@ void DMA::WriteRegister(u32 offset, u32 value)
       case 0x00:
       {
         state.base_address = value & BASE_ADDRESS_MASK;
-        Log_TracePrintf("DMA channel %u base address <- 0x%08X", channel_index, state.base_address);
+        Log_TraceFmt("DMA channel {} base address <- 0x{:08X}", static_cast<Channel>(channel_index),
+                     state.base_address);
         return;
       }
       case 0x04:
       {
-        Log_TracePrintf("DMA channel %u block control <- 0x%08X", channel_index, value);
+        Log_TraceFmt("DMA channel {} block control <- 0x{:08X}", static_cast<Channel>(channel_index), value);
         state.block_control.bits = value;
         return;
       }
@@ -352,14 +367,42 @@ void DMA::WriteRegister(u32 offset, u32 value)
 
         state.channel_control.bits = (state.channel_control.bits & ~ChannelState::ChannelControl::WRITE_MASK) |
                                      (value & ChannelState::ChannelControl::WRITE_MASK);
-        Log_TracePrintf("DMA channel %u channel control <- 0x%08X", channel_index, state.channel_control.bits);
+        Log_TracePrintf("DMA channel {} channel control <- 0x{:08X}", static_cast<Channel>(channel_index),
+                        state.channel_control.bits);
 
         // start/trigger bit must be enabled for OTC
         if (static_cast<Channel>(channel_index) == Channel::OTC)
           SetRequest(static_cast<Channel>(channel_index), state.channel_control.start_trigger);
 
         if (CanTransferChannel(static_cast<Channel>(channel_index), ignore_halt))
-          s_channel_transfer_functions[channel_index]();
+        {
+          if (static_cast<Channel>(channel_index) != Channel::OTC &&
+              state.channel_control.sync_mode == SyncMode::Manual && state.channel_control.chopping_enable)
+          {
+            // Figure out how roughly many CPU cycles it'll take for the transfer to complete, and delay the transfer.
+            // Needed for Lagnacure Legend, which sets DICR to enable interrupts after CHCR to kickstart the transfer.
+            // This has an artificial 500 cycle cap, setting it too high causes Namco Museum Vol. 4 and a couple of
+            // other games to crash... so clearly something is missing here.
+            const u32 block_words = (1u << state.channel_control.chopping_dma_window_size);
+            const u32 cpu_cycles_per_block = (1u << state.channel_control.chopping_cpu_window_size);
+            const u32 blocks = state.block_control.manual.word_count / block_words;
+            const TickCount delay_cycles = std::min(static_cast<TickCount>(cpu_cycles_per_block * blocks), 500);
+            if (delay_cycles > 1 && true)
+            {
+              Log_DevFmt("Delaying {} transfer by {} cycles due to chopping", static_cast<Channel>(channel_index),
+                         delay_cycles);
+              HaltTransfer(delay_cycles);
+            }
+            else
+            {
+              s_channel_transfer_functions[channel_index]();
+            }
+          }
+          else
+          {
+            s_channel_transfer_functions[channel_index]();
+          }
+        }
         return;
       }
 
@@ -393,7 +436,7 @@ void DMA::WriteRegister(u32 offset, u32 value)
         Log_TracePrintf("DCIR <- 0x%08X", value);
         s_DICR.bits = (s_DICR.bits & ~DICR_WRITE_MASK) | (value & DICR_WRITE_MASK);
         s_DICR.bits = s_DICR.bits & ~(value & DICR_RESET_MASK);
-        s_DICR.UpdateMasterFlag();
+        UpdateIRQ();
         return;
       }
 
@@ -450,10 +493,8 @@ void DMA::UpdateIRQ()
 {
   s_DICR.UpdateMasterFlag();
   if (s_DICR.master_flag)
-  {
     Log_TracePrintf("Firing DMA master interrupt");
-    InterruptController::InterruptRequest(InterruptController::IRQ::DMA);
-  }
+  InterruptController::SetLineState(InterruptController::IRQ::DMA, s_DICR.master_flag);
 }
 
 // Plenty of games seem to suffer from this issue where they have a linked list DMA going while polling the
@@ -576,10 +617,10 @@ bool DMA::TransferChannel()
 
     case SyncMode::Request:
     {
-      Log_DebugPrintf("DMA%u: Copying %u blocks of size %u (%u total words) %s 0x%08X", static_cast<u32>(channel),
-                      cs.block_control.request.GetBlockCount(), cs.block_control.request.GetBlockSize(),
-                      cs.block_control.request.GetBlockCount() * cs.block_control.request.GetBlockSize(),
-                      copy_to_device ? "from" : "to", current_address & mask);
+      Log_DebugFmt("DMA[{}]: Copying {} blocks of size {} ({} total words) {} 0x{:08X}", channel,
+                   cs.block_control.request.GetBlockCount(), cs.block_control.request.GetBlockSize(),
+                   cs.block_control.request.GetBlockCount() * cs.block_control.request.GetBlockSize(),
+                   copy_to_device ? "from" : "to", current_address);
 
       const u32 block_size = cs.block_control.request.GetBlockSize();
       u32 blocks_remaining = cs.block_control.request.GetBlockCount();
@@ -638,10 +679,11 @@ bool DMA::TransferChannel()
   }
 
   // start/busy bit is cleared on end of transfer
+  Log_DebugFmt("DMA transfer for channel {} complete", channel);
   cs.channel_control.enable_busy = false;
   if (s_DICR.IsIRQEnabled(channel))
   {
-    Log_DebugPrintf("Set DMA interrupt for channel %u", static_cast<u32>(channel));
+    Log_DebugFmt("Setting DMA interrupt for channel {}", channel);
     s_DICR.SetIRQFlag(channel);
     UpdateIRQ();
   }
@@ -816,8 +858,6 @@ void DMA::DrawDebugStateWindow()
   static constexpr u32 NUM_COLUMNS = 10;
   static constexpr std::array<const char*, NUM_COLUMNS> column_names = {
     {"#", "Req", "Direction", "Chopping", "Mode", "Busy", "Enable", "Priority", "IRQ", "Flag"}};
-  static constexpr std::array<const char*, NUM_CHANNELS> channel_names = {
-    {"MDECin", "MDECout", "GPU", "CDROM", "SPU", "PIO", "OTC"}};
   static constexpr std::array<const char*, 4> sync_mode_names = {{"Manual", "Request", "LinkedList", "Reserved"}};
 
   const float framebuffer_scale = Host::GetOSDScale();
@@ -854,7 +894,7 @@ void DMA::DrawDebugStateWindow()
   {
     const ChannelState& cs = s_state[i];
 
-    ImGui::TextColored(cs.channel_control.enable_busy ? active : inactive, "%u[%s]", i, channel_names[i]);
+    ImGui::TextColored(cs.channel_control.enable_busy ? active : inactive, "%u[%s]", i, s_channel_names[i]);
     ImGui::NextColumn();
     ImGui::TextColored(cs.request ? active : inactive, cs.request ? "Yes" : "No");
     ImGui::NextColumn();

diff --git a/src/core/gpu.cpp b/src/core/gpu.cpp
@@ -961,6 +961,7 @@ void GPU::CRTCTickEvent(TickCount ticks)
         m_crtc_state.current_scanline >= m_crtc_state.vertical_display_end)
     {
       Timers::SetGate(HBLANK_TIMER_INDEX, false);
+      InterruptController::SetLineState(InterruptController::IRQ::VBLANK, false);
       m_crtc_state.in_vblank = false;
     }
 
@@ -971,7 +972,6 @@ void GPU::CRTCTickEvent(TickCount ticks)
       if (new_vblank)
       {
         Log_DebugPrintf("Now in v-blank");
-        InterruptController::InterruptRequest(InterruptController::IRQ::VBLANK);
 
         // flush any pending draws and "scan out" the image
         // TODO: move present in here I guess
@@ -987,6 +987,7 @@ void GPU::CRTCTickEvent(TickCount ticks)
       }
 
       Timers::SetGate(HBLANK_TIMER_INDEX, new_vblank);
+      InterruptController::SetLineState(InterruptController::IRQ::VBLANK, new_vblank);
       m_crtc_state.in_vblank = new_vblank;
     }
 

diff --git a/src/core/gpu_commands.cpp b/src/core/gpu_commands.cpp
@@ -208,11 +208,9 @@ bool GPU::HandleClearCacheCommand()
 bool GPU::HandleInterruptRequestCommand()
 {
   Log_DebugPrintf("GP0 interrupt request");
-  if (!m_GPUSTAT.interrupt_request)
-  {
-    m_GPUSTAT.interrupt_request = true;
-    InterruptController::InterruptRequest(InterruptController::IRQ::GPU);
-  }
+
+  m_GPUSTAT.interrupt_request = true;
+  InterruptController::SetLineState(InterruptController::IRQ::GPU, m_GPUSTAT.interrupt_request);
 
   m_fifo.RemoveOne();
   AddCommandTicks(1);

diff --git a/src/core/interrupt_controller.cpp b/src/core/interrupt_controller.cpp
@@ -1,54 +1,64 @@
-// SPDX-FileCopyrightText: 2019-2022 Connor McLaughlin <[email protected]>
+// SPDX-FileCopyrightText: 2019-2024 Connor McLaughlin <[email protected]>
 // SPDX-License-Identifier: (GPL-3.0 OR CC-BY-NC-ND-4.0)
 
 #include "interrupt_controller.h"
-#include "common/log.h"
 #include "cpu_core.h"
+
 #include "util/state_wrapper.h"
+
+#include "common/log.h"
+
 Log_SetChannel(InterruptController);
 
 namespace InterruptController {
 
 static constexpr u32 REGISTER_WRITE_MASK = (u32(1) << NUM_IRQS) - 1;
-static constexpr u32 DEFAULT_INTERRUPT_MASK = 0; //(u32(1) << NUM_IRQS) - 1;
+static constexpr u32 DEFAULT_INTERRUPT_MASK = 0;
 
 static void UpdateCPUInterruptRequest();
 
 static u32 s_interrupt_status_register = 0;
 static u32 s_interrupt_mask_register = DEFAULT_INTERRUPT_MASK;
+static u32 s_interrupt_line_state = 0;
 
-} // namespace InterruptController
+[[maybe_unused]] static constexpr std::array<const char*, static_cast<size_t>(IRQ::MaxCount)> s_irq_names = {
+  {"VBLANK", "GPU", "CDROM", "DMA", "TMR0", "TMR1", "TMR2", "PAD", "SIO", "SPU", "IRQ10"}};
 
-void InterruptController::Initialize()
-{
-  Reset();
-}
-
-void InterruptController::Shutdown() {}
+} // namespace InterruptController
 
 void InterruptController::Reset()
 {
   s_interrupt_status_register = 0;
   s_interrupt_mask_register = DEFAULT_INTERRUPT_MASK;
+  s_interrupt_line_state = 0;
 }
 
 bool InterruptController::DoState(StateWrapper& sw)
 {
   sw.Do(&s_interrupt_status_register);
   sw.Do(&s_interrupt_mask_register);
+  sw.DoEx(&s_interrupt_line_state, 63, s_interrupt_status_register);
 
   return !sw.HasError();
 }
 
-bool InterruptController::GetIRQLineState()
-{
-  return (s_interrupt_status_register != 0);
-}
-
-void InterruptController::InterruptRequest(IRQ irq)
+void InterruptController::SetLineState(IRQ irq, bool state)
 {
-  const u32 bit = (u32(1) << static_cast<u32>(irq));
-  s_interrupt_status_register |= bit;
+  // Interupts are edge-triggered, so only set the flag in the status register on a 0-1 transition.
+  const u32 bit = (1u << static_cast<u32>(irq));
+  const u32 prev_state = s_interrupt_line_state;
+  s_interrupt_line_state = (s_interrupt_line_state & ~bit) | (state ? bit : 0u);
+  if (s_interrupt_line_state == prev_state)
+    return;
+
+#ifdef _DEBUG
+  if (!(prev_state & bit) && state)
+    Log_DebugFmt("{} IRQ triggered", s_irq_names[static_cast<size_t>(irq)]);
+  else if ((prev_state & bit) && !state)
+    Log_DebugFmt("{} IRQ line inactive", s_irq_names[static_cast<size_t>(irq)]);
+#endif
+
+  s_interrupt_status_register |= (state ? (prev_state ^ s_interrupt_line_state) : 0u) & s_interrupt_line_state;
   UpdateCPUInterruptRequest();
 }
 
@@ -74,8 +84,14 @@ void InterruptController::WriteRegister(u32 offset, u32 value)
   {
     case 0x00: // I_STATUS
     {
-      if ((s_interrupt_status_register & ~value) != 0)
-        Log_DebugPrintf("Clearing bits 0x%08X", (s_interrupt_status_register & ~value));
+#ifdef _DEBUG
+      const u32 cleared_bits = (s_interrupt_status_register & ~value);
+      for (u32 i = 0; i < static_cast<u32>(IRQ::MaxCount); i++)
+      {
+        if (cleared_bits & (1u << i))
+          Log_DebugFmt("{} IRQ cleared", s_irq_names[i]);
+      }
+#endif
 
       s_interrupt_status_register = s_interrupt_status_register & (value & REGISTER_WRITE_MASK);
       UpdateCPUInterruptRequest();
@@ -96,11 +112,8 @@ void InterruptController::WriteRegister(u32 offset, u32 value)
   }
 }
 
-void InterruptController::UpdateCPUInterruptRequest()
+ALWAYS_INLINE_RELEASE void InterruptController::UpdateCPUInterruptRequest()
 {
-  // external interrupts set bit 10 only?
-  if ((s_interrupt_status_register & s_interrupt_mask_register) != 0)
-    CPU::SetExternalInterrupt(2);
-  else
-    CPU::ClearExternalInterrupt(2);
+  const bool state = (s_interrupt_status_register & s_interrupt_mask_register) != 0;
+  CPU::SetIRQRequest(state);
 }