[PATCH vkd3d 5/7] vkd3d: Create and write descriptor sets for root signature unbounded ranges.

[Henri Verbeet]

On Fri, 13 Aug 2021 at 16:57, Conor McCarthy <cmccarthy at codeweavers.com> wrote:
> @@ -2581,9 +2594,44 @@ static void d3d12_command_list_prepare_descriptors(struct d3d12_command_list *li
>       *   by an update command, or freed) between when the command is recorded
>       *   and when the command completes executing on the queue."
>       */
> -    bindings->descriptor_sets[0] = d3d12_command_allocator_allocate_descriptor_set(list->allocator,
> -            root_signature->vk_set_layouts[root_signature->main_set]);
> -    bindings->descriptor_set_count = 1;
> +    bindings->descriptor_set_count = 0;
> +    for (i = root_signature->main_set; i < root_signature->vk_set_count; ++i)
> +    {
> +        unsigned int unbounded_range_offset = root_signature->vk_set_layout_unbounded_offsets[i];
> +        unsigned int unbounded_table = root_signature->vk_set_layout_table_indices[i];
> +        unsigned int variable_binding_size = 0;
> +
> +        if (unbounded_range_offset != UINT_MAX)
> +        {
> +            const struct d3d12_desc *base_descriptor
> +                    = d3d12_desc_from_gpu_handle(bindings->descriptor_tables[unbounded_table]);
> +            /* Descriptors may not be set, eg. WoW. */
> +            if (base_descriptor)
> +            {
> +                unsigned int heap_size;
> +                int rc;
> +
> +                rc = pthread_mutex_lock(&device->mutex);
> +                if (rc)
> +                    ERR("Failed to lock mutex, error %d.\n", rc);
> +                heap_size = d3d12_device_descriptor_heap_size_from_descriptor(device, base_descriptor);
> +                if (!rc)
> +                    pthread_mutex_unlock(&device->mutex);
> +
Would it make sense to do the locking for
d3d12_device_descriptor_heap_size_from_descriptor() inside that
function? Doing it outside the function is perhaps a little more
flexible, but it doesn't seem to gain us much here.

> @@ -2687,7 +2749,31 @@ static void d3d12_command_list_update_descriptor_table(struct d3d12_command_list
>
>          descriptor = base_descriptor + range->offset;
>
> -        for (j = 0; j < range->descriptor_count; ++j, ++descriptor)
> +        descriptor_count = range->descriptor_count;
> +        if (descriptor_count == UINT_MAX)
> +        {
> +            int rc;
> +
> +            /* The first unbounded range of each type is written until the heap end is reached. Do not repeat. */
> +            if (i && range[-1].descriptor_magic == range->descriptor_magic && range[-1].descriptor_count == UINT_MAX)
> +                continue;
> +
I get that "range[-1]" is convenient, but I don't like it much. There
are a few more instances of this in this patch.

> +void d3d12_device_track_descriptor_heap(struct d3d12_device *device,
> +        const struct d3d12_descriptor_heap *heap)
> +{
> +    if (!device->vk_info.EXT_descriptor_indexing)
> +        return;
> +
> +    if (!vkd3d_array_reserve((void **)&device->descriptor_heaps, &device->descriptor_heap_capacity,
> +            device->descriptor_heap_count + 1, sizeof(*device->descriptor_heaps)))
> +    {
> +        ERR("Out of memory. Cannot track descriptor heap for unbounded arrays.\n");
> +        return;
> +    }
> +
> +    device->descriptor_heaps[device->descriptor_heap_count++] = heap;
> +    /* Do not increment the heap reference count. This reference is deleted on heap destruction. */
> +}
> +
It seems like it would make sense to group the "descriptor_heaps",
"descriptor_heap_capacity", and "descriptor_heap_count" fields
together in their own structure, similar to how that's done for e.g.
struct vkd3d_render_pass_cache.

> +void d3d12_device_untrack_descriptor_heap(struct d3d12_device *device,
> +        const struct d3d12_descriptor_heap *heap)
> +{
> +    size_t i;
> +
> +    if (!device->vk_info.EXT_descriptor_indexing)
> +        return;
> +
> +    for (i = 0; i < device->descriptor_heap_count; ++i)
> +    {
> +        if (device->descriptor_heaps[i] != heap)
> +            continue;
> +
> +        memmove(&device->descriptor_heaps[i], &device->descriptor_heaps[i + 1],
> +                (device->descriptor_heap_count - i - 1) * sizeof(*device->descriptor_heaps));
> +        --device->descriptor_heap_count;
> +
> +        return;
> +    }
> +
> +    ERR("Attempted to untrack an already untracked heap.\n");
> +}
> +
If we don't need to maintain the order in which heaps were inserted,
we don't really need to use memmove() here; we'd only need to move the
last element to the index of the element that's to be removed.

> +/* Return the available size from the specified descriptor to the heap end. */
> +uint32_t d3d12_device_descriptor_heap_size_from_descriptor(struct d3d12_device *device,
> +        const struct d3d12_desc *desc)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < device->descriptor_heap_count; ++i)
> +    {
> +        unsigned int size;
> +        size_t offset;
> +
> +        if (device->descriptor_heaps[i]->descriptors > (const BYTE*)desc)
> +            continue;
> +
> +        size = d3d12_device_get_descriptor_handle_increment_size(device, device->descriptor_heaps[i]->desc.Type);
> +        offset = ((const BYTE*)desc - device->descriptor_heaps[i]->descriptors) / size;
> +        if (device->descriptor_heaps[i]->desc.NumDescriptors <= offset)
> +            continue;
> +
> +        return device->descriptor_heaps[i]->desc.NumDescriptors - (uint32_t)offset;
> +    }
> +
> +    ERR("Failed to find descriptor heap size from descriptor pointer.\n");
> +    return 0;
> +}
> +
Presumably we could just skip checking heaps that don't have the
correct type, and then "size" would always be "sizeof(*desc)".

> @@ -3547,6 +3555,12 @@ HRESULT d3d12_descriptor_heap_create(struct d3d12_device *device,
>
>      memset(object->descriptors, 0, descriptor_size * desc->NumDescriptors);
>
> +    if ((rc = pthread_mutex_lock(&device->mutex)))
> +        ERR("Failed to lock mutex, error %d.\n", rc);
> +    d3d12_device_track_descriptor_heap(device, object);
> +    if (!rc)
> +        pthread_mutex_unlock(&device->mutex);
> +
Do we need to track RTV and DSV heaps?

> @@ -332,14 +334,33 @@ static HRESULT d3d12_root_signature_info_count_descriptors(struct d3d12_root_sig
>          const D3D12_DESCRIPTOR_RANGE *range = &table->pDescriptorRanges[i];
>          unsigned int binding_count;
>
> -        if (range->NumDescriptors == 0xffffffff)
> -        {
> -            FIXME("Unhandled unbound descriptor range.\n");
> -            return E_NOTIMPL;
> +        if (unbounded)
> +         {
Some stray whitespace above.

> +            if (range->NumDescriptors != UINT_MAX)
> +            {
> +                ERR("Static range occurs after unbounded range.\n");
> +                return E_INVALIDARG;
> +            }
> +            if (range->OffsetInDescriptorsFromTableStart == D3D12_DESCRIPTOR_RANGE_OFFSET_APPEND)
> +            {
> +                ERR("Unbounded range with offset D3D12_DESCRIPTOR_RANGE_OFFSET_APPEND occurs after "
> +                        "another unbounded range.\n");
> +                return E_INVALIDARG;
> +            }
>          }
>
>          binding_count = use_array ? 1 : range->NumDescriptors;
>
> +        if (range->NumDescriptors == UINT_MAX)
> +        {
> +            if (!use_array)
> +            {
> +                FIXME("The device does not support unbounded descriptor ranges.\n");
> +                return E_NOTIMPL;
> +            }
> +            unbounded = true;
> +        }
> +
The validation changes above could perhaps be in a separate patch.

> @@ -692,6 +694,8 @@ struct d3d12_root_signature
>      VkPipelineLayout vk_pipeline_layout;
>      uint32_t vk_set_count;
>      VkDescriptorSetLayout vk_set_layouts[VKD3D_MAX_DESCRIPTOR_SETS];
> +    unsigned int vk_set_layout_unbounded_offsets[VKD3D_MAX_DESCRIPTOR_SETS];
> +    unsigned int vk_set_layout_table_indices[VKD3D_MAX_DESCRIPTOR_SETS];
>      bool use_descriptor_arrays;
>
Like the descriptor heap tracking fields mentioned earlier, it seems
like it would make sense to group the "vk_set_layouts",
"vk_set_layout_unbounded_offsets", and "vk_set_layout_table_indices"
together in their own structure.