From 67e452669ed967097cce057e0b19c18434e41c3d Mon Sep 17 00:00:00 2001 From: Lionel Landwerlin Date: Sat, 5 Jul 2025 23:12:03 +0300 Subject: [PATCH] anv: do not rely on sampler objects for pipeline compilation Descriptor set layout lifetime can be shorter than what the implementation requires. One example is : * create descriptor set layout * create graphics pipeline library * destroy descriptor set layout * link optimize library in a final pipeline The last step might need the descriptor set layout information again. We've so far worked around this by taking a reference on the descriptor set layout in the pipelines. But we forgot that descriptor set layouts have pointers to samplers (for immutable & embedded samplers). We could take a reference to samplers but that sucks for various reasons : - it consumes dynamic state heap space - it could cause issues with capture-replay placement So instead we copy the information from the samplers that might be needed in cases like link optimization. This includes : - ycbcr conversion state (used for NIR lowering) - embedded sampler data (to recreate the sampler) Signed-off-by: Lionel Landwerlin Cc: mesa-stable Reviewed-by: Ivan Briano Part-of: --- src/intel/vulkan/anv_descriptor_set.c | 85 ++++++++++--------- .../vulkan/anv_nir_apply_pipeline_layout.c | 15 ++-- src/intel/vulkan/anv_pipeline.c | 10 +-- src/intel/vulkan/anv_private.h | 50 +++++++---- 4 files changed, 93 insertions(+), 67 deletions(-) diff --git a/src/intel/vulkan/anv_descriptor_set.c b/src/intel/vulkan/anv_descriptor_set.c index 4544ba394f7..37b198f206b 100644 --- a/src/intel/vulkan/anv_descriptor_set.c +++ b/src/intel/vulkan/anv_descriptor_set.c @@ -668,7 +668,7 @@ VkResult anv_CreateDescriptorSetLayout( VK_MULTIALLOC_DECL(&ma, struct anv_descriptor_set_layout, set_layout, 1); VK_MULTIALLOC_DECL(&ma, struct anv_descriptor_set_binding_layout, bindings, num_bindings); - VK_MULTIALLOC_DECL(&ma, struct anv_sampler *, samplers, + VK_MULTIALLOC_DECL(&ma, struct anv_descriptor_set_layout_sampler, samplers, immutable_sampler_count); if (!vk_object_multizalloc(&device->vk, &ma, NULL, @@ -689,7 +689,7 @@ VkResult anv_CreateDescriptorSetLayout( set_layout->binding[b].data = 0; set_layout->binding[b].max_plane_count = 0; set_layout->binding[b].array_size = 0; - set_layout->binding[b].immutable_samplers = NULL; + set_layout->binding[b].samplers = NULL; } /* Initialize all samplers to 0 */ @@ -711,7 +711,7 @@ VkResult anv_CreateDescriptorSetLayout( * immutable_samplers pointer. This provides us with a quick-and-dirty * way to sort the bindings by binding number. */ - set_layout->binding[b].immutable_samplers = (void *)(uintptr_t)(j + 1); + set_layout->binding[b].samplers = (void *)(uintptr_t)(j + 1); } const VkDescriptorSetLayoutBindingFlagsCreateInfo *binding_flags_info = @@ -722,16 +722,20 @@ VkResult anv_CreateDescriptorSetLayout( vk_find_struct_const(pCreateInfo->pNext, MUTABLE_DESCRIPTOR_TYPE_CREATE_INFO_EXT); + const bool has_embedded_samplers = + pCreateInfo->flags & + VK_DESCRIPTOR_SET_LAYOUT_CREATE_EMBEDDED_IMMUTABLE_SAMPLERS_BIT_EXT; + for (uint32_t b = 0; b < num_bindings; b++) { /* We stashed the pCreateInfo->pBindings[] index (plus one) in the * immutable_samplers pointer. Check for NULL (empty binding) and then * reset it and compute the index. */ - if (set_layout->binding[b].immutable_samplers == NULL) + if (set_layout->binding[b].samplers == NULL) continue; const uint32_t info_idx = - (uintptr_t)(void *)set_layout->binding[b].immutable_samplers - 1; - set_layout->binding[b].immutable_samplers = NULL; + (uintptr_t)(void *)set_layout->binding[b].samplers - 1; + set_layout->binding[b].samplers = NULL; const VkDescriptorSetLayoutBinding *binding = &pCreateInfo->pBindings[info_idx]; @@ -790,14 +794,27 @@ VkResult anv_CreateDescriptorSetLayout( case VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER: case VK_DESCRIPTOR_TYPE_MUTABLE_EXT: if (binding->pImmutableSamplers) { - set_layout->binding[b].immutable_samplers = samplers; + set_layout->binding[b].samplers = samplers; samplers += binding->descriptorCount; for (uint32_t i = 0; i < binding->descriptorCount; i++) { ANV_FROM_HANDLE(anv_sampler, sampler, binding->pImmutableSamplers[i]); - set_layout->binding[b].immutable_samplers[i] = sampler; + set_layout->binding[b].samplers[i] = + (struct anv_descriptor_set_layout_sampler) { + .immutable_sampler = sampler, + }; + if (has_embedded_samplers) { + set_layout->binding[b].samplers[i].embedded_key = + sampler->embedded_key; + } + if (sampler->vk.ycbcr_conversion) { + set_layout->binding[b].samplers[i].has_ycbcr_conversion = true; + set_layout->binding[b].samplers[i].ycbcr_conversion_state = + sampler->vk.ycbcr_conversion->state; + } + if (set_layout->binding[b].max_plane_count < sampler->n_planes) set_layout->binding[b].max_plane_count = sampler->n_planes; } @@ -886,8 +903,7 @@ VkResult anv_CreateDescriptorSetLayout( set_layout->descriptor_buffer_surface_size = descriptor_buffer_surface_size; set_layout->descriptor_buffer_sampler_size = descriptor_buffer_sampler_size; - if (pCreateInfo->flags & - VK_DESCRIPTOR_SET_LAYOUT_CREATE_EMBEDDED_IMMUTABLE_SAMPLERS_BIT_EXT) { + if (has_embedded_samplers) { assert(set_layout->descriptor_buffer_surface_size == 0); assert(set_layout->descriptor_buffer_sampler_size == 0); set_layout->embedded_sampler_count = sampler_count; @@ -1031,26 +1047,6 @@ anv_descriptor_set_layout_print(const struct anv_descriptor_set_layout *layout) #define SHA1_UPDATE_VALUE(ctx, x) _mesa_sha1_update(ctx, &(x), sizeof(x)); -static void -sha1_update_immutable_sampler(struct mesa_sha1 *ctx, - bool embedded_sampler, - const struct anv_sampler *sampler) -{ - /* Hash the conversion if any as this affect shader compilation due to NIR - * lowering. - */ - if (sampler->vk.ycbcr_conversion) - SHA1_UPDATE_VALUE(ctx, sampler->vk.ycbcr_conversion->state); - - /* For embedded samplers, we need to hash the sampler parameters as the - * sampler handle is baked into the shader and this ultimately is part of - * the shader hash key. We can only consider 2 shaders identical if all - * their embedded samplers parameters are identical. - */ - if (embedded_sampler) - SHA1_UPDATE_VALUE(ctx, sampler->embedded_key); -} - static void sha1_update_descriptor_set_binding_layout(struct mesa_sha1 *ctx, bool embedded_samplers, @@ -1066,10 +1062,21 @@ sha1_update_descriptor_set_binding_layout(struct mesa_sha1 *ctx, SHA1_UPDATE_VALUE(ctx, layout->descriptor_surface_offset); SHA1_UPDATE_VALUE(ctx, layout->descriptor_sampler_offset); - if (layout->immutable_samplers) { + if (layout->samplers) { for (uint16_t i = 0; i < layout->array_size; i++) { - sha1_update_immutable_sampler(ctx, embedded_samplers, - layout->immutable_samplers[i]); + /* For embedded samplers, we need to hash the sampler parameters as + * the sampler handle is baked into the shader and this ultimately is + * part of the shader hash key. We can only consider 2 shaders + * identical if all their embedded samplers parameters are identical. + */ + if (embedded_samplers) + SHA1_UPDATE_VALUE(ctx, layout->samplers[i].embedded_key); + + /* Hash the conversion if any as this affect shader compilation due + * to NIR lowering. + */ + if (layout->samplers[i].has_ycbcr_conversion) + SHA1_UPDATE_VALUE(ctx, layout->samplers[i].ycbcr_conversion_state); } } } @@ -1087,7 +1094,7 @@ sha1_update_descriptor_set_layout(struct mesa_sha1 *ctx, SHA1_UPDATE_VALUE(ctx, layout->descriptor_buffer_surface_size); SHA1_UPDATE_VALUE(ctx, layout->descriptor_buffer_sampler_size); - bool embedded_samplers = + const bool embedded_samplers = layout->flags & VK_DESCRIPTOR_SET_LAYOUT_CREATE_EMBEDDED_IMMUTABLE_SAMPLERS_BIT_EXT; for (uint16_t i = 0; i < layout->binding_count; i++) { @@ -1817,7 +1824,7 @@ anv_descriptor_set_create(struct anv_device *device, /* Go through and fill out immutable samplers if we have any */ for (uint32_t b = 0; b < layout->binding_count; b++) { - if (layout->binding[b].immutable_samplers) { + if (layout->binding[b].samplers) { for (uint32_t i = 0; i < layout->binding[b].array_size; i++) { /* The type will get changed to COMBINED_IMAGE_SAMPLER in * UpdateDescriptorSets if needed. However, if the descriptor @@ -2187,15 +2194,15 @@ anv_descriptor_set_write_image_view(struct anv_device *device, switch (type) { case VK_DESCRIPTOR_TYPE_SAMPLER: - sampler = bind_layout->immutable_samplers ? - bind_layout->immutable_samplers[element] : + sampler = bind_layout->samplers ? + bind_layout->samplers[element].immutable_sampler : anv_sampler_from_handle(info->sampler); break; case VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER: image_view = anv_image_view_from_handle(info->imageView); - sampler = bind_layout->immutable_samplers ? - bind_layout->immutable_samplers[element] : + sampler = bind_layout->samplers ? + bind_layout->samplers[element].immutable_sampler : anv_sampler_from_handle(info->sampler); break; diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c index 0c9b4161d48..52b259667ba 100644 --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c @@ -2287,7 +2287,8 @@ add_embedded_sampler_entry(struct apply_pipeline_layout_state *state, state->layout->set[set].layout; const struct anv_descriptor_set_binding_layout *bind_layout = &set_layout->binding[binding]; - const struct anv_sampler *sampler = bind_layout->immutable_samplers[0]; + const struct anv_descriptor_set_layout_sampler *sampler = + &bind_layout->samplers[0]; *sampler_bind = (struct anv_pipeline_embedded_sampler_binding) { .set = set, @@ -2488,12 +2489,16 @@ build_packed_binding_table(struct apply_pipeline_layout_state *state, } else { state->set[set].binding[b].surface_offset = map->surface_count; if (binding->dynamic_offset_index < 0) { - struct anv_sampler **samplers = binding->immutable_samplers; - uint8_t max_planes = bti_multiplier(state, set, b); + const uint8_t max_planes = bti_multiplier(state, set, b); for (unsigned i = 0; i < binding->array_size; i++) { - uint8_t planes = samplers ? samplers[i]->n_planes : 1; + const uint8_t max_sampler_planes = + (binding->samplers && + binding->samplers[i].has_ycbcr_conversion) ? + vk_format_get_plane_count( + binding->samplers[i].ycbcr_conversion_state.format) : + 1; for (uint8_t p = 0; p < max_planes; p++) { - if (p < planes) { + if (p < max_sampler_planes) { add_bti_entry(map, set, b, i, p, binding); } else { add_null_bti_entry(map); diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c index fec0b2b550c..68a10a0f02c 100644 --- a/src/intel/vulkan/anv_pipeline.c +++ b/src/intel/vulkan/anv_pipeline.c @@ -813,16 +813,16 @@ lookup_ycbcr_conversion(const void *_sets_layout, uint32_t set, const struct anv_descriptor_set_binding_layout *bind_layout = &sets_layout->set[set].layout->binding[binding]; - if (bind_layout->immutable_samplers == NULL) + if (bind_layout->samplers == NULL) return NULL; array_index = MIN2(array_index, bind_layout->array_size - 1); - const struct anv_sampler *sampler = - bind_layout->immutable_samplers[array_index]; + const struct anv_descriptor_set_layout_sampler *sampler = + &bind_layout->samplers[array_index]; - return sampler && sampler->vk.ycbcr_conversion ? - &sampler->vk.ycbcr_conversion->state : NULL; + return sampler->has_ycbcr_conversion ? + &sampler->ycbcr_conversion_state : NULL; } static void diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h index 67955ade6f4..89a948f901b 100644 --- a/src/intel/vulkan/anv_private.h +++ b/src/intel/vulkan/anv_private.h @@ -2918,6 +2918,36 @@ enum anv_descriptor_data { ANV_DESCRIPTOR_SURFACE_SAMPLER = BITFIELD_BIT(9), }; +struct anv_embedded_sampler_key { + /** No need to track binding elements for embedded samplers as : + * + * VUID-VkDescriptorSetLayoutBinding-flags-08006: + * + * "If VkDescriptorSetLayoutCreateInfo:flags contains + * VK_DESCRIPTOR_SET_LAYOUT_CREATE_EMBEDDED_IMMUTABLE_SAMPLERS_BIT_EXT, + * descriptorCount must: less than or equal to 1" + * + * The following struct can be safely hash as it doesn't include in + * address/offset. + */ + uint32_t sampler[4]; + uint32_t color[4]; +}; + +struct anv_descriptor_set_layout_sampler { + /* Immutable sampler used to populate descriptor sets on allocation */ + struct anv_sampler *immutable_sampler; + + /* Hashing key for embedded samplers */ + struct anv_embedded_sampler_key embedded_key; + + /* Whether ycbcr_conversion_state hold any data */ + bool has_ycbcr_conversion; + + /* YCbCr conversion state (only valid if has_ycbcr_conversion is true) */ + struct vk_ycbcr_conversion_state ycbcr_conversion_state; +}; + struct anv_descriptor_set_binding_layout { /* The type of the descriptors in this binding */ VkDescriptorType type; @@ -2969,8 +2999,8 @@ struct anv_descriptor_set_binding_layout { */ uint16_t descriptor_sampler_stride; - /* Immutable samplers (or NULL if no immutable samplers) */ - struct anv_sampler **immutable_samplers; + /* Sampler data (or NULL if no embedded/immutable samplers) */ + struct anv_descriptor_set_layout_sampler *samplers; }; enum anv_descriptor_set_layout_type { @@ -3360,22 +3390,6 @@ struct anv_pipeline_binding { }; }; -struct anv_embedded_sampler_key { - /** No need to track binding elements for embedded samplers as : - * - * VUID-VkDescriptorSetLayoutBinding-flags-08006: - * - * "If VkDescriptorSetLayoutCreateInfo:flags contains - * VK_DESCRIPTOR_SET_LAYOUT_CREATE_EMBEDDED_IMMUTABLE_SAMPLERS_BIT_EXT, - * descriptorCount must: less than or equal to 1" - * - * The following struct can be safely hash as it doesn't include in - * address/offset. - */ - uint32_t sampler[4]; - uint32_t color[4]; -}; - struct anv_pipeline_embedded_sampler_binding { /** The descriptor set this sampler belongs to */ uint8_t set;