Address PR comments

Move enablement check for prewarming to the platform API, and add the
ability to specify devices that are or aren't supported. Additionally,
update naming for enablement APIs, and add missing calls to terminate,
etc.

Author: anishmgoyal

filament/backend/include/backend/Platform.h

@@ -341,22 +341,23 @@ class UTILS_PUBLIC Platform {
         GpuContextPriority gpuContextPriority = GpuContextPriority::DEFAULT;
 
         /**
-         * Bypass the staging buffer because the device is of Unified Memory Architecture.
+         * Allows asynchronous pipeline cache preloading, if supported on this device.
          * This is only supported for:
          *      - VulkanPlatform
+         * When the following device extensions are available:
+         *      - VK_KHR_dynamic_rendering
+         *      - VK_EXT_vertex_input_dynamic_state
+         * Only if the selected implementation of VulkanPlatform returns true for
+         * VulkanPlatform::isPipelineCachePrewarmingDeviceSupported() for the current device.
          */
-        bool vulkanEnableStagingBufferBypass = false;
+        bool vulkanAllowAsyncPipelineCachePrewarming = false;
 
         /**
-         * Enables asynchronous pipeline cache preloading.
+         * Bypass the staging buffer because the device is of Unified Memory Architecture.
          * This is only supported for:
          *      - VulkanPlatform
-         * When the following device extensions are available:
-         *      - VK_KHR_dynamic_rendering
-         *      - VK_EXT_vertex_input_dynamic_state
-         * And is dependent on Vulkan driver implementation on the current device.
          */
-        bool vulkanEnableAsyncPipelineCachePrewarming = false;
+        bool vulkanEnableStagingBufferBypass = false;
 
         /**
          * Asynchronous mode for the engine. Defines how asynchronous operations are handled.

filament/backend/include/backend/platforms/VulkanPlatform.h

@@ -263,6 +263,30 @@ class VulkanPlatform : public Platform, utils::PrivateImplementation<VulkanPlatf
      */
     virtual ExtensionSet getRequiredInstanceExtensions() { return {}; }
 
+    /**
+     * Determines if pipeline cache prewarming is supported by the current device. Should be
+     * implemented by derived classes, as by default, this will simply return false.
+     *
+     * @return true if pipeline cache prewarming is safe to be attempted on this device, false
+     *         if not.
+     */
+    virtual bool isPipelineCachePrewarmingDeviceSupported() const noexcept;
+
+    /**
+     * This determines, regardless of whether or not pipeline cache prewarming
+     * is supported by a specific device, if async pipeline cache prewarming should
+     * be enabled in the current application. This depends on:
+     * - if it has been marked as supported for the current device
+     * - if it is allowed in the driver config
+     * - if parallel shader compilation is NOT disabled in the driver config
+     * - if dynamic rendering is supported by the current device
+     * - if vertex input dynamic state is supported by the current device
+     *
+     * @return true if pipeline cache prewarming has been enabled (supported + allowed) on
+     *         this device AND in this application, false if not.
+     */
+    bool isAsyncPipelineCachePrewarmingEnabled() const noexcept;
+
     /**
      * Destroy the swapchain.
      * @param handle    The handle returned by createSwapChain()

filament/backend/src/vulkan/VulkanContext.h

@@ -158,16 +158,20 @@ struct VulkanContext {
         return mVertexInputDynamicStateSupported;
     }
 
-    inline bool asyncPipelineCachePrewarmingEnabled() const noexcept {
-        return mAsyncPipelineCachePrewarmingEnabled;
+    inline bool pipelineCreationFeedbackSupported() const noexcept {
+        return mPipelineCreationFeedbackSupported;
     }
 
-    inline bool stagingBufferBypassEnabled() const noexcept {
-        return mStagingBufferBypassEnabled;
+    inline bool asyncPipelineCachePrewarmingAllowed() const noexcept {
+        return mAsyncPipelineCachePrewarmingAllowed;
     }
 
-    inline bool pipelineCreationFeedbackSupported() const noexcept {
-        return mPipelineCreationFeedbackSupported;
+    inline bool parallelShaderCompilationDisabled() const noexcept {
+        return mParallelShaderCompileDisabled;
+    }
+
+    inline bool stagingBufferBypassEnabled() const noexcept {
+        return mStagingBufferBypassEnabled;
     }
 
 private:
@@ -191,10 +195,6 @@ struct VulkanContext {
 
     VkExternalFenceHandleTypeFlags mFenceExportFlags = {};
 
-    // These are options that can be enabled or disabled at an application level.
-    bool mAsyncPipelineCachePrewarmingEnabled = false;
-    bool mStagingBufferBypassEnabled = false;
-
     // These are options that are either supported or not supported in the current
     // device and instance.
     bool mDebugMarkersSupported = false;
@@ -206,6 +206,11 @@ struct VulkanContext {
     bool mProtectedMemorySupported = false;
     bool mVertexInputDynamicStateSupported = false;
 
+    // These are options that can be enabled or disabled at an application level.
+    bool mAsyncPipelineCachePrewarmingAllowed = false;
+    bool mParallelShaderCompileDisabled = false;
+    bool mStagingBufferBypassEnabled = false;
+
     fvkutils::VkFormatList mDepthStencilFormats;
     fvkutils::VkFormatList mBlittableDepthStencilFormats;
 

filament/backend/src/vulkan/VulkanDriver.cpp

@@ -1500,7 +1500,7 @@ bool VulkanDriver::isStereoSupported() {
 }
 
 bool VulkanDriver::isParallelShaderCompileSupported() {
-    return VulkanPipelineCache::isAsyncPrewarmingSupported(mContext);
+    return mPlatform->isAsyncPipelineCachePrewarmingEnabled();
 }
 
 bool VulkanDriver::isDepthStencilResolveSupported() {

filament/backend/src/vulkan/VulkanPipelineCache.cpp

@@ -69,16 +69,17 @@ void printPipelineFeedbackInfo(VkPipelineCreationFeedbackCreateInfo const& feedb
 
 } // namespace
 
-VulkanPipelineCache::VulkanPipelineCache(DriverBase& driver, VkDevice device, VulkanContext const& context)
+VulkanPipelineCache::VulkanPipelineCache(DriverBase& driver, VkDevice device, VulkanContext const& context, bool isAsyncPrewarmingEnabled)
         : mDevice(device),
           mCallbackManager(driver),
-          mContext(context) {
+          mContext(context),
+          mIsAsyncPrewarmingEnabled(isAsyncPrewarmingEnabled) {
     VkPipelineCacheCreateInfo createInfo = {
         .sType = VK_STRUCTURE_TYPE_PIPELINE_CACHE_CREATE_INFO,
     };
     bluevk::vkCreatePipelineCache(mDevice, &createInfo, VKALLOC, &mPipelineCache);
 
-    if (isAsyncPrewarmingSupported(context)) {
+    if (mIsAsyncPrewarmingEnabled) {
         mCompilerThreadPool.init(
             /*threadCount=*/1,
             []() {
@@ -107,6 +108,7 @@ VulkanPipelineCache::PipelineCacheEntry* VulkanPipelineCache::getOrCreatePipelin
         .handle = createPipeline(mPipelineRequirements),
         .lastUsed = mCurrentTime,
     };
+    assert_invariant(cacheEntry.handle != nullptr && "Pipeline handle is nullptr");
     return &mPipelines.emplace(mPipelineRequirements, cacheEntry).first.value();
 }
 
