Fix #138236: Vulkan: destroyed pipeline layouts could be reused

Due to an oversight in the pipeline pool/shader, pipelines that were
invalid (as subresources where destroyed) could still be selected for
reused. This happened more often when using the compositor as it
recreates shaders a lot. It also depended on how handles are reuses by
driver implementation.

This is fixed by discarded any associated pipeline when the pipeline
layout is discarded.

Pull Request: https://projects.blender.org/blender/blender/pulls/138800

source/blender/gpu/vulkan/vk_pipeline_pool.cc

@@ -646,32 +646,23 @@ VkPipeline VKPipelinePool::get_or_create_graphics_pipeline(VKGraphicsInfo &graph
   return pipeline;
 }
 
-void VKPipelinePool::remove(Span<VkShaderModule> vk_shader_modules)
+void VKPipelinePool::discard(VKDiscardPool &discard_pool, VkPipelineLayout vk_pipeline_layout)
 {
   std::scoped_lock lock(mutex_);
-  Vector<VkPipeline> pipelines_to_destroy;
   compute_pipelines_.remove_if([&](auto item) {
-    if (vk_shader_modules.contains(item.key.vk_shader_module)) {
-      pipelines_to_destroy.append(item.value);
+    if (item.key.vk_pipeline_layout == vk_pipeline_layout) {
+      discard_pool.discard_pipeline(item.value);
       return true;
     }
     return false;
   });
   graphic_pipelines_.remove_if([&](auto item) {
-    if (vk_shader_modules.contains(item.key.pre_rasterization.vk_vertex_module) ||
-        vk_shader_modules.contains(item.key.pre_rasterization.vk_geometry_module) ||
-        vk_shader_modules.contains(item.key.fragment_shader.vk_fragment_module))
-    {
-      pipelines_to_destroy.append(item.value);
+    if (item.key.vk_pipeline_layout == vk_pipeline_layout) {
+      discard_pool.discard_pipeline(item.value);
       return true;
     }
     return false;
   });
-
-  VKDevice &device = VKBackend::get().device;
-  for (VkPipeline vk_pipeline : pipelines_to_destroy) {
-    vkDestroyPipeline(device.vk_handle(), vk_pipeline, nullptr);
-  }
 }
 
 void VKPipelinePool::free_data()

source/blender/gpu/vulkan/vk_pipeline_pool.hh

@@ -19,8 +19,9 @@
 
 #include "vk_common.hh"
 
-namespace blender {
-namespace gpu {
+namespace blender::gpu {
+class VKDevice;
+class VKDiscardPool;
 
 /**
  * Struct containing key information to identify a compute pipeline.
@@ -216,8 +217,6 @@ struct VKGraphicsInfo {
   }
 };
 
-class VKDevice;
-
 /**
  * Pipelines are lazy initialized and same pipelines should share their handle.
  *
@@ -317,9 +316,9 @@ class VKPipelinePool : public NonCopyable {
                                              VkPipeline vk_pipeline_base);
 
   /**
-   * Remove all shader pipelines that uses the given shader_module.
+   * Discard all pipelines that uses the given pipeline_layout.
    */
-  void remove(Span<VkShaderModule> vk_shader_modules);
+  void discard(VKDiscardPool &discard_pool, VkPipelineLayout vk_pipeline_layout);
 
   /**
    * Destroy all created pipelines.
@@ -367,6 +366,4 @@ class VKPipelinePool : public NonCopyable {
   void specialization_info_reset();
 };
 
-}  // namespace gpu
-
-}  // namespace blender
+}  // namespace blender::gpu

source/blender/gpu/vulkan/vk_resource_pool.cc

@@ -40,6 +40,7 @@ void VKDiscardPool::move_data(VKDiscardPool &src_pool, TimelineValue timeline)
   src_pool.image_views_.update_timeline(timeline);
   src_pool.images_.update_timeline(timeline);
   src_pool.shader_modules_.update_timeline(timeline);
+  src_pool.pipelines_.update_timeline(timeline);
   src_pool.pipeline_layouts_.update_timeline(timeline);
   src_pool.framebuffers_.update_timeline(timeline);
   src_pool.render_passes_.update_timeline(timeline);
@@ -49,6 +50,7 @@ void VKDiscardPool::move_data(VKDiscardPool &src_pool, TimelineValue timeline)
   image_views_.extend(std::move(src_pool.image_views_));
   images_.extend(std::move(src_pool.images_));
   shader_modules_.extend(std::move(src_pool.shader_modules_));
+  pipelines_.extend(std::move(src_pool.pipelines_));
   pipeline_layouts_.extend(std::move(src_pool.pipeline_layouts_));
   framebuffers_.extend(std::move(src_pool.framebuffers_));
   render_passes_.extend(std::move(src_pool.render_passes_));
@@ -84,6 +86,11 @@ void VKDiscardPool::discard_shader_module(VkShaderModule vk_shader_module)
   std::scoped_lock mutex(mutex_);
   shader_modules_.append_timeline(timeline_, vk_shader_module);
 }
+void VKDiscardPool::discard_pipeline(VkPipeline vk_pipeline)
+{
+  std::scoped_lock mutex(mutex_);
+  pipelines_.append_timeline(timeline_, vk_pipeline);
+}
 void VKDiscardPool::discard_pipeline_layout(VkPipelineLayout vk_pipeline_layout)
 {
   std::scoped_lock mutex(mutex_);
@@ -131,6 +138,10 @@ void VKDiscardPool::destroy_discarded_resources(VKDevice &device, bool force)
         device.mem_allocator_get(), buffer_allocation.first, buffer_allocation.second);
   });
 
+  pipelines_.remove_old(current_timeline, [&](VkPipeline vk_pipeline) {
+    vkDestroyPipeline(device.vk_handle(), vk_pipeline, nullptr);
+  });
+
   pipeline_layouts_.remove_old(current_timeline, [&](VkPipelineLayout vk_pipeline_layout) {
     vkDestroyPipelineLayout(device.vk_handle(), vk_pipeline_layout, nullptr);
   });

source/blender/gpu/vulkan/vk_resource_pool.hh

@@ -83,6 +83,7 @@ class VKDiscardPool {
   TimelineResources<VkImageView> image_views_;
   TimelineResources<VkBufferView> buffer_views_;
   TimelineResources<VkShaderModule> shader_modules_;
+  TimelineResources<VkPipeline> pipelines_;
   TimelineResources<VkPipelineLayout> pipeline_layouts_;
   TimelineResources<VkRenderPass> render_passes_;
   TimelineResources<VkFramebuffer> framebuffers_;
@@ -100,6 +101,7 @@ class VKDiscardPool {
   void discard_buffer(VkBuffer vk_buffer, VmaAllocation vma_allocation);
   void discard_buffer_view(VkBufferView vk_buffer_view);
   void discard_shader_module(VkShaderModule vk_shader_module);
+  void discard_pipeline(VkPipeline vk_pipeline);
   void discard_pipeline_layout(VkPipelineLayout vk_pipeline_layout);
   void discard_framebuffer(VkFramebuffer vk_framebuffer);
   void discard_render_pass(VkRenderPass vk_render_pass);

source/blender/gpu/vulkan/vk_shader.cc

@@ -519,9 +519,11 @@ void VKShader::init(const shader::ShaderCreateInfo &info, bool is_batch_compilat
 
 VKShader::~VKShader()
 {
+  VKDevice &device = VKBackend::get().device;
   VKDiscardPool &discard_pool = VKDiscardPool::discard_pool_get();
 
   if (vk_pipeline_layout != VK_NULL_HANDLE) {
+    device.pipelines.discard(discard_pool, vk_pipeline_layout);
     discard_pool.discard_pipeline_layout(vk_pipeline_layout);
     vk_pipeline_layout = VK_NULL_HANDLE;
   }