Merge pull request #16090 from hrydgard/more-vulkan-cleanup-work

Simplify synchronization in VulkanRenderManager

Common/GPU/Vulkan/VulkanFrameData.cpp

@@ -1,3 +1,5 @@
+#include <mutex>
+
 #include "VulkanFrameData.h"
 #include "Common/Log.h"
 
@@ -26,6 +28,7 @@ void FrameData::Init(VulkanContext *vulkan, int index) {
 
 	// Creating the frame fence with true so they can be instantly waited on the first frame
 	fence = vulkan->CreateFence(true);
+	readyForFence = true;
 
 	// This fence one is used for synchronizing readbacks. Does not need preinitialization.
 	readbackFence = vulkan->CreateFence(false);
@@ -169,7 +172,18 @@ void FrameData::SubmitPending(VulkanContext *vulkan, FrameSubmitType type, Frame
 		submit_info.signalSemaphoreCount = 1;
 		submit_info.pSignalSemaphores = &sharedData.renderingCompleteSemaphore;
 	}
-	VkResult res = vkQueueSubmit(vulkan->GetGraphicsQueue(), 1, &submit_info, fenceToTrigger);
+
+	VkResult res;
+	if (fenceToTrigger == fence) {
+		// The fence is waited on by the main thread, they are not allowed to access it simultaneously.
+		res = vkQueueSubmit(vulkan->GetGraphicsQueue(), 1, &submit_info, fenceToTrigger);
+		std::lock_guard<std::mutex> lock(fenceMutex);
+		readyForFence = true;
+		fenceCondVar.notify_one();
+	} else {
+		res = vkQueueSubmit(vulkan->GetGraphicsQueue(), 1, &submit_info, fenceToTrigger);
+	}
+
 	if (res == VK_ERROR_DEVICE_LOST) {
 		_assert_msg_(false, "Lost the Vulkan device in vkQueueSubmit! If this happens again, switch Graphics Backend away from Vulkan");
 	} else {
@@ -180,14 +194,7 @@ void FrameData::SubmitPending(VulkanContext *vulkan, FrameSubmitType type, Frame
 		// Hard stall of the GPU, not ideal, but necessary so the CPU has the contents of the readback.
 		vkWaitForFences(vulkan->GetDevice(), 1, &readbackFence, true, UINT64_MAX);
 		vkResetFences(vulkan->GetDevice(), 1, &readbackFence);
-	}
-
-	// When !triggerFence, we notify after syncing with Vulkan.
-	if (type == FrameSubmitType::Present || type == FrameSubmitType::Sync) {
-		VERBOSE_LOG(G3D, "PULL: Frame %d.readyForFence = true", index);
-		std::unique_lock<std::mutex> lock(push_mutex);
-		readyForFence = true;  // misnomer in sync mode!
-		push_condVar.notify_all();
+		syncDone = true;
 	}
 }
 

Common/GPU/Vulkan/VulkanFrameData.h

@@ -7,15 +7,14 @@
 
 #include "Common/GPU/Vulkan/VulkanContext.h"
 
-struct VKRStep;
-
 enum {
 	MAX_TIMESTAMP_QUERIES = 128,
 };
 
 enum class VKRRunType {
-	END,
+	PRESENT,
 	SYNC,
+	EXIT,
 };
 
 struct QueueProfileContext {
@@ -43,15 +42,11 @@ enum class FrameSubmitType {
 
 // Per-frame data, round-robin so we can overlap submission with execution of the previous frame.
 struct FrameData {
-	std::mutex push_mutex;
-	std::condition_variable push_condVar;
-
-	std::mutex pull_mutex;
-	std::condition_variable pull_condVar;
+	bool skipSwap = false;
 
+	std::mutex fenceMutex;
+	std::condition_variable fenceCondVar;
 	bool readyForFence = true;
-	bool readyForRun = false;  // protected by pull_mutex
-	bool skipSwap = false;
 
 	VkFence fence;
 	VkFence readbackFence;  // Strictly speaking we might only need one global of these.
@@ -68,9 +63,10 @@ struct FrameData {
 	bool hasMainCommands = false;
 	bool hasPresentCommands = false;
 
+	bool hasFencePending = false;
 	bool hasAcquired = false;
 
-	std::vector<VKRStep *> steps;
+	bool syncDone = false;
 
 	// Swapchain.
 	uint32_t curSwapchainImage = -1;
@@ -89,12 +85,6 @@ struct FrameData {
 	// This will only submit if we are actually recording init commands.
 	void SubmitPending(VulkanContext *vulkan, FrameSubmitType type, FrameDataShared &shared);
 
-	VKRRunType RunType() const {
-		return runType_;
-	}
-
-	VKRRunType runType_ = VKRRunType::END;
-
 private:
 	// Metadata for logging etc
 	int index;

Common/GPU/Vulkan/VulkanQueueRunner.cpp

@@ -554,7 +554,7 @@ void VulkanQueueRunner::PreprocessSteps(std::vector<VKRStep *> &steps) {
 	}
 }
 
-void VulkanQueueRunner::RunSteps(FrameData &frameData, FrameDataShared &frameDataShared) {
+void VulkanQueueRunner::RunSteps(std::vector<VKRStep *> &steps, FrameData &frameData, FrameDataShared &frameDataShared) {
 	QueueProfileContext *profile = frameData.profilingEnabled_ ? &frameData.profile : nullptr;
 
 	if (profile)
@@ -564,8 +564,8 @@ void VulkanQueueRunner::RunSteps(FrameData &frameData, FrameDataShared &frameDat
 
 	VkCommandBuffer cmd = frameData.hasPresentCommands ? frameData.presentCmd : frameData.mainCmd;
 
-	for (size_t i = 0; i < frameData.steps.size(); i++) {
-		const VKRStep &step = *frameData.steps[i];
+	for (size_t i = 0; i < steps.size(); i++) {
+		const VKRStep &step = *steps[i];
 
 		if (emitLabels) {
 			VkDebugUtilsLabelEXT labelInfo{ VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT };
@@ -625,11 +625,11 @@ void VulkanQueueRunner::RunSteps(FrameData &frameData, FrameDataShared &frameDat
 
 	// Deleting all in one go should be easier on the instruction cache than deleting
 	// them as we go - and easier to debug because we can look backwards in the frame.
-	for (auto step : frameData.steps) {
+	for (auto step : steps) {
 		delete step;
 	}
 
-	frameData.steps.clear();
+	steps.clear();
 
 	if (profile)
 		profile->cpuEndTime = time_now_d();

Common/GPU/Vulkan/VulkanQueueRunner.h

@@ -247,6 +247,14 @@ class VKRRenderPass {
 	RPKey key_;
 };
 
+// These are enqueued from the main thread,
+// and the render thread pops them off
+struct VKRRenderThreadTask {
+	std::vector<VKRStep *> steps;
+	int frame;
+	VKRRunType runType;
+};
+
 class VulkanQueueRunner {
 public:
 	VulkanQueueRunner(VulkanContext *vulkan) : vulkan_(vulkan), renderPasses_(16) {}
@@ -257,7 +265,7 @@ class VulkanQueueRunner {
 	}
 
 	void PreprocessSteps(std::vector<VKRStep *> &steps);
-	void RunSteps(FrameData &frameData, FrameDataShared &frameDataShared);
+	void RunSteps(std::vector<VKRStep *> &steps, FrameData &frameData, FrameDataShared &frameDataShared);
 	void LogSteps(const std::vector<VKRStep *> &steps, bool verbose);
 
 	std::string StepToString(const VKRStep &step) const;