From b7d28cd10ad7086c9ea9b4fb7bf4b1315194465f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 18 Sep 2023 14:38:22 +0200 Subject: [PATCH 1/5] Remove redundant fail state. Bail from shader cache load if a fragment shader fails to generate, too. --- GPU/GLES/ShaderManagerGLES.cpp | 20 ++++++++++++++++---- GPU/GLES/ShaderManagerGLES.h | 2 -- GPU/Vulkan/DrawEngineVulkan.cpp | 2 +- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/GPU/GLES/ShaderManagerGLES.cpp b/GPU/GLES/ShaderManagerGLES.cpp index ac52ea01ee92..4c28ef04e8c1 100644 --- a/GPU/GLES/ShaderManagerGLES.cpp +++ b/GPU/GLES/ShaderManagerGLES.cpp @@ -748,6 +748,8 @@ void ShaderManagerGLES::DirtyLastShader() { lastVShaderSame_ = false; } +// Can only fail by failing to generate the code (bad FSID). +// Any actual failures driver-side happens later in the render manager. Shader *ShaderManagerGLES::CompileFragmentShader(FShaderID FSID) { uint64_t uniformMask; std::string errorString; @@ -762,6 +764,8 @@ Shader *ShaderManagerGLES::CompileFragmentShader(FShaderID FSID) { return new Shader(render_, codeBuffer_, desc, params); } +// Can only fail by failing to generate the code (bad VSID). +// Any actual failures driver-side happens later in the render manager. Shader *ShaderManagerGLES::CompileVertexShader(VShaderID VSID) { bool useHWTransform = VSID.Bit(VS_BIT_USE_HW_TRANSFORM); uint32_t attrMask; @@ -802,13 +806,12 @@ Shader *ShaderManagerGLES::ApplyVertexShader(bool useHWTransform, bool useHWTess // Vertex shader not in cache. Let's compile it. vs = CompileVertexShader(*VSID); - if (!vs || vs->Failed()) { + if (!vs) { auto gr = GetI18NCategory(I18NCat::GRAPHICS); ERROR_LOG(G3D, "Vertex shader generation failed, falling back to software transform"); if (!g_Config.bHideSlowWarnings) { g_OSD.Show(OSDType::MESSAGE_ERROR, gr->T("hardware transform error - falling back to software"), 2.5f); } - delete vs; // TODO: Look for existing shader with the appropriate ID, use that instead of generating a new one - however, need to make sure // that that shader ID is not used when computing the linked shader ID below, because then IDs won't match @@ -1099,7 +1102,7 @@ bool ShaderManagerGLES::ContinuePrecompile(float sliceTime) { } Shader *vs = CompileVertexShader(id); - if (!vs || vs->Failed()) { + if (!vs) { // Give up on using the cache, just bail. We can't safely create the fallback shaders here // without trying to deduce the vertType from the VSID. ERROR_LOG(G3D, "Failed to compile a vertex shader loading from cache. Skipping rest of shader cache."); @@ -1121,7 +1124,16 @@ bool ShaderManagerGLES::ContinuePrecompile(float sliceTime) { const FShaderID &id = pending.frag[i]; if (!fsCache_.ContainsKey(id)) { - fsCache_.Insert(id, CompileFragmentShader(id)); + Shader *fs = CompileFragmentShader(id); + if (!fs) { + // Give up on using the cache - something went wrong. + // We'll still keep the shaders we generated so far around. + ERROR_LOG(G3D, "Failed to compile a fragment shader loading from cache. Skipping rest of shader cache."); + delete fs; + pending.Clear(); + return false; + } + fsCache_.Insert(id, fs); } else { WARN_LOG(G3D, "Duplicate fragment shader found in GL shader cache, ignoring"); } diff --git a/GPU/GLES/ShaderManagerGLES.h b/GPU/GLES/ShaderManagerGLES.h index 4ae3db315403..db834dce3735 100644 --- a/GPU/GLES/ShaderManagerGLES.h +++ b/GPU/GLES/ShaderManagerGLES.h @@ -141,7 +141,6 @@ class Shader { ~Shader(); GLRShader *shader; - bool Failed() const { return failed_; } bool UseHWTransform() const { return useHWTransform_; } // only relevant for vtx shaders std::string GetShaderString(DebugShaderStringType type, ShaderID id) const; @@ -152,7 +151,6 @@ class Shader { private: GLRenderManager *render_; std::string source_; - bool failed_ = false; bool useHWTransform_; bool isFragment_; uint32_t attrMask_; // only used in vertex shaders diff --git a/GPU/Vulkan/DrawEngineVulkan.cpp b/GPU/Vulkan/DrawEngineVulkan.cpp index 3104d5f33f3d..491c3a44efc1 100644 --- a/GPU/Vulkan/DrawEngineVulkan.cpp +++ b/GPU/Vulkan/DrawEngineVulkan.cpp @@ -792,7 +792,7 @@ void DrawEngineVulkan::DoFlush() { shaderManager_->GetShaders(prim, dec_, &vshader, &fshader, &gshader, pipelineState_, true, useHWTessellation_, decOptions_.expandAllWeightsToFloat, decOptions_.applySkinInDecode); if (!vshader) { - // We're screwed. + // We're screwed, can't do anything. return; } _dbg_assert_msg_(vshader->UseHWTransform(), "Bad vshader"); From 3c810521e22dafc9cb5b9bef1c0fd21c2670cc94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 18 Sep 2023 14:49:13 +0200 Subject: [PATCH 2/5] TextureBatch tiny optimization: Use vector.reserve --- Common/GPU/Vulkan/thin3d_vulkan.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/Common/GPU/Vulkan/thin3d_vulkan.cpp b/Common/GPU/Vulkan/thin3d_vulkan.cpp index 7d7a3ea7609e..dc9aaf60ef0e 100644 --- a/Common/GPU/Vulkan/thin3d_vulkan.cpp +++ b/Common/GPU/Vulkan/thin3d_vulkan.cpp @@ -821,6 +821,7 @@ void VKTexture::UpdateInternal(VkCommandBuffer cmd, VulkanPushPool *pushBuffer, int bpp = GetBpp(vulkanFormat); int bytesPerPixel = bpp / 8; TextureCopyBatch batch; + batch.reserve(numLevels); for (i = 0; i < numLevels; i++) { uint32_t offset; VkBuffer buf; From 946d4b62519c7b06bf90ee1de20eaa715b8be2c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 18 Sep 2023 16:12:27 +0200 Subject: [PATCH 3/5] Avoid causing shader gen failures due to bad blend eq values --- GPU/Common/ShaderId.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/GPU/Common/ShaderId.cpp b/GPU/Common/ShaderId.cpp index a9e1e3485e05..7dadda15e8cf 100644 --- a/GPU/Common/ShaderId.cpp +++ b/GPU/Common/ShaderId.cpp @@ -275,6 +275,21 @@ bool FragmentIdNeedsFramebufferRead(const FShaderID &id) { (ReplaceBlendType)id.Bits(FS_BIT_REPLACE_BLEND, 3) == REPLACE_BLEND_READ_FRAMEBUFFER; } +static GEBlendMode SanitizeBlendEq(GEBlendMode beq) { + switch (beq) { + case GE_BLENDMODE_MUL_AND_ADD: + case GE_BLENDMODE_MUL_AND_SUBTRACT: + case GE_BLENDMODE_MUL_AND_SUBTRACT_REVERSE: + case GE_BLENDMODE_MIN: + case GE_BLENDMODE_MAX: + case GE_BLENDMODE_ABSDIFF: + return beq; + default: + // Just return something that won't cause a shader gen failure. + return GE_BLENDMODE_MUL_AND_ADD; + } +} + // Here we must take all the bits of the gstate that determine what the fragment shader will // look like, and concatenate them together into an ID. void ComputeFragmentShaderID(FShaderID *id_out, const ComputedPipelineState &pipelineState, const Draw::Bugs &bugs) { @@ -369,7 +384,7 @@ void ComputeFragmentShaderID(FShaderID *id_out, const ComputedPipelineState &pip // 3 bits. id.SetBits(FS_BIT_REPLACE_BLEND, 3, replaceBlend); // 11 bits total. - id.SetBits(FS_BIT_BLENDEQ, 3, gstate.getBlendEq()); + id.SetBits(FS_BIT_BLENDEQ, 3, SanitizeBlendEq(gstate.getBlendEq())); id.SetBits(FS_BIT_BLENDFUNC_A, 4, gstate.getBlendFuncA()); id.SetBits(FS_BIT_BLENDFUNC_B, 4, gstate.getBlendFuncB()); } From f4b0cddda371ed0a0cf47dc61dc4751f0aba6410 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 18 Sep 2023 16:25:00 +0200 Subject: [PATCH 4/5] ShaderId: Safer way to check for backend. --- GPU/Common/ShaderId.cpp | 2 +- GPU/GPUState.h | 2 +- GPU/Vulkan/GPU_Vulkan.cpp | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/GPU/Common/ShaderId.cpp b/GPU/Common/ShaderId.cpp index 7dadda15e8cf..3e9b77111177 100644 --- a/GPU/Common/ShaderId.cpp +++ b/GPU/Common/ShaderId.cpp @@ -392,7 +392,7 @@ void ComputeFragmentShaderID(FShaderID *id_out, const ComputedPipelineState &pip id.SetBit(FS_BIT_COLOR_WRITEMASK, colorWriteMask); // All framebuffers are array textures in Vulkan now. - if (gstate_c.textureIsArray && g_Config.iGPUBackend == (int)GPUBackend::VULKAN) { + if (gstate_c.textureIsArray && gstate_c.Use(GPU_USE_FRAMEBUFFER_ARRAYS)) { id.SetBit(FS_BIT_SAMPLE_ARRAY_TEXTURE); } diff --git a/GPU/GPUState.h b/GPU/GPUState.h index 6a1f14b81ff7..6b1446c5e891 100644 --- a/GPU/GPUState.h +++ b/GPU/GPUState.h @@ -491,7 +491,7 @@ enum { GPU_USE_DEPTH_TEXTURE = FLAG_BIT(16), GPU_USE_ACCURATE_DEPTH = FLAG_BIT(17), GPU_USE_GS_CULLING = FLAG_BIT(18), // Geometry shader - // Bit 19 free. + GPU_USE_FRAMEBUFFER_ARRAYS = FLAG_BIT(19), GPU_USE_FRAMEBUFFER_FETCH = FLAG_BIT(20), GPU_SCALE_DEPTH_FROM_24BIT_TO_16BIT = FLAG_BIT(21), GPU_ROUND_FRAGMENT_DEPTH_TO_16BIT = FLAG_BIT(22), diff --git a/GPU/Vulkan/GPU_Vulkan.cpp b/GPU/Vulkan/GPU_Vulkan.cpp index b4734dc2d3b2..9d6221209244 100644 --- a/GPU/Vulkan/GPU_Vulkan.cpp +++ b/GPU/Vulkan/GPU_Vulkan.cpp @@ -290,6 +290,7 @@ u32 GPU_Vulkan::CheckGPUFeatures() const { features &= ~GPU_USE_LIGHT_UBERSHADER; } + features |= GPU_USE_FRAMEBUFFER_ARRAYS; return CheckGPUFeaturesLate(features); } From 0bfd166200838c8a28792ac16745639f9c220c12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 18 Sep 2023 16:42:22 +0200 Subject: [PATCH 5/5] Try to prevent a weird shutdown race condition that I'm not sure can happen - but crash logs show it --- Common/GPU/Vulkan/VulkanRenderManager.cpp | 27 +++++++++++++++-------- Common/GPU/Vulkan/VulkanRenderManager.h | 4 +++- GPU/Vulkan/GPU_Vulkan.cpp | 7 +++++- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/Common/GPU/Vulkan/VulkanRenderManager.cpp b/Common/GPU/Vulkan/VulkanRenderManager.cpp index cb0e819dc8df..14c7a4666dc9 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.cpp +++ b/Common/GPU/Vulkan/VulkanRenderManager.cpp @@ -500,14 +500,20 @@ void VulkanRenderManager::CompileThreadFunc() { } } -void VulkanRenderManager::DrainCompileQueue() { +void VulkanRenderManager::DrainAndBlockCompileQueue() { std::unique_lock lock(compileMutex_); + compileBlocked_ = true; compileCond_.notify_all(); while (!compileQueue_.empty()) { queueRunner_.WaitForCompileNotification(); } } +void VulkanRenderManager::ReleaseCompileQueue() { + std::unique_lock lock(compileMutex_); + compileBlocked_ = false; +} + void VulkanRenderManager::ThreadFunc() { SetCurrentThreadName("RenderMan"); while (true) { @@ -709,14 +715,12 @@ VkCommandBuffer VulkanRenderManager::GetInitCmd() { } VKRGraphicsPipeline *VulkanRenderManager::CreateGraphicsPipeline(VKRGraphicsPipelineDesc *desc, PipelineFlags pipelineFlags, uint32_t variantBitmask, VkSampleCountFlagBits sampleCount, bool cacheLoad, const char *tag) { - VKRGraphicsPipeline *pipeline = new VKRGraphicsPipeline(pipelineFlags, tag); - if (!desc->vertexShader || !desc->fragmentShader) { ERROR_LOG(G3D, "Can't create graphics pipeline with missing vs/ps: %p %p", desc->vertexShader, desc->fragmentShader); - delete pipeline; return nullptr; } + VKRGraphicsPipeline *pipeline = new VKRGraphicsPipeline(pipelineFlags, tag); pipeline->desc = desc; pipeline->desc->AddRef(); if (curRenderStep_ && !cacheLoad) { @@ -733,7 +737,11 @@ VKRGraphicsPipeline *VulkanRenderManager::CreateGraphicsPipeline(VKRGraphicsPipe VKRRenderPassStoreAction::STORE, VKRRenderPassStoreAction::DONT_CARE, VKRRenderPassStoreAction::DONT_CARE, }; VKRRenderPass *compatibleRenderPass = queueRunner_.GetRenderPass(key); - compileMutex_.lock(); + std::lock_guard lock(compileMutex_); + if (compileBlocked_) { + delete pipeline; + return nullptr; + } bool needsCompile = false; for (size_t i = 0; i < (size_t)RenderPassType::TYPE_COUNT; i++) { if (!(variantBitmask & (1 << i))) @@ -757,18 +765,19 @@ VKRGraphicsPipeline *VulkanRenderManager::CreateGraphicsPipeline(VKRGraphicsPipe } if (needsCompile) compileCond_.notify_one(); - compileMutex_.unlock(); } return pipeline; } VKRComputePipeline *VulkanRenderManager::CreateComputePipeline(VKRComputePipelineDesc *desc) { + std::lock_guard lock(compileMutex_); + if (compileBlocked_) { + return nullptr; + } VKRComputePipeline *pipeline = new VKRComputePipeline(); pipeline->desc = desc; - compileMutex_.lock(); compileQueue_.push_back(CompileQueueEntry(pipeline)); compileCond_.notify_one(); - compileMutex_.unlock(); return pipeline; } @@ -814,7 +823,7 @@ void VulkanRenderManager::EndCurRenderStep() { compileMutex_.lock(); bool needsCompile = false; for (VKRGraphicsPipeline *pipeline : pipelinesToCheck_) { - if (!pipeline) { + if (!pipeline || compileBlocked_) { // Not good, but let's try not to crash. continue; } diff --git a/Common/GPU/Vulkan/VulkanRenderManager.h b/Common/GPU/Vulkan/VulkanRenderManager.h index 80270eb3787a..1c912cd0fee1 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.h +++ b/Common/GPU/Vulkan/VulkanRenderManager.h @@ -455,7 +455,8 @@ class VulkanRenderManager { } void ResetStats(); - void DrainCompileQueue(); + void DrainAndBlockCompileQueue(); + void ReleaseCompileQueue(); private: void EndCurRenderStep(); @@ -527,6 +528,7 @@ class VulkanRenderManager { std::condition_variable compileCond_; std::mutex compileMutex_; std::vector compileQueue_; + bool compileBlocked_ = false; // Thread for measuring presentation delay. std::thread presentWaitThread_; diff --git a/GPU/Vulkan/GPU_Vulkan.cpp b/GPU/Vulkan/GPU_Vulkan.cpp index 9d6221209244..6846a1d52e5e 100644 --- a/GPU/Vulkan/GPU_Vulkan.cpp +++ b/GPU/Vulkan/GPU_Vulkan.cpp @@ -182,7 +182,7 @@ void GPU_Vulkan::SaveCache(const Path &filename) { GPU_Vulkan::~GPU_Vulkan() { if (draw_) { VulkanRenderManager *rm = (VulkanRenderManager *)draw_->GetNativeObject(Draw::NativeObject::RENDER_MANAGER); - rm->DrainCompileQueue(); + rm->DrainAndBlockCompileQueue(); } SaveCache(shaderCachePath_); @@ -193,6 +193,11 @@ GPU_Vulkan::~GPU_Vulkan() { delete pipelineManager_; // other managers are deleted in ~GPUCommonHW. + + if (draw_) { + VulkanRenderManager *rm = (VulkanRenderManager *)draw_->GetNativeObject(Draw::NativeObject::RENDER_MANAGER); + rm->ReleaseCompileQueue(); + } } u32 GPU_Vulkan::CheckGPUFeatures() const {