MDEC: Refactoring and fixes

src/core/dma.cpp

@@ -206,10 +206,6 @@ TickCount DMA::GetTransferDelay(Channel channel) const
   const ChannelState& cs = m_state[static_cast<u32>(channel)];
   switch (channel)
   {
-    case Channel::MDECin:
-    case Channel::MDECout:
-      return 1;
-
     case Channel::SPU:
     {
       if (cs.channel_control.sync_mode == SyncMode::Request)

src/core/mdec.cpp

@@ -31,7 +31,7 @@ bool MDEC::DoState(StateWrapper& sw)
   sw.Do(&m_enable_dma_out);
   sw.Do(&m_data_in_fifo);
   sw.Do(&m_data_out_fifo);
-  sw.Do(&m_command);
+  sw.Do(&m_state);
   sw.Do(&m_remaining_halfwords);
   sw.Do(&m_iq_uv);
   sw.Do(&m_iq_y);
@@ -91,7 +91,7 @@ void MDEC::WriteRegister(u32 offset, u32 value)
 
       m_enable_dma_in = cr.enable_dma_in;
       m_enable_dma_out = cr.enable_dma_out;
-      UpdateStatus();
+      Execute();
       return;
     }
 
@@ -105,35 +105,35 @@ void MDEC::WriteRegister(u32 offset, u32 value)
 
 void MDEC::DMARead(u32* words, u32 word_count)
 {
-  do
+  if (m_data_out_fifo.GetSize() < word_count)
   {
-    const u32 words_to_read = std::min(word_count, m_data_out_fifo.GetSize());
+    Log_WarningPrintf("Insufficient data in output FIFO (requested %u, have %u)", word_count,
-    if (words_to_read > 0)
+                      m_data_out_fifo.GetSize());
-    {
+  }
-      m_data_out_fifo.PopRange(words, words_to_read);
-      words += words_to_read;
-      word_count -= words_to_read;
-    }
-    else
-    {
-      UpdateStatus();
-      break;
-    }
 
-    ExecutePendingCommand();
+  const u32 words_to_read = std::min(word_count, m_data_out_fifo.GetSize());
-  } while (word_count > 0);
+  if (words_to_read > 0)
+  {
+    m_data_out_fifo.PopRange(words, words_to_read);
+    words += words_to_read;
+    word_count -= words_to_read;
+  }
+
+  Log_DebugPrintf("DMA read complete, %u bytes left", m_data_out_fifo.GetSize() * sizeof(u32));
+  if (m_data_out_fifo.IsEmpty())
+    Execute();
 }
 
 void MDEC::DMAWrite(const u32* words, u32 word_count)
 {
-  do
+  if (m_data_in_fifo.GetSpace() < (word_count * 2))
   {
-    const u32 halfwords_to_write = std::min(word_count * 2, m_data_in_fifo.GetSpace() & ~u32(2));
+    Log_WarningPrintf("Input FIFO overflow (writing %u, space %u)", word_count * 2, m_data_in_fifo.GetSpace());
-    m_data_in_fifo.PushRange(reinterpret_cast<const u16*>(words), halfwords_to_write);
+  }
-    words += halfwords_to_write / 2;
+
-    word_count -= halfwords_to_write / 2;
+  const u32 halfwords_to_write = std::min(word_count * 2, m_data_in_fifo.GetSpace() & ~u32(2));
-    ExecutePendingCommand();
+  m_data_in_fifo.PushRange(reinterpret_cast<const u16*>(words), halfwords_to_write);
-  } while (word_count > 0);
+  Execute();
 }
 
 bool MDEC::HasPendingBlockCopyOut() const
@@ -148,7 +148,7 @@ void MDEC::SoftReset()
   m_enable_dma_out = false;
   m_data_in_fifo.Clear();
   m_data_out_fifo.Clear();
-  m_command = Command::None;
+  m_state = State::Idle;
   m_remaining_halfwords = 0;
   m_current_block = 0;
   m_current_coefficient = 64;
@@ -157,22 +157,29 @@ void MDEC::SoftReset()
   UpdateStatus();
 }
 
