Merge pull request #18211 from hrydgard/more-crash-fix-attempts

More crash fix attempts

Common/Data/Collections/Hashmaps.h

@@ -135,6 +135,7 @@ class DenseHashMap {
 		return false;
 	}
 
+	// This will never crash if you call it without locking - but, the value might not be right.
 	size_t size() const {
 		return count_;
 	}

Common/GPU/Vulkan/thin3d_vulkan.cpp

@@ -874,8 +874,11 @@ VKContext::VKContext(VulkanContext *vulkan, bool useRenderThread)
 	caps_.tesselationShaderSupported = vulkan->GetDeviceFeatures().enabled.standard.tessellationShader != 0;
 	caps_.dualSourceBlend = vulkan->GetDeviceFeatures().enabled.standard.dualSrcBlend != 0;
 	caps_.depthClampSupported = vulkan->GetDeviceFeatures().enabled.standard.depthClamp != 0;
+
+	// Comment out these two to test geometry shader culling on any geometry shader-supporting hardware.
 	caps_.clipDistanceSupported = vulkan->GetDeviceFeatures().enabled.standard.shaderClipDistance != 0;
 	caps_.cullDistanceSupported = vulkan->GetDeviceFeatures().enabled.standard.shaderCullDistance != 0;
+
 	caps_.framebufferBlitSupported = true;
 	caps_.framebufferCopySupported = true;
 	caps_.framebufferDepthBlitSupported = vulkan->GetDeviceInfo().canBlitToPreferredDepthStencilFormat;

GPU/Vulkan/DrawEngineVulkan.cpp

