Merge pull request #18793 from hrydgard/render-pass-cleanup

Fix GE framedump playback on Vulkan

Author: hrydgard

Common/GPU/Vulkan/VulkanFrameData.cpp

@@ -169,7 +169,7 @@ VkCommandBuffer FrameData::GetInitCmd(VulkanContext *vulkan) {
 	return initCmd;
 }
 
-void FrameData::SubmitPending(VulkanContext *vulkan, FrameSubmitType type, FrameDataShared &sharedData) {
+void FrameData::Submit(VulkanContext *vulkan, FrameSubmitType type, FrameDataShared &sharedData) {
 	VkCommandBuffer cmdBufs[3];
 	int numCmdBufs = 0;
 
@@ -200,14 +200,16 @@ void FrameData::SubmitPending(VulkanContext *vulkan, FrameSubmitType type, Frame
 		hasMainCommands = false;
 	}
 
-	if (hasPresentCommands && type != FrameSubmitType::Pending) {
+	if (hasPresentCommands) {
+		_dbg_assert_(type == FrameSubmitType::FinishFrame);
 		VkResult res = vkEndCommandBuffer(presentCmd);
+
 		_assert_msg_(res == VK_SUCCESS, "vkEndCommandBuffer failed (present)! result=%s", VulkanResultToString(res));
 
 		cmdBufs[numCmdBufs++] = presentCmd;
 		hasPresentCommands = false;
 
-		if (type == FrameSubmitType::Present) {
+		if (type == FrameSubmitType::FinishFrame) {
 			fenceToTrigger = fence;
 		}
 	}
@@ -219,15 +221,15 @@ void FrameData::SubmitPending(VulkanContext *vulkan, FrameSubmitType type, Frame
 
 	VkSubmitInfo submit_info{ VK_STRUCTURE_TYPE_SUBMIT_INFO };
 	VkPipelineStageFlags waitStage[1]{ VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT };
-	if (type == FrameSubmitType::Present && !skipSwap) {
+	if (type == FrameSubmitType::FinishFrame && !skipSwap) {
 		_dbg_assert_(hasAcquired);
 		submit_info.waitSemaphoreCount = 1;
 		submit_info.pWaitSemaphores = &acquireSemaphore;
 		submit_info.pWaitDstStageMask = waitStage;
 	}
 	submit_info.commandBufferCount = (uint32_t)numCmdBufs;
 	submit_info.pCommandBuffers = cmdBufs;
-	if (type == FrameSubmitType::Present && !skipSwap) {
+	if (type == FrameSubmitType::FinishFrame && !skipSwap) {
 		submit_info.signalSemaphoreCount = 1;
 		submit_info.pSignalSemaphores = &renderingCompleteSemaphore;
 	}

Common/GPU/Vulkan/VulkanFrameData.h

@@ -65,7 +65,7 @@ struct FrameDataShared {
 enum class FrameSubmitType {
 	Pending,
 	Sync,
-	Present,
+	FinishFrame,
 };
 
 // Per-frame data, round-robin so we can overlap submission with execution of the previous frame.
@@ -121,8 +121,8 @@ struct FrameData {
 	// Generally called from the main thread, unlike most of the rest.
 	VkCommandBuffer GetInitCmd(VulkanContext *vulkan);
 
-	// This will only submit if we are actually recording init commands.
-	void SubmitPending(VulkanContext *vulkan, FrameSubmitType type, FrameDataShared &shared);
+	// Submits pending command buffers.
+	void Submit(VulkanContext *vulkan, FrameSubmitType type, FrameDataShared &shared);
 
 private:
 	// Metadata for logging etc

Common/GPU/Vulkan/VulkanQueueRunner.cpp

@@ -369,7 +369,7 @@ void VulkanQueueRunner::RunSteps(std::vector<VKRStep *> &steps, int curFrame, Fr
 				if (emitLabels) {
 					vkCmdEndDebugUtilsLabelEXT(cmd);
 				}
-				frameData.SubmitPending(vulkan_, FrameSubmitType::Pending, frameDataShared);
+				frameData.Submit(vulkan_, FrameSubmitType::Pending, frameDataShared);
 
 				// When stepping in the GE debugger, we can end up here multiple times in a "frame".
 				// So only acquire once.

Common/GPU/Vulkan/VulkanRenderManager.cpp

@@ -952,6 +952,11 @@ void VulkanRenderManager::BindFramebufferAsRenderTarget(VKRFramebuffer *fb, VKRR
 		EndCurRenderStep();
 	}
 
+	// Sanity check that we don't have binds to the backbuffer before binds to other buffers. It must always be bound last.
+	if (steps_.size() >= 1 && steps_.back()->stepType == VKRStepType::RENDER && steps_.back()->render.framebuffer == nullptr && fb != nullptr) {
+		_dbg_assert_(false);
+	}
+
 	// Older Mali drivers have issues with depth and stencil don't match load/clear/etc.
 	// TODO: Determine which versions and do this only where necessary.
 	u32 lateClearMask = 0;
@@ -1383,6 +1388,7 @@ void VulkanRenderManager::Finish() {
 	EndCurRenderStep();
 
 	// Let's do just a bit of cleanup on render commands now.
+	// TODO: Should look into removing this.
 	for (auto &step : steps_) {
 		if (step->stepType == VKRStepType::RENDER) {
 			CleanupRenderCommands(&step->commands);
@@ -1469,7 +1475,7 @@ void VulkanRenderManager::Run(VKRRenderThreadTask &task) {
 	if (!frameTimeHistory_[frameData.frameId].firstSubmit) {
 		frameTimeHistory_[frameData.frameId].firstSubmit = time_now_d();
 	}
-	frameData.SubmitPending(vulkan_, FrameSubmitType::Pending, frameDataShared_);
+	frameData.Submit(vulkan_, FrameSubmitType::Pending, frameDataShared_);
 
 	// Flush descriptors.
 	double descStart = time_now_d();
@@ -1506,12 +1512,12 @@ void VulkanRenderManager::Run(VKRRenderThreadTask &task) {
 
 	switch (task.runType) {
 	case VKRRunType::SUBMIT:
-		frameData.SubmitPending(vulkan_, FrameSubmitType::Present, frameDataShared_);
+		frameData.Submit(vulkan_, FrameSubmitType::FinishFrame, frameDataShared_);
 		break;
 
 	case VKRRunType::SYNC:
 		// The submit will trigger the readbackFence, and also do the wait for it.
-		frameData.SubmitPending(vulkan_, FrameSubmitType::Sync, frameDataShared_);
+		frameData.Submit(vulkan_, FrameSubmitType::Sync, frameDataShared_);
 
 		if (useRenderThread_) {
 			std::unique_lock<std::mutex> lock(syncMutex_);

GPU/Common/FramebufferManagerCommon.cpp

@@ -1547,6 +1547,7 @@ void FramebufferManagerCommon::CopyDisplayToOutput(bool reallyDirty) {
 		// No framebuffer to display! Clear to black.
 		if (useBufferedRendering_) {
 			draw_->BindFramebufferAsRenderTarget(nullptr, { Draw::RPAction::CLEAR, Draw::RPAction::CLEAR, Draw::RPAction::CLEAR }, "CopyDisplayToOutput");
+			presentation_->NotifyPresent();
 		}
 		gstate_c.Dirty(DIRTY_VIEWPORTSCISSOR_STATE);
 		return;

GPU/Common/PresentationCommon.h

@@ -104,6 +104,10 @@ class PresentationCommon {
 	bool PresentedThisFrame() const {
 		return presentedThisFrame_;
 	}
+	void NotifyPresent() {
+		// Something else did the present, skipping PresentationCommon.
+		presentedThisFrame_ = true;
+	}
 
 	void DeviceLost();
 	void DeviceRestore(Draw::DrawContext *draw);

GPU/Debugger/Playback.cpp

@@ -308,7 +308,7 @@ class DumpExecute {
 	void Memcpy(u32 ptr, u32 sz);
 	void Texture(int level, u32 ptr, u32 sz);
 	void Framebuf(int level, u32 ptr, u32 sz);
-	void Display(u32 ptr, u32 sz);
+	void Display(u32 ptr, u32 sz, bool allowFlip);
 	void EdramTrans(u32 ptr, u32 sz);
 
 	u32 execMemcpyDest = 0;
@@ -616,7 +616,7 @@ void DumpExecute::Framebuf(int level, u32 ptr, u32 sz) {
 	}
 }
 
-void DumpExecute::Display(u32 ptr, u32 sz) {
+void DumpExecute::Display(u32 ptr, u32 sz, bool allowFlip) {
 	struct DisplayBufData {
 		PSPPointer<u8> topaddr;
 		int linesize, pixelFormat;
@@ -628,7 +628,9 @@ void DumpExecute::Display(u32 ptr, u32 sz) {
 	SyncStall();
 
 	__DisplaySetFramebuf(disp->topaddr.ptr, disp->linesize, disp->pixelFormat, 1);
-	__DisplaySetFramebuf(disp->topaddr.ptr, disp->linesize, disp->pixelFormat, 0);
+	if (allowFlip) {
+		__DisplaySetFramebuf(disp->topaddr.ptr, disp->linesize, disp->pixelFormat, 0);
+	}
 }
 
 void DumpExecute::EdramTrans(u32 ptr, u32 sz) {
@@ -657,7 +659,8 @@ bool DumpExecute::Run() {
 	if (gpu)
 		gpu->SetAddrTranslation(0x400);
 
-	for (const Command &cmd : commands_) {
+	for (size_t i = 0; i < commands_.size(); i++) {
+		const Command &cmd = commands_[i];
 		switch (cmd.type) {
 		case CommandType::INIT:
 			Init(cmd.ptr, cmd.sz);
@@ -726,7 +729,7 @@ bool DumpExecute::Run() {
 			break;
 
 		case CommandType::DISPLAY:
-			Display(cmd.ptr, cmd.sz);
+			Display(cmd.ptr, cmd.sz, i == commands_.size() - 1);
 			break;
 
 		default:

UI/EmuScreen.cpp

@@ -1342,13 +1342,13 @@ ScreenRenderFlags EmuScreen::render(ScreenRenderMode mode) {
 	if (mode & ScreenRenderMode::TOP) {
 		System_Notify(SystemNotification::KEEP_SCREEN_AWAKE);
 	} else if (!Core_ShouldRunBehind() && strcmp(screenManager()->topScreen()->tag(), "DevMenu") != 0) {
-		// Not on top. Let's not execute, only draw the image.
-		draw->BindFramebufferAsRenderTarget(nullptr, { RPAction::CLEAR, RPAction::CLEAR, RPAction::CLEAR, }, "EmuScreen_Stepping");
 		// Just to make sure.
 		if (PSP_IsInited() && !g_Config.bSkipBufferEffects) {
 			PSP_BeginHostFrame();
 			gpu->CopyDisplayToOutput(true);
 			PSP_EndHostFrame();
+		} else {
+			draw->BindFramebufferAsRenderTarget(nullptr, { RPAction::CLEAR, RPAction::CLEAR, RPAction::CLEAR, }, "EmuScreen_Stepping");
 		}
 		// Need to make sure the UI texture is available, for "darken".
 		screenManager()->getUIContext()->BeginFrame();