From f64117fa26d112485ec9c0f481ec49fb1bbf188a Mon Sep 17 00:00:00 2001 From: Powei Feng Date: Mon, 14 Jul 2025 12:29:55 -0700 Subject: [PATCH] vk: clean-up VulkanContext usage (#8945) - Make sure that we don't allocate a VulkanContext on the function stack since this was passed erroroneously to other components and stored as a pointer - Use const& to refer to VulkanContext across components for consistency. - Remove unused VkPhysicalDeviceProtectedMemoryProperties struct. --- .../backend/src/vulkan/VulkanCommands.cpp | 20 +++++++++---------- filament/backend/src/vulkan/VulkanCommands.h | 14 ++++++------- filament/backend/src/vulkan/VulkanDriver.cpp | 18 +++++++++-------- filament/backend/src/vulkan/VulkanDriver.h | 8 ++++---- .../src/vulkan/platform/VulkanPlatform.cpp | 15 ++++++-------- 5 files changed, 37 insertions(+), 38 deletions(-) diff --git a/filament/backend/src/vulkan/VulkanCommands.cpp b/filament/backend/src/vulkan/VulkanCommands.cpp index 5b0c7d6066..9feb8b3f5c 100644 --- a/filament/backend/src/vulkan/VulkanCommands.cpp +++ b/filament/backend/src/vulkan/VulkanCommands.cpp @@ -86,8 +86,8 @@ bool VulkanGroupMarkers::empty() const noexcept { } #endif // FVK_DEBUG_GROUP_MARKERS -VulkanCommandBuffer::VulkanCommandBuffer(VulkanContext* context, VkDevice device, VkQueue queue, - VkCommandPool pool, bool isProtected) +VulkanCommandBuffer::VulkanCommandBuffer(VulkanContext const& context, VkDevice device, + VkQueue queue, VkCommandPool pool, bool isProtected) : mContext(context), mMarkerCount(0), isProtected(isProtected), @@ -120,14 +120,14 @@ void VulkanCommandBuffer::reset() noexcept { } void VulkanCommandBuffer::pushMarker(char const* marker) noexcept { - if (mContext->isDebugUtilsSupported()) { + if (mContext.isDebugUtilsSupported()) { VkDebugUtilsLabelEXT labelInfo = { .sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT, .pLabelName = marker, .color = {0, 1, 0, 1}, }; vkCmdBeginDebugUtilsLabelEXT(mBuffer, &labelInfo); - } else if (mContext->isDebugMarkersSupported()) { + } else if (mContext.isDebugMarkersSupported()) { VkDebugMarkerMarkerInfoEXT markerInfo = { .sType = VK_STRUCTURE_TYPE_DEBUG_MARKER_MARKER_INFO_EXT, .pMarkerName = marker, @@ -140,23 +140,23 @@ void VulkanCommandBuffer::pushMarker(char const* marker) noexcept { void VulkanCommandBuffer::popMarker() noexcept{ assert_invariant(mMarkerCount > 0); - if (mContext->isDebugUtilsSupported()) { + if (mContext.isDebugUtilsSupported()) { vkCmdEndDebugUtilsLabelEXT(mBuffer); - } else if (mContext->isDebugMarkersSupported()) { + } else if (mContext.isDebugMarkersSupported()) { vkCmdDebugMarkerEndEXT(mBuffer); } mMarkerCount--; } void VulkanCommandBuffer::insertEvent(char const* marker) noexcept { - if (mContext->isDebugUtilsSupported()) { + if (mContext.isDebugUtilsSupported()) { VkDebugUtilsLabelEXT labelInfo = { .sType = VK_STRUCTURE_TYPE_DEBUG_UTILS_LABEL_EXT, .pLabelName = marker, .color = {1, 1, 0, 1}, }; vkCmdInsertDebugUtilsLabelEXT(mBuffer, &labelInfo); - } else if (mContext->isDebugMarkersSupported()) { + } else if (mContext.isDebugMarkersSupported()) { VkDebugMarkerMarkerInfoEXT markerInfo = { .sType = VK_STRUCTURE_TYPE_DEBUG_MARKER_MARKER_INFO_EXT, .pMarkerName = marker, @@ -232,7 +232,7 @@ VkSemaphore VulkanCommandBuffer::submit() { return mSubmission; } -CommandBufferPool::CommandBufferPool(VulkanContext* context, VkDevice device, VkQueue queue, +CommandBufferPool::CommandBufferPool(VulkanContext const& context, VkDevice device, VkQueue queue, uint8_t queueFamilyIndex, bool isProtected) : mDevice(device), mRecording(INVALID) { @@ -384,7 +384,7 @@ void CommandBufferPool::insertEvent(char const* marker) { #endif // FVK_DEBUG_GROUP_MARKERS VulkanCommands::VulkanCommands(VkDevice device, VkQueue queue, uint32_t queueFamilyIndex, - VkQueue protectedQueue, uint32_t protectedQueueFamilyIndex, VulkanContext* context) + VkQueue protectedQueue, uint32_t protectedQueueFamilyIndex, VulkanContext const& context) : mDevice(device), mProtectedQueue(protectedQueue), mProtectedQueueFamilyIndex(protectedQueueFamilyIndex), diff --git a/filament/backend/src/vulkan/VulkanCommands.h b/filament/backend/src/vulkan/VulkanCommands.h index 0afd4b4b46..ba1e86cc7c 100644 --- a/filament/backend/src/vulkan/VulkanCommands.h +++ b/filament/backend/src/vulkan/VulkanCommands.h @@ -23,6 +23,7 @@ #include "VulkanAsyncHandles.h" #include "VulkanConstants.h" +#include "VulkanContext.h" #include "vulkan/memory/ResourcePointer.h" #include "vulkan/utils/StaticVector.h" @@ -41,8 +42,6 @@ namespace filament::backend { using namespace fvkmemory; -struct VulkanContext; - #if FVK_ENABLED(FVK_DEBUG_GROUP_MARKERS) class VulkanGroupMarkers { public: @@ -64,7 +63,7 @@ private: // DriverApi fence object and should not be destroyed until both the DriverApi object is freed and // we're done waiting on the most recent submission of the given command buffer. struct VulkanCommandBuffer { - VulkanCommandBuffer(VulkanContext* mContext, + VulkanCommandBuffer(VulkanContext const& mContext, VkDevice device, VkQueue queue, VkCommandPool pool, bool isProtected); VulkanCommandBuffer(VulkanCommandBuffer const&) = delete; @@ -110,7 +109,7 @@ struct VulkanCommandBuffer { } private: - VulkanContext* mContext; + VulkanContext const& mContext; uint8_t mMarkerCount; bool const isProtected; VkDevice mDevice; @@ -127,7 +126,7 @@ struct CommandBufferPool { using ActiveBuffers = utils::bitset64; static constexpr int8_t INVALID = -1; - CommandBufferPool(VulkanContext* context, VkDevice device, VkQueue queue, + CommandBufferPool(VulkanContext const& context, VkDevice device, VkQueue queue, uint8_t queueFamilyIndex, bool isProtected); ~CommandBufferPool(); @@ -195,7 +194,8 @@ private: class VulkanCommands { public: VulkanCommands(VkDevice device, VkQueue queue, uint32_t queueFamilyIndex, - VkQueue protectedQueue, uint32_t protectedQueueFamilyIndex, VulkanContext* context); + VkQueue protectedQueue, uint32_t protectedQueueFamilyIndex, + VulkanContext const& context); void terminate(); @@ -246,7 +246,7 @@ private: VkQueue const mProtectedQueue; // For defered initialization if/when we need protected content uint32_t const mProtectedQueueFamilyIndex; - VulkanContext* mContext; + VulkanContext const& mContext; std::unique_ptr mPool; std::unique_ptr mProtectedPool; diff --git a/filament/backend/src/vulkan/VulkanDriver.cpp b/filament/backend/src/vulkan/VulkanDriver.cpp index 689abc0c6b..dc0eadcfc4 100644 --- a/filament/backend/src/vulkan/VulkanDriver.cpp +++ b/filament/backend/src/vulkan/VulkanDriver.cpp @@ -143,8 +143,10 @@ VKAPI_ATTR VkBool32 VKAPI_CALL debugUtilsCallback(VkDebugUtilsMessageSeverityFla using DebugUtils = VulkanDriver::DebugUtils; DebugUtils* DebugUtils::mSingleton = nullptr; -DebugUtils::DebugUtils(VkInstance instance, VkDevice device, VulkanContext const* context) - : mInstance(instance), mDevice(device), mEnabled(context->isDebugUtilsSupported()) { +DebugUtils::DebugUtils(VkInstance instance, VkDevice device, VulkanContext const& context) + : mInstance(instance), + mDevice(device), + mEnabled(context.isDebugUtilsSupported()) { #if FVK_ENABLED(FVK_DEBUG_VALIDATION) // Also initialize debug utils messenger here @@ -199,7 +201,7 @@ Dispatcher VulkanDriver::getDispatcher() const noexcept { return ConcreteDispatcher::make(); } -VulkanDriver::VulkanDriver(VulkanPlatform* platform, VulkanContext const& context, +VulkanDriver::VulkanDriver(VulkanPlatform* platform, VulkanContext& context, Platform::DriverConfig const& driverConfig) : mPlatform(platform), mResourceManager(driverConfig.handleArenaSize, driverConfig.disableHandleUseAfterFreeCheck, @@ -209,11 +211,11 @@ VulkanDriver::VulkanDriver(VulkanPlatform* platform, VulkanContext const& contex mContext(context), mCommands(mPlatform->getDevice(), mPlatform->getGraphicsQueue(), mPlatform->getGraphicsQueueFamilyIndex(), mPlatform->getProtectedGraphicsQueue(), - mPlatform->getProtectedGraphicsQueueFamilyIndex(), &mContext), + mPlatform->getProtectedGraphicsQueueFamilyIndex(), mContext), mPipelineLayoutCache(mPlatform->getDevice()), mPipelineCache(mPlatform->getDevice()), mStagePool(mAllocator, &mResourceManager, &mCommands, &mContext.getPhysicalDeviceLimits()), - mBufferCache(context, mResourceManager, mAllocator), + mBufferCache(mContext, mResourceManager, mAllocator), mFramebufferCache(mPlatform->getDevice()), mYcbcrConversionCache(mPlatform->getDevice()), mSamplerCache(mPlatform->getDevice()), @@ -229,13 +231,13 @@ VulkanDriver::VulkanDriver(VulkanPlatform* platform, VulkanContext const& contex #if FVK_ENABLED(FVK_DEBUG_DEBUG_UTILS) DebugUtils::mSingleton = - new DebugUtils(mPlatform->getInstance(), mPlatform->getDevice(), &context); + new DebugUtils(mPlatform->getInstance(), mPlatform->getDevice(), &mContext); #endif #if FVK_ENABLED(FVK_DEBUG_VALIDATION) UTILS_UNUSED const PFN_vkCreateDebugReportCallbackEXT createDebugReportCallback = vkCreateDebugReportCallbackEXT; - if (!context.isDebugUtilsSupported() && createDebugReportCallback) { + if (!mContext.isDebugUtilsSupported() && createDebugReportCallback) { VkDebugReportCallbackCreateInfoEXT const cbinfo = { .sType = VK_STRUCTURE_TYPE_DEBUG_REPORT_CALLBACK_CREATE_INFO_EXT, .flags = VK_DEBUG_REPORT_WARNING_BIT_EXT | VK_DEBUG_REPORT_ERROR_BIT_EXT, @@ -253,7 +255,7 @@ VulkanDriver::VulkanDriver(VulkanPlatform* platform, VulkanContext const& contex VulkanDriver::~VulkanDriver() noexcept = default; UTILS_NOINLINE -Driver* VulkanDriver::create(VulkanPlatform* platform, VulkanContext const& context, +Driver* VulkanDriver::create(VulkanPlatform* platform, VulkanContext& context, Platform::DriverConfig const& driverConfig) { #if 0 // this is useful for development, but too verbose even for debug builds diff --git a/filament/backend/src/vulkan/VulkanDriver.h b/filament/backend/src/vulkan/VulkanDriver.h index 3d0802968a..5ac879409d 100644 --- a/filament/backend/src/vulkan/VulkanDriver.h +++ b/filament/backend/src/vulkan/VulkanDriver.h @@ -56,7 +56,7 @@ constexpr uint8_t MAX_RENDERTARGET_ATTACHMENT_TEXTURES = class VulkanDriver final : public DriverBase { public: - static Driver* create(VulkanPlatform* platform, VulkanContext const& context, + static Driver* create(VulkanPlatform* platform, VulkanContext& context, Platform::DriverConfig const& driverConfig); #if FVK_ENABLED(FVK_DEBUG_DEBUG_UTILS) @@ -69,7 +69,7 @@ public: private: static DebugUtils* get(); - DebugUtils(VkInstance instance, VkDevice device, VulkanContext const* context); + DebugUtils(VkInstance instance, VkDevice device, VulkanContext const& context); ~DebugUtils(); VkInstance const mInstance; @@ -92,7 +92,7 @@ private: void debugCommandBegin(CommandStream* cmds, bool synchronous, const char* methodName) noexcept override; - VulkanDriver(VulkanPlatform* platform, VulkanContext const& context, + VulkanDriver(VulkanPlatform* platform, VulkanContext& context, Platform::DriverConfig const& driverConfig); ~VulkanDriver() noexcept override; @@ -134,7 +134,7 @@ private: VmaAllocator mAllocator = VK_NULL_HANDLE; VkDebugReportCallbackEXT mDebugCallback = VK_NULL_HANDLE; - VulkanContext mContext = {}; + VulkanContext& mContext; VulkanCommands mCommands; VulkanPipelineLayoutCache mPipelineLayoutCache; diff --git a/filament/backend/src/vulkan/platform/VulkanPlatform.cpp b/filament/backend/src/vulkan/platform/VulkanPlatform.cpp index 060ea421eb..43aa578817 100644 --- a/filament/backend/src/vulkan/platform/VulkanPlatform.cpp +++ b/filament/backend/src/vulkan/platform/VulkanPlatform.cpp @@ -740,7 +740,7 @@ Driver* VulkanPlatform::createDriver(void* sharedContext, mImpl->mSharedContext = true; } - VulkanContext context; + VulkanContext& context = mImpl->mContext; ExtensionSet instExts; // If using a shared context, we do not assume any extensions. if (!mImpl->mSharedContext) { @@ -787,12 +787,11 @@ Driver* VulkanPlatform::createDriver(void* sharedContext, VkPhysicalDeviceProtectedMemoryFeatures queryProtectedMemoryFeatures = { .sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_PROTECTED_MEMORY_FEATURES, }; - VkPhysicalDeviceProtectedMemoryProperties protectedMemoryProperties = { - .sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_PROTECTED_MEMORY_PROPERTIES, - }; + // Note that we're chaining a struct on the stack into a struct in context, which will then be + // passed to the driver. However, this should be ok since the use of + // queryProtectedMemoryFeatures is only in this function. chainStruct(&context.mPhysicalDeviceFeatures, &queryProtectedMemoryFeatures); chainStruct(&context.mPhysicalDeviceFeatures, &context.mPhysicalDeviceVk11Features); - chainStruct(&context.mPhysicalDeviceProperties, &protectedMemoryProperties); // Initialize the following fields: physicalDeviceProperties, memoryProperties, // physicalDeviceFeatures, graphicsQueueFamilyIndex. @@ -923,10 +922,8 @@ Driver* VulkanPlatform::createDriver(void* sharedContext, printDepthFormats(mImpl->mPhysicalDevice); #endif - // Keep a copy of context for swapchains. - mImpl->mContext = context; - - return VulkanDriver::create(this, mImpl->mContext, driverConfig); + // Note that `context` is an alias of mImpl->mContext. + return VulkanDriver::create(this, context, driverConfig); } // This needs to be explictly written for