@@ -99,8 +99,8 @@ void DrawEngineVulkan::InitDeviceObjects() {
 	bindings[3].descriptorCount = 1;
 	bindings[3].descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC;
 	bindings[3].stageFlags = VK_SHADER_STAGE_VERTEX_BIT | VK_SHADER_STAGE_FRAGMENT_BIT;
-	if (gstate_c.Use(GPU_USE_GS_CULLING))
-		bindings[3].stageFlags |= VK_SHADER_STAGE_GEOMETRY_BIT;
+	if (draw_->GetDeviceCaps().geometryShaderSupported)
+		bindings[3].stageFlags |= VK_SHADER_STAGE_GEOMETRY_BIT;  // unlikely to have a penalty. if we check GPU_USE_GS_CULLING, we have problems on runtime toggle.
 	bindings[3].binding = DRAW_BINDING_DYNUBO_BASE;
 	bindings[4].descriptorCount = 1;
 	bindings[4].descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC;

GPU/Vulkan/GPU_Vulkan.cpp

@@ -25,6 +25,7 @@
 #include "Common/GraphicsContext.h"
 #include "Common/Serialize/Serializer.h"
 #include "Common/TimeUtil.h"
+#include "Common/Thread/ThreadUtil.h"
 
 #include "Core/Config.h"
 #include "Core/Debugger/Breakpoints.h"
@@ -94,11 +95,12 @@ GPU_Vulkan::GPU_Vulkan(GraphicsContext *gfxCtx, Draw::DrawContext *draw)
 		shaderCachePath_ = GetSysDirectory(DIRECTORY_APP_CACHE) / (discID + ".vkshadercache");
 		shaderCacheLoaded_ = false;
 
-		std::thread th([&] {
+		shaderCacheLoadThread_ = std::thread([&] {
+			SetCurrentThreadName("VulkanLoadCache");
+			AndroidJNIThreadContext ctx;
 			LoadCache(shaderCachePath_);
 			shaderCacheLoaded_ = true;
 		});
-		th.detach();
 	} else {
 		shaderCacheLoaded_ = true;
 	}
@@ -180,6 +182,10 @@ void GPU_Vulkan::SaveCache(const Path &filename) {
 }
 
 GPU_Vulkan::~GPU_Vulkan() {
+	if (shaderCacheLoadThread_.joinable()) {
+		shaderCacheLoadThread_.join();
+	}
+
 	if (draw_) {
 		VulkanRenderManager *rm = (VulkanRenderManager *)draw_->GetNativeObject(Draw::NativeObject::RENDER_MANAGER);
 		rm->DrainAndBlockCompileQueue();

GPU/Vulkan/GPU_Vulkan.h

@@ -19,6 +19,7 @@
 
 #include <string>
 #include <vector>
+#include <thread>
 
 #include "Common/File/Path.h"
 
@@ -84,4 +85,6 @@ class GPU_Vulkan : public GPUCommonHW {
 
 	Path shaderCachePath_;
 	std::atomic<bool> shaderCacheLoaded_{};
+
+	std::thread shaderCacheLoadThread_;
 };

GPU/Vulkan/ShaderManagerVulkan.cpp

@@ -239,6 +239,8 @@ void ShaderManagerVulkan::DeviceRestore(Draw::DrawContext *draw) {
 }
 
 void ShaderManagerVulkan::Clear() {
+	std::lock_guard<std::mutex> guard(cacheLock_);
+
 	fsCache_.Iterate([&](const FShaderID &key, VulkanFragmentShader *shader) {
 		delete shader;
 	});
@@ -333,6 +335,8 @@ void ShaderManagerVulkan::GetShaders(int prim, VertexDecoder *decoder, VulkanVer
 		return;
 	}
 
+	std::lock_guard<std::mutex> guard(cacheLock_);
+
 	VulkanContext *vulkan = (VulkanContext *)draw_->GetNativeObject(Draw::NativeObject::CONTEXT);
 	VulkanVertexShader *vs = nullptr;
 	if (!vsCache_.Get(VSID, &vs)) {
@@ -345,14 +349,12 @@ void ShaderManagerVulkan::GetShaders(int prim, VertexDecoder *decoder, VulkanVer
 		_assert_msg_(success, "VS gen error: %s", genErrorString.c_str());
 		_assert_msg_(strlen(codeBuffer_) < CODE_BUFFER_SIZE, "VS length error: %d", (int)strlen(codeBuffer_));
 
-		std::lock_guard<std::mutex> guard(cacheLock_);
-		if (!vsCache_.Get(VSID, &vs)) {
-			vs = new VulkanVertexShader(vulkan, VSID, flags, codeBuffer_, useHWTransform);
-			vsCache_.Insert(VSID, vs);
-		}
+		// Don't need to re-lookup anymore, now that we lock wider.
+		vs = new VulkanVertexShader(vulkan, VSID, flags, codeBuffer_, useHWTransform);
+		vsCache_.Insert(VSID, vs);
 	}
 
-	VulkanFragmentShader *fs;
+	VulkanFragmentShader *fs = nullptr;
 	if (!fsCache_.Get(FSID, &fs)) {
 		// Fragment shader not in cache. Let's compile it.
 		std::string genErrorString;
@@ -362,14 +364,11 @@ void ShaderManagerVulkan::GetShaders(int prim, VertexDecoder *decoder, VulkanVer
 		_assert_msg_(success, "FS gen error: %s", genErrorString.c_str());
 		_assert_msg_(strlen(codeBuffer_) < CODE_BUFFER_SIZE, "FS length error: %d", (int)strlen(codeBuffer_));
 
-		std::lock_guard<std::mutex> guard(cacheLock_);
-		if (!fsCache_.Get(FSID, &fs)) {
-			fs = new VulkanFragmentShader(vulkan, FSID, flags, codeBuffer_);
-			fsCache_.Insert(FSID, fs);
-		}
+		fs = new VulkanFragmentShader(vulkan, FSID, flags, codeBuffer_);
+		fsCache_.Insert(FSID, fs);
 	}
 
-	VulkanGeometryShader *gs;
+	VulkanGeometryShader *gs = nullptr;
 	if (GSID.Bit(GS_BIT_ENABLED)) {
 		if (!gsCache_.Get(GSID, &gs)) {
 			// Geometry shader not in cache. Let's compile it.
@@ -378,11 +377,8 @@ void ShaderManagerVulkan::GetShaders(int prim, VertexDecoder *decoder, VulkanVer
 			_assert_msg_(success, "GS gen error: %s", genErrorString.c_str());
 			_assert_msg_(strlen(codeBuffer_) < CODE_BUFFER_SIZE, "GS length error: %d", (int)strlen(codeBuffer_));
 
-			std::lock_guard<std::mutex> guard(cacheLock_);
-			if (!gsCache_.Get(GSID, &gs)) {
-				gs = new VulkanGeometryShader(vulkan, GSID, codeBuffer_);
-				gsCache_.Insert(GSID, gs);
-			}
+			gs = new VulkanGeometryShader(vulkan, GSID, codeBuffer_);
+			gsCache_.Insert(GSID, gs);
 		}
 	} else {
 		gs = nullptr;
@@ -403,6 +399,7 @@ void ShaderManagerVulkan::GetShaders(int prim, VertexDecoder *decoder, VulkanVer
 }
 
 std::vector<std::string> ShaderManagerVulkan::DebugGetShaderIDs(DebugShaderType type) {
+	std::lock_guard<std::mutex> guard(cacheLock_);
 	std::vector<std::string> ids;
 	switch (type) {
 	case SHADER_TYPE_VERTEX:
@@ -621,24 +618,27 @@ bool ShaderManagerVulkan::LoadCache(FILE *f) {
 		}
 	}
 
-	for (int i = 0; i < header.numGeometryShaders; i++) {
-		GShaderID id;
-		if (fread(&id, sizeof(id), 1, f) != 1) {
-			ERROR_LOG(G3D, "Vulkan shader cache truncated (in GeometryShaders)");
-			return false;
-		}
-		std::string genErrorString;
-		if (!GenerateGeometryShader(id, codeBuffer_, compat_, draw_->GetBugs(), &genErrorString)) {
-			ERROR_LOG(G3D, "Failed to generate geometry shader during cache load");
-			// We just ignore this one and carry on.
-			failCount++;
-			continue;
-		}
-		_assert_msg_(strlen(codeBuffer_) < CODE_BUFFER_SIZE, "GS length error: %d", (int)strlen(codeBuffer_));
-		std::lock_guard<std::mutex> guard(cacheLock_);
-		if (!gsCache_.ContainsKey(id)) {
-			VulkanGeometryShader *gs = new VulkanGeometryShader(vulkan, id, codeBuffer_);
-			gsCache_.Insert(id, gs);
+	// If it's not enabled, don't create shaders cached from earlier runs - creation will likely fail.
+	if (gstate_c.Use(GPU_USE_GS_CULLING)) {
+		for (int i = 0; i < header.numGeometryShaders; i++) {
+			GShaderID id;
+			if (fread(&id, sizeof(id), 1, f) != 1) {
+				ERROR_LOG(G3D, "Vulkan shader cache truncated (in GeometryShaders)");
+				return false;
+			}
+			std::string genErrorString;
+			if (!GenerateGeometryShader(id, codeBuffer_, compat_, draw_->GetBugs(), &genErrorString)) {
+				ERROR_LOG(G3D, "Failed to generate geometry shader during cache load");
+				// We just ignore this one and carry on.
+				failCount++;
+				continue;
+			}
+			_assert_msg_(strlen(codeBuffer_) < CODE_BUFFER_SIZE, "GS length error: %d", (int)strlen(codeBuffer_));
+			std::lock_guard<std::mutex> guard(cacheLock_);
+			if (!gsCache_.ContainsKey(id)) {
+				VulkanGeometryShader *gs = new VulkanGeometryShader(vulkan, id, codeBuffer_);
+				gsCache_.Insert(id, gs);
+			}
 		}
 	}
 

GPU/Vulkan/ShaderManagerVulkan.h

@@ -121,9 +121,10 @@ class ShaderManagerVulkan : public ShaderManagerCommon {
 	int GetNumGeometryShaders() const { return (int)gsCache_.size(); }
 
 	// Used for saving/loading the cache. Don't need to be particularly fast.
-	VulkanVertexShader *GetVertexShaderFromID(VShaderID id) { return vsCache_.GetOrNull(id); }
-	VulkanFragmentShader *GetFragmentShaderFromID(FShaderID id) { return fsCache_.GetOrNull(id); }
-	VulkanGeometryShader *GetGeometryShaderFromID(GShaderID id) { return gsCache_.GetOrNull(id); }
+	VulkanVertexShader *GetVertexShaderFromID(VShaderID id) { std::lock_guard<std::mutex> guard(cacheLock_); return vsCache_.GetOrNull(id); }
+	VulkanFragmentShader *GetFragmentShaderFromID(FShaderID id) { std::lock_guard<std::mutex> guard(cacheLock_); return fsCache_.GetOrNull(id); }
+	VulkanGeometryShader *GetGeometryShaderFromID(GShaderID id) { std::lock_guard<std::mutex> guard(cacheLock_); return gsCache_.GetOrNull(id); }
+
 	VulkanVertexShader *GetVertexShaderFromModule(VkShaderModule module);
 	VulkanFragmentShader *GetFragmentShaderFromModule(VkShaderModule module);
 	VulkanGeometryShader *GetGeometryShaderFromModule(VkShaderModule module);