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

Vulkan: Fix threading issues around pipeline layouts and the delete list

Common/GPU/OpenGL/GLFeatures.cpp

@@ -306,6 +306,13 @@ bool CheckGLExtensions() {
 			// Otherwise, let's trust GL_MAJOR_VERSION.  Note that Mali is intentionally not banned here.
 			if (gl_extensions.ver[0] >= 3) {
 				gl_extensions.GLES3 = gl3stubInit();
+				if (!gl_extensions.GLES3) {
+					// We failed to load the ES3 entry points, so we can't use any of them - including
+					// glGetStringi below, which is one of the ones gl3stubInit() checks for. Drop back
+					// to 2.0 so nothing downstream keys off the version and calls into a null pointer.
+					gl_extensions.ver[0] = 2;
+					gl_extensions.ver[1] = 0;
+				}
 			}
 		}
 #else
@@ -331,6 +338,9 @@ bool CheckGLExtensions() {
 		g_set_gl_extensions.clear();
 		for (GLint i = 0; i < numExtensions; ++i) {
 			const char *ext = (const char *)glGetStringi(GL_EXTENSIONS, i);
+			if (!ext) {
+				continue;
+			}
 			g_set_gl_extensions.insert(ext);
 			g_all_gl_extensions += ext;
 			g_all_gl_extensions += " ";

Common/GPU/OpenGL/GLSLProgram.cpp

@@ -1,19 +1,12 @@
-#include <set>
-
 #include <cstdio>
 #include <cstring>
-#include <sys/stat.h>
 
-#include "Common/File/VFS/VFS.h"
-#include "Common/File/FileUtil.h"
 #include "Common/GPU/OpenGL/GLSLProgram.h"
 
 #include "Common/Log.h"
 
-static std::set<GLSLProgram *> active_programs;
-
-bool CompileShader(const char *source, GLuint shader, const char *filename, std::string *error_message) {
-	glShaderSource(shader, 1, &source, NULL);
+static bool CompileShader(const char *source, GLuint shader, const char *stageName, std::string *error_message) {
+	glShaderSource(shader, 1, &source, nullptr);
 	glCompileShader(shader);
 	GLint success;
 	glGetShaderiv(shader, GL_COMPILE_STATUS, &success);
@@ -23,9 +16,9 @@ bool CompileShader(const char *source, GLuint shader, const char *filename, std:
 		GLsizei len;
 		glGetShaderInfoLog(shader, MAX_INFO_LOG_SIZE, &len, infoLog);
 		infoLog[len] = '\0';
-		ERROR_LOG(Log::G3D, "Error in shader compilation of %s!\n", filename);
+		ERROR_LOG(Log::G3D, "Error compiling %s shader!\n", stageName);
 		ERROR_LOG(Log::G3D, "Info log: %s\n", infoLog);
-		ERROR_LOG(Log::G3D, "Shader source:\n%s\n", (const char *)source);
+		ERROR_LOG(Log::G3D, "Shader source:\n%s\n", source);
 		if (error_message)
 			*error_message = infoLog;
 		return false;
@@ -33,104 +26,17 @@ bool CompileShader(const char *source, GLuint shader, const char *filename, std:
 	return true;
 }
 
-GLSLProgram *glsl_create_source(const char *vshader_src, const char *fshader_src, std::string *error_message) {
-	GLSLProgram *program = new GLSLProgram();
-	program->program_ = 0;
-	program->vsh_ = 0;
-	program->fsh_ = 0;
-	program->vshader_source = vshader_src;
-	program->fshader_source = fshader_src;
-	strcpy(program->name, "[srcshader]");
-	strcpy(program->vshader_filename, "");
-	strcpy(program->fshader_filename, "");
-	if (glsl_recompile(program, error_message)) {
-		active_programs.insert(program);
-	} else {
-		ERROR_LOG(Log::G3D, "Failed compiling GLSL program from source strings");
-		delete program;
-		return 0;
-	}
-	return program;
-}
-
-// Not wanting to change ReadLocalFile semantics.
-// TODO: Use C++11 unique_ptr, remove delete[]
-struct AutoCharArrayBuf {
-	AutoCharArrayBuf(char *buf = nullptr) : buf_(buf) {
-	}
-	~AutoCharArrayBuf() {
-		delete [] buf_;
-		buf_ = nullptr;
-	}
-	void reset(char *buf) {
-		delete[] buf_;
-		buf_ = buf;
-	}
-	operator char *() {
-		return buf_;
-	}
-
-private:
-	char *buf_;
-};
-
-bool glsl_recompile(GLSLProgram *program, std::string *error_message) {
-	struct stat vs, fs;
-	AutoCharArrayBuf vsh_src, fsh_src;
-
-	if (strlen(program->vshader_filename) > 0 && 0 == stat(program->vshader_filename, &vs)) {
-		program->vshader_mtime = vs.st_mtime;
-		if (!program->vshader_source) {
-			size_t sz;
-			vsh_src.reset((char *)File::ReadLocalFile(Path(program->vshader_filename), &sz));
-		}
-	} else {
-		program->vshader_mtime = 0;
-	}
-
-	if (strlen(program->fshader_filename) > 0 && 0 == stat(program->fshader_filename, &fs)) {
-		program->fshader_mtime = fs.st_mtime;
-		if (!program->fshader_source) {
-			size_t sz;
-			fsh_src.reset((char *)File::ReadLocalFile(Path(program->fshader_filename), &sz));
-		}
-	} else {
-		program->fshader_mtime = 0;
-	}
-
-	if (!program->vshader_source && !vsh_src) {
-		size_t sz;
-		vsh_src.reset((char *)g_VFS.ReadFile(program->vshader_filename, &sz));
-	}
-	if (!program->vshader_source && !vsh_src) {
-		ERROR_LOG(Log::G3D, "File missing: %s", program->vshader_filename);
-		if (error_message) {
-			*error_message = std::string("File missing: ") + program->vshader_filename;
-		}
-		return false;
-	}
-	if (!program->fshader_source && !fsh_src) {
-		size_t sz;
-		fsh_src.reset((char *)g_VFS.ReadFile(program->fshader_filename, &sz));
-	}
-	if (!program->fshader_source && !fsh_src) {
-		ERROR_LOG(Log::G3D, "File missing: %s", program->fshader_filename);
-		if (error_message) {
-			*error_message = std::string("File missing: ") + program->fshader_filename;
-		}
-		return false;
-	}
-
+static bool glsl_compile(GLSLProgram *program, const char *vshader_src, const char *fshader_src, std::string *error_message) {
 	GLuint vsh = glCreateShader(GL_VERTEX_SHADER);
-	const GLchar *vsh_str = program->vshader_source ? program->vshader_source : (const GLchar *)(vsh_src);
-	if (!CompileShader(vsh_str, vsh, program->vshader_filename, error_message)) {
+	if (!CompileShader(vshader_src, vsh, "vertex", error_message)) {
+		glDeleteShader(vsh);
 		return false;
 	}
 
-	const GLchar *fsh_str = program->fshader_source ? program->fshader_source : (const GLchar *)(fsh_src);
 	GLuint fsh = glCreateShader(GL_FRAGMENT_SHADER);
-	if (!CompileShader(fsh_str, fsh, program->fshader_filename, error_message)) {
+	if (!CompileShader(fshader_src, fsh, "fragment", error_message)) {
 		glDeleteShader(vsh);
+		glDeleteShader(fsh);
 		return false;
 	}
 
@@ -146,63 +52,47 @@ bool glsl_recompile(GLSLProgram *program, std::string *error_message) {
 		GLint bufLength = 0;
 		glGetProgramiv(prog, GL_INFO_LOG_LENGTH, &bufLength);
 		if (bufLength) {
-			char* buf = new char[bufLength + 1];  // safety
-			glGetProgramInfoLog(prog, bufLength, NULL, buf);
-			INFO_LOG(Log::G3D, "vsh: %i   fsh: %i", vsh, fsh);
+			char *buf = new char[bufLength + 1];  // safety
+			glGetProgramInfoLog(prog, bufLength, nullptr, buf);
 			ERROR_LOG(Log::G3D, "Could not link shader program (linkstatus=%i):\n %s  \n", linkStatus, buf);
 			if (error_message) {
 				*error_message = buf;
 			}
-			delete [] buf;
+			delete[] buf;
 		} else {
-			INFO_LOG(Log::G3D, "vsh: %i   fsh: %i", vsh, fsh);
 			ERROR_LOG(Log::G3D, "Could not link shader program (linkstatus=%i). No OpenGL error log was available.", linkStatus);
 			if (error_message) {
 				*error_message = "(no error message available)";
 			}
 		}
+		glDeleteProgram(prog);
 		glDeleteShader(vsh);
 		glDeleteShader(fsh);
 		return false;
 	}
 
-	// Destroy the old program, if any.
-	if (program->program_) {
-		glDeleteProgram(program->program_);
-	}
-
 	program->program_ = prog;
 	program->vsh_ = vsh;
 	program->fsh_ = fsh;
 
 	program->sampler0 = glGetUniformLocation(program->program_, "sampler0");
-	program->sampler1 = glGetUniformLocation(program->program_, "sampler1");
-
-	program->a_position	= glGetAttribLocation(program->program_, "a_position");
-	program->a_color = glGetAttribLocation(program->program_, "a_color");
-	program->a_normal = glGetAttribLocation(program->program_, "a_normal");
-	program->a_texcoord0 = glGetAttribLocation(program->program_, "a_texcoord0");
-	program->a_texcoord1 = glGetAttribLocation(program->program_, "a_texcoord1");
-
-	program->u_worldviewproj = glGetUniformLocation(program->program_, "u_worldviewproj");
-	program->u_world = glGetUniformLocation(program->program_, "u_world");
 	program->u_viewproj = glGetUniformLocation(program->program_, "u_viewproj");
-	program->u_fog = glGetUniformLocation(program->program_, "u_fog");
-	program->u_sundir = glGetUniformLocation(program->program_, "u_sundir");
-	program->u_camerapos = glGetUniformLocation(program->program_, "u_camerapos");
-
-	//INFO_LOG(Log::G3D, "Shader compilation success: %s %s",
-	//		 program->vshader_filename,
-	//		 program->fshader_filename);
+	program->a_position = glGetAttribLocation(program->program_, "a_position");
+	program->a_texcoord0 = glGetAttribLocation(program->program_, "a_texcoord0");
 	return true;
 }
 
-int glsl_attrib_loc(const GLSLProgram *program, const char *name) {
-	return glGetAttribLocation(program->program_, name);
-}
-
-int glsl_uniform_loc(const GLSLProgram *program, const char *name) {
-	return glGetUniformLocation(program->program_, name);
+GLSLProgram *glsl_create_source(const char *vshader_src, const char *fshader_src, std::string *error_message) {
+	GLSLProgram *program = new GLSLProgram();
+	program->program_ = 0;
+	program->vsh_ = 0;
+	program->fsh_ = 0;
+	if (!glsl_compile(program, vshader_src, fshader_src, error_message)) {
+		ERROR_LOG(Log::G3D, "Failed compiling GLSL program from source strings");
+		delete program;
+		return nullptr;
+	}
+	return program;
 }
 
 void glsl_destroy(GLSLProgram *program) {
@@ -210,28 +100,19 @@ void glsl_destroy(GLSLProgram *program) {
 		glDeleteShader(program->vsh_);
 		glDeleteShader(program->fsh_);
 		glDeleteProgram(program->program_);
-		active_programs.erase(program);
 	} else {
 		ERROR_LOG(Log::G3D, "Deleting null GLSL program!");
 	}
 	delete program;
 }
 
-static const GLSLProgram *curProgram;
-
 void glsl_bind(const GLSLProgram *program) {
 	if (program)
 		glUseProgram(program->program_);
 	else
 		glUseProgram(0);
-	curProgram = program;
 }
 
 void glsl_unbind() {
 	glUseProgram(0);
-	curProgram = nullptr;
-}
-
-const GLSLProgram *glsl_get_program() {
-	return curProgram;
 }

Common/GPU/OpenGL/GLSLProgram.h

@@ -1,40 +1,24 @@
 // Utility code for loading GLSL shaders.
-// Has support for auto-reload, see glsl_refresh
+//
+// This is a small leftover from the old "native" library, only used by the Win32 GE debugger's
+// preview windows, which talk to GL directly rather than going through thin3d. Everything here
+// works on shader source strings - the file loading and auto-reload support that used to live
+// here was never used in PPSSPP and is gone.
 
 #pragma once
 
 #include <string>
-#include <time.h>
 
 #include "Common/GPU/OpenGL/GLCommon.h"
 
 // Represent a compiled and linked vshader/fshader pair.
-// A just-constructed object is valid but cannot be used as a shader program, meaning that
-// yes, you can declare these as globals if you like.
 struct GLSLProgram {
-	char name[16];
-	char vshader_filename[256];
-	char fshader_filename[256];
-	const char *vshader_source;
-	const char *fshader_source;
-	time_t vshader_mtime;
-	time_t fshader_mtime;
-
-	// Locations to some common uniforms. Hardcoded for speed.
+	// Locations of the uniforms and attributes the callers use, looked up once at link time.
+	// Add to these as needed - each one costs a lookup per link.
 	GLint sampler0;
-	GLint sampler1;
-	GLint u_worldviewproj;
-	GLint u_world;
 	GLint u_viewproj;
-	GLint u_fog;	// rgb = color, a = density
-	GLint u_sundir;
-	GLint u_camerapos;
-
 	GLint a_position;
-	GLint a_color;
-	GLint a_normal;
 	GLint a_texcoord0;
-	GLint a_texcoord1;
 
 	// Private to the implementation, do not touch
 	GLuint vsh_;
@@ -42,19 +26,10 @@ struct GLSLProgram {
 	GLuint program_;
 };
 
-// C API, old skool. Not much point either...
-
-// From files (VFS)
-GLSLProgram *glsl_create(const char *vshader_file, const char *fshader_file, std::string *error_message = 0);
-// Directly from source code
-GLSLProgram *glsl_create_source(const char *vshader_src, const char *fshader_src, std::string *error_message = 0);
+// Compiles and links a program. Returns nullptr on failure, having logged the reason and,
+// if error_message is non-null, copied it there.
+GLSLProgram *glsl_create_source(const char *vshader_src, const char *fshader_src, std::string *error_message = nullptr);
 void glsl_destroy(GLSLProgram *program);
 
-// If recompilation of the program fails, the program is untouched and error messages
-// are logged and the function returns false.
-bool glsl_recompile(GLSLProgram *program, std::string *error_message = 0);
 void glsl_bind(const GLSLProgram *program);
-const GLSLProgram *glsl_get_program();
 void glsl_unbind();
-int glsl_attrib_loc(const GLSLProgram *program, const char *name);
-int glsl_uniform_loc(const GLSLProgram *program, const char *name);

Common/GPU/OpenGL/thin3d_gl.cpp

@@ -534,26 +534,6 @@ private:
 	PresentMode requestedPresentMode_{};
 };
 
-static constexpr int MakeIntelSimpleVer(int v1, int v2, int v3) {
-	return (v1 << 16) | (v2 << 8) | v3;
-}
-
-static bool HasIntelDualSrcBug(const int versions[4]) {
-	// Intel uses a confusing set of at least 3 version numbering schemes.  This is the one given to OpenGL.
-	switch (MakeIntelSimpleVer(versions[0], versions[1], versions[2])) {
-	case MakeIntelSimpleVer(9, 17, 10):
-	case MakeIntelSimpleVer(9, 18, 10):
-		return false;
-	case MakeIntelSimpleVer(10, 18, 10):
-		return versions[3] < 4061;
-	case MakeIntelSimpleVer(10, 18, 14):
-		return versions[3] < 4080;
-	default:
-		// Older than above didn't support dual src anyway, newer should have the fix.
-		return false;
-	}
-}
-
 OpenGLContext::OpenGLContext(bool canChangeSwapInterval) : renderManager_(frameTimeHistory_) {
 	if (gl_extensions.IsGLES) {
 		if (gl_extensions.OES_packed_depth_stencil || gl_extensions.OES_depth24) {
@@ -647,17 +627,6 @@ OpenGLContext::OpenGLContext(bool canChangeSwapInterval) : renderManager_(frameT
 	if (!gl_extensions.VersionGEThan(3, 0, 0)) {
 		// Don't use this extension on sub 3.0 OpenGL versions as it does not seem reliable.
 		bugs_.Infest(Bugs::DUAL_SOURCE_BLENDING_BROKEN);
-	} else if (caps_.vendor == GPUVendor::VENDOR_INTEL) {
-		// Note: this is for Intel drivers with GL3+.
-		// Also on Intel, see https://github.com/hrydgard/ppsspp/issues/10117
-		// TODO: Remove entirely sometime reasonably far in driver years after 2015.
-		const std::string ver = OpenGLContext::GetInfoString(Draw::InfoField::APIVERSION);
-		int versions[4]{};
-		if (sscanf(ver.c_str(), "Build %d.%d.%d.%d", &versions[0], &versions[1], &versions[2], &versions[3]) == 4) {
-			if (HasIntelDualSrcBug(versions)) {
-				bugs_.Infest(Bugs::DUAL_SOURCE_BLENDING_BROKEN);
-			}
-		}
 	}
 
 #if PPSSPP_ARCH(ARMV7)

Common/GPU/Vulkan/VulkanBarrier.cpp

@@ -212,7 +212,7 @@ void VulkanBarrierBatch::TransitionDepthStencilImageAuto(
 		dstStageMask_ |= VK_PIPELINE_STAGE_TRANSFER_BIT;
 		break;
 	case VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL:
-		dstAccessMask = VK_ACCESS_TRANSFER_READ_BIT;
+		dstAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT;
 		dstStageMask_ |= VK_PIPELINE_STAGE_TRANSFER_BIT;
 		break;
 	case VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL:

Common/GPU/Vulkan/VulkanContext.cpp

@@ -907,9 +907,13 @@ VkResult VulkanContext::CreateDevice(int physical_device, const std::vector<cons
 	if (res != VK_SUCCESS) {
 		init_error_ = "Unable to create Vulkan device";
 		ERROR_LOG(Log::G3D, "%s", init_error_.c_str());
-	} else {
-		VulkanLoadDeviceFunctions(device_, extensionsLookup_, vulkanDeviceApiVersion_);
+		// Don't fall through - there's nothing below that makes sense without a device, and creating the
+		// VMA allocator on a null one asserts. The caller falls back to another backend on failure.
+		return res;
 	}
+
+	VulkanLoadDeviceFunctions(device_, extensionsLookup_, vulkanDeviceApiVersion_);
+
 	INFO_LOG(Log::G3D, "Vulkan Device created: %s", physicalDeviceProperties_[physical_device_].properties.deviceName);
 
 	// Since we successfully created a device (however we got here, might be interesting in debug), we force the choice to be visible in the menu.
@@ -1845,7 +1849,12 @@ void finalize_glslang() {
 	glslang::FinalizeProcess();
 }
 
+// NOTE: Every vector in the class has to be listed both here and in PerformDeletes. If one is missing
+// from Take, the objects in it linger on the global list until device teardown instead of being deleted
+// a few frames later; if one is missing from PerformDeletes, they leak outright. Both are bugs.
 void VulkanDeleteList::Take(VulkanDeleteList &del) {
+	// The render thread can be queueing deletes into del (the global list) while we do this.
+	std::lock_guard<std::mutex> lock(del.mutex_);
 	_dbg_assert_(cmdPools_.empty());
 	_dbg_assert_(descPools_.empty());
 	_dbg_assert_(modules_.empty());
@@ -1862,6 +1871,7 @@ void VulkanDeleteList::Take(VulkanDeleteList &del) {
 	_dbg_assert_(framebuffers_.empty());
 	_dbg_assert_(pipelineLayouts_.empty());
 	_dbg_assert_(descSetLayouts_.empty());
+	_dbg_assert_(queryPools_.empty());
 	_dbg_assert_(callbacks_.empty());
 	cmdPools_ = std::move(del.cmdPools_);
 	descPools_ = std::move(del.descPools_);
@@ -1879,12 +1889,14 @@ void VulkanDeleteList::Take(VulkanDeleteList &del) {
 	framebuffers_ = std::move(del.framebuffers_);
 	pipelineLayouts_ = std::move(del.pipelineLayouts_);
 	descSetLayouts_ = std::move(del.descSetLayouts_);
+	queryPools_ = std::move(del.queryPools_);
 	callbacks_ = std::move(del.callbacks_);
 	del.cmdPools_.clear();
 	del.descPools_.clear();
 	del.modules_.clear();
 	del.buffers_.clear();
 	del.buffersWithAllocs_.clear();
+	del.bufferViews_.clear();
 	del.imageViews_.clear();
 	del.imagesWithAllocs_.clear();
 	del.deviceMemory_.clear();
@@ -1895,10 +1907,33 @@ void VulkanDeleteList::Take(VulkanDeleteList &del) {
 	del.framebuffers_.clear();
 	del.pipelineLayouts_.clear();
 	del.descSetLayouts_.clear();
+	del.queryPools_.clear();
 	del.callbacks_.clear();
 }
 
 void VulkanDeleteList::PerformDeletes(VulkanContext *vulkan, VmaAllocator allocator) {
+	// Drain into a local list before destroying anything - a callback is allowed to queue more deletes
+	// (~VKFramebuffer does, via ~VKRFramebuffer), and we mustn't append to a vector we're iterating.
+	// Anything queued back onto this list gets picked up by the next lap, so keep going until a lap
+	// comes up empty. In the normal per-frame case that's a single extra lap, since callbacks queue
+	// onto the global list rather than the frame's own list - the loop matters for
+	// VulkanContext::PerformPendingDeletes(), which drains the global list itself just before the
+	// device goes away, with no later pass to catch the stragglers.
+	int deleteCount = 0;
+	for (;;) {
+		VulkanDeleteList taken;
+		taken.Take(*this);
+		int count = taken.PerformDeletesInternal(vulkan, allocator);
+		if (!count) {
+			break;
+		}
+		deleteCount += count;
+	}
+	deleteCount_ = deleteCount;
+}
+
+// See the note on Take() - every vector in the class must be handled here too, or it leaks.
+int VulkanDeleteList::PerformDeletesInternal(VulkanContext *vulkan, VmaAllocator allocator) {
 	int deleteCount = 0;
 
 	for (auto &callback : callbacks_) {
@@ -1993,7 +2028,7 @@ void VulkanDeleteList::PerformDeletes(VulkanContext *vulkan, VmaAllocator alloca
 		deleteCount++;
 	}
 	queryPools_.clear();
-	deleteCount_ = deleteCount;
+	return deleteCount;
 }
 
 void VulkanContext::GetImageMemoryRequirements(VkImage image, VkMemoryRequirements *mem_reqs, bool *dedicatedAllocation) {

Common/GPU/Vulkan/VulkanContext.h

@@ -3,6 +3,7 @@
 #include <cstdint>
 #include <cstring>
 #include <memory>
+#include <mutex>
 #include <string>
 #include <string_view>
 #include <vector>
@@ -70,6 +71,19 @@ enum class PerfClass {
 typedef std::function<void(VulkanContext *)> DeleteCallback;
 
 // This is a bit repetitive...
+//
+// Thread safety: The queueing functions are locked, because the global delete list gets written from
+// more than one thread. Most callers are on the main thread, but not all:
+//   * VulkanDescSetPool::Recreate, when a descriptor pool has to grow, runs from FlushDescSets on
+//     the render thread. This one really happens - some games go past the initial 1024 descriptors.
+//   * VulkanQueueRunner::ResizeReadbackBuffer runs from PerformReadback on the render thread. For
+//     blocking readbacks the main thread is parked in FlushSync so it can't collide, and the delayed
+//     ones never actually resize (the readback key contains the dimensions), but it's not worth
+//     relying on that staying true.
+// Meanwhile the main thread moves the global list into the current frame's list in EndFrame().
+//
+// PerformDeletes needs no lock of its own: it drains into a private local list (under Take's lock) and
+// destroys from that, so the destruction never touches a list another thread can reach.
 class VulkanDeleteList {
 	struct BufferWithAlloc {
 		VkBuffer buffer;
@@ -91,36 +105,40 @@ class VulkanDeleteList {
 
 public:
 	// NOTE: These all take reference handles so they can zero the input value.
-	void QueueDeleteCommandPool(VkCommandPool &pool) { _dbg_assert_(pool != VK_NULL_HANDLE); cmdPools_.push_back(pool); pool = VK_NULL_HANDLE; }
-	void QueueDeleteDescriptorPool(VkDescriptorPool &pool) { _dbg_assert_(pool != VK_NULL_HANDLE); descPools_.push_back(pool); pool = VK_NULL_HANDLE; }
-	void QueueDeleteShaderModule(VkShaderModule &module) { _dbg_assert_(module != VK_NULL_HANDLE); modules_.push_back(module); module = VK_NULL_HANDLE; }
-	void QueueDeleteBuffer(VkBuffer &buffer) { _dbg_assert_(buffer != VK_NULL_HANDLE); buffers_.push_back(buffer); buffer = VK_NULL_HANDLE; }
-	void QueueDeleteBufferView(VkBufferView &bufferView) { _dbg_assert_(bufferView != VK_NULL_HANDLE); bufferViews_.push_back(bufferView); bufferView = VK_NULL_HANDLE; }
-	void QueueDeleteImageView(VkImageView &imageView) { _dbg_assert_(imageView != VK_NULL_HANDLE); imageViews_.push_back(imageView); imageView = VK_NULL_HANDLE; }
-	void QueueDeleteDeviceMemory(VkDeviceMemory &deviceMemory) { _dbg_assert_(deviceMemory != VK_NULL_HANDLE); deviceMemory_.push_back(deviceMemory); deviceMemory = VK_NULL_HANDLE; }
-	void QueueDeleteSampler(VkSampler &sampler) { _dbg_assert_(sampler != VK_NULL_HANDLE); samplers_.push_back(sampler); sampler = VK_NULL_HANDLE; }
-	void QueueDeletePipeline(VkPipeline &pipeline) { _dbg_assert_(pipeline != VK_NULL_HANDLE); pipelines_.push_back(pipeline); pipeline = VK_NULL_HANDLE; }
-	void QueueDeletePipelineCache(VkPipelineCache &pipelineCache) { _dbg_assert_(pipelineCache != VK_NULL_HANDLE); pipelineCaches_.push_back(pipelineCache); pipelineCache = VK_NULL_HANDLE; }
-	void QueueDeleteRenderPass(VkRenderPass &renderPass) { _dbg_assert_(renderPass != VK_NULL_HANDLE); renderPasses_.push_back(renderPass); renderPass = VK_NULL_HANDLE; }
-	void QueueDeleteFramebuffer(VkFramebuffer &framebuffer) { _dbg_assert_(framebuffer != VK_NULL_HANDLE); framebuffers_.push_back(framebuffer); framebuffer = VK_NULL_HANDLE; }
-	void QueueDeletePipelineLayout(VkPipelineLayout &pipelineLayout) { _dbg_assert_(pipelineLayout != VK_NULL_HANDLE); pipelineLayouts_.push_back(pipelineLayout); pipelineLayout = VK_NULL_HANDLE; }
-	void QueueDeleteDescriptorSetLayout(VkDescriptorSetLayout &descSetLayout) { _dbg_assert_(descSetLayout != VK_NULL_HANDLE); descSetLayouts_.push_back(descSetLayout); descSetLayout = VK_NULL_HANDLE; }
-	void QueueDeleteQueryPool(VkQueryPool &queryPool) { _dbg_assert_(queryPool != VK_NULL_HANDLE); queryPools_.push_back(queryPool); queryPool = VK_NULL_HANDLE; }
-	void QueueCallback(DeleteCallback func) { callbacks_.push_back(func); }
+	void QueueDeleteCommandPool(VkCommandPool &pool) { _dbg_assert_(pool != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); cmdPools_.push_back(pool); pool = VK_NULL_HANDLE; }
+	void QueueDeleteDescriptorPool(VkDescriptorPool &pool) { _dbg_assert_(pool != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); descPools_.push_back(pool); pool = VK_NULL_HANDLE; }
+	void QueueDeleteShaderModule(VkShaderModule &module) { _dbg_assert_(module != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); modules_.push_back(module); module = VK_NULL_HANDLE; }
+	void QueueDeleteBuffer(VkBuffer &buffer) { _dbg_assert_(buffer != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); buffers_.push_back(buffer); buffer = VK_NULL_HANDLE; }
+	void QueueDeleteBufferView(VkBufferView &bufferView) { _dbg_assert_(bufferView != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); bufferViews_.push_back(bufferView); bufferView = VK_NULL_HANDLE; }
+	void QueueDeleteImageView(VkImageView &imageView) { _dbg_assert_(imageView != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); imageViews_.push_back(imageView); imageView = VK_NULL_HANDLE; }
+	void QueueDeleteDeviceMemory(VkDeviceMemory &deviceMemory) { _dbg_assert_(deviceMemory != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); deviceMemory_.push_back(deviceMemory); deviceMemory = VK_NULL_HANDLE; }
+	void QueueDeleteSampler(VkSampler &sampler) { _dbg_assert_(sampler != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); samplers_.push_back(sampler); sampler = VK_NULL_HANDLE; }
+	void QueueDeletePipeline(VkPipeline &pipeline) { _dbg_assert_(pipeline != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); pipelines_.push_back(pipeline); pipeline = VK_NULL_HANDLE; }
+	void QueueDeletePipelineCache(VkPipelineCache &pipelineCache) { _dbg_assert_(pipelineCache != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); pipelineCaches_.push_back(pipelineCache); pipelineCache = VK_NULL_HANDLE; }
+	void QueueDeleteRenderPass(VkRenderPass &renderPass) { _dbg_assert_(renderPass != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); renderPasses_.push_back(renderPass); renderPass = VK_NULL_HANDLE; }
+	void QueueDeleteFramebuffer(VkFramebuffer &framebuffer) { _dbg_assert_(framebuffer != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); framebuffers_.push_back(framebuffer); framebuffer = VK_NULL_HANDLE; }
+	void QueueDeletePipelineLayout(VkPipelineLayout &pipelineLayout) { _dbg_assert_(pipelineLayout != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); pipelineLayouts_.push_back(pipelineLayout); pipelineLayout = VK_NULL_HANDLE; }
+	void QueueDeleteDescriptorSetLayout(VkDescriptorSetLayout &descSetLayout) { _dbg_assert_(descSetLayout != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); descSetLayouts_.push_back(descSetLayout); descSetLayout = VK_NULL_HANDLE; }
+	void QueueDeleteQueryPool(VkQueryPool &queryPool) { _dbg_assert_(queryPool != VK_NULL_HANDLE); std::lock_guard<std::mutex> lock(mutex_); queryPools_.push_back(queryPool); queryPool = VK_NULL_HANDLE; }
+	void QueueCallback(DeleteCallback func) { std::lock_guard<std::mutex> lock(mutex_); callbacks_.push_back(func); }
 
-	void QueueDeleteBufferAllocation(VkBuffer &buffer, VmaAllocation &alloc) { 
-		_dbg_assert_(buffer != VK_NULL_HANDLE); 
+	void QueueDeleteBufferAllocation(VkBuffer &buffer, VmaAllocation &alloc) {
+		_dbg_assert_(buffer != VK_NULL_HANDLE);
+		std::lock_guard<std::mutex> lock(mutex_);
 		buffersWithAllocs_.push_back(BufferWithAlloc{ buffer, alloc });
 		buffer = VK_NULL_HANDLE;
 		alloc = VK_NULL_HANDLE;
 	}
 	void QueueDeleteImageAllocation(VkImage &image, VmaAllocation &alloc) {
 		_dbg_assert_(image != VK_NULL_HANDLE && alloc != VK_NULL_HANDLE);
+		std::lock_guard<std::mutex> lock(mutex_);
 		imagesWithAllocs_.push_back(ImageWithAlloc{ image, alloc });
 		image = VK_NULL_HANDLE;
 		alloc = VK_NULL_HANDLE;
 	}
 
+	// Moves everything from del into this list. Only the source list is locked - the destination is
+	// either a frame's own list or a stack local, neither of which another thread can reach.
 	void Take(VulkanDeleteList &del);
 	void PerformDeletes(VulkanContext *vulkan, VmaAllocator allocator);
 
@@ -129,6 +147,10 @@ public:
 	}
 
 private:
+	// Does the actual destruction, on a list that's been drained out of the shared one. Returns the count.
+	int PerformDeletesInternal(VulkanContext *vulkan, VmaAllocator allocator);
+
+	std::mutex mutex_;
 	std::vector<VkCommandPool> cmdPools_;
 	std::vector<VkDescriptorPool> descPools_;
 	std::vector<VkShaderModule> modules_;

Common/GPU/Vulkan/VulkanLoader.cpp

@@ -537,7 +537,8 @@ bool VulkanMayBeAvailable() {
 			INFO_LOG(Log::G3D, "VulkanMayBeAvailable: Found platform surface extension '%s'", platformSurfaceExtension);
 			instanceExtensions[ci.enabledExtensionCount++] = platformSurfaceExtension;
 			platformSurfaceExtensionFound = true;
-			break;
+			// Note: Can't stop here - the enumeration order isn't specified anywhere, so VK_KHR_surface
+			// may well come after the platform one, and we need both.
 		} else if (!strcmp(iter.extensionName, VK_KHR_SURFACE_EXTENSION_NAME)) {
 			instanceExtensions[ci.enabledExtensionCount++] = VK_KHR_SURFACE_EXTENSION_NAME;
 			surfaceExtensionFound = true;
@@ -618,8 +619,9 @@ bool VulkanMayBeAvailable() {
 					}
 				}
 			}
-			anyGood = !blacklisted;
-			if (anyGood) {
+			// Note: Must not overwrite the verdict from a previously seen device, one good one is enough.
+			anyGood = anyGood || !blacklisted;
+			if (!blacklisted) {
 				INFO_LOG(Log::G3D, "VulkanMayBeAvailable: Eligible device found: '%s'", props.deviceName);
 			} else {
 				INFO_LOG(Log::G3D, "VulkanMayBeAvailable: Blacklisted device found and ignored: '%s'", props.deviceName);

Common/GPU/Vulkan/VulkanRenderManager.cpp

@@ -694,7 +694,7 @@ void VulkanRenderManager::PollPresentTiming() {
 }
 
 void VulkanRenderManager::BeginFrame(bool enableProfiling, bool enableLogProfiler) {
-	double frameBeginTime = time_now_d()
+	double frameBeginTime = time_now_d();
 	VLOG("BeginFrame");
 	VkDevice device = vulkan_->GetDevice();
 
@@ -1806,12 +1806,22 @@ VKRPipelineLayout *VulkanRenderManager::CreatePipelineLayout(BindingType *bindin
 		layout->frameData[i].pool.Create(vulkan_, bindingTypes, (uint32_t)bindingTypesCount, 1024);
 	}
 
-	pipelineLayouts_.push_back(layout);
+	{
+		std::lock_guard<std::mutex> lock(pipelineLayoutsMutex_);
+		pipelineLayouts_.push_back(layout);
+	}
 	return layout;
 }
 
 void VulkanRenderManager::DestroyPipelineLayout(VKRPipelineLayout *layout) {
+	// The layout has to stay in pipelineLayouts_ until the frames that were recorded with it have been
+	// flushed by the render thread, otherwise their descriptor sets never get written. So, we can't
+	// remove it here - instead we let it ride along on the delete list, which won't be run until the
+	// fence for the frame it was queued in has been waited on.
 	vulkan_->Delete().QueueCallback([this, layout](VulkanContext *vulkan) {
+		// Runs on the main thread, while the render thread may be in FlushDescriptors - so both the
+		// erase and the destruction of the layout itself have to be under the lock.
+		std::lock_guard<std::mutex> lock(pipelineLayoutsMutex_);
 		for (auto iter = pipelineLayouts_.begin(); iter != pipelineLayouts_.end(); iter++) {
 			if (*iter == layout) {
 				pipelineLayouts_.erase(iter);
@@ -1828,13 +1838,17 @@ void VulkanRenderManager::DestroyPipelineLayout(VKRPipelineLayout *layout) {
 	});
 }
 
+// Called on the render thread.
 void VulkanRenderManager::FlushDescriptors(int frame) {
+	std::lock_guard<std::mutex> lock(pipelineLayoutsMutex_);
 	for (VKRPipelineLayout *iter : pipelineLayouts_) {
 		iter->FlushDescSets(vulkan_, frame, &frameData_[frame].profile);
 	}
 }
 
+// Called on the main thread, from BeginFrame.
 void VulkanRenderManager::ResetDescriptorLists(int frame) {
+	std::lock_guard<std::mutex> lock(pipelineLayoutsMutex_);
 	for (VKRPipelineLayout *iter : pipelineLayouts_) {
 		VKRPipelineLayout::FrameData &data = iter->frameData[frame];
 

Common/GPU/Vulkan/VulkanRenderManager.h

@@ -643,5 +643,12 @@ private:
 	HistoryBuffer<FrameTimeData, FRAME_TIME_HISTORY_LENGTH> &frameTimeHistory_;
 
 	VKRPipelineLayout *curPipelineLayout_ = nullptr;
+
+	// Guards pipelineLayouts_, both the vector itself and the lifetime of the layouts in it.
+	// The list is added to by CreatePipelineLayout and erased from by the deferred callback queued by
+	// DestroyPipelineLayout, both of which run on the main thread, while the render thread walks it
+	// every frame in FlushDescriptors. Contention is negligible - layouts are only created and destroyed
+	// when a game starts or stops, and the lock is otherwise taken exactly twice per frame.
+	std::mutex pipelineLayoutsMutex_;
 	std::vector<VKRPipelineLayout *> pipelineLayouts_;
 };