From 2e171b22ecfb5b440a101ee3886792fdca359239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 20 Sep 2023 22:25:22 +0200 Subject: [PATCH] Vulkan: Remove an assert that didn't give much actionable information. Replace with reporting. --- Common/GPU/Vulkan/VulkanQueueRunner.cpp | 16 ++++++------ Common/GPU/Vulkan/VulkanQueueRunner.h | 2 +- Common/GPU/Vulkan/VulkanRenderManager.cpp | 16 ++++++++++++ Common/GPU/Vulkan/VulkanRenderManager.h | 13 +++++++-- GPU/GPUCommon.h | 2 +- GPU/Vulkan/DrawEngineVulkan.cpp | 32 +++++++++++++++-------- GPU/Vulkan/DrawEngineVulkan.h | 10 +++++++ GPU/Vulkan/PipelineManagerVulkan.cpp | 4 +-- 8 files changed, 70 insertions(+), 25 deletions(-) diff --git a/Common/GPU/Vulkan/VulkanQueueRunner.cpp b/Common/GPU/Vulkan/VulkanQueueRunner.cpp index 99b82cc50a..c4a04f90bc 100644 --- a/Common/GPU/Vulkan/VulkanQueueRunner.cpp +++ b/Common/GPU/Vulkan/VulkanQueueRunner.cpp @@ -414,7 +414,7 @@ void VulkanQueueRunner::RunSteps(std::vector &steps, FrameData &frame if (profile && profile->timestampsEnabled && profile->timestampDescriptions.size() + 1 < MAX_TIMESTAMP_QUERIES) { vkCmdWriteTimestamp(cmd, VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT, profile->queryPool, (uint32_t)profile->timestampDescriptions.size()); - profile->timestampDescriptions.push_back(StepToString(step)); + profile->timestampDescriptions.push_back(StepToString(vulkan_, step)); } if (emitLabels) { @@ -694,13 +694,13 @@ static const char *rpTypeDebugNames[] = { "BACKBUF", }; -std::string VulkanQueueRunner::StepToString(const VKRStep &step) const { +std::string VulkanQueueRunner::StepToString(VulkanContext *vulkan, const VKRStep &step) { char buffer[256]; switch (step.stepType) { case VKRStepType::RENDER: { - int w = step.render.framebuffer ? step.render.framebuffer->width : vulkan_->GetBackbufferWidth(); - int h = step.render.framebuffer ? step.render.framebuffer->height : vulkan_->GetBackbufferHeight(); + int w = step.render.framebuffer ? step.render.framebuffer->width : vulkan->GetBackbufferWidth(); + int h = step.render.framebuffer ? step.render.framebuffer->height : vulkan->GetBackbufferHeight(); int actual_w = step.render.renderArea.extent.width; int actual_h = step.render.renderArea.extent.height; const char *renderCmd = rpTypeDebugNames[(size_t)step.render.renderPassType]; @@ -938,19 +938,19 @@ void VulkanQueueRunner::LogRenderPass(const VKRStep &pass, bool verbose) { } void VulkanQueueRunner::LogCopy(const VKRStep &step) { - INFO_LOG(G3D, "%s", StepToString(step).c_str()); + INFO_LOG(G3D, "%s", StepToString(vulkan_, step).c_str()); } void VulkanQueueRunner::LogBlit(const VKRStep &step) { - INFO_LOG(G3D, "%s", StepToString(step).c_str()); + INFO_LOG(G3D, "%s", StepToString(vulkan_, step).c_str()); } void VulkanQueueRunner::LogReadback(const VKRStep &step) { - INFO_LOG(G3D, "%s", StepToString(step).c_str()); + INFO_LOG(G3D, "%s", StepToString(vulkan_, step).c_str()); } void VulkanQueueRunner::LogReadbackImage(const VKRStep &step) { - INFO_LOG(G3D, "%s", StepToString(step).c_str()); + INFO_LOG(G3D, "%s", StepToString(vulkan_, step).c_str()); } void TransitionToOptimal(VkCommandBuffer cmd, VkImage colorImage, VkImageLayout colorLayout, VkImage depthStencilImage, VkImageLayout depthStencilLayout, int numLayers, VulkanBarrier *recordBarrier) { diff --git a/Common/GPU/Vulkan/VulkanQueueRunner.h b/Common/GPU/Vulkan/VulkanQueueRunner.h index 9c1bd765a4..7b0d3bf0ed 100644 --- a/Common/GPU/Vulkan/VulkanQueueRunner.h +++ b/Common/GPU/Vulkan/VulkanQueueRunner.h @@ -233,7 +233,7 @@ public: void RunSteps(std::vector &steps, FrameData &frameData, FrameDataShared &frameDataShared, bool keepSteps = false); void LogSteps(const std::vector &steps, bool verbose); - std::string StepToString(const VKRStep &step) const; + static std::string StepToString(VulkanContext *vulkan, const VKRStep &step); void CreateDeviceObjects(); void DestroyDeviceObjects(); diff --git a/Common/GPU/Vulkan/VulkanRenderManager.cpp b/Common/GPU/Vulkan/VulkanRenderManager.cpp index 00409ab23f..fff194adc2 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.cpp +++ b/Common/GPU/Vulkan/VulkanRenderManager.cpp @@ -12,6 +12,7 @@ #include "Common/GPU/Vulkan/VulkanContext.h" #include "Common/GPU/Vulkan/VulkanRenderManager.h" +#include "Common/LogReporting.h" #include "Common/Thread/ThreadUtil.h" #include "Common/VR/PPSSPPVR.h" @@ -726,6 +727,21 @@ VkCommandBuffer VulkanRenderManager::GetInitCmd() { return frameData_[curFrame].GetInitCmd(vulkan_); } +void VulkanRenderManager::ReportBadStateForDraw() { + const char *cause1 = ""; + char cause2[256]; + cause2[0] = '\0'; + if (!curRenderStep_) { + cause1 = "No current render step"; + } + if (curRenderStep_ && curRenderStep_->stepType != VKRStepType::RENDER) { + cause1 = "Not a render step: "; + std::string str = VulkanQueueRunner::StepToString(vulkan_, *curRenderStep_); + truncate_cpy(cause2, str.c_str()); + } + ERROR_LOG_REPORT_ONCE(baddraw, G3D, "Can't draw: %s%s. Step count: %d", cause1, cause2, (int)steps_.size()); +} + VKRGraphicsPipeline *VulkanRenderManager::CreateGraphicsPipeline(VKRGraphicsPipelineDesc *desc, PipelineFlags pipelineFlags, uint32_t variantBitmask, VkSampleCountFlagBits sampleCount, bool cacheLoad, const char *tag) { if (!desc->vertexShader || !desc->fragmentShader) { ERROR_LOG(G3D, "Can't create graphics pipeline with missing vs/ps: %p %p", desc->vertexShader, desc->fragmentShader); diff --git a/Common/GPU/Vulkan/VulkanRenderManager.h b/Common/GPU/Vulkan/VulkanRenderManager.h index 16c7123f6b..53a213e68f 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.h +++ b/Common/GPU/Vulkan/VulkanRenderManager.h @@ -238,14 +238,22 @@ public: VKRGraphicsPipeline *CreateGraphicsPipeline(VKRGraphicsPipelineDesc *desc, PipelineFlags pipelineFlags, uint32_t variantBitmask, VkSampleCountFlagBits sampleCount, bool cacheLoad, const char *tag); VKRComputePipeline *CreateComputePipeline(VKRComputePipelineDesc *desc); + void ReportBadStateForDraw(); + void NudgeCompilerThread() { compileMutex_.lock(); compileCond_.notify_one(); compileMutex_.unlock(); } - void BindPipeline(VKRGraphicsPipeline *pipeline, PipelineFlags flags, VkPipelineLayout pipelineLayout) { - _assert_(curRenderStep_ && curRenderStep_->stepType == VKRStepType::RENDER && pipeline != nullptr); + // This is the first call in a draw operation. Instead of asserting like we used to, you can now check the + // return value and skip the draw if we're in a bad state. In that case, call ReportBadState. + // The old assert wasn't very helpful in figuring out what caused it anyway... + bool BindPipeline(VKRGraphicsPipeline *pipeline, PipelineFlags flags, VkPipelineLayout pipelineLayout) { + _dbg_assert_(curRenderStep_ && curRenderStep_->stepType == VKRStepType::RENDER && pipeline != nullptr); + if (!curRenderStep_ || curRenderStep_->stepType != VKRStepType::RENDER) { + return false; + } VkRenderData &data = curRenderStep_->commands.push_uninitialized(); data.cmd = VKRRenderCommand::BIND_GRAPHICS_PIPELINE; pipelinesToCheck_.push_back(pipeline); @@ -256,6 +264,7 @@ public: // DebugBreak(); // } curPipelineFlags_ |= flags; + return true; } void BindPipeline(VKRComputePipeline *pipeline, PipelineFlags flags, VkPipelineLayout pipelineLayout) { diff --git a/GPU/GPUCommon.h b/GPU/GPUCommon.h index ef7d9187ad..8b3f59b9dd 100644 --- a/GPU/GPUCommon.h +++ b/GPU/GPUCommon.h @@ -254,7 +254,7 @@ protected: void FlushImm(); void DoBlockTransfer(u32 skipDrawReason); - // TODO: Unify this. The only backend that differs is Vulkan. + // TODO: Unify this. Vulkan and OpenGL are different due to how they buffer data. virtual void FinishDeferred() {} void AdvanceVerts(u32 vertType, int count, int bytesRead) { diff --git a/GPU/Vulkan/DrawEngineVulkan.cpp b/GPU/Vulkan/DrawEngineVulkan.cpp index 0f541f8258..5d53a1e32d 100644 --- a/GPU/Vulkan/DrawEngineVulkan.cpp +++ b/GPU/Vulkan/DrawEngineVulkan.cpp @@ -791,20 +791,20 @@ void DrawEngineVulkan::DoFlush() { VulkanGeometryShader *gshader = nullptr; shaderManager_->GetShaders(prim, dec_, &vshader, &fshader, &gshader, pipelineState_, true, useHWTessellation_, decOptions_.expandAllWeightsToFloat, decOptions_.applySkinInDecode); - if (!vshader) { - // We're screwed, can't do anything. - return; - } _dbg_assert_msg_(vshader->UseHWTransform(), "Bad vshader"); - VulkanPipeline *pipeline = pipelineManager_->GetOrCreatePipeline(renderManager, pipelineLayout_, pipelineKey_, &dec_->decFmt, vshader, fshader, gshader, true, 0, framebufferManager_->GetMSAALevel(), false); if (!pipeline || !pipeline->pipeline) { // Already logged, let's bail out. + ResetAfterDraw(); return; } BindShaderBlendTex(); // This might cause copies so important to do before BindPipeline. - renderManager->BindPipeline(pipeline->pipeline, pipeline->pipelineFlags, pipelineLayout_); + if (!renderManager->BindPipeline(pipeline->pipeline, pipeline->pipelineFlags, pipelineLayout_)) { + renderManager->ReportBadStateForDraw(); + ResetAfterDraw(); + return; + } if (pipeline != lastPipeline_) { if (lastPipeline_ && !(lastPipeline_->UsesBlendConstant() && pipeline->UsesBlendConstant())) { gstate_c.Dirty(DIRTY_BLEND_STATE); @@ -932,15 +932,16 @@ void DrawEngineVulkan::DoFlush() { VulkanPipeline *pipeline = pipelineManager_->GetOrCreatePipeline(renderManager, pipelineLayout_, pipelineKey_, &dec_->decFmt, vshader, fshader, gshader, false, 0, framebufferManager_->GetMSAALevel(), false); if (!pipeline || !pipeline->pipeline) { // Already logged, let's bail out. - decodedVerts_ = 0; - numDrawCalls_ = 0; - decodeCounter_ = 0; - decOptions_.applySkinInDecode = g_Config.bSoftwareSkinning; + ResetAfterDraw(); return; } BindShaderBlendTex(); // This might cause copies so super important to do before BindPipeline. - renderManager->BindPipeline(pipeline->pipeline, pipeline->pipelineFlags, pipelineLayout_); + if (!renderManager->BindPipeline(pipeline->pipeline, pipeline->pipelineFlags, pipelineLayout_)) { + renderManager->ReportBadStateForDraw(); + ResetAfterDraw(); + return; + } if (pipeline != lastPipeline_) { if (lastPipeline_ && !lastPipeline_->UsesBlendConstant() && pipeline->UsesBlendConstant()) { gstate_c.Dirty(DIRTY_BLEND_STATE); @@ -1027,6 +1028,15 @@ void DrawEngineVulkan::DoFlush() { GPUDebug::NotifyDraw(); } +void DrawEngineVulkan::ResetAfterDraw() { + indexGen.Reset(); + decodedVerts_ = 0; + numDrawCalls_ = 0; + decodeCounter_ = 0; + decOptions_.applySkinInDecode = g_Config.bSoftwareSkinning; + gstate_c.vertexFullAlpha = true; +} + void DrawEngineVulkan::UpdateUBOs(FrameData *frame) { if ((dirtyUniforms_ & DIRTY_BASE_UNIFORMS) || baseBuf == VK_NULL_HANDLE) { baseUBOOffset = shaderManager_->PushBaseBuffer(pushUBO_, &baseBuf); diff --git a/GPU/Vulkan/DrawEngineVulkan.h b/GPU/Vulkan/DrawEngineVulkan.h index 9f53638bcf..b599c35051 100644 --- a/GPU/Vulkan/DrawEngineVulkan.h +++ b/GPU/Vulkan/DrawEngineVulkan.h @@ -45,6 +45,14 @@ #include "GPU/Vulkan/StateMappingVulkan.h" #include "GPU/Vulkan/VulkanRenderManager.h" + +// TODO: Move to some appropriate header. +#ifdef _MSC_VER +#define NO_INLINE __declspec(noinline) +#else +#define NO_INLINE __attribute__((noinline)) +#endif + struct DecVtxFormat; struct UVScale; @@ -230,6 +238,8 @@ private: void UpdateUBOs(FrameData *frame); FrameData &GetCurFrame(); + NO_INLINE void ResetAfterDraw(); + VkDescriptorSet GetOrCreateDescriptorSet(VkImageView imageView, VkSampler sampler, VkBuffer base, VkBuffer light, VkBuffer bone, bool tess); Draw::DrawContext *draw_; diff --git a/GPU/Vulkan/PipelineManagerVulkan.cpp b/GPU/Vulkan/PipelineManagerVulkan.cpp index 43f67709da..ad978c524f 100644 --- a/GPU/Vulkan/PipelineManagerVulkan.cpp +++ b/GPU/Vulkan/PipelineManagerVulkan.cpp @@ -191,11 +191,11 @@ static VulkanPipeline *CreateVulkanPipeline(VulkanRenderManager *renderManager, const DecVtxFormat *decFmt, VulkanVertexShader *vs, VulkanFragmentShader *fs, VulkanGeometryShader *gs, bool useHwTransform, u32 variantBitmask, bool cacheLoad) { _assert_(fs && vs); - if (!fs->GetModule()) { + if (!fs || !fs->GetModule()) { ERROR_LOG(G3D, "Fragment shader missing in CreateVulkanPipeline"); return nullptr; } - if (!vs->GetModule()) { + if (!vs || !vs->GetModule()) { ERROR_LOG(G3D, "Vertex shader missing in CreateVulkanPipeline"); return nullptr; }