all repos — mgba @ ac115422264c242439c19aaf383b6b743a309c69

mGBA Game Boy Advance Emulator

GBA Video: Another attempt at patching the threaded renderer race conditions
Jeffrey Pfau jeffrey@endrift.com
Sun, 14 Aug 2016 19:40:24 -0700
commit

ac115422264c242439c19aaf383b6b743a309c69

parent

812d063b73cf9231123e9600160d4f4d0eb7a8cf

4 files changed, 66 insertions(+), 34 deletions(-)

jump to
M src/gba/renderers/thread-proxy.c → src/gba/renderers/thread-proxy.c 
@@ -69,7 +69,7 @@ struct GBAVideoThreadProxyRenderer* proxyRenderer = (struct GBAVideoThreadProxyRenderer*) renderer;
 	ConditionInit(&proxyRenderer->fromThreadCond);
 	ConditionInit(&proxyRenderer->toThreadCond);
 	MutexInit(&proxyRenderer->mutex);
-	RingFIFOInit(&proxyRenderer->dirtyQueue, 0x48000, 0x1000);
+	RingFIFOInit(&proxyRenderer->dirtyQueue, 0x40000);
 
 	proxyRenderer->vramProxy = anonymousMemoryMap(SIZE_VRAM);
 	proxyRenderer->backend->palette = proxyRenderer->paletteProxy;

@@ -123,6 +123,23 @@
 	mappedMemoryFree(proxyRenderer->vramProxy, SIZE_VRAM);
 }
 
+static bool _writeData(struct GBAVideoThreadProxyRenderer* proxyRenderer, void* data, size_t length) {
+	while (!RingFIFOWrite(&proxyRenderer->dirtyQueue, data, length)) {
+		mLOG(GBA_VIDEO, WARN, "Can't write 0x%z bytes. Proxy thread asleep?", length);
+		mLOG(GBA_VIDEO, DEBUG, "Queue status: read: %p, write: %p", proxyRenderer->dirtyQueue.readPtr, proxyRenderer->dirtyQueue.writePtr);
+		MutexLock(&proxyRenderer->mutex);
+		if (proxyRenderer->threadState == PROXY_THREAD_STOPPED) {
+			mLOG(GBA_VIDEO, ERROR, "Proxy thread stopped prematurely!");
+			MutexUnlock(&proxyRenderer->mutex);
+			return false;
+		}
+		ConditionWake(&proxyRenderer->toThreadCond);
+		ConditionWait(&proxyRenderer->fromThreadCond, &proxyRenderer->mutex);
+		MutexUnlock(&proxyRenderer->mutex);
+	}
+	return true;
+}
+
 uint16_t GBAVideoThreadProxyRendererWriteVideoRegister(struct GBAVideoRenderer* renderer, uint32_t address, uint16_t value) {
 	struct GBAVideoThreadProxyRenderer* proxyRenderer = (struct GBAVideoThreadProxyRenderer*) renderer;
 	switch (address) {

@@ -153,7 +170,7 @@ address,
 		value,
 		0xDEADBEEF,
 	};
-	RingFIFOWrite(&proxyRenderer->dirtyQueue, &dirty, sizeof(dirty));
+	_writeData(proxyRenderer, &dirty, sizeof(dirty));
 	return value;
 }
 

@@ -174,7 +191,7 @@ address,
 		value,
 		0xDEADBEEF,
 	};
-	RingFIFOWrite(&proxyRenderer->dirtyQueue, &dirty, sizeof(dirty));
+	_writeData(proxyRenderer, &dirty, sizeof(dirty));
 	if (renderer->cache) {
 		GBAVideoTileCacheWritePalette(renderer->cache, address);
 	}

@@ -188,7 +205,7 @@ oam,
 		proxyRenderer->d.oam->raw[oam],
 		0xDEADBEEF,
 	};