+void MDEC::ResetDecoder()
+{
+  m_current_block = 0;
+  m_current_coefficient = 64;
+  m_current_q_scale = 0;
+}
+
 void MDEC::UpdateStatus()
 {
   m_status.data_out_fifo_empty = m_data_out_fifo.IsEmpty();
   m_status.data_in_fifo_full = m_data_in_fifo.IsFull();
 
-  m_status.command_busy = m_command != Command::None;
+  m_status.command_busy = (m_state != State::Idle);
   m_status.parameter_words_remaining = Truncate16((m_remaining_halfwords / 2) - 1);
   m_status.current_block = (m_current_block + 4) % NUM_BLOCKS;
 
   // we always want data in if it's enabled
-  const bool data_in_request = m_enable_dma_in && m_data_in_fifo.GetSpace() >= (32 * 2) && !m_data_out_fifo.IsFull();
+  const bool data_in_request = m_enable_dma_in && m_data_in_fifo.GetSpace() >= (32 * 2);
   m_status.data_in_request = data_in_request;
   m_dma->SetRequest(DMA::Channel::MDECin, data_in_request);
 
   // we only want to send data out if we have some in the fifo
-  const bool data_out_request = m_enable_dma_out && m_data_out_fifo.GetSize() >= 32;
+  const bool data_out_request = m_enable_dma_out && !m_data_out_fifo.IsEmpty();
   m_status.data_out_request = data_out_request;
   m_dma->SetRequest(DMA::Channel::MDECout, data_out_request);
 }
@@ -196,7 +203,7 @@ u32 MDEC::ReadDataRegister()
 
   const u32 value = m_data_out_fifo.Pop();
   if (m_data_out_fifo.IsEmpty())
-    ExecutePendingCommand();
+    Execute();
   else
     UpdateStatus();
 
@@ -210,126 +217,120 @@ void MDEC::WriteCommandRegister(u32 value)
   m_data_in_fifo.Push(Truncate16(value));
   m_data_in_fifo.Push(Truncate16(value >> 16));
 
-  ExecutePendingCommand();
+  Execute();
 }
 
