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