[XMA] Fix dedicated thread stalls by blocking guest audio thread

Includes various other thread safety and timing related adjustments

src/xenia/apu/xma_context.cc

@@ -35,7 +35,9 @@ extern "C" {
 namespace xe {
 namespace apu {
 
-XmaContext::XmaContext() = default;
+XmaContext::XmaContext()
+    : work_completion_event_(
+          xe::threading::Event::CreateAutoResetEvent(false)) {}
 
 XmaContext::~XmaContext() {}
 

src/xenia/apu/xma_context.h

@@ -15,6 +15,7 @@
 #include <mutex>
 #include <queue>
 
+#include "xenia/base/threading.h"
 #include "xenia/memory.h"
 #include "xenia/xbox.h"
 
@@ -212,11 +213,28 @@ class XmaContext {
 
   uint32_t id() { return id_; }
   uint32_t guest_ptr() { return guest_ptr_; }
-  bool is_allocated() { return is_allocated_; }
+  bool is_allocated() { return is_allocated_.load(std::memory_order_acquire); }
-  bool is_enabled() { return is_enabled_; }
+  bool is_enabled() { return is_enabled_.load(std::memory_order_acquire); }
 
-  void set_is_allocated(bool is_allocated) { is_allocated_ = is_allocated; }
+  void set_is_allocated(bool is_allocated) {
-  void set_is_enabled(bool is_enabled) { is_enabled_ = is_enabled; }
+    is_allocated_.store(is_allocated, std::memory_order_release);
+  }
+  void set_is_enabled(bool is_enabled) {
+    is_enabled_.store(is_enabled, std::memory_order_release);
+  }
+
+  // Signals that the worker has finished processing this context after a kick.
+  void SignalWorkDone() {
+    if (work_completion_event_) {
+      work_completion_event_->Set();
+    }
+  }
+  // Blocks until the worker has finished processing this context.
+  void WaitForWorkDone() {
+    if (work_completion_event_) {
+      xe::threading::Wait(work_completion_event_.get(), false);
+    }
+  }
 
  protected:
   static void DumpRaw(AVFrame* frame, int id);
@@ -229,8 +247,9 @@ class XmaContext {
   uint32_t id_ = 0;
   uint32_t guest_ptr_ = 0;
   xe_mutex lock_;
-  volatile bool is_allocated_ = false;
+  std::atomic<bool> is_allocated_ = false;
-  volatile bool is_enabled_ = false;
+  std::atomic<bool> is_enabled_ = false;
+  std::unique_ptr<xe::threading::Event> work_completion_event_;
 
   // ffmpeg structures
   AVPacket* av_packet_ = nullptr;

src/xenia/apu/xma_context_new.cc

@@ -121,6 +121,7 @@ bool XmaContextNew::Work() {
 
   auto context_ptr = memory()->TranslateVirtual(guest_ptr());
   XMA_CONTEXT_DATA data(context_ptr);
+  const XMA_CONTEXT_DATA initial_data = data;
 
   if (!data.output_buffer_valid) {
     return true;
@@ -142,7 +143,7 @@ bool XmaContextNew::Work() {
         data.output_buffer_read_offset == data.output_buffer_write_offset) {
       ClearLocked(&data);
     }
-    data.Store(context_ptr);
+    StoreContextMerged(data, initial_data, context_ptr);
     return true;
   }
 
@@ -156,7 +157,7 @@ bool XmaContextNew::Work() {
     XELOGD("XmaContext {}: No space for subframe decoding {}/{}!", id(),
            minimum_subframe_decode_count,
            remaining_subframe_blocks_in_output_buffer_);
-    data.Store(context_ptr);
+    StoreContextMerged(data, initial_data, context_ptr);
     return true;
   }
 
@@ -192,26 +193,11 @@ bool XmaContextNew::Work() {
     data.output_buffer_valid = 0;
   }
 
-  // TODO: Rewrite!
+  StoreContextMerged(data, initial_data, context_ptr);
-  // There is a case when game can modify certain parts of context mid-play
-  // and decoder should be aware of it
-  data.Store(context_ptr);
   return true;
 }
 
-void XmaContextNew::Enable() {
+void XmaContextNew::Enable() { set_is_enabled(true); }
-  std::lock_guard<xe_mutex> lock(lock_);
-
-  auto context_ptr = memory()->TranslateVirtual(guest_ptr());
-  XMA_CONTEXT_DATA data(context_ptr);
-
-  XELOGAPU("XmaContext: kicking context {} (buffer {} {}/{} bits)", id(),
-           data.current_buffer, data.input_buffer_read_offset,
-           data.GetCurrentInputBufferPacketCount() * kBitsPerPacket);
-
-  data.Store(context_ptr);
-  set_is_enabled(true);
-}
 
 bool XmaContextNew::Block(bool poll) {
   if (!lock_.try_lock()) {
@@ -247,16 +233,12 @@ void XmaContextNew::ClearLocked(XMA_CONTEXT_DATA* data) {
   current_frame_remaining_subframes_ = 0;
 }
 
-void XmaContextNew::Disable() {
+void XmaContextNew::Disable() { set_is_enabled(false); }
-  std::lock_guard<xe_mutex> lock(lock_);
-  XELOGAPU("XmaContext: disabling context {}", id());
-  set_is_enabled(false);
-}
 
 void XmaContextNew::Release() {
   // Lock it in case the decoder thread is working on it now.
   std::lock_guard<xe_mutex> lock(lock_);
-  assert_true(is_allocated_ == true);
+  assert_true(is_allocated());
 
   set_is_allocated(false);
   auto context_ptr = memory()->TranslateVirtual(guest_ptr());
@@ -757,5 +739,36 @@ bool XmaContextNew::DecodePacket(AVCodecContext* av_context,
   return true;
 }
 
+void XmaContextNew::StoreContextMerged(const XMA_CONTEXT_DATA& data,
+                                       const XMA_CONTEXT_DATA& initial_data,
+                                       uint8_t* context_ptr) {
+  XMA_CONTEXT_DATA fresh(context_ptr);
+
+  // DWORD 0: decoder owns loop_count, output_buffer_write_offset.
+  // Only clear valid flags the decoder actually consumed (was 1, now 0).
+  fresh.loop_count = data.loop_count;
+  fresh.output_buffer_write_offset = data.output_buffer_write_offset;
+  if (initial_data.input_buffer_0_valid && !data.input_buffer_0_valid) {
+    fresh.input_buffer_0_valid = 0;
+  }
+  if (initial_data.input_buffer_1_valid && !data.input_buffer_1_valid) {
+    fresh.input_buffer_1_valid = 0;
+  }
+
+  // DWORD 1: decoder conditionally clears output_buffer_valid
+  if (initial_data.output_buffer_valid && !data.output_buffer_valid) {
+    fresh.output_buffer_valid = 0;
+  }
+
+  // DWORD 2: decoder owns input_buffer_read_offset, error_status
+  fresh.input_buffer_read_offset = data.input_buffer_read_offset;
+  fresh.error_status = data.error_status;
+
+  // DWORD 4: decoder owns current_buffer
+  fresh.current_buffer = data.current_buffer;
+
+  fresh.Store(context_ptr);
+}
+
 }  // namespace apu
 }  // namespace xe

src/xenia/apu/xma_context_new.h

@@ -99,6 +99,12 @@ class XmaContextNew : public XmaContext {
   bool DecodePacket(AVCodecContext* av_context, const AVPacket* av_packet,
                     AVFrame* av_frame);
 
+  // Re-reads context from guest memory and merges only decoder-owned fields,
+  // preserving any game modifications made during decoding.
+  void StoreContextMerged(const XMA_CONTEXT_DATA& data,
+                          const XMA_CONTEXT_DATA& initial_data,
+                          uint8_t* context_ptr);
+
   std::array<uint8_t, kBytesPerPacketData * 2> input_buffer_;
   // first byte contains bit offset information
   std::array<uint8_t, 1 + 4096> xma_frame_;

src/xenia/apu/xma_decoder.cc

@@ -193,17 +193,15 @@ X_STATUS XmaDecoder::Setup(kernel::KernelState* kernel_state) {
 }
 
 void XmaDecoder::WorkerThreadMain() {
-  uint32_t idle_loop_count = 0;
   while (worker_running_) {
     // Okay, let's loop through XMA contexts to find ones we need to decode!
     bool did_work = false;
     for (uint32_t n = 0; n < kContextCount; n++) {
-      did_work = contexts_[n]->Work() || did_work;
+      bool worked = contexts_[n]->Work();
-
+      if (worked) {
-      // TODO: Need thread safety to do this.
+        contexts_[n]->SignalWorkDone();
-      // Probably not too important though.
+      }
-      // registers_.current_context = n;
+      did_work = did_work || worked;
-      // registers_.next_context = (n + 1) % kContextCount;
     }
 
     if (paused_) {
@@ -211,10 +209,8 @@ void XmaDecoder::WorkerThreadMain() {
       resume_fence_.Wait();
     }
 
-    if (!did_work) {
+    if (did_work) {
-      idle_loop_count++;
+      continue;
-    } else {
-      idle_loop_count = 0;
     }
     xe::threading::Wait(work_event_.get(), false);
   }
@@ -340,6 +336,7 @@ void XmaDecoder::WriteRegister(uint32_t addr, uint32_t value) {
 
     // The context ID is a bit in the range of the entire context array.
     const uint32_t base_context_id = (r - XmaRegister::Context0Kick) * 32;
+    const uint32_t kicked_value = value;
     while (value) {
       const uint32_t context_id = base_context_id + std::countr_zero(value);
       auto& context = *contexts_[context_id];
@@ -351,6 +348,16 @@ void XmaDecoder::WriteRegister(uint32_t addr, uint32_t value) {
     }
     // Signal the decoder thread to start processing.
     work_event_->SetBoostPriority();
+    if (cvars::use_dedicated_xma_thread) {
+      // Block until the worker finishes, so the game sees updated context data.
+      uint32_t remaining = kicked_value;
+      while (remaining) {
+        const uint32_t context_id =
+            base_context_id + std::countr_zero(remaining);
+        contexts_[context_id]->WaitForWorkDone();
+        remaining &= remaining - 1;
+      }
+    }
   } else if (r >= XmaRegister::Context0Lock && r <= XmaRegister::Context9Lock) {
     // Context lock command.
     // This requests a lock by flagging the context.
@@ -360,11 +367,10 @@ void XmaDecoder::WriteRegister(uint32_t addr, uint32_t value) {
       const uint32_t context_id = base_context_id + std::countr_zero(value);
       auto& context = *contexts_[context_id];
       context.Disable();
+      // Ensure the worker isn't mid-processing this context.
+      context.Block(false);
       value &= value - 1;
     }
-
-    // Signal the decoder thread to start processing.
-    // work_event_->Set();
   } else if (r >= XmaRegister::Context0Clear &&
              r <= XmaRegister::Context9Clear) {
     // Context clear command.

src/xenia/apu/xma_decoder.h

@@ -47,7 +47,7 @@ class XmaDecoder {
   uint32_t ReadRegister(uint32_t addr);
   void WriteRegister(uint32_t addr, uint32_t value);
 
-  bool is_paused() const { return paused_; }
+  bool is_paused() const { return paused_.load(std::memory_order_acquire); }
   void Pause();
   void Resume();
 
@@ -74,7 +74,7 @@ class XmaDecoder {
   kernel::object_ref<kernel::XHostThread> worker_thread_;
   std::unique_ptr<xe::threading::Event> work_event_ = nullptr;
 
-  bool paused_ = false;
+  std::atomic<bool> paused_ = false;
   xe::threading::Fence pause_fence_;   // Signaled when worker paused.
   xe::threading::Fence resume_fence_;  // Signaled when resume requested.