Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
Thanks for fixing this!
On Fri, Jan 26, 2018 at 1:26 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."
>
> v2: allocate off the device allocator with DEVICE scope. This is
> expected to warnings in some CTS tests (Jason)
>
> 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>
> ---
>
> As expected, this patch adds the following warnings in CTS:
>
> Test case 'dEQP-VK.api.object_management.alloc_callback_
> fail.descriptor_set_layout_empty'..
> QualityWarning (Allocation callbacks not called)
>
> Test case 'dEQP-VK.api.object_management.alloc_callback_
> fail.descriptor_set_layout_single'..
> QualityWarning (Allocation callbacks not called)
>
> src/intel/vulkan/anv_cmd_buffer.c | 6 ++----
> src/intel/vulkan/anv_descriptor_set.c | 23 ++++++++++++++++++-----
> src/intel/vulkan/anv_private.h | 23 +++++++++++++++++++++--
> 3 files changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_cmd_buffer.c b/src/intel/vulkan/anv_cmd_
> buffer.c
> index bf80061c6d..521cf6b6a5 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 1d4df264ae..edb829601e 100644
> --- a/src/intel/vulkan/anv_descriptor_set.c
> +++ b/src/intel/vulkan/anv_descriptor_set.c
> @@ -57,16 +57,21 @@ VkResult anv_CreateDescriptorSetLayout(
> struct anv_descriptor_set_binding_layout *bindings;
> struct anv_sampler **samplers;
>
> + /* We need to allocate decriptor set layouts off the device allocator
> + * with DEVICE scope because they are reference counted and may not be
> + * destroyed when vkDestroyDescriptorSetLayout is called.
> + */
> ANV_MULTIALLOC(ma);
> anv_multialloc_add(&ma, &set_layout, 1);
> anv_multialloc_add(&ma, &bindings, max_binding + 1);
> anv_multialloc_add(&ma, &samplers, immutable_sampler_count);
>
> - if (!anv_multialloc_alloc2(&ma, &device->alloc, pAllocator,
> - VK_SYSTEM_ALLOCATION_SCOPE_OBJECT))
> + if (!anv_multialloc_alloc(&ma, &device->alloc,
> + VK_SYSTEM_ALLOCATION_SCOPE_DEVICE))
> return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY);
>
> memset(set_layout, 0, sizeof(*set_layout));
> + set_layout->ref_cnt = 1;
> set_layout->binding_count = max_binding + 1;
>
> for (uint32_t b = 0; b <= max_binding; b++) {
> @@ -204,7 +209,7 @@ void anv_DestroyDescriptorSetLayout(
> if (!set_layout)
> return;
>
> - vk_free2(&device->alloc, pAllocator, set_layout);
> + anv_descriptor_set_layout_unref(device, set_layout);
> }
>
> static void
> @@ -246,6 +251,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 +296,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 +432,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 +464,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 +523,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 b351c6f63b..701a49823e 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -1199,6 +1199,9 @@ 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;
> +
> /* Number of bindings in this descriptor set */
> uint16_t binding_count;
>
> @@ -1218,6 +1221,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_free(&device->alloc, layout);
> +}
> +
> struct anv_descriptor {
> VkDescriptorType type;
>
> @@ -1239,7 +1258,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 +1382,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
