[PATCH vkd3d v3 2/2] vkd3d: Back descriptor heaps with Vulkan descriptor sets if descriptor indexing is available.

[Conor McCarthy]

February 1, 2022 3:08 AM, "Henri Verbeet" <hverbeet at gmail.com> wrote:

>> * VKD3D_CONFIG - a list of options that change the behavior of libvkd3d.
>> + * virtual_heaps - Create descriptors for each D3D12 root signature
>> + descriptor range instead of entire descriptor heaps. Useful when push
>> + constant or bound descriptor limits are exceeded.
>> * vk_debug - enables Vulkan debug extensions.
> 
> Being able to explicitly select the older implementation is useful,
> but it seems unfortunate that it would be required for some
> applications. What would be required in order to fall back to the
> current implementation at run-time for applications that need it?

It's complicated. By the time the problem appears, some root signatures and pipeline state objects could have already been created. To fall back to the current implementation they would need to be reconfigured for it. It's worse if the problem only arises when Vulkan descriptor sets are bound. I'll see if the maximum number of descriptor sets used can be reduced to 8, which would help a bit.


>> + /* This occurs occasionally in Rise of the Tomb Raider apparently
>> + * due to a race condition (one of several). */
>> + ERR("List %p uses descriptors from more than one CBV/SRV/UAV heap.\n", list);
>> + return ~0u;
>> + }
> 
> Things like memory corruption aside, applications should generally not
> be able to trigger ERRs. Should this be a WARN instead, or are we
> missing some synchronisation somewhere?

Back when I tried using a mutex to prevent descriptor tables being updated while descriptor sets were bound, it had no effect. The problem is intermittent and isn't occurring at all at the moment, so I've been unable to check it again.


> Does this need a mutex per descriptor set? It seems like we always
> lock/bind all the sets that exist for a particular heap; would it make
> sense to have a single mutex per heap?

The only potential problem is slowing down multithreaded copying of descriptors where different threads copy different types, but I think that situation is unlikely to arise.


> The descriptor limit changes would probably make sense as a separate
> patch. Note that we need these limits for
> d3d12_command_allocator_allocate_descriptor_pool() and
> vk_binding_count_from_descriptor_range() as well, as pointed out by
> Giovanni back in October.

So use an arbitrary fixed size for the descriptor set layout in the initial patch?


> For small/trivial functions like e.g.
> d3d12_descriptor_heap_write_image(), calling them through a function
> pointer tends to not be much faster than just using a switch. I don't
> think it helps much in terms of clarity either. In terms of
> efficiency, it would probably help to write an entire range of
> descriptors in a single call, instead of writing single descriptors at
> a time.

Optimisation of descriptor copying is in a separate patch. It still uses function pointers but I'll try it without those.


>> static HRESULT d3d12_descriptor_heap_init(struct d3d12_descriptor_heap *descriptor_heap,
>> struct d3d12_device *device, const D3D12_DESCRIPTOR_HEAP_DESC *desc)
>> {
>> + static LONG serial_id;
>> HRESULT hr;
>> 
>> descriptor_heap->ID3D12DescriptorHeap_iface.lpVtbl = &d3d12_descriptor_heap_vtbl;
>> descriptor_heap->refcount = 1;
>> + descriptor_heap->serial_id = InterlockedIncrement(&serial_id);
>> 
>> descriptor_heap->desc = *desc;
>> 
>> if (FAILED(hr = vkd3d_private_store_init(&descriptor_heap->private_store)))
>> return hr;
>> 
>> + d3d12_descriptor_heap_vk_descriptor_sets_init(descriptor_heap, device, desc);
>> +
>> d3d12_device_add_ref(descriptor_heap->device = device);
>> 
>> return S_OK;
> 
> "serial_id" above can wrap, and for a 32-bit value that may not even
> take all that long.

Even if 10 descriptor heaps are created/destroyed per frame it would take 1000 hours of rendering at 120 fps to wrap. Since only two heaps can be bound per command list I figured it's unlikely to ever cause a problem. 


> I notice there are a fair amount of "use_vk_heaps" checks sprinkled
> through the code. While not the worst thing in the world, it's not
> quite ideal either. Anything we could do about that?

A few function pointers would be neater.