Merge pull request #23037 from hrydgard/vulkan-pipeline-wait-lock

Vulkan: Don't hold a pipeline's lock while waiting for it to compile

Common/GPU/Vulkan/VulkanQueueRunner.cpp

@@ -1281,21 +1281,29 @@ void VulkanQueueRunner::PerformRenderPass(const VKRStep &step, VkCommandBuffer c
 						"expected %d sample count, got %d", fbSampleCount, graphicsPipeline->SampleCount());
 				}
 
-				VkPipeline pipeline;
-
+				Promise<VkPipeline> *promise;
+				bool compileHere = false;
 				{
 					std::lock_guard<std::mutex> lock(graphicsPipeline->mutex_);
-					if (!graphicsPipeline->pipeline[(size_t)rpType]) {
+					promise = graphicsPipeline->pipeline[(size_t)rpType];
+					if (!promise) {
 						// NOTE: If render steps got merged, it can happen that, as they ended during recording,
 						// they didn't know their final render pass type so they created the wrong pipelines in EndCurRenderStep().
 						// Unfortunately I don't know if we can fix it in any more sensible place than here.
 						// Maybe a middle pass. But let's try to just block and compile here for now, this doesn't
 						// happen all that much.
-						graphicsPipeline->pipeline[(size_t)rpType] = Promise<VkPipeline>::CreateEmpty();
-						graphicsPipeline->Create(vulkan_, renderPass->Get(vulkan_, rpType, fbSampleCount), rpType, fbSampleCount, time_now_d(), -1);
+						promise = Promise<VkPipeline>::CreateEmpty();
+						graphicsPipeline->pipeline[(size_t)rpType] = promise;
+						compileHere = true;
 					}
-					pipeline = graphicsPipeline->pipeline[(size_t)rpType]->BlockUntilReady();
 				}
+				// Not under the lock: the main thread takes it for every pipeline a render step used
+				// (EndCurRenderStep), and waiting for the shaders and the compile can take hundreds of ms.
+				// DestroyVariants deletes Promises later, so this one stays valid.
+				if (compileHere) {
+					graphicsPipeline->Create(vulkan_, renderPass->Get(vulkan_, rpType, fbSampleCount), rpType, fbSampleCount, time_now_d(), -1);
+				}
+				VkPipeline pipeline = promise->BlockUntilReady();
 
 				if (pipeline != VK_NULL_HANDLE) {
 					vkCmdBindPipeline(cmd, VK_PIPELINE_BIND_POINT_GRAPHICS, pipeline);

Common/GPU/Vulkan/VulkanRenderManager.cpp

@@ -171,8 +171,8 @@ 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.
+	// as protecting the array. The render thread waits for a Promise without the lock, so the Promise
+	// is deleted later, like the VkPipeline.
 	std::lock_guard<std::mutex> lock(mutex_);
 
 	for (size_t i = 0; i < (size_t)RenderPassType::TYPE_COUNT; i++) {
@@ -188,7 +188,9 @@ void VKRGraphicsPipeline::DestroyVariants(VulkanContext *vulkan, bool msaaOnly)
 		}
 		// 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];
+		vulkan->Delete().QueueCallback([promise = this->pipeline[i]](VulkanContext *vulkan) {
+			delete promise;
+		});
 		this->pipeline[i] = nullptr;
 	}
 	sampleCount_ = VK_SAMPLE_COUNT_FLAG_BITS_MAX_ENUM;