Core: Switch MTGS and SysThreadBase to WorkSema

common/PersistentThread.h

@@ -100,7 +100,7 @@ namespace Threading
 		uptr m_native_id; // typically an id, but implementing platforms can do whatever.
 		uptr m_native_handle; // typically a pointer/handle, but implementing platforms can do whatever.
 
-		Semaphore m_sem_event; // general wait event that's needed by most threads
+		WorkSema m_sem_event; // general wait event that's needed by most threads
 		Semaphore m_sem_startup; // startup sync tool
 		Mutex m_mtx_InThread; // used for canceling and closing threads in a deadlock-safe manner
 		MutexRecursive m_mtx_start; // used to lock the Start() code from starting simultaneous threads accidentally.

pcsx2/GS.h

@@ -329,15 +329,13 @@ public:
 	std::atomic<unsigned int> m_ReadPos;  // cur pos gs is reading from
 	std::atomic<unsigned int> m_WritePos; // cur pos ee thread is writing to
 
-	std::atomic<bool>	m_RingBufferIsBusy;
 	std::atomic<bool>	m_SignalRingEnable;
 	std::atomic<int>	m_SignalRingPosition;
 
 	std::atomic<int>	m_QueuedFrameCount;
 	std::atomic<bool>	m_VsyncSignalListener;
 
-	Mutex			m_mtx_RingBufferBusy;  // Is obtained while processing ring-buffer data
-	Mutex			m_mtx_RingBufferBusy2; // This one gets released on semaXGkick waiting...
+	Mutex			m_mtx_RingBufferBusy2; // Gets released on semaXGkick waiting...
 	Mutex			m_mtx_WaitGS;
 	Semaphore		m_sem_OnRingReset;
 	Semaphore		m_sem_Vsync;

pcsx2/MTGS.cpp

@@ -19,6 +19,7 @@
 #include <list>
 #include <wx/datetime.h>
 
+#include "common/ScopedGuard.h"
 #include "common/StringUtil.h"
 
 #include "GS.h"
@@ -76,7 +77,6 @@ void SysMtgsThread::OnStart()
 
 	m_ReadPos = 0;
 	m_WritePos = 0;
-	m_RingBufferIsBusy = false;
 	m_packet_size = 0;
 	m_packet_writepos = 0;
 
@@ -176,12 +176,6 @@ void SysMtgsThread::PostVsyncStart(bool registers_written)
 	m_VsyncSignalListener.store(true, std::memory_order_release);
 	//Console.WriteLn( Color_Blue, "(EEcore Sleep) Vsync\t\tringpos=0x%06x, writepos=0x%06x", m_ReadPos.load(), m_WritePos.load() );
 
-	// We will wait a vsync event from the MTGS ring. If the ring is already purged, the event will never come !
-	// To avoid this potential deadlock, ring must be wake up after m_VsyncSignalListener
-	// Note: potentially we can also miss the previous wake up if we optimize away the post just before the release of busy signal of the ring
-	// So let's ensure the ring doesn't sleep
-	m_sem_event.Post();
-
 	m_sem_Vsync.WaitNoCancel();
 }
 
@@ -219,46 +213,6 @@ void SysMtgsThread::OpenGS()
 	GSsetGameCRC(ElfCRC, 0);
 }
 
