From 9e00ce097366b1bea0a016243eafd30873963f1c Mon Sep 17 00:00:00 2001 From: Sasha Szpakowski Date: Sun, 10 Mar 2024 13:38:28 -0300 Subject: [PATCH] metal: fix uniform data corruption The wrong buffer binding offset was being used in 2/3rds of frames. --- src/modules/graphics/StreamBuffer.h | 2 ++ src/modules/graphics/metal/Graphics.h | 1 + src/modules/graphics/metal/Graphics.mm | 21 +++++++++++++++----- src/modules/graphics/metal/StreamBuffer.mm | 5 +++++ src/modules/graphics/opengl/StreamBuffer.cpp | 15 ++++++++++++++ src/modules/graphics/vulkan/StreamBuffer.cpp | 5 +++++ 6 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/modules/graphics/StreamBuffer.h b/src/modules/graphics/StreamBuffer.h index d016ce5bf..ceaa87f94 100644 --- a/src/modules/graphics/StreamBuffer.h +++ b/src/modules/graphics/StreamBuffer.h @@ -57,6 +57,8 @@ public: BufferUsage getMode() const { return mode; } size_t getUsableSize() const { return bufferSize - frameGPUReadOffset; } + virtual size_t getGPUReadOffset() const = 0; + virtual MapInfo map(size_t minsize) = 0; virtual size_t unmap(size_t usedsize) = 0; virtual void markUsed(size_t usedsize) = 0; diff --git a/src/modules/graphics/metal/Graphics.h b/src/modules/graphics/metal/Graphics.h index 4a7ebf219..41aa71be9 100644 --- a/src/modules/graphics/metal/Graphics.h +++ b/src/modules/graphics/metal/Graphics.h @@ -242,6 +242,7 @@ private: StreamBuffer *uniformBuffer; StreamBuffer::MapInfo uniformBufferData; size_t uniformBufferOffset; + size_t uniformBufferGPUStart; Buffer *defaultAttributesBuffer; diff --git a/src/modules/graphics/metal/Graphics.mm b/src/modules/graphics/metal/Graphics.mm index 9ef7230dc..ecad1ea24 100644 --- a/src/modules/graphics/metal/Graphics.mm +++ b/src/modules/graphics/metal/Graphics.mm @@ -280,6 +280,7 @@ Graphics::Graphics() , attachmentStoreActions() , renderBindings() , uniformBufferOffset(0) + , uniformBufferGPUStart(0) , defaultAttributesBuffer(nullptr) , families() { @autoreleasepool { @@ -1035,14 +1036,19 @@ bool Graphics::applyShaderUniforms(id encoder, love::g if (uniformBuffer->getSize() < uniformBufferOffset + size) { size_t newsize = uniformBuffer->getSize() * 2; + if (uniformBufferOffset > 0) + uniformBuffer->nextFrame(); uniformBuffer->release(); - uniformBuffer = CreateStreamBuffer(device, BUFFERUSAGE_VERTEX, newsize); + uniformBuffer = CreateStreamBuffer(device, BUFFERUSAGE_UNIFORM, newsize); uniformBufferData = {}; uniformBufferOffset = 0; } if (uniformBufferData.data == nullptr) + { uniformBufferData = uniformBuffer->map(uniformBuffer->getSize()); + uniformBufferGPUStart = uniformBuffer->getGPUReadOffset(); + } memcpy(uniformBufferData.data + uniformBufferOffset, bufferdata, size); @@ -1050,7 +1056,7 @@ bool Graphics::applyShaderUniforms(id encoder, love::g int uniformindex = Shader::getUniformBufferBinding(); auto &bindings = renderBindings; - setBuffer(encoder, bindings, uniformindex, buffer, uniformBufferOffset); + setBuffer(encoder, bindings, uniformindex, buffer, uniformBufferGPUStart + uniformBufferOffset); uniformBufferOffset += alignUp(size, alignment); @@ -1141,14 +1147,19 @@ void Graphics::applyShaderUniforms(id renderEncoder, lo if (uniformBuffer->getSize() < uniformBufferOffset + size) { size_t newsize = uniformBuffer->getSize() * 2; + if (uniformBufferOffset > 0) + uniformBuffer->nextFrame(); uniformBuffer->release(); - uniformBuffer = CreateStreamBuffer(device, BUFFERUSAGE_VERTEX, newsize); + uniformBuffer = CreateStreamBuffer(device, BUFFERUSAGE_UNIFORM, newsize); uniformBufferData = {}; uniformBufferOffset = 0; } if (uniformBufferData.data == nullptr) + { uniformBufferData = uniformBuffer->map(uniformBuffer->getSize()); + uniformBufferGPUStart = uniformBuffer->getGPUReadOffset(); + } memcpy(uniformBufferData.data + uniformBufferOffset, bufferdata, size); @@ -1156,8 +1167,8 @@ void Graphics::applyShaderUniforms(id renderEncoder, lo int uniformindex = Shader::getUniformBufferBinding(); auto &bindings = renderBindings; - setBuffer(renderEncoder, bindings, SHADERSTAGE_VERTEX, uniformindex, buffer, uniformBufferOffset); - setBuffer(renderEncoder, bindings, SHADERSTAGE_PIXEL, uniformindex, buffer, uniformBufferOffset); + setBuffer(renderEncoder, bindings, SHADERSTAGE_VERTEX, uniformindex, buffer, uniformBufferGPUStart + uniformBufferOffset); + setBuffer(renderEncoder, bindings, SHADERSTAGE_PIXEL, uniformindex, buffer, uniformBufferGPUStart + uniformBufferOffset); uniformBufferOffset += alignUp(size, alignment); diff --git a/src/modules/graphics/metal/StreamBuffer.mm b/src/modules/graphics/metal/StreamBuffer.mm index 857fdb07d..00d3e33c9 100644 --- a/src/modules/graphics/metal/StreamBuffer.mm +++ b/src/modules/graphics/metal/StreamBuffer.mm @@ -67,6 +67,11 @@ public: } }} + size_t getGPUReadOffset() const override + { + return (frameIndex * bufferSize) + frameGPUReadOffset; + } + MapInfo map(size_t /*minsize*/) override { // Make sure this frame's section of the buffer is done being used. diff --git a/src/modules/graphics/opengl/StreamBuffer.cpp b/src/modules/graphics/opengl/StreamBuffer.cpp index c5919cdd7..9317ead3a 100644 --- a/src/modules/graphics/opengl/StreamBuffer.cpp +++ b/src/modules/graphics/opengl/StreamBuffer.cpp @@ -63,6 +63,11 @@ public: delete[] data; } + size_t getGPUReadOffset() const override + { + return (size_t) data; + } + MapInfo map(size_t /*minsize*/) override { return MapInfo(data, bufferSize); @@ -111,6 +116,11 @@ public: delete[] data; } + size_t getGPUReadOffset() const override + { + return frameGPUReadOffset; + } + MapInfo map(size_t /*minsize*/) override { if (orphan) @@ -192,6 +202,11 @@ public: virtual ~StreamBufferSync() {} + size_t getGPUReadOffset() const override + { + return (frameIndex * bufferSize) + frameGPUReadOffset; + } + void nextFrame() override { // Insert a GPU fence for this frame's section of the data, we'll wait diff --git a/src/modules/graphics/vulkan/StreamBuffer.cpp b/src/modules/graphics/vulkan/StreamBuffer.cpp index 8c37de985..de4e4d73c 100644 --- a/src/modules/graphics/vulkan/StreamBuffer.cpp +++ b/src/modules/graphics/vulkan/StreamBuffer.cpp @@ -96,6 +96,11 @@ ptrdiff_t StreamBuffer::getHandle() const return (ptrdiff_t) buffer; } +size_t getGPUReadOffset() const override +{ + return (frameIndex * bufferSize) + frameGPUReadOffset; +} + love::graphics::StreamBuffer::MapInfo StreamBuffer::map(size_t /*minsize*/) { // TODO: do we also need to wait until a fence is complete, here?