Merge pull request #18183 from hrydgard/shader-race-condition-fix

Pipeline/shader race-condition-during-shutdown crash fix

Common/GPU/Vulkan/VulkanImage.cpp

@@ -130,9 +130,12 @@ bool VulkanTexture::CreateDirect(VkCommandBuffer cmd, int w, int h, int depth, i
 
 	res = vkCreateImageView(vulkan_->GetDevice(), &view_info, NULL, &view_);
 	if (res != VK_SUCCESS) {
-		ERROR_LOG(G3D, "vkCreateImageView failed: %s", VulkanResultToString(res));
-		// This leaks the image.
+		ERROR_LOG(G3D, "vkCreateImageView failed: %s. Destroying image.", VulkanResultToString(res));
 		_assert_(res == VK_ERROR_OUT_OF_HOST_MEMORY || res == VK_ERROR_OUT_OF_DEVICE_MEMORY || res == VK_ERROR_TOO_MANY_OBJECTS);
+		vmaDestroyImage(vulkan_->Allocator(), image_, allocation_);
+		view_ = VK_NULL_HANDLE;
+		image_ = VK_NULL_HANDLE;
+		allocation_ = VK_NULL_HANDLE;
 		return false;
 	}
 	vulkan_->SetDebugName(view_, VK_OBJECT_TYPE_IMAGE_VIEW, tag_);
@@ -141,6 +144,7 @@ bool VulkanTexture::CreateDirect(VkCommandBuffer cmd, int w, int h, int depth, i
 	if (view_info.viewType == VK_IMAGE_VIEW_TYPE_2D) {
 		view_info.viewType = VK_IMAGE_VIEW_TYPE_2D_ARRAY;
 		res = vkCreateImageView(vulkan_->GetDevice(), &view_info, NULL, &arrayView_);
+		// Assume that if the above view creation succeeded, so will this.
 		_assert_(res == VK_SUCCESS);
 		vulkan_->SetDebugName(arrayView_, VK_OBJECT_TYPE_IMAGE_VIEW, tag_);
 	}

Common/GPU/Vulkan/VulkanRenderManager.cpp

@@ -29,6 +29,10 @@ using namespace PPSSPP_VK;
 
 // renderPass is an example of the "compatibility class" or RenderPassType type.
 bool VKRGraphicsPipeline::Create(VulkanContext *vulkan, VkRenderPass compatibleRenderPass, RenderPassType rpType, VkSampleCountFlagBits sampleCount, double scheduleTime, int countToCompile) {
+	// Good torture test to test the shutdown-while-precompiling-shaders issue on PC where it's normally
+	// hard to catch because shaders compile so fast.
+	// sleep_ms(200);
+
 	bool multisample = RenderPassTypeHasMultisample(rpType);
 	if (multisample) {
 		if (sampleCount_ != VK_SAMPLE_COUNT_FLAG_BITS_MAX_ENUM) {
@@ -195,6 +199,14 @@ VKRGraphicsPipeline::~VKRGraphicsPipeline() {
 		desc->Release();
 }
 
+void VKRGraphicsPipeline::BlockUntilCompiled() {
+	for (size_t i = 0; i < (size_t)RenderPassType::TYPE_COUNT; i++) {
+		if (pipeline[i]) {
+			pipeline[i]->BlockUntilReady();
+		}
+	}
+}
+
 void VKRGraphicsPipeline::QueueForDeletion(VulkanContext *vulkan) {
 	// Can't destroy variants here, the pipeline still lives for a while.
 	vulkan->Delete().QueueCallback([](VulkanContext *vulkan, void *p) {

Common/GPU/Vulkan/VulkanRenderManager.h

@@ -131,6 +131,10 @@ struct VKRGraphicsPipeline {
 	// This deletes the whole VKRGraphicsPipeline, you must remove your last pointer to it when doing this.
 	void QueueForDeletion(VulkanContext *vulkan);
 
+	// This blocks until any background compiles are finished.
+	// Used during game shutdown before we clear out shaders that these compiles depend on.
+	void BlockUntilCompiled();
+
 	u32 GetVariantsBitmask() const;
 
 	void LogCreationFailure() const;

GPU/Vulkan/GPU_Vulkan.cpp

@@ -186,12 +186,16 @@ GPU_Vulkan::~GPU_Vulkan() {
 	}
 
 	SaveCache(shaderCachePath_);
+
+	// Super important to delete pipeline manager FIRST, before clearing shaders, so we wait for all pending pipelines to finish compiling.
+	delete pipelineManager_;
+	pipelineManager_ = nullptr;
+
 	// Note: We save the cache in DeviceLost
 	DestroyDeviceObjects();
 	drawEngine_.DeviceLost();
 	shaderManager_->ClearShaders();
 
-	delete pipelineManager_;
 	// other managers are deleted in ~GPUCommonHW.
 
 	if (draw_) {

GPU/Vulkan/PipelineManagerVulkan.cpp

@@ -29,6 +29,15 @@ PipelineManagerVulkan::PipelineManagerVulkan(VulkanContext *vulkan) : pipelines_
 }
 
 PipelineManagerVulkan::~PipelineManagerVulkan() {
+	// Block on all pipelines to make sure any background compiles are done.
+	// This is very important to do before we start trying to tear down the shaders - otherwise, we might
+	// be deleting shaders before queued pipeline creations that use them are performed.
+	pipelines_.Iterate([&](const VulkanPipelineKey &key, VulkanPipeline *value) {
+		if (value->pipeline) {
+			value->pipeline->BlockUntilCompiled();
+		}
+	});
+
 	Clear();
 	if (pipelineCache_ != VK_NULL_HANDLE)
 		vulkan_->Delete().QueueDeletePipelineCache(pipelineCache_);

GPU/Vulkan/TextureCacheVulkan.cpp

@@ -474,11 +474,6 @@ void TextureCacheVulkan::BuildTexture(TexCacheEntry *const entry) {
 
 	delete entry->vkTex;
 
-	char texName[64]{};
-	snprintf(texName, sizeof(texName), "tex_%08x_%s", entry->addr, GeTextureFormatToString((GETextureFormat)entry->format, gstate.getClutPaletteFormat()));
-	entry->vkTex = new VulkanTexture(vulkan, texName);
-	VulkanTexture *image = entry->vkTex;
-
 	VkImageLayout imageLayout = VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL;
 	VkImageUsageFlags usage = VK_IMAGE_USAGE_TRANSFER_SRC_BIT | VK_IMAGE_USAGE_TRANSFER_DST_BIT | VK_IMAGE_USAGE_SAMPLED_BIT;
 
@@ -507,6 +502,10 @@ void TextureCacheVulkan::BuildTexture(TexCacheEntry *const entry) {
 	default:                 mapping = &VULKAN_8888_SWIZZLE; break;  // no swizzle
 	}
 
+	char texName[64]{};
+	snprintf(texName, sizeof(texName), "tex_%08x_%s", entry->addr, GeTextureFormatToString((GETextureFormat)entry->format, gstate.getClutPaletteFormat()));
+	entry->vkTex = new VulkanTexture(vulkan, texName);
+	VulkanTexture *image = entry->vkTex;
 	bool allocSuccess = image->CreateDirect(cmdInit, plan.createW, plan.createH, plan.depth, plan.levelsToCreate, actualFmt, imageLayout, usage, mapping);
 	if (!allocSuccess && !lowMemoryMode_) {
 		WARN_LOG_REPORT(G3D, "Texture cache ran out of GPU memory; switching to low memory mode");