-class RingBufferLock
-{
-	ScopedLock m_lock1;
-	ScopedLock m_lock2;
-	SysMtgsThread& m_mtgs;
-
-public:
-	RingBufferLock(SysMtgsThread& mtgs)
-		: m_lock1(mtgs.m_mtx_RingBufferBusy)
-		, m_lock2(mtgs.m_mtx_RingBufferBusy2)
-		, m_mtgs(mtgs)
-	{
-		m_mtgs.m_RingBufferIsBusy.store(true, std::memory_order_relaxed);
-	}
-	virtual ~RingBufferLock()
-	{
-		m_mtgs.m_RingBufferIsBusy.store(false, std::memory_order_relaxed);
-	}
-	void Acquire()
-	{
-		m_lock1.Acquire();
-		m_lock2.Acquire();
-		m_mtgs.m_RingBufferIsBusy.store(true, std::memory_order_relaxed);
-	}
-	void Release()
-	{
-		m_mtgs.m_RingBufferIsBusy.store(false, std::memory_order_relaxed);
-		m_lock2.Release();
-		m_lock1.Release();
-	}
-	void PartialAcquire()
-	{
-		m_lock2.Acquire();
-	}
-	void PartialRelease()
-	{
-		m_lock2.Release();
-	}
-};
-
 void SysMtgsThread::ExecuteTaskInThread()
 {
 	// Threading info: run in MTGS thread
@@ -268,19 +222,22 @@ void SysMtgsThread::ExecuteTaskInThread()
 	PacketTagType prevCmd;
 #endif
 
-	RingBufferLock busy(*this);
+	ScopedGuard kill_on_exception([this]{ m_sem_event.Kill(); });
+	ScopedLock mtvu_lock(m_mtx_RingBufferBusy2);
 
 	while (true)
 	{
-		busy.Release();
 
 		// Performance note: Both of these perform cancellation tests, but pthread_testcancel
 		// is very optimized (only 1 instruction test in most cases), so no point in trying
 		// to avoid it.
 
-		m_sem_event.Wait();
+		m_mtx_RingBufferBusy2.Release();
+
+		m_sem_event.WaitForWork();
 		StateCheckInThread();
-		busy.Acquire();
+
+		m_mtx_RingBufferBusy2.Acquire();
 
 		// note: m_ReadPos is intentionally not volatile, because it should only
 		// ever be modified by this thread.
@@ -402,10 +359,10 @@ void SysMtgsThread::ExecuteTaskInThread()
 					MTVU_LOG("MTGS - Waiting on semaXGkick!");
 					if (!vu1Thread.semaXGkick.TryWait())
 					{
-						busy.PartialRelease();
+						mtvu_lock.Release();
 						// Wait for MTVU to complete vu1 program
 						vu1Thread.semaXGkick.WaitWithoutYield();
-						busy.PartialAcquire();
+						mtvu_lock.Acquire();
 					}
 					Gif_Path& path = gifUnit.gifPath[GIF_PATH_1];
 					GS_Packet gsPack = path.GetGSPacketMTVU(); // Get vu1 program's xgkick packet(s)
@@ -537,7 +494,7 @@ void SysMtgsThread::ExecuteTaskInThread()
 			}
 		}
 
-		busy.Release();
+		// TODO: With the new race-free WorkSema do we still need these?
 
 		// Safety valve in case standard signals fail for some reason -- this ensures the EEcore
 		// won't sleep the eternity, even if SignalRingPosition didn't reach 0 for some reason.
@@ -605,33 +562,43 @@ void SysMtgsThread::WaitGS(bool syncRegs, bool weakWait, bool isMTVU)
 		return;
 
 	Gif_Path& path = gifUnit.gifPath[GIF_PATH_1];
-	u32 startP1Packs = weakWait ? path.GetPendingGSPackets() : 0;
 
 	// Both m_ReadPos and m_WritePos can be relaxed as we only want to test if the queue is empty but
 	// we don't want to access the content of the queue
 