-	RingFIFOWrite(&proxyRenderer->dirtyQueue, &dirty, sizeof(dirty));
+	_writeData(proxyRenderer, &dirty, sizeof(dirty));
 }
 
 void GBAVideoThreadProxyRendererDrawScanline(struct GBAVideoRenderer* renderer, int y) {

@@ -207,14 +224,8 @@ j * 0x1000,
 				0xABCD,
 				0xDEADBEEF,
 			};
-			RingFIFOWrite(&proxyRenderer->dirtyQueue, &dirty, sizeof(dirty));
-			while (!RingFIFOWrite(&proxyRenderer->dirtyQueue, &proxyRenderer->d.vram[j * 0x800], 0x1000)) {
-				ConditionWake(&proxyRenderer->toThreadCond);
-				if (proxyRenderer->threadState == PROXY_THREAD_STOPPED) {
-					mLOG(GBA_VIDEO, FATAL, "Proxy thread stopped prematurely!");
-					return;
-				}
-			}
+			_writeData(proxyRenderer, &dirty, sizeof(dirty));
+			_writeData(proxyRenderer, &proxyRenderer->d.vram[j * 0x800], 0x1000);
 		}
 	}
 	struct GBAVideoDirtyInfo dirty = {

@@ -223,7 +234,7 @@ y,
 		0,
 		0xDEADBEEF,
 	};
-	RingFIFOWrite(&proxyRenderer->dirtyQueue, &dirty, sizeof(dirty));
+	_writeData(proxyRenderer, &dirty, sizeof(dirty));
 	if ((y & 15) == 15) {
 		ConditionWake(&proxyRenderer->toThreadCond);
 	}

@@ -232,7 +243,9 @@
 void GBAVideoThreadProxyRendererFinishFrame(struct GBAVideoRenderer* renderer) {
 	struct GBAVideoThreadProxyRenderer* proxyRenderer = (struct GBAVideoThreadProxyRenderer*) renderer;
 	if (proxyRenderer->threadState == PROXY_THREAD_STOPPED) {
-		mLOG(GBA_VIDEO, FATAL, "Proxy thread stopped prematurely!");
+		mLOG(GBA_VIDEO, ERROR, "Proxy thread stopped prematurely!");
+		GBAVideoThreadProxyRendererDeinit(renderer);
+		GBAVideoThreadProxyRendererInit(renderer);
 		return;
 	}
 	MutexLock(&proxyRenderer->mutex);

@@ -243,9 +256,8 @@ 0,
 		0,
 		0xDEADBEEF,
 	};
-	RingFIFOWrite(&proxyRenderer->dirtyQueue, &dirty, sizeof(dirty));
+	_writeData(proxyRenderer, &dirty, sizeof(dirty));
 	do {
-		RingFIFOWrite(&proxyRenderer->dirtyQueue, &dirty, sizeof(dirty));
 		ConditionWake(&proxyRenderer->toThreadCond);
 		ConditionWait(&proxyRenderer->fromThreadCond, &proxyRenderer->mutex);
 	} while (proxyRenderer->threadState == PROXY_THREAD_BUSY);

