On Thu, Jan 25, 2018 at 4:24 AM, Iago Toral Quiroga <ito...@igalia.com>
wrote:
> 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 <ja...@jlekstrand.net>
> ---
> 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;
>
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.
Other than that, looks good!
> 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
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