-	if (isMTVU || m_ReadPos.load(std::memory_order_relaxed) != m_WritePos.load(std::memory_order_relaxed))
-	{
 	SetEvent();
-		RethrowException();
-		for (;;)
-		{
 	if (weakWait)
-				m_mtx_RingBufferBusy2.Wait();
-			else
-				m_mtx_RingBufferBusy.Wait();
-			RethrowException();
-			if (!isMTVU && m_ReadPos.load(std::memory_order_relaxed) == m_WritePos.load(std::memory_order_relaxed))
-				break;
-			u32 curP1Packs = weakWait ? path.GetPendingGSPackets() : 0;
-			if (weakWait && ((startP1Packs - curP1Packs) || !curP1Packs))
-				break;
+	{
 		// On weakWait we will stop waiting on the MTGS thread if the
 		// MTGS thread has processed a vu1 xgkick packet, or is pending on
 		// its final vu1 xgkick packet (!curP1Packs)...
 		// Note: m_WritePos doesn't seem to have proper atomic write
 		// code, so reading it from the MTVU thread might be dangerous;
 		// hence it has been avoided...
+		u32 startP1Packs = path.GetPendingGSPackets();
+		if (startP1Packs)
+		{
+			while (true)
+			{
+				m_mtx_RingBufferBusy2.Wait();
+				RethrowException();
+				if (path.GetPendingGSPackets() != startP1Packs)
+					break;
+			}
+		}
+	}
+	else
+	{
+		if (!m_sem_event.WaitForEmpty())
+		{
+			// There's a small race here as the semaphore is killed before the exception is set
+			// Try a few times to recover the actual exception before throwing something more generic
+			for (int i = 0; i < 5; i++)
+			{
+				std::this_thread::yield();
+				RethrowException();
+			}
+			throw Exception::RuntimeError(std::runtime_error("MTGS Thread Died"));
 		}
 	}
 
@@ -647,9 +614,7 @@ void SysMtgsThread::WaitGS(bool syncRegs, bool weakWait, bool isMTVU)
 // For use in loops that wait on the GS thread to do certain things.
 void SysMtgsThread::SetEvent()
 {
-	if (!m_RingBufferIsBusy.load(std::memory_order_relaxed))
-		m_sem_event.Post();
-
+	m_sem_event.NotifyOfWork();
 	m_CopyDataTally = 0;
 }
 
@@ -677,7 +642,7 @@ void SysMtgsThread::SendDataPacket()
 	{
 		WaitGS();
 	}
-	else if (!m_RingBufferIsBusy.load(std::memory_order_relaxed))
+	else
 	{
 		m_CopyDataTally += m_packet_size;
 		if (m_CopyDataTally > 0x2000)
@@ -839,15 +804,12 @@ void SysMtgsThread::SendSimpleGSPacket(MTGS_RingCommand type, u32 offset, u32 si
 	SendSimplePacket(type, (int)offset, (int)size, (int)path);
 
 	if (!EmuConfig.GS.SynchronousMTGS)
-	{
-		if (!m_RingBufferIsBusy.load(std::memory_order_relaxed))
 	{
 		m_CopyDataTally += size / 16;
 		if (m_CopyDataTally > 0x2000)
 			SetEvent();
 	}
 }
-}
 
 void SysMtgsThread::SendPointerPacket(MTGS_RingCommand type, u32 data0, void* data1)
 {

pcsx2/System/SysCoreThread.cpp

@@ -316,7 +316,7 @@ void SysCoreThread::DoCpuExecute()
 void SysCoreThread::ExecuteTaskInThread()
 {
 	Threading::EnableHiresScheduler(); // Note that *something* in SPU2 and GS also set the timer resolution to 1ms.
-	m_sem_event.WaitWithoutYield();
+	m_sem_event.WaitForWork();
 
 	m_mxcsr_saved.bitmask = _mm_getcsr();
 

pcsx2/System/SysThreadBase.cpp

@@ -39,7 +39,7 @@ void SysThreadBase::Start()
 	pxAssertDev((m_ExecMode == ExecMode_Closing) || (m_ExecMode == ExecMode_Closed),
 				"Unexpected thread status during SysThread startup.");
 
-	m_sem_event.Post();
+	m_sem_event.NotifyOfWork();
 }
 
 
@@ -118,7 +118,7 @@ void SysThreadBase::Suspend(bool isBlocking)
 		}
 
 		pxAssertDev(m_ExecMode == ExecMode_Closing, "ExecMode should be nothing other than Closing...");
-		m_sem_event.Post();
+		m_sem_event.NotifyOfWork();
 	}
 
 	if (isBlocking)
@@ -156,7 +156,7 @@ void SysThreadBase::Pause(SystemsMask systemsToTearDown, bool debug)
 			OnPauseDebug();
 		else
 			OnPause();
-		m_sem_event.Post();
+		m_sem_event.NotifyOfWork();
 	}
 
 	m_RunningLock.Wait();
@@ -174,7 +174,7 @@ void SysThreadBase::PauseSelf()
 			m_ExecMode = ExecMode_Pausing;
 
 		OnPause();
-		m_sem_event.Post();
+		m_sem_event.NotifyOfWork();
 	}
 }
 
@@ -190,7 +190,7 @@ void SysThreadBase::PauseSelfDebug()
 			m_ExecMode = ExecMode_Pausing;
 
 		OnPauseDebug();
-		m_sem_event.Post();
+		m_sem_event.NotifyOfWork();
 	}
 }