From bd8067a631b765126f83bd7d9fbb192305d61a3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sat, 11 Nov 2017 19:38:43 +0100 Subject: [PATCH 1/3] Reduce a ERROR_LOG_REPORT to a warning (vfpu branches in delay slots) --- Core/MIPS/x86/CompBranch.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Core/MIPS/x86/CompBranch.cpp b/Core/MIPS/x86/CompBranch.cpp index b87cd5e6f9..7d3a52091a 100644 --- a/Core/MIPS/x86/CompBranch.cpp +++ b/Core/MIPS/x86/CompBranch.cpp @@ -545,7 +545,9 @@ void Jit::BranchVFPUFlag(MIPSOpcode op, Gen::CCFlags cc, bool likely) { CONDITIONAL_LOG; if (js.inDelaySlot) { - ERROR_LOG_REPORT(JIT, "Branch in VFPU delay slot at %08x in block starting at %08x", GetCompilerPC(), js.blockStart); + // I think we can safely just warn-log this without reporting, it's pretty clear that this type + // of branch is ignored. + WARN_LOG(JIT, "Branch in VFPU delay slot at %08x in block starting at %08x", GetCompilerPC(), js.blockStart); return; } int offset = _IMM16 << 2; From e18a023ce8c6a8d53fce2a058b7df1289c20bee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sat, 11 Nov 2017 19:41:43 +0100 Subject: [PATCH 2/3] Vulkan: Noticed a framebuffer refcount leak, and changed my mind regarding those :) Let's do it this way instead. --- ext/native/thin3d/VulkanQueueRunner.cpp | 13 ------- ext/native/thin3d/VulkanRenderManager.cpp | 42 ----------------------- ext/native/thin3d/VulkanRenderManager.h | 9 +---- ext/native/thin3d/thin3d_vulkan.cpp | 6 +++- 4 files changed, 6 insertions(+), 64 deletions(-) diff --git a/ext/native/thin3d/VulkanQueueRunner.cpp b/ext/native/thin3d/VulkanQueueRunner.cpp index bcaaf086a3..b2d94a6912 100644 --- a/ext/native/thin3d/VulkanQueueRunner.cpp +++ b/ext/native/thin3d/VulkanQueueRunner.cpp @@ -348,7 +348,6 @@ void VulkanQueueRunner::PerformRenderPass(const VKRStep &step, VkCommandBuffer c vkCmdPipelineBarrier(cmd, srcStage, dstStage, 0, 0, nullptr, 0, nullptr, 1, &barrier); iter.fb->color.layout = barrier.newLayout; } - iter.fb->Release(); } // Don't execute empty renderpasses. @@ -601,10 +600,6 @@ void VulkanQueueRunner::PerformBindFramebufferAsRenderTarget(const VKRStep &step rp_begin.clearValueCount = numClearVals; rp_begin.pClearValues = numClearVals ? clearVal : nullptr; vkCmdBeginRenderPass(cmd, &rp_begin, VK_SUBPASS_CONTENTS_INLINE); - - if (step.render.framebuffer) { - step.render.framebuffer->Release(); - } } void VulkanQueueRunner::PerformCopy(const VKRStep &step, VkCommandBuffer cmd) { @@ -678,9 +673,6 @@ void VulkanQueueRunner::PerformCopy(const VKRStep &step, VkCommandBuffer cmd) { } vkCmdCopyImage(cmd, src->depth.image, src->depth.layout, dst->depth.image, dst->depth.layout, 1, ©); } - - src->Release(); - dst->Release(); } void VulkanQueueRunner::PerformBlit(const VKRStep &step, VkCommandBuffer cmd) { @@ -764,9 +756,6 @@ void VulkanQueueRunner::PerformBlit(const VKRStep &step, VkCommandBuffer cmd) { } vkCmdBlitImage(cmd, src->depth.image, src->depth.layout, dst->depth.image, dst->depth.layout, 1, &blit, step.blit.filter); } - - src->Release(); - dst->Release(); } void VulkanQueueRunner::SetupTransitionToTransferSrc(VKRImage &img, VkImageMemoryBarrier &barrier, VkPipelineStageFlags &stage, VkImageAspectFlags aspect) { @@ -867,8 +856,6 @@ void VulkanQueueRunner::PerformReadback(const VKRStep &step, VkCommandBuffer cmd vkCmdCopyImageToBuffer(cmd, srcImage->image, srcImage->layout, readbackBuffer_, 1, ®ion); // NOTE: Can't read the buffer using the CPU here - need to sync first. - - step.readback.src->Release(); } void VulkanQueueRunner::PerformReadbackImage(const VKRStep &step, VkCommandBuffer cmd) { diff --git a/ext/native/thin3d/VulkanRenderManager.cpp b/ext/native/thin3d/VulkanRenderManager.cpp index c2fce09b6f..c29b3294e8 100644 --- a/ext/native/thin3d/VulkanRenderManager.cpp +++ b/ext/native/thin3d/VulkanRenderManager.cpp @@ -96,16 +96,6 @@ void CreateImage(VulkanContext *vulkan, VkCommandBuffer cmd, VKRImage &img, int img.format = format; } -bool VKRFramebuffer::Release() { - if (--refcount_ == 0) { - delete this; - return true; - } else if (refcount_ >= 10000 || refcount_ < 0) { - ELOG("Refcount (%d) invalid for object %p - corrupt?", refcount_.load(), this); - } - return false; -} - VulkanRenderManager::VulkanRenderManager(VulkanContext *vulkan) : vulkan_(vulkan), queueRunner_(vulkan) { VkSemaphoreCreateInfo semaphoreCreateInfo = { VK_STRUCTURE_TYPE_SEMAPHORE_CREATE_INFO }; semaphoreCreateInfo.flags = 0; @@ -406,9 +396,6 @@ void VulkanRenderManager::BindFramebufferAsRenderTarget(VKRFramebuffer *fb, VKRR step->render.numDraws = 0; step->render.finalColorLayout = !fb ? VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL : VK_IMAGE_LAYOUT_UNDEFINED; steps_.push_back(step); - if (fb) { - fb->AddRef(); - } curRenderStep_ = step; curWidth_ = fb ? fb->width : vulkan_->GetBackbufferWidth(); @@ -422,7 +409,6 @@ void VulkanRenderManager::CopyFramebufferToMemorySync(VKRFramebuffer *src, int a step->readback.srcRect.offset = { x, y }; step->readback.srcRect.extent = { (uint32_t)w, (uint32_t)h }; steps_.push_back(step); - src->AddRef(); curRenderStep_ = nullptr; @@ -599,8 +585,6 @@ void VulkanRenderManager::CopyFramebuffer(VKRFramebuffer *src, VkRect2D srcRect, step->copy.srcRect = srcRect; step->copy.dst = dst; step->copy.dstPos = dstPos; - src->AddRef(); - dst->AddRef(); std::unique_lock lock(mutex_); steps_.push_back(step); @@ -632,8 +616,6 @@ void VulkanRenderManager::BlitFramebuffer(VKRFramebuffer *src, VkRect2D srcRect, step->blit.dst = dst; step->blit.dstRect = dstRect; step->blit.filter = filter; - src->AddRef(); - dst->AddRef(); std::unique_lock lock(mutex_); steps_.push_back(step); @@ -657,7 +639,6 @@ VkImageView VulkanRenderManager::BindFramebufferAsTexture(VKRFramebuffer *fb, in } curRenderStep_->preTransitions.push_back({ fb, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL }); - fb->AddRef(); return fb->color.imageView; } @@ -684,29 +665,6 @@ void VulkanRenderManager::Finish() { void VulkanRenderManager::Wipe() { for (auto step : steps_) { - // Need to release held framebuffers. - switch (step->stepType) { - case VKRStepType::RENDER: - for (const auto &iter : step->preTransitions) { - iter.fb->Release(); - } - break; - case VKRStepType::COPY: - step->copy.src->Release(); - step->copy.dst->Release(); - break; - case VKRStepType::BLIT: - step->blit.src->Release(); - step->blit.dst->Release(); - break; - case VKRStepType::READBACK: - step->readback.src->Release(); - break; - case VKRStepType::READBACK_IMAGE: - break; - default: - assert(false); - } delete step; } steps_.clear(); diff --git a/ext/native/thin3d/VulkanRenderManager.h b/ext/native/thin3d/VulkanRenderManager.h index 5a47111226..1c17b42ac4 100644 --- a/ext/native/thin3d/VulkanRenderManager.h +++ b/ext/native/thin3d/VulkanRenderManager.h @@ -29,7 +29,6 @@ void CreateImage(VulkanContext *vulkan, VkCommandBuffer cmd, VKRImage &img, int class VKRFramebuffer { public: VKRFramebuffer(VulkanContext *vk, VkCommandBuffer initCmd, VkRenderPass renderPass, int _width, int _height) : vulkan_(vk) { - refcount_ = 1; width = _width; height = _height; @@ -50,6 +49,7 @@ public: vkCreateFramebuffer(vulkan_->GetDevice(), &fbci, nullptr, &framebuf); } + ~VKRFramebuffer() { vulkan_->Delete().QueueDeleteImage(color.image); vulkan_->Delete().QueueDeleteImage(depth.image); @@ -60,11 +60,6 @@ public: vulkan_->Delete().QueueDeleteFramebuffer(framebuf); } - void AddRef() { - refcount_++; - } - bool Release(); - int numShadows = 1; // TODO: Support this. VkFramebuffer framebuf = VK_NULL_HANDLE; @@ -73,9 +68,7 @@ public: int width = 0; int height = 0; -private: VulkanContext *vulkan_; - std::atomic refcount_; }; enum class VKRRunType { diff --git a/ext/native/thin3d/thin3d_vulkan.cpp b/ext/native/thin3d/thin3d_vulkan.cpp index 4e7597b147..8cb46c7421 100644 --- a/ext/native/thin3d/thin3d_vulkan.cpp +++ b/ext/native/thin3d/thin3d_vulkan.cpp @@ -1251,10 +1251,14 @@ public: // Inherits ownership so no AddRef. VKFramebuffer(VKRFramebuffer *fb) : buf_(fb) {} ~VKFramebuffer() { - buf_->Release(); + buf_->vulkan_->Delete().QueueCallback([](void *fb) { + VKRFramebuffer *vfb = static_cast(fb); + delete vfb; + }, buf_); } VKRFramebuffer *GetFB() const { return buf_; } private: + VulkanContext *vulkan_; // Unfortunate to have to keep this in each VKFramebuffer. VKRFramebuffer *buf_; }; From edfea7aed365fe4b8f4e820c7b037be6d1616f85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sat, 11 Nov 2017 20:22:45 +0100 Subject: [PATCH 3/3] Remove unused pointer. --- ext/native/thin3d/thin3d_vulkan.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/ext/native/thin3d/thin3d_vulkan.cpp b/ext/native/thin3d/thin3d_vulkan.cpp index 8cb46c7421..929e84581a 100644 --- a/ext/native/thin3d/thin3d_vulkan.cpp +++ b/ext/native/thin3d/thin3d_vulkan.cpp @@ -1258,7 +1258,6 @@ public: } VKRFramebuffer *GetFB() const { return buf_; } private: - VulkanContext *vulkan_; // Unfortunate to have to keep this in each VKFramebuffer. VKRFramebuffer *buf_; };