@@ -361,6 +363,9 @@ void VulkanPipelineCache::terminate() noexcept {
     mPipelines.clear();
     resetBoundPipeline();
 
+    mCallbackManager.terminate();
+    mCompilerThreadPool.terminate();
+
     vkDestroyPipelineCache(mDevice, mPipelineCache, VKALLOC);
 }
 

filament/backend/src/vulkan/VulkanPipelineCache.h

@@ -48,12 +48,6 @@ class VulkanPipelineCache {
     VulkanPipelineCache(VulkanPipelineCache const&) = delete;
     VulkanPipelineCache& operator=(VulkanPipelineCache const&) = delete;
 
-    static inline bool isAsyncPrewarmingSupported(VulkanContext const& context) {
-        return context.asyncPipelineCachePrewarmingEnabled() &&
-               context.isDynamicRenderingSupported() &&
-               context.isVertexInputDynamicStateSupported();
-    }
-
     static constexpr uint32_t SHADER_MODULE_COUNT = 2;
     static constexpr uint32_t VERTEX_ATTRIBUTE_COUNT = MAX_VERTEX_ATTRIBUTE_COUNT;
 
@@ -94,7 +88,18 @@ class VulkanPipelineCache {
 
     static_assert(sizeof(RasterState) == 16, "RasterState must not have implicit padding.");
 
-    VulkanPipelineCache(DriverBase& driver, VkDevice device, VulkanContext const& context);
+    /**
+     * Creates a new instance of a pipeline cache for graphics pipelines.
+     *
+     * @param driver The driver this is being instantiated for. This is used only for construction of
+     *               the callback manager, which references the driver for scheduling callbacks.
+     * @param device The device that the pipelines will be created and run on.
+     * @param context Information about the current instance of Vulkan, such as supported extensions,
+     *                and enabled features.
+     * @param isAsyncPrewarmingEnabled true if async cache prewarming is enabled (in which case a
+     *                                 threadpool for such jobs will be spawned), false if not.
+     */
+    VulkanPipelineCache(DriverBase& driver, VkDevice device, VulkanContext const& context, bool isAsyncPrewarmingEnabled = false);
 
     // Loads a fake pipeline into memory on a separate thread, with the intent of
     // preloading the Vulkan cache with enough information to have a cache hit when
@@ -235,6 +240,9 @@ class VulkanPipelineCache {
     CallbackManager mCallbackManager;
 
     [[maybe_unused]] VulkanContext const& mContext;
+
+    // Keep track of whether or not cache prewarming is enabled.
+    bool mIsAsyncPrewarmingEnabled = false;
 };
 
 } // namespace filament::backend

filament/backend/src/vulkan/platform/VulkanPlatform.cpp

@@ -227,8 +227,9 @@ ExtensionSet getDeviceExtensions(VkPhysicalDevice device) {
 #if defined(__APPLE__)
         VK_KHR_PORTABILITY_SUBSET_EXTENSION_NAME,
 #endif
+        VK_EXT_VERTEX_INPUT_DYNAMIC_STATE_EXTENSION_NAME,
         VK_KHR_MULTIVIEW_EXTENSION_NAME,
-
+        VK_KHR_DYNAMIC_RENDERING_EXTENSION_NAME,
 #if FVK_ENABLED(FVK_DEBUG_SHADER_MODULE)
         VK_EXT_PIPELINE_CREATION_FEEDBACK_EXTENSION_NAME,
 #endif
@@ -743,6 +744,19 @@ void VulkanPlatform::destroySync(Platform::Sync* sync) noexcept {
     delete sync;
 }
 
+bool VulkanPlatform::isPipelineCachePrewarmingDeviceSupported() const noexcept {
+    // By default, this should not be enabled, as it will not work on most devices.
+    return false;
+}
+
+bool VulkanPlatform::isAsyncPipelineCachePrewarmingEnabled() const noexcept {
+    return isPipelineCachePrewarmingDeviceSupported() &&
+           mImpl->mContext.asyncPipelineCachePrewarmingAllowed() &&
+           !mImpl->mContext.parallelShaderCompilationDisabled() &&
+           mImpl->mContext.isDynamicRenderingSupported() &&
+           mImpl->mContext.isVertexInputDynamicStateSupported();
+}
+
 VkInstance VulkanPlatform::getInstance() const noexcept {
     return mImpl->mInstance;
 }
@@ -936,7 +950,8 @@ void VulkanPlatform::queryAndSetDeviceFeatures(Platform::DriverConfig const& dri
     }
 
     // Pass along relevant driver config (feature flags)
-    context.mAsyncPipelineCachePrewarmingEnabled = driverConfig.vulkanEnableAsyncPipelineCachePrewarming;
+    context.mAsyncPipelineCachePrewarmingAllowed = driverConfig.vulkanAllowAsyncPipelineCachePrewarming;
+    context.mParallelShaderCompileDisabled = driverConfig.disableParallelShaderCompile;
     context.mStagingBufferBypassEnabled = driverConfig.vulkanEnableStagingBufferBypass;
 
     // We know we need to allocate the protected version of the VK objects

filament/src/details/Engine.cpp

@@ -115,6 +115,8 @@ backend::Platform::DriverConfig getDriverConfig(FEngine* instance) {
         .metalDisablePanicOnDrawableFailure =
                 instance->getConfig().metalDisablePanicOnDrawableFailure,
         .gpuContextPriority = instance->getConfig().gpuContextPriority,
+        .vulkanAllowAsyncPipelineCachePrewarming =
+                instance->features.backend.vulkan.allow_pipeline_cache_prewarming,
         .vulkanEnableStagingBufferBypass =
                 instance->features.backend.vulkan.enable_staging_buffer_bypass,
         .asynchronousMode = instance->features.backend.enable_asynchronous_operation ?

filament/src/details/Engine.h

@@ -783,6 +783,10 @@ class FEngine : public Engine {
                 bool assert_native_window_is_valid = false;
             } opengl;
             struct {
+                // In certain GPU drivers, graphics pipelines are cached based on a subset of their
+                // parameters. In those cases, we can create fake pipelines ahead of time to ensure
+                // a cache hit when creating graphics pipelines at draw time, eliminating hitching.
+                bool allow_pipeline_cache_prewarming = false;
                 // On Unified Memory Architecture device, it is possible to bypass using the staging
                 // buffer. This is an experimental feature that still needs to be implemented fully
                 // before it can be fully enabled.