@@ -255,7 +267,8 @@ MutexUnlock(&proxyRenderer->mutex);
 }
 
 static void GBAVideoThreadProxyRendererGetPixels(struct GBAVideoRenderer* renderer, unsigned* stride, const void** pixels) {
-	struct GBAVideoThreadProxyRenderer* proxyRenderer = (struct GBAVideoThreadProxyRenderer*) renderer;	MutexLock(&proxyRenderer->mutex);
+	struct GBAVideoThreadProxyRenderer* proxyRenderer = (struct GBAVideoThreadProxyRenderer*) renderer;
+	MutexLock(&proxyRenderer->mutex);
 	// Insert an extra item into the queue to make sure it gets flushed
 	struct GBAVideoDirtyInfo dirty = {
 		DIRTY_FLUSH,

@@ -263,7 +276,7 @@ 0,
 		0,
 		0xDEADBEEF,
 	};
-	RingFIFOWrite(&proxyRenderer->dirtyQueue, &dirty, sizeof(dirty));
+	_writeData(proxyRenderer, &dirty, sizeof(dirty));
 	while (proxyRenderer->threadState == PROXY_THREAD_BUSY) {
 		ConditionWake(&proxyRenderer->toThreadCond);
 		ConditionWait(&proxyRenderer->fromThreadCond, &proxyRenderer->mutex);

@@ -273,7 +286,8 @@ MutexUnlock(&proxyRenderer->mutex);
 }
 
 static void GBAVideoThreadProxyRendererPutPixels(struct GBAVideoRenderer* renderer, unsigned stride, void* pixels) {
-	struct GBAVideoThreadProxyRenderer* proxyRenderer = (struct GBAVideoThreadProxyRenderer*) renderer;	MutexLock(&proxyRenderer->mutex);
+	struct GBAVideoThreadProxyRenderer* proxyRenderer = (struct GBAVideoThreadProxyRenderer*) renderer;
+	MutexLock(&proxyRenderer->mutex);
 	// Insert an extra item into the queue to make sure it gets flushed
 	struct GBAVideoDirtyInfo dirty = {
 		DIRTY_FLUSH,

@@ -281,7 +295,7 @@ 0,
 		0,
 		0xDEADBEEF,
 	};
-	RingFIFOWrite(&proxyRenderer->dirtyQueue, &dirty, sizeof(dirty));
+	_writeData(proxyRenderer, &dirty, sizeof(dirty));
 	while (proxyRenderer->threadState == PROXY_THREAD_BUSY) {
 		ConditionWake(&proxyRenderer->toThreadCond);
 		ConditionWait(&proxyRenderer->fromThreadCond, &proxyRenderer->mutex);

@@ -319,7 +333,13 @@ proxyRenderer->backend->writeOAM(proxyRenderer->backend, item.address);
 					break;
 				case DIRTY_VRAM:
 					while (!RingFIFORead(&proxyRenderer->dirtyQueue, &proxyRenderer->vramProxy[item.address >> 1], 0x1000)) {
+						mLOG(GBA_VIDEO, WARN, "Proxy thread can't read VRAM. CPU thread asleep?");
+						MutexLock(&proxyRenderer->mutex);
 						ConditionWake(&proxyRenderer->fromThreadCond);
+						proxyRenderer->threadState = PROXY_THREAD_IDLE;
+						ConditionWait(&proxyRenderer->toThreadCond, &proxyRenderer->mutex);
+						proxyRenderer->threadState = PROXY_THREAD_BUSY;
+						MutexUnlock(&proxyRenderer->mutex);
 					}
 					proxyRenderer->backend->writeVRAM(proxyRenderer->backend, item.address);
 					break;

@@ -333,7 +353,7 @@ default:
 					// FIFO was corrupted
 					MutexLock(&proxyRenderer->mutex);
 					proxyRenderer->threadState = PROXY_THREAD_STOPPED;
-					mLOG(GBA_VIDEO, FATAL, "Proxy thread queue got corrupted!");
+					mLOG(GBA_VIDEO, ERROR, "Proxy thread queue got corrupted!");
 					goto out;
 				}
 			} while (proxyRenderer->threadState == PROXY_THREAD_BUSY && RingFIFORead(&proxyRenderer->dirtyQueue, &item, sizeof(item)));
M src/platform/psp2/psp2-context.c → src/platform/psp2/psp2-context.c 
@@ -233,7 +233,7 @@ default:
 		break;
 	}
 
-	RingFIFOInit(&audioContext.buffer, PSP2_AUDIO_BUFFER_SIZE * sizeof(struct GBAStereoSample), PSP2_SAMPLES * 4);
+	RingFIFOInit(&audioContext.buffer, PSP2_AUDIO_BUFFER_SIZE * sizeof(struct GBAStereoSample));
 	MutexInit(&audioContext.mutex);
 	ConditionInit(&audioContext.cond);
 	audioContext.running = true;
