From ea180e54c9c5f16c45761663c5aa987693bd6066 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 30 Jan 2024 10:44:18 +0100 Subject: [PATCH 1/3] Fix some "double-binds" of the backbuffer. These are already eliminated by the queuerunner, but better not to generate them in the first place, for easier sanity checks. --- Common/GPU/Vulkan/VulkanRenderManager.cpp | 1 + GPU/Common/FramebufferManagerCommon.cpp | 1 + GPU/Common/PresentationCommon.h | 4 ++++ UI/EmuScreen.cpp | 4 ++-- 4 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Common/GPU/Vulkan/VulkanRenderManager.cpp b/Common/GPU/Vulkan/VulkanRenderManager.cpp index d86452e96a2d..1ad544237597 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.cpp +++ b/Common/GPU/Vulkan/VulkanRenderManager.cpp @@ -1383,6 +1383,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); diff --git a/GPU/Common/FramebufferManagerCommon.cpp b/GPU/Common/FramebufferManagerCommon.cpp index c50f59e94c44..f705277f5833 100644 --- a/GPU/Common/FramebufferManagerCommon.cpp +++ b/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; diff --git a/GPU/Common/PresentationCommon.h b/GPU/Common/PresentationCommon.h index 21be2359b3f5..7e4877f11d00 100644 --- a/GPU/Common/PresentationCommon.h +++ b/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); diff --git a/UI/EmuScreen.cpp b/UI/EmuScreen.cpp index 2075fb33381a..8b4adcde240d 100644 --- a/UI/EmuScreen.cpp +++ b/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(); From 25a1e6aa1438570d6ab45c3510aa7d6bd3a978f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 30 Jan 2024 11:14:21 +0100 Subject: [PATCH 2/3] Some renaming, add a bunch of sanity debug-asserts --- Common/GPU/Vulkan/VulkanFrameData.cpp | 12 +++++++----- Common/GPU/Vulkan/VulkanFrameData.h | 6 +++--- Common/GPU/Vulkan/VulkanQueueRunner.cpp | 2 +- Common/GPU/Vulkan/VulkanRenderManager.cpp | 11 ++++++++--- 4 files changed, 19 insertions(+), 12 deletions(-) diff --git a/Common/GPU/Vulkan/VulkanFrameData.cpp b/Common/GPU/Vulkan/VulkanFrameData.cpp index 093d32133573..c9cbb8931e2a 100644 --- a/Common/GPU/Vulkan/VulkanFrameData.cpp +++ b/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,7 +221,7 @@ 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; @@ -227,7 +229,7 @@ void FrameData::SubmitPending(VulkanContext *vulkan, FrameSubmitType type, Frame } 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; } diff --git a/Common/GPU/Vulkan/VulkanFrameData.h b/Common/GPU/Vulkan/VulkanFrameData.h index 97d387efe838..3a6e9d879f91 100644 --- a/Common/GPU/Vulkan/VulkanFrameData.h +++ b/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 diff --git a/Common/GPU/Vulkan/VulkanQueueRunner.cpp b/Common/GPU/Vulkan/VulkanQueueRunner.cpp index cc84a0eefb3f..07a2f994869f 100644 --- a/Common/GPU/Vulkan/VulkanQueueRunner.cpp +++ b/Common/GPU/Vulkan/VulkanQueueRunner.cpp @@ -369,7 +369,7 @@ void VulkanQueueRunner::RunSteps(std::vector &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. diff --git a/Common/GPU/Vulkan/VulkanRenderManager.cpp b/Common/GPU/Vulkan/VulkanRenderManager.cpp index 1ad544237597..8bfb9d0834b8 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.cpp +++ b/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; @@ -1470,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(); @@ -1507,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 lock(syncMutex_); From 8b99c9f9d9e81609fc4cb693b07fb0365b28d8d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 30 Jan 2024 11:14:38 +0100 Subject: [PATCH 3/3] GE dump playback: Don't flip unless DISPLAY is the last command. This messes up the frame structure. --- GPU/Debugger/Playback.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/GPU/Debugger/Playback.cpp b/GPU/Debugger/Playback.cpp index 413fd8de7644..f66a9c3e92f9 100644 --- a/GPU/Debugger/Playback.cpp +++ b/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 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: