From 214d1d296a21ba39788496bbdd6d61c773fbfc7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 3 May 2023 22:15:22 +0200 Subject: [PATCH 01/11] Add a couple of TODOs for the future (not appropriate for current release process) --- GPU/GPUCommonHW.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/GPU/GPUCommonHW.cpp b/GPU/GPUCommonHW.cpp index 51cd770fe66c..9c9645be9684 100644 --- a/GPU/GPUCommonHW.cpp +++ b/GPU/GPUCommonHW.cpp @@ -806,6 +806,7 @@ void GPUCommonHW::FastRunLoop(DisplayList &list) { if (info.flags & FLAG_EXECUTE) { downcount = dc; (this->*info.func)(op, diff); + // TODO: Check pc here, and break if invalid, as the func can change it. Might an have a performance impact though. dc = downcount; } } else { @@ -819,6 +820,7 @@ void GPUCommonHW::FastRunLoop(DisplayList &list) { if (flags & (FLAG_EXECUTE | FLAG_EXECUTEONCHANGE)) { downcount = dc; (this->*info.func)(op, diff); + // TODO: Check pc here, and break if invalid, as the func can change it. Might have a performance impact though. dc = downcount; } else { uint64_t dirty = flags >> 8; From 0e2fb13c61eae70cda0486e78bd65b80da43ccfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 3 May 2023 22:22:54 +0200 Subject: [PATCH 02/11] Make sure we never end up with a null vertex decoder. --- GPU/Common/DrawEngineCommon.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/GPU/Common/DrawEngineCommon.cpp b/GPU/Common/DrawEngineCommon.cpp index 45353bcbc8a4..ac0d368b227c 100644 --- a/GPU/Common/DrawEngineCommon.cpp +++ b/GPU/Common/DrawEngineCommon.cpp @@ -66,6 +66,7 @@ VertexDecoder *DrawEngineCommon::GetVertexDecoder(u32 vtype) { if (dec) return dec; dec = new VertexDecoder(); + _assert_(dec); dec->SetVertexType(vtype, decOptions_, decJitCache_); decoderMap_.Insert(vtype, dec); return dec; @@ -809,7 +810,7 @@ void DrawEngineCommon::SubmitPrim(const void *verts, const void *inds, GEPrimiti } // If vtype has changed, setup the vertex decoder. - if (vertTypeID != lastVType_) { + if (vertTypeID != lastVType_ || !dec_) { dec_ = GetVertexDecoder(vertTypeID); lastVType_ = vertTypeID; } From c9b7c815a105ea5f083e685fff71145ffafd123b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 3 May 2023 22:33:34 +0200 Subject: [PATCH 03/11] ~GPU_Vulkan: Check that we still have a draw_ pointer before trying to drain the compile queue. --- Common/GPU/OpenGL/GLRenderManager.cpp | 2 +- GPU/Vulkan/GPU_Vulkan.cpp | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/Common/GPU/OpenGL/GLRenderManager.cpp b/Common/GPU/OpenGL/GLRenderManager.cpp index be72f8aff1e5..da1efe7e2624 100644 --- a/Common/GPU/OpenGL/GLRenderManager.cpp +++ b/Common/GPU/OpenGL/GLRenderManager.cpp @@ -640,7 +640,7 @@ void GLPushBuffer::Defragment() { _dbg_assert_msg_(!OnRenderThread(), "Defragment must not run on the render thread"); if (buffers_.size() <= 1) { - // Let's take this chance to jetison localMemory we don't need. + // Let's take this opportunity to jettison any localMemory we don't need. for (auto &info : buffers_) { if (info.deviceMemory) { FreeAlignedMemory(info.localMemory); diff --git a/GPU/Vulkan/GPU_Vulkan.cpp b/GPU/Vulkan/GPU_Vulkan.cpp index 9c0f1b2a064d..31ddcb09131a 100644 --- a/GPU/Vulkan/GPU_Vulkan.cpp +++ b/GPU/Vulkan/GPU_Vulkan.cpp @@ -182,8 +182,10 @@ void GPU_Vulkan::SaveCache(const Path &filename) { } GPU_Vulkan::~GPU_Vulkan() { - VulkanRenderManager *rm = (VulkanRenderManager *)draw_->GetNativeObject(Draw::NativeObject::RENDER_MANAGER); - rm->DrainCompileQueue(); + if (draw_) { + VulkanRenderManager *rm = (VulkanRenderManager *)draw_->GetNativeObject(Draw::NativeObject::RENDER_MANAGER); + rm->DrainCompileQueue(); + } SaveCache(shaderCachePath_); // Note: We save the cache in DeviceLost From c80671d9ea7c14584643647f9158197080c52873 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 3 May 2023 22:42:03 +0200 Subject: [PATCH 04/11] Debug-assert that there's a renderpass in Flush instead of asserting, and skip if not. buildfix --- GPU/GLES/DrawEngineGLES.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/GPU/GLES/DrawEngineGLES.cpp b/GPU/GLES/DrawEngineGLES.cpp index b191014219d6..fb9c51105b24 100644 --- a/GPU/GLES/DrawEngineGLES.cpp +++ b/GPU/GLES/DrawEngineGLES.cpp @@ -249,9 +249,20 @@ void DrawEngineGLES::Invalidate(InvalidationCallbackFlags flags) { void DrawEngineGLES::DoFlush() { PROFILE_THIS_SCOPE("flush"); FrameData &frameData = frameData_[render_->GetCurFrame()]; - - // Attempt to gather some information (asserts now upload the game name). - _assert_(render_->IsInRenderPass()); + VShaderID vsid; + + if (!render_->IsInRenderPass()) { + // Something went badly wrong. Try to survive by simply skipping the draw, though. + _dbg_assert_msg_(false, "Trying to DoFlush while not in a render pass. This is bad."); + // can't goto bail here, skips too many variable initializations. So let's wipe the most important stuff. + indexGen.Reset(); + decodedVerts_ = 0; + numDrawCalls = 0; + vertexCountInDrawCalls_ = 0; + decodeCounter_ = 0; + dcid_ = 0; + return; + } bool textureNeedsApply = false; if (gstate_c.IsDirty(DIRTY_TEXTURE_IMAGE | DIRTY_TEXTURE_PARAMS) && !gstate.isModeClear() && gstate.isTextureMapEnabled()) { @@ -265,7 +276,6 @@ void DrawEngineGLES::DoFlush() { GEPrimitiveType prim = prevPrim_; - VShaderID vsid; Shader *vshader = shaderManager_->ApplyVertexShader(CanUseHardwareTransform(prim), useHWTessellation_, dec_, decOptions_.expandAllWeightsToFloat, decOptions_.applySkinInDecode || !CanUseHardwareTransform(prim), &vsid); GLRBuffer *vertexBuffer = nullptr; From d56e27aa2c5e1c8e263fc167ffd1881c8547015f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 3 May 2023 22:49:59 +0200 Subject: [PATCH 05/11] Let's have DispatchFlush check for drawcalls before calling DoFlush, too. --- GPU/D3D11/DrawEngineD3D11.h | 6 +++++- GPU/Directx9/DrawEngineDX9.h | 6 +++++- GPU/GLES/DrawEngineGLES.h | 6 +++++- GPU/Vulkan/DrawEngineVulkan.h | 6 +++++- 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/GPU/D3D11/DrawEngineD3D11.h b/GPU/D3D11/DrawEngineD3D11.h index 7c9b46ac5ee6..80e7b606fc2d 100644 --- a/GPU/D3D11/DrawEngineD3D11.h +++ b/GPU/D3D11/DrawEngineD3D11.h @@ -149,7 +149,11 @@ class DrawEngineD3D11 : public DrawEngineCommon { DecodeVerts(decoded); } - void DispatchFlush() override { Flush(); } + void DispatchFlush() override { + if (!numDrawCalls) + return; + Flush(); + } void ClearTrackedVertexArrays() override; diff --git a/GPU/Directx9/DrawEngineDX9.h b/GPU/Directx9/DrawEngineDX9.h index 1cea91376b97..5d7cbf71657d 100644 --- a/GPU/Directx9/DrawEngineDX9.h +++ b/GPU/Directx9/DrawEngineDX9.h @@ -139,7 +139,11 @@ class DrawEngineDX9 : public DrawEngineCommon { DecodeVerts(decoded); } - void DispatchFlush() override { Flush(); } + void DispatchFlush() override { + if (!numDrawCalls) + return; + Flush(); + } protected: // Not currently supported. diff --git a/GPU/GLES/DrawEngineGLES.h b/GPU/GLES/DrawEngineGLES.h index 17f773c0b295..a6c5cc117336 100644 --- a/GPU/GLES/DrawEngineGLES.h +++ b/GPU/GLES/DrawEngineGLES.h @@ -97,7 +97,11 @@ class DrawEngineGLES : public DrawEngineCommon { DoFlush(); } - void DispatchFlush() override { Flush(); } + void DispatchFlush() override { + if (!numDrawCalls) + return; + Flush(); + } GLPushBuffer *GetPushVertexBuffer() { return frameData_[render_->GetCurFrame()].pushVertex; diff --git a/GPU/Vulkan/DrawEngineVulkan.h b/GPU/Vulkan/DrawEngineVulkan.h index c84ae1960231..bf7d6940a387 100644 --- a/GPU/Vulkan/DrawEngineVulkan.h +++ b/GPU/Vulkan/DrawEngineVulkan.h @@ -177,7 +177,11 @@ class DrawEngineVulkan : public DrawEngineCommon { DoFlush(); } - void DispatchFlush() override { Flush(); } + void DispatchFlush() override { + if (!numDrawCalls) + return; + Flush(); + } VkPipelineLayout GetPipelineLayout() const { return pipelineLayout_; From 5724bbd8e96a69e5ba3afd0ef4078855f0693807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 3 May 2023 23:03:34 +0200 Subject: [PATCH 06/11] Correct size calculations in GLPushBuffer. I don't see how this failed as badly as in that crash report though... --- Common/GPU/OpenGL/GLRenderManager.cpp | 23 ++++++++++++++++++----- Common/GPU/OpenGL/GLRenderManager.h | 1 + 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/Common/GPU/OpenGL/GLRenderManager.cpp b/Common/GPU/OpenGL/GLRenderManager.cpp index da1efe7e2624..48b2f4dd339a 100644 --- a/Common/GPU/OpenGL/GLRenderManager.cpp +++ b/Common/GPU/OpenGL/GLRenderManager.cpp @@ -589,6 +589,7 @@ bool GLPushBuffer::AddBuffer() { if (!info.localMemory) return false; info.buffer = render_->CreateBuffer(target_, size_, GL_DYNAMIC_DRAW); + info.size = size_; buf_ = buffers_.size(); buffers_.push_back(info); return true; @@ -605,7 +606,6 @@ void GLPushBuffer::Destroy(bool onRenderThread) { } else { render_->DeleteBuffer(info.buffer); } - FreeAlignedMemory(info.localMemory); } buffers_.clear(); @@ -652,18 +652,31 @@ void GLPushBuffer::Defragment() { } // Okay, we have more than one. Destroy them all and start over with a larger one. - size_t newSize = size_ * buffers_.size(); + + // When calling AddBuffer, we sometimes increase size_. So if we allocated multiple buffers in a frame, + // they won't all have the same size. Sum things up properly. + size_t newSize = 0; + for (int i = 0; i < (int)buffers_.size(); i++) { + newSize += buffers_[i].size; + } + Destroy(false); - size_ = newSize; + // Set some sane but very free limits. If there's another spike, we'll just allocate more anyway. + size_ = std::min(std::max(newSize, (size_t)65536), (size_t)(512 * 1024 * 1024)); bool res = AddBuffer(); _assert_msg_(res, "AddBuffer failed"); } size_t GLPushBuffer::GetTotalSize() const { size_t sum = 0; - if (buffers_.size() > 1) - sum += size_ * (buffers_.size() - 1); + // When calling AddBuffer, we sometimes increase size_. So if we allocated multiple buffers in a frame, + // they won't all have the same size. Sum things up properly. + if (buffers_.size() > 1) { + for (int i = 0; i < (int)buffers_.size() - 1; i++) { + sum += buffers_[i].size; + } + } sum += offset_; return sum; } diff --git a/Common/GPU/OpenGL/GLRenderManager.h b/Common/GPU/OpenGL/GLRenderManager.h index 07f54ffab40c..f06724a6cbef 100644 --- a/Common/GPU/OpenGL/GLRenderManager.h +++ b/Common/GPU/OpenGL/GLRenderManager.h @@ -242,6 +242,7 @@ class GLPushBuffer { uint8_t *localMemory = nullptr; uint8_t *deviceMemory = nullptr; size_t flushOffset = 0; + size_t size; }; GLPushBuffer(GLRenderManager *render, GLuint target, size_t size); From 1d053d2ea8e1762723ed33366e77524e3fcf5629 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 3 May 2023 23:16:58 +0200 Subject: [PATCH 07/11] GLPushBuffer::Flush: Add debug-assert-and-bail prompted by suspicious callstack --- Common/GPU/OpenGL/GLRenderManager.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Common/GPU/OpenGL/GLRenderManager.cpp b/Common/GPU/OpenGL/GLRenderManager.cpp index 48b2f4dd339a..3b0acd83a71c 100644 --- a/Common/GPU/OpenGL/GLRenderManager.cpp +++ b/Common/GPU/OpenGL/GLRenderManager.cpp @@ -554,6 +554,11 @@ void GLPushBuffer::Flush() { // Must be called from the render thread. _dbg_assert_(OnRenderThread()); + if (buf_ >= buffers_.size()) { + _dbg_assert_msg_(false, "buf_ somehow got out of sync: %d vs %d", (int)buf_, (int)buffers_.size()); + return; + } + buffers_[buf_].flushOffset = offset_; if (!buffers_[buf_].deviceMemory && writePtr_) { auto &info = buffers_[buf_]; From a132b72ba14a59f0fcdded67b95b74574ad3afb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 4 May 2023 01:24:31 +0200 Subject: [PATCH 08/11] Paranoia --- Common/UI/UIScreen.cpp | 1 + UI/GameInfoCache.cpp | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Common/UI/UIScreen.cpp b/Common/UI/UIScreen.cpp index 69739dcc249d..af640e49e878 100644 --- a/Common/UI/UIScreen.cpp +++ b/Common/UI/UIScreen.cpp @@ -111,6 +111,7 @@ void UIScreen::postRender() { if (!draw) { return; } + screenManager()->getUIContext()->Flush(); draw->EndFrame(); } diff --git a/UI/GameInfoCache.cpp b/UI/GameInfoCache.cpp index ceef80cedd13..2a6ff5b33f4b 100644 --- a/UI/GameInfoCache.cpp +++ b/UI/GameInfoCache.cpp @@ -365,7 +365,7 @@ class GameInfoWorkItem : public Task { } // In case of a remote file, check if it actually exists before locking. - if (!info_->GetFileLoader()->Exists()) { + if (!info_->GetFileLoader() || !info_->GetFileLoader()->Exists()) { return; } From 3148a8a437a9a5113715150b7d4afcc8ed5a0bb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 4 May 2023 08:58:52 +0200 Subject: [PATCH 09/11] PopupMultiChoiceDynamic: Check that valueStr_ isn't null before writing to it. Don't see how this happened. --- Common/UI/PopupScreens.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Common/UI/PopupScreens.h b/Common/UI/PopupScreens.h index acb37a030a36..0e0c13bcbc5e 100644 --- a/Common/UI/PopupScreens.h +++ b/Common/UI/PopupScreens.h @@ -272,7 +272,9 @@ class PopupMultiChoiceDynamic : public PopupMultiChoice { protected: void PostChoiceCallback(int num) override { - *valueStr_ = choices_[num]; + if (valueStr_) { + *valueStr_ = choices_[num]; + } } private: From 7ddcf62955880da485b331423ce084f64176fe93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 4 May 2023 09:00:48 +0200 Subject: [PATCH 10/11] Change TODO to a better idea --- GPU/GPUCommonHW.cpp | 2 -- GPU/GPUCommonHW.h | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/GPU/GPUCommonHW.cpp b/GPU/GPUCommonHW.cpp index 9c9645be9684..51cd770fe66c 100644 --- a/GPU/GPUCommonHW.cpp +++ b/GPU/GPUCommonHW.cpp @@ -806,7 +806,6 @@ void GPUCommonHW::FastRunLoop(DisplayList &list) { if (info.flags & FLAG_EXECUTE) { downcount = dc; (this->*info.func)(op, diff); - // TODO: Check pc here, and break if invalid, as the func can change it. Might an have a performance impact though. dc = downcount; } } else { @@ -820,7 +819,6 @@ void GPUCommonHW::FastRunLoop(DisplayList &list) { if (flags & (FLAG_EXECUTE | FLAG_EXECUTEONCHANGE)) { downcount = dc; (this->*info.func)(op, diff); - // TODO: Check pc here, and break if invalid, as the func can change it. Might have a performance impact though. dc = downcount; } else { uint64_t dirty = flags >> 8; diff --git a/GPU/GPUCommonHW.h b/GPU/GPUCommonHW.h index c1928de99e15..6ab7151f570f 100644 --- a/GPU/GPUCommonHW.h +++ b/GPU/GPUCommonHW.h @@ -62,6 +62,7 @@ class GPUCommonHW : public GPUCommon { void Execute_TexFlush(u32 op, u32 diff); + // TODO: Have these return an error code if they jump to a bad address. If bad, stop the FastRunLoop. typedef void (GPUCommonHW::*CmdFunc)(u32 op, u32 diff); void FastRunLoop(DisplayList &list) override; From 6a51b6f7bffb5438624590c7732d62dcef7a26bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 4 May 2023 09:20:05 +0200 Subject: [PATCH 11/11] Quick attempt to add some thread safety to GameInfo::fileLoader. --- UI/GameInfoCache.cpp | 16 ++++++++++++---- UI/GameInfoCache.h | 3 +++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/UI/GameInfoCache.cpp b/UI/GameInfoCache.cpp index 2a6ff5b33f4b..bdecb6412f43 100644 --- a/UI/GameInfoCache.cpp +++ b/UI/GameInfoCache.cpp @@ -197,9 +197,12 @@ bool GameInfo::LoadFromPath(const Path &gamePath) { std::lock_guard guard(lock); // No need to rebuild if we already have it loaded. if (filePath_ != gamePath) { - fileLoader.reset(ConstructFileLoader(gamePath)); - if (!fileLoader) - return false; + { + std::lock_guard guard(loaderLock); + fileLoader.reset(ConstructFileLoader(gamePath)); + if (!fileLoader) + return false; + } filePath_ = gamePath; // This is a fallback title, while we're loading / if unable to load. @@ -215,13 +218,18 @@ std::shared_ptr GameInfo::GetFileLoader() { // because Priority() calls GetFileLoader()... gnarly. return fileLoader; } + + std::lock_guard guard(loaderLock); if (!fileLoader) { - fileLoader.reset(ConstructFileLoader(filePath_)); + FileLoader *loader = ConstructFileLoader(filePath_); + fileLoader.reset(loader); + return fileLoader; } return fileLoader; } void GameInfo::DisposeFileLoader() { + std::lock_guard guard(loaderLock); fileLoader.reset(); } diff --git a/UI/GameInfoCache.h b/UI/GameInfoCache.h index 1df4f64ad54f..6c8d6c40d27e 100644 --- a/UI/GameInfoCache.h +++ b/UI/GameInfoCache.h @@ -119,6 +119,9 @@ class GameInfo { // to it. std::mutex lock; + // Controls access to the fileLoader pointer. + std::mutex loaderLock; + std::string id; std::string id_version; int disc_total = 0;