From db0291c235592a7eecceb788ef622e3a630f98aa Mon Sep 17 00:00:00 2001 From: Connor Abbott Date: Thu, 22 Feb 2024 13:58:11 +0000 Subject: [PATCH] tu: Follow pipeline compatibility rules for dynamic descriptors When we bind a descriptor set with dynamic descriptors, we can't ignore dynamic descriptors in previously-bound higher descriptor sets. For example, assume we have descriptor sets A and B, each of which has one dynamic storage buffer, and we do: CmdBindDescriptorSets(firstSet=1, descriptorSetCount=1, A) CmdBindDescriptorSets(firstSet=0, descriptorSetCount=1, B) and in the first CmdBindDescriptorSets the pipeline layout includes a descriptor set layout compatible with B in set 0. Then, following "Pipeline Layout Compatibility," set 0 is disturbed: When binding a descriptor set to set number N, a previously bound descriptor set bound with lower index M than N is disturbed if the pipeline layouts for set M and N are not compatible for set M. Otherwise, the bound descriptor set in M is not disturbed When it's disturbed, it's effectively turned into a set with 1 undefined dynamic storage buffer: When a descriptor set is disturbed by binding descriptor sets, the disturbed set is considered to contain undefined descriptors bound with the same pipeline layout as the disturbing descriptor set. This disturbed set is compatible with B, so in the second CmdBindDescriptorSets this clause doesn't apply: If, additionally, the previously bound descriptor set for set N was bound using a pipeline layout not compatible for set N, then all bindings in sets numbered greater than N are disturbed. and A remains valid to access. The code before 88db7364 worked only if the pipeline layout when binding B contained a descriptor layout compatible with A in set 1, because it used the pipeline layout's total size when allocating the internal dynamic descriptors array, but that isn't actually a requirement, so the previous code was already broken. After 88db7364 we only allocate as much space as required by the current descriptors being bound, because I misread the rules here, which made it more broken and broke 3DMark Wildlife Extreme that does something like this. In order to properly fix this we need to keep track of the maximum ever seen dynamic descriptor size, similar to what we already do for descriptor sets, and use that. We have no idea what needs to be preserved when binding a descriptor set with dynamic descriptors, so we have to be conservative. Fixes: 88db7364 ("tu: Rework dynamic offset handling") Part-of: --- src/freedreno/vulkan/tu_cmd_buffer.cc | 22 +++++++++++++--------- src/freedreno/vulkan/tu_cmd_buffer.h | 2 +- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/freedreno/vulkan/tu_cmd_buffer.cc b/src/freedreno/vulkan/tu_cmd_buffer.cc index b61837c1484..0386f7ee54a 100644 --- a/src/freedreno/vulkan/tu_cmd_buffer.cc +++ b/src/freedreno/vulkan/tu_cmd_buffer.cc @@ -2151,7 +2151,7 @@ tu_reset_cmd_buffer(struct vk_command_buffer *vk_cmd_buffer, memset(&cmd_buffer->descriptors[i].push_set, 0, sizeof(cmd_buffer->descriptors[i].push_set)); cmd_buffer->descriptors[i].push_set.base.type = VK_OBJECT_TYPE_DESCRIPTOR_SET; cmd_buffer->descriptors[i].max_sets_bound = 0; - cmd_buffer->descriptors[i].dynamic_bound = 0; + cmd_buffer->descriptors[i].max_dynamic_offset_size = 0; } u_trace_fini(&cmd_buffer->trace); @@ -2441,12 +2441,12 @@ tu6_emit_descriptor_sets(struct tu_cmd_buffer *cmd, cmd->state.desc_sets = tu_cs_draw_state(&cmd->sub_cs, &state_cs, 4 + 4 * descriptors_state->max_sets_bound + - (descriptors_state->dynamic_bound ? 6 : 0)); + (descriptors_state->max_dynamic_offset_size ? 6 : 0)); } else { cmd->state.desc_sets = tu_cs_draw_state(&cmd->sub_cs, &state_cs, 3 + 2 * descriptors_state->max_sets_bound + - (descriptors_state->dynamic_bound ? 3 : 0)); + (descriptors_state->max_dynamic_offset_size ? 3 : 0)); } cs = &state_cs; } else { @@ -2466,7 +2466,7 @@ tu6_emit_descriptor_sets(struct tu_cmd_buffer *cmd, } /* Dynamic descriptors get the reserved descriptor set. */ - if (descriptors_state->dynamic_bound) { + if (descriptors_state->max_dynamic_offset_size) { int reserved_set_idx = cmd->device->physical_device->reserved_set_idx; assert(reserved_set_idx >= 0); /* reserved set must be bound */ @@ -2617,22 +2617,26 @@ tu_CmdBindDescriptorSets(VkCommandBuffer commandBuffer, assert(dyn_idx == dynamicOffsetCount); if (dynamic_offset_offset) { + descriptors_state->max_dynamic_offset_size = + MAX2(descriptors_state->max_dynamic_offset_size, dynamic_offset_offset); + /* allocate and fill out dynamic descriptor set */ struct tu_cs_memory dynamic_desc_set; int reserved_set_idx = cmd->device->physical_device->reserved_set_idx; - VkResult result = tu_cs_alloc(&cmd->sub_cs, - dynamic_offset_offset / (4 * A6XX_TEX_CONST_DWORDS), - A6XX_TEX_CONST_DWORDS, &dynamic_desc_set); + VkResult result = + tu_cs_alloc(&cmd->sub_cs, + descriptors_state->max_dynamic_offset_size / + (4 * A6XX_TEX_CONST_DWORDS), + A6XX_TEX_CONST_DWORDS, &dynamic_desc_set); if (result != VK_SUCCESS) { vk_command_buffer_set_error(&cmd->vk, result); return; } memcpy(dynamic_desc_set.map, descriptors_state->dynamic_descriptors, - dynamic_offset_offset); + descriptors_state->max_dynamic_offset_size); assert(reserved_set_idx >= 0); /* reserved set must be bound */ descriptors_state->set_iova[reserved_set_idx] = dynamic_desc_set.iova | BINDLESS_DESCRIPTOR_64B; - descriptors_state->dynamic_bound = true; } tu_dirty_desc_sets(cmd, pipelineBindPoint); diff --git a/src/freedreno/vulkan/tu_cmd_buffer.h b/src/freedreno/vulkan/tu_cmd_buffer.h index 2e19777acbd..182b6037a6e 100644 --- a/src/freedreno/vulkan/tu_cmd_buffer.h +++ b/src/freedreno/vulkan/tu_cmd_buffer.h @@ -54,7 +54,7 @@ struct tu_descriptor_state uint32_t dynamic_descriptors[MAX_DYNAMIC_BUFFERS_SIZE]; uint64_t set_iova[MAX_SETS]; uint32_t max_sets_bound; - bool dynamic_bound; + uint32_t max_dynamic_offset_size; }; enum tu_cmd_dirty_bits