-void MDEC::ExecutePendingCommand()
+void MDEC::Execute()
 {
-  if (HasPendingBlockCopyOut())
+  for (;;)
   {
-    // can't do anything while waiting
+    switch (m_state)
-    UpdateStatus();
-    return;
-  }
-
-  while (!m_data_in_fifo.IsEmpty())
-  {
-    DebugAssert(!HasPendingBlockCopyOut());
-
-    if (m_command == Command::None)
     {
-      // first word
+      case State::Idle:
-      const CommandWord cw{ZeroExtend32(m_data_in_fifo.Peek(0)) | (ZeroExtend32(m_data_in_fifo.Peek(1)) << 16)};
-      m_command = cw.command;
-      m_status.data_output_depth = cw.data_output_depth;
-      m_status.data_output_signed = cw.data_output_signed;
-      m_status.data_output_bit15 = cw.data_output_bit15;
-      m_data_in_fifo.Remove(2);
-      m_data_out_fifo.Clear();
-
-      u32 num_words;
-      switch (cw.command)
       {
-        case Command::DecodeMacroblock:
+        if (m_data_in_fifo.GetSize() < 2)
-          num_words = ZeroExtend32(cw.parameter_word_count.GetValue());
+          goto finished;
-          break;
 
-        case Command::SetIqTab:
+        // first word
-          num_words = 16 + (((cw.bits & 1) != 0) ? 16 : 0);
+        const CommandWord cw{ZeroExtend32(m_data_in_fifo.Peek(0)) | (ZeroExtend32(m_data_in_fifo.Peek(1)) << 16)};
-          break;
+        m_status.data_output_depth = cw.data_output_depth;
+        m_status.data_output_signed = cw.data_output_signed;
+        m_status.data_output_bit15 = cw.data_output_bit15;
+        m_data_in_fifo.Remove(2);
+        m_data_out_fifo.Clear();
 
-        case Command::SetScale:
+        u32 num_words;
-          num_words = 32;
+        State new_state;
-          break;
+        switch (cw.command)
+        {
+          case Command::DecodeMacroblock:
+            num_words = ZeroExtend32(cw.parameter_word_count.GetValue());
+            new_state = State::DecodingMacroblock;
+            break;
 
-        default:
+          case Command::SetIqTab:
-          Panic("Unknown command");
+            num_words = 16 + (((cw.bits & 1) != 0) ? 16 : 0);
-          num_words = 0;
+            new_state = State::SetIqTable;
-          break;
+            break;
+
+          case Command::SetScale:
+            num_words = 32;
+            new_state = State::SetScaleTable;
+            break;
+
+          default:
+            Panic("Unknown command");
+            num_words = 0;
+            new_state = State::Idle;
+            break;
+        }
+
+        Log_DebugPrintf("MDEC command: 0x%08X (%u, %u words in parameter, %u expected)", cw.bits,
+                        ZeroExtend32(static_cast<u8>(cw.command.GetValue())),
+                        ZeroExtend32(cw.parameter_word_count.GetValue()), num_words);
+
+        m_remaining_halfwords = num_words * 2;
+        m_state = new_state;
+        UpdateStatus();
+        continue;
       }
 
-      m_remaining_halfwords = num_words * 2;
+      case State::DecodingMacroblock:
-
-      Log_DebugPrintf("MDEC command: 0x%08X (%u, %u words in parameter, %u expected)", cw.bits,
-                      ZeroExtend32(static_cast<u8>(cw.command.GetValue())),
-                      ZeroExtend32(cw.parameter_word_count.GetValue()), num_words);
-    }
-
-    switch (m_command)
-    {
-      case Command::DecodeMacroblock:
       {
         if (HandleDecodeMacroblockCommand())
         {
-          // block decoded, waiting to copy out
+          // we should be writing out now
-          UpdateStatus();
+          Assert(m_state == State::WritingMacroblock);
-          return;
+          goto finished;
         }
 
-        // data needed
+        if (m_remaining_halfwords == 0 && m_current_block != NUM_BLOCKS)
-        if (m_remaining_halfwords == 0)
         {
-          // but no more data expected, abort command here
+          // expecting data, but nothing more will be coming. bail out
-          EndCommand();
+          ResetDecoder();
-        }
+          m_state = State::Idle;
-        else
+          continue;
-        {
-          // waiting for data
-          UpdateStatus();
         }
 
-        return;
+        goto finished;
       }
 
-      case Command::SetIqTab:
+      case State::WritingMacroblock:
+      {
+        // this gets executed via the event, so if we get here, wait.
+        goto finished;
+      }
+
+      case State::SetIqTable:
       {
         if (m_data_in_fifo.GetSize() < m_remaining_halfwords)
-        {
+          goto finished;
-          UpdateStatus();
-          return;
-        }
 
         HandleSetQuantTableCommand();
-        EndCommand();
+        m_state = State::Idle;
+        UpdateStatus();
+        continue;
       }
-      break;
 
-      case Command::SetScale:
+      case State::SetScaleTable:
       {
         if (m_data_in_fifo.GetSize() < m_remaining_halfwords)
-        {
+          goto finished;
-          UpdateStatus();
-          return;
-        }
 
         HandleSetScaleCommand();
-        EndCommand();
+        m_state = State::Idle;
+        UpdateStatus();
+        continue;
       }
-      break;
 
       default:
         UnreachableCode();
         return;
     }
   }
-}
 
-void MDEC::EndCommand()
+finished:
-{
+  // if we get here, it's because the FIFO is now empty
-  m_command = Command::None;
-  m_current_block = 0;
-  m_current_coefficient = 64;
-  m_current_q_scale = 0;
   UpdateStatus();
 }
 
