On Fri, 13 Aug 2021 at 16:57, Conor McCarthy <cmccarthy(a)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.