Merge pull request #22161 from hrydgard/vulkan-sync-fixes

More Vulkan sync fixes, plus an OpenGL one

Common/GPU/OpenGL/GLRenderManager.cpp

@@ -397,10 +397,13 @@ void GLRenderManager::Finish() {
 	task->frame = curFrame;
 	{
 		std::unique_lock<std::mutex> lock(pushMutex_);
-		task->initSteps = std::move(initSteps_);
+		{
+			std::lock_guard<std::mutex> 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<std::mutex> 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<std::mutex> lock(pushBuffersMutex_);
 		for (auto iter : frameData.activePushBuffers) {
 			iter->MapDevice(bufferStrategy_);
 		}
@@ -537,7 +542,11 @@ void GLRenderManager::FlushSync() {
 
 		std::unique_lock<std::mutex> lock(pushMutex_);
 		renderThreadQueue_.push(task);
-		renderThreadQueue_.back()->initSteps = std::move(initSteps_);
+		{
+			std::lock_guard<std::mutex> initLock(initStepsMutex_);
+			renderThreadQueue_.back()->initSteps = std::move(initSteps_);
+			initSteps_.clear();
+		}
 		renderThreadQueue_.back()->steps = std::move(steps_);
 		pushCondVar_.notify_one();
 		steps_.clear();

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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<GLRShader *> shaders, std::vector<GLRProgram::Semantic> semantics, std::vector<GLRProgram::UniformLocQuery> queries,
 		std::vector<GLRProgram::Initializer> initializers, GLRProgramLocData *locData, const GLRProgramFlags &flags) {
+		std::lock_guard<std::mutex> 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<GLRInputLayout::Entry> &entries, int stride) {
+		std::lock_guard<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> lock(pushBuffersMutex_);
 		frameData_[frame].activePushBuffers.insert(buffer);
 	}
 
@@ -860,8 +871,18 @@ private:
 
 	GLRStep *curRenderStep_ = nullptr;
 	std::vector<GLRStep *> 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<GLRInitStep> 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.

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<std::mutex> lock(mutex_);
+
 	if (!pass[(int)rpType] || sampleCounts[(int)rpType] != sampleCount) {
 		if (pass[(int)rpType]) {
 			vulkan->Delete().QueueDeleteRenderPass(pass[(int)rpType]);

Common/GPU/Vulkan/VulkanFramebuffer.h

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <mutex>
+
 #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]{};

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<std::mutex> 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;
 }
 

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<std::mutex> 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<RPKey, VKRRenderPass *> renderPasses_;
 
 	// Readback buffer. Currently we only support synchronous readback, so we only really need one.

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<std::mutex> 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;

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;
 		}