<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jan 25, 2018 at 4:24 AM, Iago Toral Quiroga <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The spec states that descriptor set layouts can be destroyed almost at any time: "VkDescriptorSetLayout objects may be accessed by commands that operate on descriptor sets allocated using that layout, and those descriptor sets must not be updated with vkUpdateDescriptorSets after the descriptor set layout has been destroyed. Otherwise, descriptor set layouts can be destroyed any time they are not in use by an API command." Fixes the following work-in-progress CTS tests: dEQP-VK.api.descriptor_set.descriptor_set_layout_lifetime.graphics dEQP-VK.api.descriptor_set.descriptor_set_layout_lifetime.compute Suggested-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> --- src/intel/vulkan/anv_cmd_buffer.c | 6 ++---- src/intel/vulkan/anv_descriptor_set.c | 17 ++++++++++++++--- src/intel/vulkan/anv_private.h | 26 ++++++++++++++++++++++++-- 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/intel/vulkan/anv_cmd_buffer.c b/src/intel/vulkan/anv_cmd_buffer.c index bf80061c6d4..521cf6b6a54 100644 --- a/src/intel/vulkan/anv_cmd_buffer.c +++ b/src/intel/vulkan/anv_cmd_buffer.c @@ -913,8 +913,7 @@ void anv_CmdPushDescriptorSetKHR( assert(_set < MAX_SETS); - const struct anv_descriptor_set_layout *set_layout = - layout->set[_set].layout; + struct anv_descriptor_set_layout *set_layout = layout->set[_set].layout; struct anv_push_descriptor_set *push_set = anv_cmd_buffer_get_push_descriptor_set(cmd_buffer, @@ -1006,8 +1005,7 @@ void anv_CmdPushDescriptorSetWithTemplateKHR( assert(_set < MAX_PUSH_DESCRIPTORS); - const struct anv_descriptor_set_layout *set_layout = - layout->set[_set].layout; + struct anv_descriptor_set_layout *set_layout = layout->set[_set].layout; struct anv_push_descriptor_set *push_set = anv_cmd_buffer_get_push_descriptor_set(cmd_buffer, diff --git a/src/intel/vulkan/anv_descriptor_set.c b/src/intel/vulkan/anv_descriptor_set.c index 1d4df264ae6..99122aed229 100644 --- a/src/intel/vulkan/anv_descriptor_set.c +++ b/src/intel/vulkan/anv_descriptor_set.c @@ -67,6 +67,8 @@ VkResult anv_CreateDescriptorSetLayout( return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY); memset(set_layout, 0, sizeof(*set_layout)); + set_layout->ref_cnt = 1; + set_layout->allocator = pAllocator; </blockquote><div> </div><div>There's a sticky bit here around allocators. Because we're reference counting, there's no guarantee that it will get freed when they call vkDestroyDescriptorSetLayout. This means that VK_SYSTEM_ALLOCATION_SCOPE_OBJECT is not appropriate. Instead, we should be using VK_SYSTEM_ALLOCATION_SCOPE_DEVICE and allocating it off the device allocator ignoring pAllocator. That will probably cause a CTS warning (not an error) in the allocation tests but I think it's the right thing to do.</div><div> </div><div>Other than that, looks good! </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> set_layout->binding_count = max_binding + 1; for (uint32_t b = 0; b <= max_binding; b++) { @@ -204,7 +206,8 @@ void anv_DestroyDescriptorSetLayout( if (!set_layout) return; - vk_free2(&device->alloc, pAllocator, set_layout); + assert(pAllocator == set_layout->allocator); + anv_descriptor_set_layout_unref(device, set_layout); } static void @@ -246,6 +249,7 @@ VkResult anv_CreatePipelineLayout( ANV_FROM_HANDLE(anv_descriptor_set_layout, set_layout, pCreateInfo->pSetLayouts[set]); layout->set[set].layout = set_layout; + anv_descriptor_set_layout_ref(set_layout); layout->set[set].dynamic_offset_start = dynamic_offset_count; for (uint32_t b = 0; b < set_layout->binding_count; b++) { @@ -290,6 +294,9 @@ void anv_DestroyPipelineLayout( if (!pipeline_layout) return; + for (uint32_t i = 0; i < pipeline_layout->num_sets; i++) + anv_descriptor_set_layout_unref(device, pipeline_layout->set[i].layout); + vk_free2(&device->alloc, pAllocator, pipeline_layout); } @@ -423,7 +430,7 @@ struct surface_state_free_list_entry { VkResult anv_descriptor_set_create(struct anv_device *device, struct anv_descriptor_pool *pool, - const struct anv_descriptor_set_layout *layout, + struct anv_descriptor_set_layout *layout, struct anv_descriptor_set **out_set) { struct anv_descriptor_set *set; @@ -455,8 +462,10 @@ anv_descriptor_set_create(struct anv_device *device, } } - set->size = size; set->layout = layout; + anv_descriptor_set_layout_ref(layout); + + set->size = size; set->buffer_views = (struct anv_buffer_view *) &set->descriptors[layout->size]; set->buffer_count = layout->buffer_count; @@ -512,6 +521,8 @@ anv_descriptor_set_destroy(struct anv_device *device, struct anv_descriptor_pool *pool, struct anv_descriptor_set *set) { + anv_descriptor_set_layout_unref(device, set->layout); + /* Put the buffer view surface state back on the free list. */ for (uint32_t b = 0; b < set->buffer_count; b++) { struct surface_state_free_list_entry *entry = diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h index b351c6f63b3..d6cf7cff249 100644 --- a/src/intel/vulkan/anv_private.h +++ b/src/intel/vulkan/anv_private.h @@ -1199,6 +1199,12 @@ struct anv_descriptor_set_binding_layout { }; struct anv_descriptor_set_layout { + /* Descriptor set layouts can be destroyed at almost any time */ + uint32_t ref_cnt; + + /* Host allocation callback to use when freeing the layout */ + const VkAllocationCallbacks *allocator; + /* Number of bindings in this descriptor set */ uint16_t binding_count; @@ -1218,6 +1224,22 @@ struct anv_descriptor_set_layout { struct anv_descriptor_set_binding_layout binding[0]; }; +static inline void +anv_descriptor_set_layout_ref(struct anv_descriptor_set_layout *layout) +{ + assert(layout && layout->ref_cnt >= 1); + p_atomic_inc(&layout->ref_cnt); +} + +static inline void +anv_descriptor_set_layout_unref(struct anv_device *device, + struct anv_descriptor_set_layout *layout) +{ + assert(layout && layout->ref_cnt >= 1); + if (p_atomic_dec_zero(&layout->ref_cnt)) + vk_free2(&device->alloc, layout->allocator, layout); +} + struct anv_descriptor { VkDescriptorType type; @@ -1239,7 +1261,7 @@ struct anv_descriptor { }; struct anv_descriptor_set { - const struct anv_descriptor_set_layout *layout; + struct anv_descriptor_set_layout *layout; uint32_t size; uint32_t buffer_count; struct anv_buffer_view *buffer_views; @@ -1363,7 +1385,7 @@ anv_descriptor_set_write_template(struct anv_descriptor_set *set, VkResult anv_descriptor_set_create(struct anv_device *device, struct anv_descriptor_pool *pool, - const struct anv_descriptor_set_layout *layout, + struct anv_descriptor_set_layout *layout, struct anv_descriptor_set **out_set); void -- 2.14.1 </blockquote></div> </div></div>