From 6862bc172110f4d39479f4244a7717555034bcce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 28 Aug 2026 22:43:58 +0200 Subject: [PATCH 1/5] Vulkan: Fix threading issues around pipeline layouts and the delete list pipelineLayouts_ was mutated from the main thread (CreatePipelineLayout, and the deferred callback queued by DestroyPipelineLayout) while the render thread walked it every frame in FlushDescriptors. Exiting a game in Vulkan mode hits this reliably: ~GPU_Vulkan stops the render thread and destroys the draw engine's layout, but the destruction is deferred onto the delete list and doesn't actually run until a BeginFrame two frames later, with the render thread running again. Guard the list, and the lifetime of the layouts in it, with a mutex. The global delete list had the same problem - VulkanDescSetPool::Recreate queues the old pool from FlushDescSets on the render thread, which happens for real once a game goes past the initial 1024 descriptors, while the main thread moves the list into the current frame's list in EndFrame(). Lock the queueing functions and Take's source list. While in there: * Take() didn't move queryPools_, so query pools queued for deletion sat on the global list until device teardown instead of being deleted a few frames later. * PerformDeletes now drains into a local list before destroying anything. A callback is allowed to queue further deletes (~VKFramebuffer's does, via ~VKRFramebuffer), which used to append to the very vector being iterated. They now get the normal deferral instead of running in the same pass. * Missing semicolon in BeginFrame that only compiles because VLOG is empty. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Vd8ntC2brCUtCrDJMqLbs8 --- Common/GPU/Vulkan/VulkanContext.cpp | 22 ++++++++- Common/GPU/Vulkan/VulkanContext.h | 58 ++++++++++++++++------- Common/GPU/Vulkan/VulkanRenderManager.cpp | 18 ++++++- Common/GPU/Vulkan/VulkanRenderManager.h | 7 +++ 4 files changed, 84 insertions(+), 21 deletions(-) diff --git a/Common/GPU/Vulkan/VulkanContext.cpp b/Common/GPU/Vulkan/VulkanContext.cpp index 9a4e4c8fd4..7366cf1085 100644 --- a/Common/GPU/Vulkan/VulkanContext.cpp +++ b/Common/GPU/Vulkan/VulkanContext.cpp @@ -1845,7 +1845,12 @@ void finalize_glslang() { glslang::FinalizeProcess(); } +// NOTE: Every vector in the class has to be listed both here and in PerformDeletes. If one is missing +// from Take, the objects in it linger on the global list until device teardown instead of being deleted +// a few frames later; if one is missing from PerformDeletes, they leak outright. Both are bugs. void VulkanDeleteList::Take(VulkanDeleteList &del) { + // The render thread can be queueing deletes into del (the global list) while we do this. + std::lock_guard lock(del.mutex_); _dbg_assert_(cmdPools_.empty()); _dbg_assert_(descPools_.empty()); _dbg_assert_(modules_.empty()); @@ -1862,6 +1867,7 @@ void VulkanDeleteList::Take(VulkanDeleteList &del) { _dbg_assert_(framebuffers_.empty()); _dbg_assert_(pipelineLayouts_.empty()); _dbg_assert_(descSetLayouts_.empty()); + _dbg_assert_(queryPools_.empty()); _dbg_assert_(callbacks_.empty()); cmdPools_ = std::move(del.cmdPools_); descPools_ = std::move(del.descPools_); @@ -1879,12 +1885,14 @@ void VulkanDeleteList::Take(VulkanDeleteList &del) { framebuffers_ = std::move(del.framebuffers_); pipelineLayouts_ = std::move(del.pipelineLayouts_); descSetLayouts_ = std::move(del.descSetLayouts_); + queryPools_ = std::move(del.queryPools_); callbacks_ = std::move(del.callbacks_); del.cmdPools_.clear(); del.descPools_.clear(); del.modules_.clear(); del.buffers_.clear(); del.buffersWithAllocs_.clear(); + del.bufferViews_.clear(); del.imageViews_.clear(); del.imagesWithAllocs_.clear(); del.deviceMemory_.clear(); @@ -1895,10 +1903,22 @@ void VulkanDeleteList::Take(VulkanDeleteList &del) { del.framebuffers_.clear(); del.pipelineLayouts_.clear(); del.descSetLayouts_.clear(); + del.queryPools_.clear(); del.callbacks_.clear(); } void VulkanDeleteList::PerformDeletes(VulkanContext *vulkan, VmaAllocator allocator) { + // Drain into a local list first. A callback is allowed to queue more deletes (~VKFramebuffer does, + // via ~VKRFramebuffer) - with the vectors already moved out, those land on an empty list and get + // performed on a later pass, rather than being appended to a vector we're iterating. It also means + // they get the normal deferral instead of being destroyed in the same pass they were queued in. + VulkanDeleteList taken; + taken.Take(*this); + deleteCount_ = taken.PerformDeletesInternal(vulkan, allocator); +} + +// See the note on Take() - every vector in the class must be handled here too, or it leaks. +int VulkanDeleteList::PerformDeletesInternal(VulkanContext *vulkan, VmaAllocator allocator) { int deleteCount = 0; for (auto &callback : callbacks_) { @@ -1993,7 +2013,7 @@ void VulkanDeleteList::PerformDeletes(VulkanContext *vulkan, VmaAllocator alloca deleteCount++; } queryPools_.clear(); - deleteCount_ = deleteCount; + return deleteCount; } void VulkanContext::GetImageMemoryRequirements(VkImage image, VkMemoryRequirements *mem_reqs, bool *dedicatedAllocation) { diff --git a/Common/GPU/Vulkan/VulkanContext.h b/Common/GPU/Vulkan/VulkanContext.h index 72e1576093..4b5e8e4040 100644 --- a/Common/GPU/Vulkan/VulkanContext.h +++ b/Common/GPU/Vulkan/VulkanContext.h @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -70,6 +71,19 @@ enum class PerfClass { typedef std::function DeleteCallback; // This is a bit repetitive... +// +// Thread safety: The queueing functions are locked, because the global delete list gets written from +// more than one thread. Most callers are on the main thread, but not all: +// * VulkanDescSetPool::Recreate, when a descriptor pool has to grow, runs from FlushDescSets on +// the render thread. This one really happens - some games go past the initial 1024 descriptors. +// * VulkanQueueRunner::ResizeReadbackBuffer runs from PerformReadback on the render thread. For +// blocking readbacks the main thread is parked in FlushSync so it can't collide, and the delayed +// ones never actually resize (the readback key contains the dimensions), but it's not worth +// relying on that staying true. +// Meanwhile the main thread moves the global list into the current frame's list in EndFrame(). +// +// PerformDeletes needs no lock of its own: it drains into a private local list (under Take's lock) and +// destroys from that, so the destruction never touches a list another thread can reach. class VulkanDeleteList { struct BufferWithAlloc { VkBuffer buffer; @@ -91,36 +105,40 @@ class VulkanDeleteList { public: // NOTE: These all take reference handles so they can zero the input value. - void QueueDeleteCommandPool(VkCommandPool &pool) { _dbg_assert_(pool != VK_NULL_HANDLE); cmdPools_.push_back(pool); pool = VK_NULL_HANDLE; } - void QueueDeleteDescriptorPool(VkDescriptorPool &pool) { _dbg_assert_(pool != VK_NULL_HANDLE); descPools_.push_back(pool); pool = VK_NULL_HANDLE; } - void QueueDeleteShaderModule(VkShaderModule &module) { _dbg_assert_(module != VK_NULL_HANDLE); modules_.push_back(module); module = VK_NULL_HANDLE; } - void QueueDeleteBuffer(VkBuffer &buffer) { _dbg_assert_(buffer != VK_NULL_HANDLE); buffers_.push_back(buffer); buffer = VK_NULL_HANDLE; } - void QueueDeleteBufferView(VkBufferView &bufferView) { _dbg_assert_(bufferView != VK_NULL_HANDLE); bufferViews_.push_back(bufferView); bufferView = VK_NULL_HANDLE; } - void QueueDeleteImageView(VkImageView &imageView) { _dbg_assert_(imageView != VK_NULL_HANDLE); imageViews_.push_back(imageView); imageView = VK_NULL_HANDLE; } - void QueueDeleteDeviceMemory(VkDeviceMemory &deviceMemory) { _dbg_assert_(deviceMemory != VK_NULL_HANDLE); deviceMemory_.push_back(deviceMemory); deviceMemory = VK_NULL_HANDLE; } - void QueueDeleteSampler(VkSampler &sampler) { _dbg_assert_(sampler != VK_NULL_HANDLE); samplers_.push_back(sampler); sampler = VK_NULL_HANDLE; } - void QueueDeletePipeline(VkPipeline &pipeline) { _dbg_assert_(pipeline != VK_NULL_HANDLE); pipelines_.push_back(pipeline); pipeline = VK_NULL_HANDLE; } - void QueueDeletePipelineCache(VkPipelineCache &pipelineCache) { _dbg_assert_(pipelineCache != VK_NULL_HANDLE); pipelineCaches_.push_back(pipelineCache); pipelineCache = VK_NULL_HANDLE; } - void QueueDeleteRenderPass(VkRenderPass &renderPass) { _dbg_assert_(renderPass != VK_NULL_HANDLE); renderPasses_.push_back(renderPass); renderPass = VK_NULL_HANDLE; } - void QueueDeleteFramebuffer(VkFramebuffer &framebuffer) { _dbg_assert_(framebuffer != VK_NULL_HANDLE); framebuffers_.push_back(framebuffer); framebuffer = VK_NULL_HANDLE; } - void QueueDeletePipelineLayout(VkPipelineLayout &pipelineLayout) { _dbg_assert_(pipelineLayout != VK_NULL_HANDLE); pipelineLayouts_.push_back(pipelineLayout); pipelineLayout = VK_NULL_HANDLE; } - void QueueDeleteDescriptorSetLayout(VkDescriptorSetLayout &descSetLayout) { _dbg_assert_(descSetLayout != VK_NULL_HANDLE); descSetLayouts_.push_back(descSetLayout); descSetLayout = VK_NULL_HANDLE; } - void QueueDeleteQueryPool(VkQueryPool &queryPool) { _dbg_assert_(queryPool != VK_NULL_HANDLE); queryPools_.push_back(queryPool); queryPool = VK_NULL_HANDLE; } - void QueueCallback(DeleteCallback func) { callbacks_.push_back(func); } + void QueueDeleteCommandPool(VkCommandPool &pool) { _dbg_assert_(pool != VK_NULL_HANDLE); std::lock_guard lock(mutex_); cmdPools_.push_back(pool); pool = VK_NULL_HANDLE; } + void QueueDeleteDescriptorPool(VkDescriptorPool &pool) { _dbg_assert_(pool != VK_NULL_HANDLE); std::lock_guard lock(mutex_); descPools_.push_back(pool); pool = VK_NULL_HANDLE; } + void QueueDeleteShaderModule(VkShaderModule &module) { _dbg_assert_(module != VK_NULL_HANDLE); std::lock_guard lock(mutex_); modules_.push_back(module); module = VK_NULL_HANDLE; } + void QueueDeleteBuffer(VkBuffer &buffer) { _dbg_assert_(buffer != VK_NULL_HANDLE); std::lock_guard lock(mutex_); buffers_.push_back(buffer); buffer = VK_NULL_HANDLE; } + void QueueDeleteBufferView(VkBufferView &bufferView) { _dbg_assert_(bufferView != VK_NULL_HANDLE); std::lock_guard lock(mutex_); bufferViews_.push_back(bufferView); bufferView = VK_NULL_HANDLE; } + void QueueDeleteImageView(VkImageView &imageView) { _dbg_assert_(imageView != VK_NULL_HANDLE); std::lock_guard lock(mutex_); imageViews_.push_back(imageView); imageView = VK_NULL_HANDLE; } + void QueueDeleteDeviceMemory(VkDeviceMemory &deviceMemory) { _dbg_assert_(deviceMemory != VK_NULL_HANDLE); std::lock_guard lock(mutex_); deviceMemory_.push_back(deviceMemory); deviceMemory = VK_NULL_HANDLE; } + void QueueDeleteSampler(VkSampler &sampler) { _dbg_assert_(sampler != VK_NULL_HANDLE); std::lock_guard lock(mutex_); samplers_.push_back(sampler); sampler = VK_NULL_HANDLE; } + void QueueDeletePipeline(VkPipeline &pipeline) { _dbg_assert_(pipeline != VK_NULL_HANDLE); std::lock_guard lock(mutex_); pipelines_.push_back(pipeline); pipeline = VK_NULL_HANDLE; } + void QueueDeletePipelineCache(VkPipelineCache &pipelineCache) { _dbg_assert_(pipelineCache != VK_NULL_HANDLE); std::lock_guard lock(mutex_); pipelineCaches_.push_back(pipelineCache); pipelineCache = VK_NULL_HANDLE; } + void QueueDeleteRenderPass(VkRenderPass &renderPass) { _dbg_assert_(renderPass != VK_NULL_HANDLE); std::lock_guard lock(mutex_); renderPasses_.push_back(renderPass); renderPass = VK_NULL_HANDLE; } + void QueueDeleteFramebuffer(VkFramebuffer &framebuffer) { _dbg_assert_(framebuffer != VK_NULL_HANDLE); std::lock_guard lock(mutex_); framebuffers_.push_back(framebuffer); framebuffer = VK_NULL_HANDLE; } + void QueueDeletePipelineLayout(VkPipelineLayout &pipelineLayout) { _dbg_assert_(pipelineLayout != VK_NULL_HANDLE); std::lock_guard lock(mutex_); pipelineLayouts_.push_back(pipelineLayout); pipelineLayout = VK_NULL_HANDLE; } + void QueueDeleteDescriptorSetLayout(VkDescriptorSetLayout &descSetLayout) { _dbg_assert_(descSetLayout != VK_NULL_HANDLE); std::lock_guard lock(mutex_); descSetLayouts_.push_back(descSetLayout); descSetLayout = VK_NULL_HANDLE; } + void QueueDeleteQueryPool(VkQueryPool &queryPool) { _dbg_assert_(queryPool != VK_NULL_HANDLE); std::lock_guard lock(mutex_); queryPools_.push_back(queryPool); queryPool = VK_NULL_HANDLE; } + void QueueCallback(DeleteCallback func) { std::lock_guard lock(mutex_); callbacks_.push_back(func); } - void QueueDeleteBufferAllocation(VkBuffer &buffer, VmaAllocation &alloc) { - _dbg_assert_(buffer != VK_NULL_HANDLE); + void QueueDeleteBufferAllocation(VkBuffer &buffer, VmaAllocation &alloc) { + _dbg_assert_(buffer != VK_NULL_HANDLE); + std::lock_guard lock(mutex_); buffersWithAllocs_.push_back(BufferWithAlloc{ buffer, alloc }); buffer = VK_NULL_HANDLE; alloc = VK_NULL_HANDLE; } void QueueDeleteImageAllocation(VkImage &image, VmaAllocation &alloc) { _dbg_assert_(image != VK_NULL_HANDLE && alloc != VK_NULL_HANDLE); + std::lock_guard lock(mutex_); imagesWithAllocs_.push_back(ImageWithAlloc{ image, alloc }); image = VK_NULL_HANDLE; alloc = VK_NULL_HANDLE; } + // Moves everything from del into this list. Only the source list is locked - the destination is + // always a frame's own list, which only the main thread touches. void Take(VulkanDeleteList &del); void PerformDeletes(VulkanContext *vulkan, VmaAllocator allocator); @@ -129,6 +147,10 @@ public: } private: + // Does the actual destruction, on a list that's been drained out of the shared one. Returns the count. + int PerformDeletesInternal(VulkanContext *vulkan, VmaAllocator allocator); + + std::mutex mutex_; std::vector cmdPools_; std::vector descPools_; std::vector modules_; diff --git a/Common/GPU/Vulkan/VulkanRenderManager.cpp b/Common/GPU/Vulkan/VulkanRenderManager.cpp index d8b7574b9c..cafd714545 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.cpp +++ b/Common/GPU/Vulkan/VulkanRenderManager.cpp @@ -694,7 +694,7 @@ void VulkanRenderManager::PollPresentTiming() { } void VulkanRenderManager::BeginFrame(bool enableProfiling, bool enableLogProfiler) { - double frameBeginTime = time_now_d() + double frameBeginTime = time_now_d(); VLOG("BeginFrame"); VkDevice device = vulkan_->GetDevice(); @@ -1806,12 +1806,22 @@ VKRPipelineLayout *VulkanRenderManager::CreatePipelineLayout(BindingType *bindin layout->frameData[i].pool.Create(vulkan_, bindingTypes, (uint32_t)bindingTypesCount, 1024); } - pipelineLayouts_.push_back(layout); + { + std::lock_guard lock(pipelineLayoutsMutex_); + pipelineLayouts_.push_back(layout); + } return layout; } void VulkanRenderManager::DestroyPipelineLayout(VKRPipelineLayout *layout) { + // The layout has to stay in pipelineLayouts_ until the frames that were recorded with it have been + // flushed by the render thread, otherwise their descriptor sets never get written. So, we can't + // remove it here - instead we let it ride along on the delete list, which won't be run until the + // fence for the frame it was queued in has been waited on. vulkan_->Delete().QueueCallback([this, layout](VulkanContext *vulkan) { + // Runs on the main thread, while the render thread may be in FlushDescriptors - so both the + // erase and the destruction of the layout itself have to be under the lock. + std::lock_guard lock(pipelineLayoutsMutex_); for (auto iter = pipelineLayouts_.begin(); iter != pipelineLayouts_.end(); iter++) { if (*iter == layout) { pipelineLayouts_.erase(iter); @@ -1828,13 +1838,17 @@ void VulkanRenderManager::DestroyPipelineLayout(VKRPipelineLayout *layout) { }); } +// Called on the render thread. void VulkanRenderManager::FlushDescriptors(int frame) { + std::lock_guard lock(pipelineLayoutsMutex_); for (VKRPipelineLayout *iter : pipelineLayouts_) { iter->FlushDescSets(vulkan_, frame, &frameData_[frame].profile); } } +// Called on the main thread, from BeginFrame. void VulkanRenderManager::ResetDescriptorLists(int frame) { + std::lock_guard lock(pipelineLayoutsMutex_); for (VKRPipelineLayout *iter : pipelineLayouts_) { VKRPipelineLayout::FrameData &data = iter->frameData[frame]; diff --git a/Common/GPU/Vulkan/VulkanRenderManager.h b/Common/GPU/Vulkan/VulkanRenderManager.h index 024d81b094..4497f3ce2d 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.h +++ b/Common/GPU/Vulkan/VulkanRenderManager.h @@ -643,5 +643,12 @@ private: HistoryBuffer &frameTimeHistory_; VKRPipelineLayout *curPipelineLayout_ = nullptr; + + // Guards pipelineLayouts_, both the vector itself and the lifetime of the layouts in it. + // The list is added to by CreatePipelineLayout and erased from by the deferred callback queued by + // DestroyPipelineLayout, both of which run on the main thread, while the render thread walks it + // every frame in FlushDescriptors. Contention is negligible - layouts are only created and destroyed + // when a game starts or stops, and the lock is otherwise taken exactly twice per frame. + std::mutex pipelineLayoutsMutex_; std::vector pipelineLayouts_; }; From f9315e9bc1b633e5ed6260f3417c56a83610df80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 28 Aug 2026 22:56:18 +0200 Subject: [PATCH 2/5] Vulkan: Keep draining the delete list until a lap comes up empty The previous commit moved everything out of the list before running callbacks, to avoid appending to a vector being iterated. That regressed device teardown: a callback can queue more deletes (~VKFramebuffer does, via ~VKRFramebuffer, which queues image views, image allocations and framebuffers), and those land back on a list that used to be picked up by the object loops later in the same pass. That's harmless for the per-frame lists, since callbacks queue onto the global list and a later frame drains it. But PerformPendingDeletes() drains the global list itself, and DestroyDevice() calls it immediately before vmaDestroyAllocator and vkDestroyDevice - so the re-queued objects were never destroyed at all. Loop instead. In the per-frame case that's one extra empty lap. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Vd8ntC2brCUtCrDJMqLbs8 --- Common/GPU/Vulkan/VulkanContext.cpp | 25 ++++++++++++++++++------- Common/GPU/Vulkan/VulkanContext.h | 2 +- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/Common/GPU/Vulkan/VulkanContext.cpp b/Common/GPU/Vulkan/VulkanContext.cpp index 7366cf1085..94af245d1d 100644 --- a/Common/GPU/Vulkan/VulkanContext.cpp +++ b/Common/GPU/Vulkan/VulkanContext.cpp @@ -1908,13 +1908,24 @@ void VulkanDeleteList::Take(VulkanDeleteList &del) { } void VulkanDeleteList::PerformDeletes(VulkanContext *vulkan, VmaAllocator allocator) { - // Drain into a local list first. A callback is allowed to queue more deletes (~VKFramebuffer does, - // via ~VKRFramebuffer) - with the vectors already moved out, those land on an empty list and get - // performed on a later pass, rather than being appended to a vector we're iterating. It also means - // they get the normal deferral instead of being destroyed in the same pass they were queued in. - VulkanDeleteList taken; - taken.Take(*this); - deleteCount_ = taken.PerformDeletesInternal(vulkan, allocator); + // Drain into a local list before destroying anything - a callback is allowed to queue more deletes + // (~VKFramebuffer does, via ~VKRFramebuffer), and we mustn't append to a vector we're iterating. + // Anything queued back onto this list gets picked up by the next lap, so keep going until a lap + // comes up empty. In the normal per-frame case that's a single extra lap, since callbacks queue + // onto the global list rather than the frame's own list - the loop matters for + // VulkanContext::PerformPendingDeletes(), which drains the global list itself just before the + // device goes away, with no later pass to catch the stragglers. + int deleteCount = 0; + for (;;) { + VulkanDeleteList taken; + taken.Take(*this); + int count = taken.PerformDeletesInternal(vulkan, allocator); + if (!count) { + break; + } + deleteCount += count; + } + deleteCount_ = deleteCount; } // See the note on Take() - every vector in the class must be handled here too, or it leaks. diff --git a/Common/GPU/Vulkan/VulkanContext.h b/Common/GPU/Vulkan/VulkanContext.h index 4b5e8e4040..a8e9e7b03f 100644 --- a/Common/GPU/Vulkan/VulkanContext.h +++ b/Common/GPU/Vulkan/VulkanContext.h @@ -138,7 +138,7 @@ public: } // Moves everything from del into this list. Only the source list is locked - the destination is - // always a frame's own list, which only the main thread touches. + // either a frame's own list or a stack local, neither of which another thread can reach. void Take(VulkanDeleteList &del); void PerformDeletes(VulkanContext *vulkan, VmaAllocator allocator); From 14ce62af9ebb108d184394afe1f9b9334c22a113 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 28 Aug 2026 23:27:13 +0200 Subject: [PATCH 3/5] Vulkan: Four small correctness fixes found while reviewing Common/GPU * TransitionDepthStencilImageAuto set dstAccessMask to TRANSFER_READ_BIT for TRANSFER_DST_OPTIMAL. The color path and this function's own source-side switch both use TRANSFER_WRITE_BIT - it's a copy-paste from the TRANSFER_SRC case two lines up. Every depth copy and blit went through it. * VulkanMayBeAvailable's per-device loop did anyGood = !blacklisted, overwriting the verdict from earlier devices, so a blacklisted GPU enumerated after a good one hid the Vulkan backend entirely. Hybrid-GPU machines are exactly what the blacklist targets. * The instance extension scan stopped as soon as it found the platform surface extension, so a driver reporting that before VK_KHR_surface made us give up with "Platform surface extension not found". Enumeration order isn't specified. * CreateDevice only logged when vkCreateDevice failed, then carried on to report success, call VulkanSetAvailable(true) and build a VMA allocator on a null device behind an assert that's live in release builds. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Vd8ntC2brCUtCrDJMqLbs8 --- Common/GPU/Vulkan/VulkanBarrier.cpp | 2 +- Common/GPU/Vulkan/VulkanContext.cpp | 8 ++++++-- Common/GPU/Vulkan/VulkanLoader.cpp | 8 +++++--- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/Common/GPU/Vulkan/VulkanBarrier.cpp b/Common/GPU/Vulkan/VulkanBarrier.cpp index abdd50ca2e..b91596c73d 100644 --- a/Common/GPU/Vulkan/VulkanBarrier.cpp +++ b/Common/GPU/Vulkan/VulkanBarrier.cpp @@ -212,7 +212,7 @@ void VulkanBarrierBatch::TransitionDepthStencilImageAuto( dstStageMask_ |= VK_PIPELINE_STAGE_TRANSFER_BIT; break; case VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL: - dstAccessMask = VK_ACCESS_TRANSFER_READ_BIT; + dstAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; dstStageMask_ |= VK_PIPELINE_STAGE_TRANSFER_BIT; break; case VK_IMAGE_LAYOUT_DEPTH_STENCIL_ATTACHMENT_OPTIMAL: diff --git a/Common/GPU/Vulkan/VulkanContext.cpp b/Common/GPU/Vulkan/VulkanContext.cpp index 94af245d1d..4b6604a2ef 100644 --- a/Common/GPU/Vulkan/VulkanContext.cpp +++ b/Common/GPU/Vulkan/VulkanContext.cpp @@ -907,9 +907,13 @@ VkResult VulkanContext::CreateDevice(int physical_device, const std::vector Date: Fri, 28 Aug 2026 23:27:21 +0200 Subject: [PATCH 4/5] OpenGL: Drop the dead Intel dual-source check, fix the ES3 init fallback The Intel workaround sscanf'd "Build %d.%d.%d.%d" against glGetString(GL_VERSION), which reads like "4.5.0 - Build 26.20.100.7870" - sscanf literals have to match from the start, so it never returned 4 and HasIntelDualSrcBug was never consulted. It's been inert since it was written, and the drivers it targeted are long gone. Removing it orphaned the two helpers, so those go too. Separately, when gl3stubInit() fails we left ver[0] at 3 while clearing GLES3. Extension enumeration keys off the version, not the flag, so it went on to call glGetStringi - one of the very entry points whose absence makes gl3stubInit() fail. Drop back to 2.0 on that path, like the branch above it already does, and null-check what glGetStringi hands back. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Vd8ntC2brCUtCrDJMqLbs8 --- Common/GPU/OpenGL/GLFeatures.cpp | 10 ++++++++++ Common/GPU/OpenGL/thin3d_gl.cpp | 31 ------------------------------- 2 files changed, 10 insertions(+), 31 deletions(-) diff --git a/Common/GPU/OpenGL/GLFeatures.cpp b/Common/GPU/OpenGL/GLFeatures.cpp index 18aa1bd714..6877dbd76c 100644 --- a/Common/GPU/OpenGL/GLFeatures.cpp +++ b/Common/GPU/OpenGL/GLFeatures.cpp @@ -306,6 +306,13 @@ bool CheckGLExtensions() { // Otherwise, let's trust GL_MAJOR_VERSION. Note that Mali is intentionally not banned here. if (gl_extensions.ver[0] >= 3) { gl_extensions.GLES3 = gl3stubInit(); + if (!gl_extensions.GLES3) { + // We failed to load the ES3 entry points, so we can't use any of them - including + // glGetStringi below, which is one of the ones gl3stubInit() checks for. Drop back + // to 2.0 so nothing downstream keys off the version and calls into a null pointer. + gl_extensions.ver[0] = 2; + gl_extensions.ver[1] = 0; + } } } #else @@ -331,6 +338,9 @@ bool CheckGLExtensions() { g_set_gl_extensions.clear(); for (GLint i = 0; i < numExtensions; ++i) { const char *ext = (const char *)glGetStringi(GL_EXTENSIONS, i); + if (!ext) { + continue; + } g_set_gl_extensions.insert(ext); g_all_gl_extensions += ext; g_all_gl_extensions += " "; diff --git a/Common/GPU/OpenGL/thin3d_gl.cpp b/Common/GPU/OpenGL/thin3d_gl.cpp index 1947cf6cf2..1c27c6e2b8 100644 --- a/Common/GPU/OpenGL/thin3d_gl.cpp +++ b/Common/GPU/OpenGL/thin3d_gl.cpp @@ -534,26 +534,6 @@ private: PresentMode requestedPresentMode_{}; }; -static constexpr int MakeIntelSimpleVer(int v1, int v2, int v3) { - return (v1 << 16) | (v2 << 8) | v3; -} - -static bool HasIntelDualSrcBug(const int versions[4]) { - // Intel uses a confusing set of at least 3 version numbering schemes. This is the one given to OpenGL. - switch (MakeIntelSimpleVer(versions[0], versions[1], versions[2])) { - case MakeIntelSimpleVer(9, 17, 10): - case MakeIntelSimpleVer(9, 18, 10): - return false; - case MakeIntelSimpleVer(10, 18, 10): - return versions[3] < 4061; - case MakeIntelSimpleVer(10, 18, 14): - return versions[3] < 4080; - default: - // Older than above didn't support dual src anyway, newer should have the fix. - return false; - } -} - OpenGLContext::OpenGLContext(bool canChangeSwapInterval) : renderManager_(frameTimeHistory_) { if (gl_extensions.IsGLES) { if (gl_extensions.OES_packed_depth_stencil || gl_extensions.OES_depth24) { @@ -647,17 +627,6 @@ OpenGLContext::OpenGLContext(bool canChangeSwapInterval) : renderManager_(frameT if (!gl_extensions.VersionGEThan(3, 0, 0)) { // Don't use this extension on sub 3.0 OpenGL versions as it does not seem reliable. bugs_.Infest(Bugs::DUAL_SOURCE_BLENDING_BROKEN); - } else if (caps_.vendor == GPUVendor::VENDOR_INTEL) { - // Note: this is for Intel drivers with GL3+. - // Also on Intel, see https://github.com/hrydgard/ppsspp/issues/10117 - // TODO: Remove entirely sometime reasonably far in driver years after 2015. - const std::string ver = OpenGLContext::GetInfoString(Draw::InfoField::APIVERSION); - int versions[4]{}; - if (sscanf(ver.c_str(), "Build %d.%d.%d.%d", &versions[0], &versions[1], &versions[2], &versions[3]) == 4) { - if (HasIntelDualSrcBug(versions)) { - bugs_.Infest(Bugs::DUAL_SOURCE_BLENDING_BROKEN); - } - } } #if PPSSPP_ARCH(ARMV7) From 86cd7c43e1a79a86822da16ef8c846019d955c06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 28 Aug 2026 23:27:28 +0200 Subject: [PATCH 5/5] GLSLProgram: Cut it down to what's actually used, fix a shader leak This is a leftover from the old "native" library. Its only users are the Win32 GE debugger's preview windows, which call glsl_create_source/destroy/bind/unbind and read four locations off the struct. Everything else was dead: glsl_create was declared but never defined anywhere, which made the entire file-loading and auto-reload half of glsl_recompile unreachable (glsl_create_source always passes empty filenames), along with the mtime fields, AutoCharArrayBuf and the VFS/stat includes. glsl_attrib_loc, glsl_uniform_loc and glsl_get_program had no callers, and the active_programs set was written and never read. The unused convenience locations cost a glGetUniformLocation round trip each at link time. The bug: the vertex shader was leaked when its own compile failed - the fragment path right below it already deleted it correctly. Failed links leaked the program object too. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Vd8ntC2brCUtCrDJMqLbs8 --- Common/GPU/OpenGL/GLSLProgram.cpp | 171 +++++------------------------- Common/GPU/OpenGL/GLSLProgram.h | 45 ++------ 2 files changed, 36 insertions(+), 180 deletions(-) diff --git a/Common/GPU/OpenGL/GLSLProgram.cpp b/Common/GPU/OpenGL/GLSLProgram.cpp index c8c93dc0e9..2de61f8b0c 100644 --- a/Common/GPU/OpenGL/GLSLProgram.cpp +++ b/Common/GPU/OpenGL/GLSLProgram.cpp @@ -1,19 +1,12 @@ -#include - #include #include -#include -#include "Common/File/VFS/VFS.h" -#include "Common/File/FileUtil.h" #include "Common/GPU/OpenGL/GLSLProgram.h" #include "Common/Log.h" -static std::set active_programs; - -bool CompileShader(const char *source, GLuint shader, const char *filename, std::string *error_message) { - glShaderSource(shader, 1, &source, NULL); +static bool CompileShader(const char *source, GLuint shader, const char *stageName, std::string *error_message) { + glShaderSource(shader, 1, &source, nullptr); glCompileShader(shader); GLint success; glGetShaderiv(shader, GL_COMPILE_STATUS, &success); @@ -23,9 +16,9 @@ bool CompileShader(const char *source, GLuint shader, const char *filename, std: GLsizei len; glGetShaderInfoLog(shader, MAX_INFO_LOG_SIZE, &len, infoLog); infoLog[len] = '\0'; - ERROR_LOG(Log::G3D, "Error in shader compilation of %s!\n", filename); + ERROR_LOG(Log::G3D, "Error compiling %s shader!\n", stageName); ERROR_LOG(Log::G3D, "Info log: %s\n", infoLog); - ERROR_LOG(Log::G3D, "Shader source:\n%s\n", (const char *)source); + ERROR_LOG(Log::G3D, "Shader source:\n%s\n", source); if (error_message) *error_message = infoLog; return false; @@ -33,104 +26,17 @@ bool CompileShader(const char *source, GLuint shader, const char *filename, std: return true; } -GLSLProgram *glsl_create_source(const char *vshader_src, const char *fshader_src, std::string *error_message) { - GLSLProgram *program = new GLSLProgram(); - program->program_ = 0; - program->vsh_ = 0; - program->fsh_ = 0; - program->vshader_source = vshader_src; - program->fshader_source = fshader_src; - strcpy(program->name, "[srcshader]"); - strcpy(program->vshader_filename, ""); - strcpy(program->fshader_filename, ""); - if (glsl_recompile(program, error_message)) { - active_programs.insert(program); - } else { - ERROR_LOG(Log::G3D, "Failed compiling GLSL program from source strings"); - delete program; - return 0; - } - return program; -} - -// Not wanting to change ReadLocalFile semantics. -// TODO: Use C++11 unique_ptr, remove delete[] -struct AutoCharArrayBuf { - AutoCharArrayBuf(char *buf = nullptr) : buf_(buf) { - } - ~AutoCharArrayBuf() { - delete [] buf_; - buf_ = nullptr; - } - void reset(char *buf) { - delete[] buf_; - buf_ = buf; - } - operator char *() { - return buf_; - } - -private: - char *buf_; -}; - -bool glsl_recompile(GLSLProgram *program, std::string *error_message) { - struct stat vs, fs; - AutoCharArrayBuf vsh_src, fsh_src; - - if (strlen(program->vshader_filename) > 0 && 0 == stat(program->vshader_filename, &vs)) { - program->vshader_mtime = vs.st_mtime; - if (!program->vshader_source) { - size_t sz; - vsh_src.reset((char *)File::ReadLocalFile(Path(program->vshader_filename), &sz)); - } - } else { - program->vshader_mtime = 0; - } - - if (strlen(program->fshader_filename) > 0 && 0 == stat(program->fshader_filename, &fs)) { - program->fshader_mtime = fs.st_mtime; - if (!program->fshader_source) { - size_t sz; - fsh_src.reset((char *)File::ReadLocalFile(Path(program->fshader_filename), &sz)); - } - } else { - program->fshader_mtime = 0; - } - - if (!program->vshader_source && !vsh_src) { - size_t sz; - vsh_src.reset((char *)g_VFS.ReadFile(program->vshader_filename, &sz)); - } - if (!program->vshader_source && !vsh_src) { - ERROR_LOG(Log::G3D, "File missing: %s", program->vshader_filename); - if (error_message) { - *error_message = std::string("File missing: ") + program->vshader_filename; - } - return false; - } - if (!program->fshader_source && !fsh_src) { - size_t sz; - fsh_src.reset((char *)g_VFS.ReadFile(program->fshader_filename, &sz)); - } - if (!program->fshader_source && !fsh_src) { - ERROR_LOG(Log::G3D, "File missing: %s", program->fshader_filename); - if (error_message) { - *error_message = std::string("File missing: ") + program->fshader_filename; - } - return false; - } - +static bool glsl_compile(GLSLProgram *program, const char *vshader_src, const char *fshader_src, std::string *error_message) { GLuint vsh = glCreateShader(GL_VERTEX_SHADER); - const GLchar *vsh_str = program->vshader_source ? program->vshader_source : (const GLchar *)(vsh_src); - if (!CompileShader(vsh_str, vsh, program->vshader_filename, error_message)) { + if (!CompileShader(vshader_src, vsh, "vertex", error_message)) { + glDeleteShader(vsh); return false; } - const GLchar *fsh_str = program->fshader_source ? program->fshader_source : (const GLchar *)(fsh_src); GLuint fsh = glCreateShader(GL_FRAGMENT_SHADER); - if (!CompileShader(fsh_str, fsh, program->fshader_filename, error_message)) { + if (!CompileShader(fshader_src, fsh, "fragment", error_message)) { glDeleteShader(vsh); + glDeleteShader(fsh); return false; } @@ -146,63 +52,47 @@ bool glsl_recompile(GLSLProgram *program, std::string *error_message) { GLint bufLength = 0; glGetProgramiv(prog, GL_INFO_LOG_LENGTH, &bufLength); if (bufLength) { - char* buf = new char[bufLength + 1]; // safety - glGetProgramInfoLog(prog, bufLength, NULL, buf); - INFO_LOG(Log::G3D, "vsh: %i fsh: %i", vsh, fsh); + char *buf = new char[bufLength + 1]; // safety + glGetProgramInfoLog(prog, bufLength, nullptr, buf); ERROR_LOG(Log::G3D, "Could not link shader program (linkstatus=%i):\n %s \n", linkStatus, buf); if (error_message) { *error_message = buf; } - delete [] buf; + delete[] buf; } else { - INFO_LOG(Log::G3D, "vsh: %i fsh: %i", vsh, fsh); ERROR_LOG(Log::G3D, "Could not link shader program (linkstatus=%i). No OpenGL error log was available.", linkStatus); if (error_message) { *error_message = "(no error message available)"; } } + glDeleteProgram(prog); glDeleteShader(vsh); glDeleteShader(fsh); return false; } - // Destroy the old program, if any. - if (program->program_) { - glDeleteProgram(program->program_); - } - program->program_ = prog; program->vsh_ = vsh; program->fsh_ = fsh; program->sampler0 = glGetUniformLocation(program->program_, "sampler0"); - program->sampler1 = glGetUniformLocation(program->program_, "sampler1"); - - program->a_position = glGetAttribLocation(program->program_, "a_position"); - program->a_color = glGetAttribLocation(program->program_, "a_color"); - program->a_normal = glGetAttribLocation(program->program_, "a_normal"); - program->a_texcoord0 = glGetAttribLocation(program->program_, "a_texcoord0"); - program->a_texcoord1 = glGetAttribLocation(program->program_, "a_texcoord1"); - - program->u_worldviewproj = glGetUniformLocation(program->program_, "u_worldviewproj"); - program->u_world = glGetUniformLocation(program->program_, "u_world"); program->u_viewproj = glGetUniformLocation(program->program_, "u_viewproj"); - program->u_fog = glGetUniformLocation(program->program_, "u_fog"); - program->u_sundir = glGetUniformLocation(program->program_, "u_sundir"); - program->u_camerapos = glGetUniformLocation(program->program_, "u_camerapos"); - - //INFO_LOG(Log::G3D, "Shader compilation success: %s %s", - // program->vshader_filename, - // program->fshader_filename); + program->a_position = glGetAttribLocation(program->program_, "a_position"); + program->a_texcoord0 = glGetAttribLocation(program->program_, "a_texcoord0"); return true; } -int glsl_attrib_loc(const GLSLProgram *program, const char *name) { - return glGetAttribLocation(program->program_, name); -} - -int glsl_uniform_loc(const GLSLProgram *program, const char *name) { - return glGetUniformLocation(program->program_, name); +GLSLProgram *glsl_create_source(const char *vshader_src, const char *fshader_src, std::string *error_message) { + GLSLProgram *program = new GLSLProgram(); + program->program_ = 0; + program->vsh_ = 0; + program->fsh_ = 0; + if (!glsl_compile(program, vshader_src, fshader_src, error_message)) { + ERROR_LOG(Log::G3D, "Failed compiling GLSL program from source strings"); + delete program; + return nullptr; + } + return program; } void glsl_destroy(GLSLProgram *program) { @@ -210,28 +100,19 @@ void glsl_destroy(GLSLProgram *program) { glDeleteShader(program->vsh_); glDeleteShader(program->fsh_); glDeleteProgram(program->program_); - active_programs.erase(program); } else { ERROR_LOG(Log::G3D, "Deleting null GLSL program!"); } delete program; } -static const GLSLProgram *curProgram; - void glsl_bind(const GLSLProgram *program) { if (program) glUseProgram(program->program_); else glUseProgram(0); - curProgram = program; } void glsl_unbind() { glUseProgram(0); - curProgram = nullptr; -} - -const GLSLProgram *glsl_get_program() { - return curProgram; } diff --git a/Common/GPU/OpenGL/GLSLProgram.h b/Common/GPU/OpenGL/GLSLProgram.h index fff73a957b..9b1cd8754e 100644 --- a/Common/GPU/OpenGL/GLSLProgram.h +++ b/Common/GPU/OpenGL/GLSLProgram.h @@ -1,40 +1,24 @@ // Utility code for loading GLSL shaders. -// Has support for auto-reload, see glsl_refresh +// +// This is a small leftover from the old "native" library, only used by the Win32 GE debugger's +// preview windows, which talk to GL directly rather than going through thin3d. Everything here +// works on shader source strings - the file loading and auto-reload support that used to live +// here was never used in PPSSPP and is gone. #pragma once #include -#include #include "Common/GPU/OpenGL/GLCommon.h" // Represent a compiled and linked vshader/fshader pair. -// A just-constructed object is valid but cannot be used as a shader program, meaning that -// yes, you can declare these as globals if you like. struct GLSLProgram { - char name[16]; - char vshader_filename[256]; - char fshader_filename[256]; - const char *vshader_source; - const char *fshader_source; - time_t vshader_mtime; - time_t fshader_mtime; - - // Locations to some common uniforms. Hardcoded for speed. + // Locations of the uniforms and attributes the callers use, looked up once at link time. + // Add to these as needed - each one costs a lookup per link. GLint sampler0; - GLint sampler1; - GLint u_worldviewproj; - GLint u_world; GLint u_viewproj; - GLint u_fog; // rgb = color, a = density - GLint u_sundir; - GLint u_camerapos; - GLint a_position; - GLint a_color; - GLint a_normal; GLint a_texcoord0; - GLint a_texcoord1; // Private to the implementation, do not touch GLuint vsh_; @@ -42,19 +26,10 @@ struct GLSLProgram { GLuint program_; }; -// C API, old skool. Not much point either... - -// From files (VFS) -GLSLProgram *glsl_create(const char *vshader_file, const char *fshader_file, std::string *error_message = 0); -// Directly from source code -GLSLProgram *glsl_create_source(const char *vshader_src, const char *fshader_src, std::string *error_message = 0); +// Compiles and links a program. Returns nullptr on failure, having logged the reason and, +// if error_message is non-null, copied it there. +GLSLProgram *glsl_create_source(const char *vshader_src, const char *fshader_src, std::string *error_message = nullptr); void glsl_destroy(GLSLProgram *program); -// If recompilation of the program fails, the program is untouched and error messages -// are logged and the function returns false. -bool glsl_recompile(GLSLProgram *program, std::string *error_message = 0); void glsl_bind(const GLSLProgram *program); -const GLSLProgram *glsl_get_program(); void glsl_unbind(); -int glsl_attrib_loc(const GLSLProgram *program, const char *name); -int glsl_uniform_loc(const GLSLProgram *program, const char *name);