From acdab738d23265cfc6c7c28ccb77cfa99ea0529d Mon Sep 17 00:00:00 2001 From: Samuel Pitoiset Date: Wed, 19 Jan 2022 16:15:33 +0100 Subject: [PATCH] radv: add reference counting for descriptor set layouts 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." Based on ANV. Gitlab: https://gitlab.freedesktop.org/mesa/mesa/-/issues/5893 Signed-off-by: Samuel Pitoiset Reviewed-by: Bas Nieuwenhuizen Part-of: (cherry picked from commit 66f7289d568db8711adb885acc56622e2aff252a) --- .pick_status.json | 2 +- src/amd/vulkan/radv_cmd_buffer.c | 15 ++++++++-- src/amd/vulkan/radv_descriptor_set.c | 45 ++++++++++++++++++++-------- src/amd/vulkan/radv_descriptor_set.h | 7 +++++ src/amd/vulkan/radv_private.h | 21 ++++++++++++- 5 files changed, 73 insertions(+), 17 deletions(-) diff --git a/.pick_status.json b/.pick_status.json index d0a8f305f7d..d09764c69d8 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -10066,7 +10066,7 @@ "description": "radv: add reference counting for descriptor set layouts", "nominated": false, "nomination_type": null, - "resolution": 4, + "resolution": 1, "main_sha": null, "because_sha": null }, diff --git a/src/amd/vulkan/radv_cmd_buffer.c b/src/amd/vulkan/radv_cmd_buffer.c index 85326820a73..46119a25ec8 100644 --- a/src/amd/vulkan/radv_cmd_buffer.c +++ b/src/amd/vulkan/radv_cmd_buffer.c @@ -440,8 +440,11 @@ radv_destroy_cmd_buffer(struct radv_cmd_buffer *cmd_buffer) cmd_buffer->device->ws->cs_destroy(cmd_buffer->cs); for (unsigned i = 0; i < MAX_BIND_POINTS; i++) { - free(cmd_buffer->descriptors[i].push_set.set.mapped_ptr); - vk_object_base_finish(&cmd_buffer->descriptors[i].push_set.set.base); + struct radv_descriptor_set_header *set = &cmd_buffer->descriptors[i].push_set.set; + free(set->mapped_ptr); + if (set->layout) + radv_descriptor_set_layout_unref(cmd_buffer->device, set->layout); + vk_object_base_finish(&set->base); } vk_object_base_finish(&cmd_buffer->meta_push_descriptors.base); @@ -4804,7 +4807,13 @@ radv_init_push_descriptor_set(struct radv_cmd_buffer *cmd_buffer, struct radv_de struct radv_descriptor_state *descriptors_state = radv_get_descriptors_state(cmd_buffer, bind_point); set->header.size = layout->size; - set->header.layout = layout; + + if (set->header.layout != layout) { + if (set->header.layout) + radv_descriptor_set_layout_unref(cmd_buffer->device, set->header.layout); + radv_descriptor_set_layout_ref(layout); + set->header.layout = layout; + } if (descriptors_state->push_set.capacity < set->header.size) { size_t new_size = MAX2(set->header.size, 1024); diff --git a/src/amd/vulkan/radv_descriptor_set.c b/src/amd/vulkan/radv_descriptor_set.c index 1365dde1b3d..a87dd93bd22 100644 --- a/src/amd/vulkan/radv_descriptor_set.c +++ b/src/amd/vulkan/radv_descriptor_set.c @@ -88,6 +88,15 @@ radv_mutable_descriptor_type_size_alignment(const VkMutableDescriptorTypeListVAL return true; } +void +radv_descriptor_set_layout_destroy(struct radv_device *device, + struct radv_descriptor_set_layout *set_layout) +{ + assert(set_layout->ref_cnt == 0); + vk_object_base_finish(&set_layout->base); + vk_free2(&device->vk.alloc, NULL, set_layout); +} + VKAPI_ATTR VkResult VKAPI_CALL radv_CreateDescriptorSetLayout(VkDevice _device, const VkDescriptorSetLayoutCreateInfo *pCreateInfo, const VkAllocationCallbacks *pAllocator, @@ -134,8 +143,12 @@ radv_CreateDescriptorSetLayout(VkDevice _device, const VkDescriptorSetLayoutCrea size += ycbcr_sampler_count * sizeof(struct radv_sampler_ycbcr_conversion); } + /* 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. + */ set_layout = - vk_zalloc2(&device->vk.alloc, pAllocator, size, 8, VK_SYSTEM_ALLOCATION_SCOPE_OBJECT); + vk_zalloc(&device->vk.alloc, size, 8, VK_SYSTEM_ALLOCATION_SCOPE_DEVICE); if (!set_layout) return vk_error(device, VK_ERROR_OUT_OF_HOST_MEMORY); @@ -165,11 +178,11 @@ radv_CreateDescriptorSetLayout(VkDevice _device, const VkDescriptorSetLayoutCrea VkResult result = vk_create_sorted_bindings(pCreateInfo->pBindings, pCreateInfo->bindingCount, &bindings); if (result != VK_SUCCESS) { - vk_object_base_finish(&set_layout->base); - vk_free2(&device->vk.alloc, pAllocator, set_layout); + radv_descriptor_set_layout_destroy(device, set_layout); return vk_error(device, result); } + set_layout->ref_cnt = 1; set_layout->binding_count = num_bindings; set_layout->shader_stages = 0; set_layout->dynamic_shader_stages = 0; @@ -346,8 +359,7 @@ radv_DestroyDescriptorSetLayout(VkDevice _device, VkDescriptorSetLayout _set_lay if (!set_layout) return; - vk_object_base_finish(&set_layout->base); - vk_free2(&device->vk.alloc, pAllocator, set_layout); + radv_descriptor_set_layout_unref(device, set_layout); } VKAPI_ATTR void VKAPI_CALL @@ -494,6 +506,7 @@ radv_CreatePipelineLayout(VkDevice _device, const VkPipelineLayoutCreateInfo *pC for (uint32_t set = 0; set < pCreateInfo->setLayoutCount; set++) { RADV_FROM_HANDLE(radv_descriptor_set_layout, set_layout, pCreateInfo->pSetLayouts[set]); layout->set[set].layout = set_layout; + radv_descriptor_set_layout_ref(set_layout); layout->set[set].dynamic_offset_start = dynamic_offset_count; @@ -502,11 +515,12 @@ radv_CreatePipelineLayout(VkDevice _device, const VkPipelineLayoutCreateInfo *pC dynamic_shader_stages |= set_layout->dynamic_shader_stages; } - /* Hash the entire set layout except for the vk_object_base. The - * rest of the set layout is carefully constructed to not have - * pointers so a full hash instead of a per-field hash should be ok. */ - _mesa_sha1_update(&ctx, (const char *)set_layout + sizeof(struct vk_object_base), - set_layout->layout_size - sizeof(struct vk_object_base)); + /* Hash the entire set layout except for the vk_object_base and the reference counter. The + * rest of the set layout is carefully constructed to not have pointers so a full hash instead + * of a per-field hash should be ok. */ + uint32_t hash_offset = sizeof(struct vk_object_base) + sizeof(uint32_t); + _mesa_sha1_update(&ctx, (const char *)set_layout + hash_offset, + set_layout->layout_size - hash_offset); } layout->dynamic_offset_count = dynamic_offset_count; @@ -536,14 +550,17 @@ radv_DestroyPipelineLayout(VkDevice _device, VkPipelineLayout _pipelineLayout, if (!pipeline_layout) return; + for (uint32_t i = 0; i < pipeline_layout->num_sets; i++) + radv_descriptor_set_layout_unref(device, pipeline_layout->set[i].layout); + vk_object_base_finish(&pipeline_layout->base); vk_free2(&device->vk.alloc, pAllocator, pipeline_layout); } static VkResult radv_descriptor_set_create(struct radv_device *device, struct radv_descriptor_pool *pool, - const struct radv_descriptor_set_layout *layout, - const uint32_t *variable_count, struct radv_descriptor_set **out_set) + struct radv_descriptor_set_layout *layout, const uint32_t *variable_count, + struct radv_descriptor_set **out_set) { struct radv_descriptor_set *set; uint32_t buffer_count = layout->buffer_count; @@ -662,6 +679,8 @@ radv_descriptor_set_create(struct radv_device *device, struct radv_descriptor_po } } } + + radv_descriptor_set_layout_ref(layout); *out_set = set; return VK_SUCCESS; } @@ -672,6 +691,8 @@ radv_descriptor_set_destroy(struct radv_device *device, struct radv_descriptor_p { assert(!pool->host_memory_base); + radv_descriptor_set_layout_unref(device, set->header.layout); + if (free_bo && !pool->host_memory_base) { for (int i = 0; i < pool->entry_count; ++i) { if (pool->entries[i].set == set) { diff --git a/src/amd/vulkan/radv_descriptor_set.h b/src/amd/vulkan/radv_descriptor_set.h index 298c4c62fcb..0271fb9075c 100644 --- a/src/amd/vulkan/radv_descriptor_set.h +++ b/src/amd/vulkan/radv_descriptor_set.h @@ -53,6 +53,13 @@ struct radv_descriptor_set_binding_layout { struct radv_descriptor_set_layout { struct vk_object_base base; + /* Descriptor set layouts can be destroyed at almost any time */ + uint32_t ref_cnt; + + /* Everything below is hashed and shouldn't contain any pointers. Be careful when modifying this + * structure. + */ + /* The create flags for this descriptor set layout */ VkDescriptorSetLayoutCreateFlags flags; diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h index 4daa8a20d8b..ea7725fcadf 100644 --- a/src/amd/vulkan/radv_private.h +++ b/src/amd/vulkan/radv_private.h @@ -894,7 +894,7 @@ struct radv_descriptor_range { struct radv_descriptor_set_header { struct vk_object_base base; - const struct radv_descriptor_set_layout *layout; + struct radv_descriptor_set_layout *layout; uint32_t size; uint32_t buffer_count; @@ -971,6 +971,25 @@ struct radv_descriptor_update_template { struct radv_descriptor_update_template_entry entry[0]; }; +void radv_descriptor_set_layout_destroy(struct radv_device *device, + struct radv_descriptor_set_layout *set_layout); + +static inline void +radv_descriptor_set_layout_ref(struct radv_descriptor_set_layout *set_layout) +{ + assert(set_layout && set_layout->ref_cnt >= 1); + p_atomic_inc(&set_layout->ref_cnt); +} + +static inline void +radv_descriptor_set_layout_unref(struct radv_device *device, + struct radv_descriptor_set_layout *set_layout) +{ + assert(set_layout && set_layout->ref_cnt >= 1); + if (p_atomic_dec_zero(&set_layout->ref_cnt)) + radv_descriptor_set_layout_destroy(device, set_layout); +} + struct radv_buffer { struct vk_object_base base; VkDeviceSize size;