diff --git a/Common/GPU/OpenGL/GLRenderManager.cpp b/Common/GPU/OpenGL/GLRenderManager.cpp index e6385773a8..50216756b5 100644 --- a/Common/GPU/OpenGL/GLRenderManager.cpp +++ b/Common/GPU/OpenGL/GLRenderManager.cpp @@ -397,10 +397,13 @@ void GLRenderManager::Finish() { task->frame = curFrame; { std::unique_lock lock(pushMutex_); - task->initSteps = std::move(initSteps_); + { + std::lock_guard initLock(initStepsMutex_); + task->initSteps = std::move(initSteps_); + initSteps_.clear(); + } task->steps = std::move(steps_); renderThreadQueue_.push(task); - initSteps_.clear(); steps_.clear(); pushCondVar_.notify_one(); } @@ -474,6 +477,7 @@ bool GLRenderManager::Run(GLRRenderThreadTask &task) { // Run this after RunInitSteps so any fresh GLRBuffers for the pushbuffers can get created. if (!skipGLCalls_) { + std::lock_guard lock(pushBuffersMutex_); for (auto iter : frameData.activePushBuffers) { iter->Flush(); iter->UnmapDevice(); @@ -500,6 +504,7 @@ bool GLRenderManager::Run(GLRRenderThreadTask &task) { } if (!skipGLCalls_) { + std::lock_guard lock(pushBuffersMutex_); for (auto iter : frameData.activePushBuffers) { iter->MapDevice(bufferStrategy_); } @@ -537,7 +542,11 @@ void GLRenderManager::FlushSync() { std::unique_lock lock(pushMutex_); renderThreadQueue_.push(task); - renderThreadQueue_.back()->initSteps = std::move(initSteps_); + { + std::lock_guard initLock(initStepsMutex_); + renderThreadQueue_.back()->initSteps = std::move(initSteps_); + initSteps_.clear(); + } renderThreadQueue_.back()->steps = std::move(steps_); pushCondVar_.notify_one(); steps_.clear(); diff --git a/Common/GPU/OpenGL/GLRenderManager.h b/Common/GPU/OpenGL/GLRenderManager.h index 805a55fca8..a58a49c3fb 100644 --- a/Common/GPU/OpenGL/GLRenderManager.h +++ b/Common/GPU/OpenGL/GLRenderManager.h @@ -263,6 +263,7 @@ public: // and then we'll also need formats and stuff. GLRTexture *CreateTexture(GLenum target, int width, int height, int depth, int numMips) { _dbg_assert_(target != 0); + std::lock_guard lock(initStepsMutex_); GLRInitStep &step = initSteps_.push_uninitialized(); step.stepType = GLRInitStepType::CREATE_TEXTURE; step.create_texture.texture = new GLRTexture(caps_, width, height, depth, numMips); @@ -271,6 +272,7 @@ public: } GLRBuffer *CreateBuffer(GLuint target, size_t size, GLuint usage) { + std::lock_guard lock(initStepsMutex_); GLRInitStep &step = initSteps_.push_uninitialized(); step.stepType = GLRInitStepType::CREATE_BUFFER; step.create_buffer.buffer = new GLRBuffer(target, size); @@ -280,6 +282,7 @@ public: } GLRShader *CreateShader(GLuint stage, const std::string &code, std::string_view desc) { + std::lock_guard lock(initStepsMutex_); GLRInitStep &step = initSteps_.push_uninitialized(); step.stepType = GLRInitStepType::CREATE_SHADER; step.create_shader.shader = new GLRShader(desc); @@ -292,6 +295,7 @@ public: GLRFramebuffer *CreateFramebuffer(int width, int height, bool z_stencil, const char *tag) { _dbg_assert_(width > 0 && height > 0 && tag != nullptr); + std::lock_guard lock(initStepsMutex_); GLRInitStep &step = initSteps_.push_uninitialized(); step.stepType = GLRInitStepType::CREATE_FRAMEBUFFER; step.create_framebuffer.framebuffer = new GLRFramebuffer(caps_, width, height, z_stencil, tag); @@ -303,6 +307,7 @@ public: GLRProgram *CreateProgram( std::vector shaders, std::vector semantics, std::vector queries, std::vector initializers, GLRProgramLocData *locData, const GLRProgramFlags &flags) { + std::lock_guard lock(initStepsMutex_); GLRInitStep &step = initSteps_.push_uninitialized(); step.stepType = GLRInitStepType::CREATE_PROGRAM; _assert_(shaders.size() <= ARRAY_SIZE(step.create_program.shaders)); @@ -332,6 +337,7 @@ public: } GLRInputLayout *CreateInputLayout(const std::vector &entries, int stride) { + std::lock_guard lock(initStepsMutex_); GLRInitStep &step = initSteps_.push_uninitialized(); step.stepType = GLRInitStepType::CREATE_INPUT_LAYOUT; step.create_input_layout.inputLayout = new GLRInputLayout(); @@ -411,6 +417,7 @@ public: void BufferSubdata(GLRBuffer *buffer, size_t offset, size_t size, uint8_t *data, bool deleteData = true) { // TODO: Maybe should be a render command instead of an init command? When possible it's better as // an init command, that's for sure. + std::lock_guard lock(initStepsMutex_); GLRInitStep &step = initSteps_.push_uninitialized(); step.stepType = GLRInitStepType::BUFFER_SUBDATA; _dbg_assert_(offset <= buffer->size_ - size); @@ -423,6 +430,7 @@ public: // Takes ownership over the data pointer and delete[]-s it. void TextureImage(GLRTexture *texture, int level, int width, int height, int depth, Draw::DataFormat format, uint8_t *data, GLRAllocType allocType = GLRAllocType::NEW, bool linearFilter = false) { + std::lock_guard lock(initStepsMutex_); GLRInitStep &step = initSteps_.push_uninitialized(); step.stepType = GLRInitStepType::TEXTURE_IMAGE; step.texture_image.texture = texture; @@ -453,6 +461,7 @@ public: } void FinalizeTexture(GLRTexture *texture, int loadedLevels, bool genMips) { + std::lock_guard lock(initStepsMutex_); GLRInitStep &step = initSteps_.push_uninitialized(); step.stepType = GLRInitStepType::TEXTURE_FINALIZE; step.texture_finalize.texture = texture; @@ -800,6 +809,7 @@ public: } void UnregisterPushBuffer(GLPushBuffer *buffer) { + std::lock_guard lock(pushBuffersMutex_); int foundCount = 0; for (int i = 0; i < MAX_INFLIGHT_FRAMES; i++) { auto iter = frameData_[i].activePushBuffers.find(buffer); @@ -850,6 +860,7 @@ private: // When using legacy functionality for push buffers (glBufferData), we need to flush them // before actually making the glDraw* calls. It's best if the render manager handles that. void RegisterPushBuffer(int frame, GLPushBuffer *buffer) { + std::lock_guard lock(pushBuffersMutex_); frameData_[frame].activePushBuffers.insert(buffer); } @@ -860,8 +871,18 @@ private: GLRStep *curRenderStep_ = nullptr; std::vector steps_; + // Guards initSteps_. Recorded into from the emu thread, but also from the loader thread during + // boot (InitGPU runs there, and GL has to record device object creation rather than just doing + // it), and moved out on the emu thread in Finish/FlushSync. Uncontended in practice. + // Lock ordering: taken while pushMutex_ is held, never the other way around. + std::mutex initStepsMutex_; FastVec initSteps_; + // Guards frameData_[].activePushBuffers, which is inserted into from whichever thread creates a + // push buffer, erased from on the render thread (GLDeleter), and walked on the render thread. + // Lock ordering: taken before initStepsMutex_ (Flush() below records init steps), never after. + std::mutex pushBuffersMutex_; + // Execution time state // Thread is managed elsewhere, and should call ThreadFrame. diff --git a/Common/GPU/Vulkan/VulkanFramebuffer.cpp b/Common/GPU/Vulkan/VulkanFramebuffer.cpp index d2073367e8..da0d06311c 100644 --- a/Common/GPU/Vulkan/VulkanFramebuffer.cpp +++ b/Common/GPU/Vulkan/VulkanFramebuffer.cpp @@ -559,6 +559,9 @@ VkRenderPass VKRRenderPass::Get(VulkanContext *vulkan, RenderPassType rpType, Vk _dbg_assert_(!((rpType & RenderPassType::MULTISAMPLE) && sampleCount == VK_SAMPLE_COUNT_1_BIT)); + // Called from both the main thread and the render thread, see the note on mutex_. + std::lock_guard lock(mutex_); + if (!pass[(int)rpType] || sampleCounts[(int)rpType] != sampleCount) { if (pass[(int)rpType]) { vulkan->Delete().QueueDeleteRenderPass(pass[(int)rpType]); diff --git a/Common/GPU/Vulkan/VulkanFramebuffer.h b/Common/GPU/Vulkan/VulkanFramebuffer.h index a21cfbc37e..0b686b3929 100644 --- a/Common/GPU/Vulkan/VulkanFramebuffer.h +++ b/Common/GPU/Vulkan/VulkanFramebuffer.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "Common/Common.h" #include "Common/GPU/Vulkan/VulkanContext.h" @@ -144,6 +146,8 @@ public: explicit VKRRenderPass(const RPKey &key) : key_(key) {} VkRenderPass Get(VulkanContext *vulkan, RenderPassType rpType, VkSampleCountFlagBits sampleCount); + + // Only called from VulkanQueueRunner::DestroyDeviceObjects, with the threads stopped - no lock needed. void Destroy(VulkanContext *vulkan) { for (size_t i = 0; i < (size_t)RenderPassType::TYPE_COUNT; i++) { if (pass[i]) { @@ -153,6 +157,14 @@ public: } private: + // Get() creates the passes lazily, and runs on both the main thread (EndCurRenderStep) and the + // render thread (PerformRenderPass), so the arrays below need guarding. Without it, two threads + // reaching the same empty slot each create a pass and one gets overwritten and leaked - and the + // sample count branch can queue a pass for deletion that the other thread is about to use. + // Lock ordering: taken while VKRGraphicsPipeline::mutex_ is held (VulkanQueueRunner), never the + // other way around. + std::mutex mutex_; + // TODO: Might be better off with a hashmap once the render pass type count grows really large.. VkRenderPass pass[(size_t)RenderPassType::TYPE_COUNT]{}; VkSampleCountFlagBits sampleCounts[(size_t)RenderPassType::TYPE_COUNT]{}; diff --git a/Common/GPU/Vulkan/VulkanQueueRunner.cpp b/Common/GPU/Vulkan/VulkanQueueRunner.cpp index 8e1282c51d..1288618c4b 100644 --- a/Common/GPU/Vulkan/VulkanQueueRunner.cpp +++ b/Common/GPU/Vulkan/VulkanQueueRunner.cpp @@ -197,6 +197,13 @@ void VulkanQueueRunner::DestroyBackBuffers() { // Self-dependency: https://github.com/gpuweb/gpuweb/issues/442#issuecomment-547604827 // Also see https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#synchronization-pipeline-barriers-subpass-self-dependencies VKRRenderPass *VulkanQueueRunner::GetRenderPass(const RPKey &key) { + // Called from the main thread (EndCurRenderStep, CreateGraphicsPipeline) and from the render thread + // (PerformBindFramebufferAsRenderTarget). The render thread really does insert new keys, not just hit + // existing ones - PreprocessSteps rewrites the load actions to CLEAR when it merges a clear-only pass + // into a later one, after the main thread already looked up the pre-merge key. Insert() can Grow(), + // which reallocates the buckets out from under a concurrent Get(). + std::lock_guard lock(renderPassesMutex_); + VKRRenderPass *foundPass; if (renderPasses_.Get(key, &foundPass)) { return foundPass; @@ -204,6 +211,8 @@ VKRRenderPass *VulkanQueueRunner::GetRenderPass(const RPKey &key) { VKRRenderPass *pass = new VKRRenderPass(key); renderPasses_.Insert(key, pass); + // Safe to hand out the pointer once the lock is dropped - entries are never erased individually, + // only all at once in DestroyDeviceObjects. return pass; } diff --git a/Common/GPU/Vulkan/VulkanQueueRunner.h b/Common/GPU/Vulkan/VulkanQueueRunner.h index 92865099a9..2e256bb3df 100644 --- a/Common/GPU/Vulkan/VulkanQueueRunner.h +++ b/Common/GPU/Vulkan/VulkanQueueRunner.h @@ -253,6 +253,7 @@ public: VKRRenderPass *GetRenderPass(const RPKey &key); bool GetRenderPassKey(VKRRenderPass *passToFind, RPKey *outKey) const { + std::lock_guard lock(renderPassesMutex_); bool found = false; renderPasses_.Iterate([passToFind, &found, outKey](const RPKey &rpkey, const VKRRenderPass *pass) { if (pass == passToFind) { @@ -302,6 +303,8 @@ private: // Renderpasses, all combinations of preserving or clearing or dont-care-ing fb contents. // Each VKRRenderPass contains all compatibility classes (which attachments they have, etc). + // Looked up and inserted into from both the main thread and the render thread - see GetRenderPass. + mutable std::mutex renderPassesMutex_; DenseHashMap renderPasses_; // Readback buffer. Currently we only support synchronous readback, so we only really need one. diff --git a/Common/GPU/Vulkan/VulkanRenderManager.cpp b/Common/GPU/Vulkan/VulkanRenderManager.cpp index cafd714545..e6bdb3cf87 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.cpp +++ b/Common/GPU/Vulkan/VulkanRenderManager.cpp @@ -168,6 +168,12 @@ bool VKRGraphicsPipeline::Create(VulkanContext *vulkan, VkRenderPass compatibleR } void VKRGraphicsPipeline::DestroyVariants(VulkanContext *vulkan, bool msaaOnly) { + // Called from InvalidateMSAAPipelines on the main thread, mid-frame, while the render thread may be + // reading and replacing these same slots in PerformRenderPass - so take the lock that's documented + // as protecting the array. It also has to be held across the delete below, or the render thread can + // be left holding a freed Promise. + std::lock_guard lock(mutex_); + for (size_t i = 0; i < (size_t)RenderPassType::TYPE_COUNT; i++) { if (!this->pipeline[i]) continue; @@ -179,6 +185,9 @@ void VKRGraphicsPipeline::DestroyVariants(VulkanContext *vulkan, bool msaaOnly) if (pipeline) { vulkan->Delete().QueueDeletePipeline(pipeline); } + // The array owns the Promise - DestroyVariantsInstant deletes it too. Forgetting it here leaked + // one per destroyed variant on every MSAA or resolution change. + delete this->pipeline[i]; this->pipeline[i] = nullptr; } sampleCount_ = VK_SAMPLE_COUNT_FLAG_BITS_MAX_ENUM; diff --git a/GPU/Common/TextureCacheCommon.cpp b/GPU/Common/TextureCacheCommon.cpp index c8954fbfc4..7fdb3cd1f1 100644 --- a/GPU/Common/TextureCacheCommon.cpp +++ b/GPU/Common/TextureCacheCommon.cpp @@ -2863,6 +2863,8 @@ bool TextureCacheCommon::PrepareBuildTexture(BuildTexturePlan &plan, TexCacheEnt // These will only work correctly in the top 512x512 part. So, I've increased the threshold quite a bit. // We probably should handle these differently, by clamping the texture size and texture coordinates, but meh. if (plan.w > 2048 || plan.h > 2048) { + // Strangely, the homebrew "Kitten Cannon" hits this a bunch, with a clearly invalid 512x32768 texture. + // Some noise bit in the texture size command that we might just want to ignore. ERROR_LOG(Log::TexCache, "Bad texture dimensions: %dx%d", plan.w, plan.h); return false; }