[PATCH vkd3d v2 4/4] vkd3d: Move atomic descriptor operations behind a config option.

[Conor McCarthy]

December 16, 2021 4:18 AM, "Henri Verbeet" <hverbeet at gmail.com> wrote:

> A couple of considerations:
> 
> - Like for some of the other patches in this series, numbers would be helpful.

Locked to 60 fps, it raised the P5 on an Nvidia Tesla T4 from ~56 to ~57, which is significant because the P5 is not easy to raise.


> - We generally try to default to the correct behaviour, and make
> trading performance for correctness optional. I.e., if we were to have
> this kind of option, it would default to being enabled.

Correct behaviour is a bit tricky here. It isn't correct for a game to write a descriptor from two threads or write it from one while reading it from another. The only reason it works is because the descriptors are identical and/or in Windows drivers the internal descriptor data structure makes them compatible somehow. But this isn't the case with vkd3d.

On the one hand, some games may not work without the config option. On the other hand, most or possibly all games except SotTR will run with unnecessary mutex operations unless a config option is used, though the performance impact will only be significant if descriptor copies per frame are in the hundreds or the thousands, probably. But the difference will be greater on a high spec GPU because the mutexes add a fixed CPU overhead not improved by a faster GPU.


> - Is the actual issue here the number of calls to
> pthread_mutex_lock(), particularly when called from
> d3d12_device_CopyDescriptors()? Or is something else going on here? In
> case of the former, we could perhaps mitigate that by doing moving
> those calls outside of the loops in d3d12_device_CopyDescriptors().

Yes, pthread_mutex_lock() is the issue. I don't see a way to do this outside the loop because the mutex changes per descriptor.


>> + void (*descriptor_vk_heap_write)(struct d3d12_desc *dst, const struct d3d12_desc *src,
>> + struct d3d12_device *device);
>> + bool atomic_desc_ops;
>> };
> 
> The "descriptor_vk_heap_write" callback appears to be unused in this patch.

Oops, I already have a new version without this. Didn't think you'd get to it this year :/