M src/util/ring-fifo.c → src/util/ring-fifo.c 
@@ -16,10 +16,9 @@ #define ATOMIC_STORE(DST, SRC) DST = SRC
 #define ATOMIC_LOAD(DST, SRC) DST = SRC
 #endif
 
-void RingFIFOInit(struct RingFIFO* buffer, size_t capacity, size_t maxalloc) {
+void RingFIFOInit(struct RingFIFO* buffer, size_t capacity) {
 	buffer->data = anonymousMemoryMap(capacity);
 	buffer->capacity = capacity;
-	buffer->maxalloc = maxalloc;
 	RingFIFOClear(buffer);
 }
 

@@ -41,15 +40,24 @@ size_t RingFIFOWrite(struct RingFIFO* buffer, const void* value, size_t length) {
 	void* data = buffer->writePtr;
 	void* end;
 	ATOMIC_LOAD(end, buffer->readPtr);
-	size_t remaining;
-	if ((intptr_t) data - (intptr_t) buffer->data + buffer->maxalloc >= buffer->capacity) {
+
+	// Wrap around if we can't fit enough in here
+	if ((intptr_t) data - (intptr_t) buffer->data + length >= buffer->capacity) {
+		if (end == buffer->data) {
+			// Oops! If we wrap now, it'll appear empty
+			return 0;
+		}
 		data = buffer->data;
 	}
+
+	size_t remaining;
 	if (data >= end) {
-		remaining = (intptr_t) buffer->data + buffer->capacity - (intptr_t) data;
+		uintptr_t bufferEnd = (uintptr_t) buffer->data + buffer->capacity;
+		remaining = bufferEnd - (uintptr_t) data;
 	} else {
-		remaining = (intptr_t) end - (intptr_t) data;
+		remaining = (uintptr_t) end - (uintptr_t) data;
 	}
+	// Note that we can't hit the end pointer
 	if (remaining <= length) {
 		return 0;
 	}

@@ -64,16 +72,21 @@ size_t RingFIFORead(struct RingFIFO* buffer, void* output, size_t length) {
 	void* data = buffer->readPtr;
 	void* end;
 	ATOMIC_LOAD(end, buffer->writePtr);
-	size_t remaining;
-	if ((intptr_t) data - (intptr_t) buffer->data + buffer->maxalloc >= buffer->capacity) {
+
+	// Wrap around if we can't fit enough in here
+	if ((intptr_t) data - (intptr_t) buffer->data + length >= buffer->capacity) {
 		data = buffer->data;
 	}
+
+	size_t remaining;
 	if (data > end) {
-		remaining = (intptr_t) buffer->data + buffer->capacity - (intptr_t) data;
+		uintptr_t bufferEnd = (uintptr_t) buffer->data + buffer->capacity;
+		remaining = bufferEnd - (uintptr_t) data;
 	} else {
 		remaining = (intptr_t) end - (intptr_t) data;
 	}
-	if (remaining <= length) {
+	// If the pointers touch, it's empty
+	if (remaining < length) {
 		return 0;
 	}
 	if (output) {
M src/util/ring-fifo.h → src/util/ring-fifo.h 
@@ -11,12 +11,11 @@
 struct RingFIFO {
 	void* data;
 	size_t capacity;
-	size_t maxalloc;
 	void* readPtr;
 	void* writePtr;
 };
 
-void RingFIFOInit(struct RingFIFO* buffer, size_t capacity, size_t maxalloc);
+void RingFIFOInit(struct RingFIFO* buffer, size_t capacity);
 void RingFIFODeinit(struct RingFIFO* buffer);
 size_t RingFIFOCapacity(const struct RingFIFO* buffer);
 void RingFIFOClear(struct RingFIFO* buffer);