@@ -343,23 +344,19 @@ bool MDEC::HandleDecodeMacroblockCommand()
 
 bool MDEC::DecodeMonoMacroblock()
 {
-  // sufficient space in output?
+  // TODO: This should guard the output not the input
-  if (m_status.data_output_depth == DataOutputDepth_4Bit)
+  if (!m_data_out_fifo.IsEmpty())
-  {
+    return false;
-    if (m_data_out_fifo.GetSpace() < (64 / 8))
-      return false;
-  }
-  else
-  {
-    if (m_data_out_fifo.GetSpace() < (64 / 4))
-      return false;
-  }
 
   if (!rl_decode_block(m_blocks[0].data(), m_iq_y.data()))
     return false;
 
   IDCT(m_blocks[0].data());
 
+  Log_DebugPrintf("Decoded mono macroblock, %u words remaining", m_remaining_halfwords / 2);
+  ResetDecoder();
+  m_state = State::WritingMacroblock;
+
   y_to_mono(m_blocks[0]);
 
   ScheduleBlockCopyOut(TICKS_PER_BLOCK);
@@ -370,18 +367,6 @@ bool MDEC::DecodeMonoMacroblock()
 
 bool MDEC::DecodeColoredMacroblock()
 {
-  // sufficient space in output?
-  if (m_status.data_output_depth == DataOutputDepth_24Bit)
-  {
-    if (m_data_out_fifo.GetSpace() < (256 - (256 / 4)))
-      return false;
-  }
-  else
-  {
-    if (m_data_out_fifo.GetSpace() < (256 / 2))
-      return false;
-  }
-
   for (; m_current_block < NUM_BLOCKS; m_current_block++)
   {
     if (!rl_decode_block(m_blocks[m_current_block].data(), (m_current_block >= 2) ? m_iq_y.data() : m_iq_uv.data()))
@@ -390,18 +375,21 @@ bool MDEC::DecodeColoredMacroblock()
     IDCT(m_blocks[m_current_block].data());
   }
 
+  if (!m_data_out_fifo.IsEmpty())
+    return false;
+
   // done decoding
-  m_current_block = 0;
   Log_DebugPrintf("Decoded colored macroblock, %u words remaining", m_remaining_halfwords / 2);
+  ResetDecoder();
+  m_state = State::WritingMacroblock;
 
   yuv_to_rgb(0, 0, m_blocks[0], m_blocks[1], m_blocks[2]);
   yuv_to_rgb(8, 0, m_blocks[0], m_blocks[1], m_blocks[3]);
   yuv_to_rgb(0, 8, m_blocks[0], m_blocks[1], m_blocks[4]);
   yuv_to_rgb(8, 8, m_blocks[0], m_blocks[1], m_blocks[5]);
-
-  ScheduleBlockCopyOut(TICKS_PER_BLOCK);
-
   m_total_blocks_decoded += 4;
+
+  ScheduleBlockCopyOut(TICKS_PER_BLOCK * 4);
   return true;
 }
 
@@ -410,16 +398,14 @@ void MDEC::ScheduleBlockCopyOut(TickCount ticks)
   DebugAssert(!HasPendingBlockCopyOut());
   Log_DebugPrintf("Scheduling block copy out in %d ticks", ticks);
 
-  m_block_copy_out_event->Schedule(TICKS_PER_BLOCK);
+  m_block_copy_out_event->Schedule(ticks);
 }
 
 void MDEC::CopyOutBlock()
 {
-  DebugAssert(m_command == Command::DecodeMacroblock);
+  Assert(m_state == State::WritingMacroblock);
   m_block_copy_out_event->Deactivate();
 
-  Log_DebugPrintf("Copying out block");
-
   switch (m_status.data_output_depth)
   {
     case DataOutputDepth_4Bit:
@@ -519,11 +505,12 @@ void MDEC::CopyOutBlock()
       break;
   }
 
+  Log_DebugPrintf("Block copied out, fifo size = %u (%u bytes)", m_data_out_fifo.GetSize(),
+                  m_data_out_fifo.GetSize() * sizeof(u32));
+
   // if we've copied out all blocks, command is complete
-  if (m_remaining_halfwords == 0)
+  m_state = (m_remaining_halfwords == 0) ? State::Idle : State::DecodingMacroblock;
-    EndCommand();
+  Execute();
-  else
-    ExecutePendingCommand();
 }
 
 static constexpr std::array<u8, 64> zigzag = {{0,  1,  5,  6,  14, 15, 27, 28, 2,  4,  7,  13, 16, 26, 29, 42,
@@ -714,15 +701,16 @@ void MDEC::DrawDebugStateWindow()
     return;
   }
 
-  static constexpr std::array<const char*, 4> command_names = {{"None", "Decode Macroblock", "SetIqTab", "SetScale"}};
+  static constexpr std::array<const char*, 5> state_names = {
+    {"None", "Decoding Macroblock", "Writing Macroblock", "SetIqTab", "SetScale"}};
   static constexpr std::array<const char*, 4> output_depths = {{"4-bit", "8-bit", "24-bit", "15-bit"}};
-  static constexpr std::array<const char*, 6> block_names = {{"Crblk", "Cbblk", "Y1", "Y2", "Y3", "Y4"}};
+  static constexpr std::array<const char*, 7> block_names = {{"Crblk", "Cbblk", "Y1", "Y2", "Y3", "Y4", "Output"}};
 
   ImGui::Text("Blocks Decoded: %u", m_total_blocks_decoded);
   ImGui::Text("Data-In FIFO Size: %u (%u bytes)", m_data_in_fifo.GetSize(), m_data_in_fifo.GetSize() * 4);
   ImGui::Text("Data-Out FIFO Size: %u (%u bytes)", m_data_out_fifo.GetSize(), m_data_out_fifo.GetSize() * 4);
   ImGui::Text("DMA Enable: %s%s", m_enable_dma_in ? "In " : "", m_enable_dma_out ? "Out" : "");
-  ImGui::Text("Current Command: %s", command_names[static_cast<u8>(m_command)]);
+  ImGui::Text("Current State: %s", state_names[static_cast<u8>(m_state)]);
   ImGui::Text("Current Block: %s", block_names[m_current_block]);
   ImGui::Text("Current Coefficient: %u", m_current_coefficient);
 

src/core/mdec.h

@@ -31,10 +31,10 @@ public:
   void DrawDebugStateWindow();
 
 private:
-  static constexpr u32 DATA_IN_FIFO_SIZE = 256 * 4;
+  static constexpr u32 DATA_IN_FIFO_SIZE = 1024;
-  static constexpr u32 DATA_OUT_FIFO_SIZE = 192 * 4;
+  static constexpr u32 DATA_OUT_FIFO_SIZE = 768;
   static constexpr u32 NUM_BLOCKS = 6;
-  static constexpr TickCount TICKS_PER_BLOCK = 3072;
+  static constexpr TickCount TICKS_PER_BLOCK = 3072 / 4;
 
   enum DataOutputDepth : u8
   {
@@ -52,6 +52,15 @@ private:
     SetScale = 3
   };
 
+  enum class State : u8
+  {
+    Idle,
+    DecodingMacroblock,
+    WritingMacroblock,
+    SetIqTable,
+    SetScaleTable
+  };
+
   union StatusRegister
   {
     u32 bits;
@@ -87,16 +96,15 @@ private:
     BitField<u32, u16, 0, 16> parameter_word_count;
   };
 
-  bool HasPendingCommand() const { return m_command != Command::None; }
   bool HasPendingBlockCopyOut() const;
 
   void SoftReset();
+  void ResetDecoder();
   void UpdateStatus();
 
   u32 ReadDataRegister();
   void WriteCommandRegister(u32 value);
-  void ExecutePendingCommand();
+  void Execute();
-  void EndCommand();
 
   bool HandleDecodeMacroblockCommand();
   void HandleSetQuantTableCommand();
@@ -124,7 +132,7 @@ private:
   // Even though the DMA is in words, we access the FIFO as halfwords.
   InlineFIFOQueue<u16, DATA_IN_FIFO_SIZE / sizeof(u16)> m_data_in_fifo;
   InlineFIFOQueue<u32, DATA_OUT_FIFO_SIZE / sizeof(u32)> m_data_out_fifo;
-  Command m_command = Command::None;
+  State m_state = State::Idle;
   u32 m_remaining_halfwords = 0;
 
   std::array<u8, 64> m_iq_uv{};