Descriptor read/write race conditions in SotTR were a result of deficiencies in the fence implementation.
Signed-off-by: Conor McCarthy cmccarthy@codeweavers.com --- libs/vkd3d/device.c | 50 ++++++++------------ libs/vkd3d/resource.c | 97 ++++++++++++-------------------------- libs/vkd3d/vkd3d_private.h | 17 +------ 3 files changed, 51 insertions(+), 113 deletions(-)
diff --git a/libs/vkd3d/device.c b/libs/vkd3d/device.c index bdc80ba3..e2533598 100644 --- a/libs/vkd3d/device.c +++ b/libs/vkd3d/device.c @@ -2615,7 +2615,6 @@ static ULONG STDMETHODCALLTYPE d3d12_device_Release(ID3D12Device *iface) { struct d3d12_device *device = impl_from_ID3D12Device(iface); ULONG refcount = InterlockedDecrement(&device->refcount); - size_t i;
TRACE("%p decreasing refcount to %u.\n", device, refcount);
@@ -2635,8 +2634,6 @@ static ULONG STDMETHODCALLTYPE d3d12_device_Release(ID3D12Device *iface) vkd3d_render_pass_cache_cleanup(&device->render_pass_cache, device); d3d12_device_destroy_pipeline_cache(device); d3d12_device_destroy_vkd3d_queues(device); - for (i = 0; i < ARRAY_SIZE(device->desc_mutex); ++i) - vkd3d_mutex_destroy(&device->desc_mutex[i]); VK_CALL(vkDestroyDevice(device->vk_device, NULL)); if (device->parent) IUnknown_Release(device->parent); @@ -3457,42 +3454,45 @@ static HRESULT STDMETHODCALLTYPE d3d12_device_CreateRootSignature(ID3D12Device * static void STDMETHODCALLTYPE d3d12_device_CreateConstantBufferView(ID3D12Device *iface, const D3D12_CONSTANT_BUFFER_VIEW_DESC *desc, D3D12_CPU_DESCRIPTOR_HANDLE descriptor) { + struct d3d12_desc *dst = d3d12_desc_from_cpu_handle(descriptor); struct d3d12_device *device = impl_from_ID3D12Device(iface); - struct d3d12_desc tmp = {0};
TRACE("iface %p, desc %p, descriptor %#lx.\n", iface, desc, descriptor.ptr);
- d3d12_desc_create_cbv(&tmp, device, desc); - d3d12_desc_write_atomic(d3d12_desc_from_cpu_handle(descriptor), &tmp, device); + d3d12_desc_create_cbv(dst, device, desc); + if (device->use_vk_heaps && dst->magic) + d3d12_desc_write_vk_heap(dst, device); }
static void STDMETHODCALLTYPE d3d12_device_CreateShaderResourceView(ID3D12Device *iface, ID3D12Resource *resource, const D3D12_SHADER_RESOURCE_VIEW_DESC *desc, D3D12_CPU_DESCRIPTOR_HANDLE descriptor) { + struct d3d12_desc *dst = d3d12_desc_from_cpu_handle(descriptor); struct d3d12_device *device = impl_from_ID3D12Device(iface); - struct d3d12_desc tmp = {0};
TRACE("iface %p, resource %p, desc %p, descriptor %#lx.\n", iface, resource, desc, descriptor.ptr);
- d3d12_desc_create_srv(&tmp, device, unsafe_impl_from_ID3D12Resource(resource), desc); - d3d12_desc_write_atomic(d3d12_desc_from_cpu_handle(descriptor), &tmp, device); + d3d12_desc_create_srv(dst, device, unsafe_impl_from_ID3D12Resource(resource), desc); + if (device->use_vk_heaps && dst->magic) + d3d12_desc_write_vk_heap(dst, device); }
static void STDMETHODCALLTYPE d3d12_device_CreateUnorderedAccessView(ID3D12Device *iface, ID3D12Resource *resource, ID3D12Resource *counter_resource, const D3D12_UNORDERED_ACCESS_VIEW_DESC *desc, D3D12_CPU_DESCRIPTOR_HANDLE descriptor) { + struct d3d12_desc *dst = d3d12_desc_from_cpu_handle(descriptor); struct d3d12_device *device = impl_from_ID3D12Device(iface); - struct d3d12_desc tmp = {0};
TRACE("iface %p, resource %p, counter_resource %p, desc %p, descriptor %#lx.\n", iface, resource, counter_resource, desc, descriptor.ptr);
- d3d12_desc_create_uav(&tmp, device, unsafe_impl_from_ID3D12Resource(resource), + d3d12_desc_create_uav(dst, device, unsafe_impl_from_ID3D12Resource(resource), unsafe_impl_from_ID3D12Resource(counter_resource), desc); - d3d12_desc_write_atomic(d3d12_desc_from_cpu_handle(descriptor), &tmp, device); + if (device->use_vk_heaps && dst->magic) + d3d12_desc_write_vk_heap(dst, device); }
static void STDMETHODCALLTYPE d3d12_device_CreateRenderTargetView(ID3D12Device *iface, @@ -3520,13 +3520,14 @@ static void STDMETHODCALLTYPE d3d12_device_CreateDepthStencilView(ID3D12Device * static void STDMETHODCALLTYPE d3d12_device_CreateSampler(ID3D12Device *iface, const D3D12_SAMPLER_DESC *desc, D3D12_CPU_DESCRIPTOR_HANDLE descriptor) { + struct d3d12_desc *sampler = d3d12_desc_from_cpu_handle(descriptor); struct d3d12_device *device = impl_from_ID3D12Device(iface); - struct d3d12_desc tmp = {0};
TRACE("iface %p, desc %p, descriptor %#lx.\n", iface, desc, descriptor.ptr);
- d3d12_desc_create_sampler(&tmp, device, desc); - d3d12_desc_write_atomic(d3d12_desc_from_cpu_handle(descriptor), &tmp, device); + d3d12_desc_create_sampler(sampler, device, desc); + if (device->use_vk_heaps && sampler->magic) + d3d12_desc_write_vk_heap(sampler, device); }
static void flush_desc_writes(struct d3d12_desc_copy_location locations[][VKD3D_DESCRIPTOR_WRITE_BUFFER_SIZE], @@ -3543,23 +3544,16 @@ static void flush_desc_writes(struct d3d12_desc_copy_location locations[][VKD3D_ } }
-static void d3d12_desc_buffered_copy_atomic(struct d3d12_desc *dst, const struct d3d12_desc *src, +static void d3d12_desc_buffered_copy(struct d3d12_desc *dst, const struct d3d12_desc *src, struct d3d12_desc_copy_location locations[][VKD3D_DESCRIPTOR_WRITE_BUFFER_SIZE], struct d3d12_desc_copy_info *infos, struct d3d12_descriptor_heap *descriptor_heap, struct d3d12_device *device) { struct d3d12_desc_copy_location *location; enum vkd3d_vk_descriptor_set_index set; - struct vkd3d_mutex *mutex; - - mutex = d3d12_device_get_descriptor_mutex(device, src); - vkd3d_mutex_lock(mutex);
if (src->magic == VKD3D_DESCRIPTOR_MAGIC_FREE) { - /* Source must be unlocked first, and therefore can't be used as a null source. */ - static const struct d3d12_desc null = {0}; - vkd3d_mutex_unlock(mutex); - d3d12_desc_write_atomic(dst, &null, device); + d3d12_desc_destroy(dst, device); return; }
@@ -3571,8 +3565,6 @@ static void d3d12_desc_buffered_copy_atomic(struct d3d12_desc *dst, const struct if (location->src.magic & VKD3D_DESCRIPTOR_MAGIC_HAS_VIEW) vkd3d_view_incref(location->src.u.view_info.view);
- vkd3d_mutex_unlock(mutex); - infos[set].uav_counter |= (location->src.magic == VKD3D_DESCRIPTOR_MAGIC_UAV) && !!location->src.u.view_info.view->vk_counter_view; location->dst = dst; @@ -3637,7 +3629,7 @@ static void d3d12_device_vk_heaps_copy_descriptors(struct d3d12_device *device, if (dst[dst_idx].magic == src[src_idx].magic && (dst[dst_idx].magic & VKD3D_DESCRIPTOR_MAGIC_HAS_VIEW) && dst[dst_idx].u.view_info.written_serial_id == src[src_idx].u.view_info.view->serial_id) continue; - d3d12_desc_buffered_copy_atomic(&dst[dst_idx], &src[src_idx], locations, infos, descriptor_heap, device); + d3d12_desc_buffered_copy(&dst[dst_idx], &src[src_idx], locations, infos, descriptor_heap, device); }
if (dst_idx >= dst_range_size) @@ -4244,7 +4236,6 @@ static HRESULT d3d12_device_init(struct d3d12_device *device, const struct vkd3d_vk_device_procs *vk_procs; VkResult vr; HRESULT hr; - size_t i;
device->ID3D12Device_iface.lpVtbl = &d3d12_device_vtbl; device->refcount = 1; @@ -4296,9 +4287,6 @@ static HRESULT d3d12_device_init(struct d3d12_device *device,
device->blocked_queue_count = 0;
- for (i = 0; i < ARRAY_SIZE(device->desc_mutex); ++i) - vkd3d_mutex_init(&device->desc_mutex[i]); - vkd3d_init_descriptor_pool_sizes(device->vk_pool_sizes, &device->vk_info.descriptor_limits);
if ((device->parent = create_info->parent)) diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c index 68c28cd1..df633afd 100644 --- a/libs/vkd3d/resource.c +++ b/libs/vkd3d/resource.c @@ -2098,9 +2098,13 @@ void vkd3d_view_incref(struct vkd3d_view *view) InterlockedIncrement(&view->refcount); }
-static void vkd3d_view_destroy(struct vkd3d_view *view, struct d3d12_device *device) +static void vkd3d_view_decref_descriptor(struct vkd3d_view *view, struct d3d12_device *device) { const struct vkd3d_vk_device_procs *vk_procs = &device->vk_procs; + ULONG refcount = InterlockedDecrement(&view->refcount); + + if (refcount) + return;
TRACE("Destroying view %p.\n", view);
@@ -2127,8 +2131,7 @@ static void vkd3d_view_destroy(struct vkd3d_view *view, struct d3d12_device *dev
void vkd3d_view_decref(struct vkd3d_view *view, struct d3d12_device *device) { - if (!InterlockedDecrement(&view->refcount)) - vkd3d_view_destroy(view, device); + vkd3d_view_decref_descriptor(view, device); }
/* TODO: write null descriptors to all applicable sets (invalid behaviour workaround). */ @@ -2220,24 +2223,21 @@ static void d3d12_desc_write_vk_heap_null_descriptor(struct d3d12_descriptor_hea } }
-/* dst and src contain the same data unless another thread overwrites dst. The array index is - * calculated from dst, and src is thread safe. */ -static void d3d12_desc_write_vk_heap(const struct d3d12_desc *dst, const struct d3d12_desc *src, - struct d3d12_device *device) +void d3d12_desc_write_vk_heap(const struct d3d12_desc *src, struct d3d12_device *device) { struct d3d12_descriptor_heap_vk_set *descriptor_set; struct d3d12_descriptor_heap *descriptor_heap; const struct vkd3d_vk_device_procs *vk_procs; bool is_null = false;
- descriptor_heap = vkd3d_gpu_descriptor_allocator_heap_from_descriptor(&device->gpu_descriptor_allocator, dst); + descriptor_heap = vkd3d_gpu_descriptor_allocator_heap_from_descriptor(&device->gpu_descriptor_allocator, src); descriptor_set = &descriptor_heap->vk_descriptor_sets[vkd3d_vk_descriptor_set_index_from_vk_descriptor_type( src->vk_descriptor_type)]; vk_procs = &device->vk_procs;
vkd3d_mutex_lock(&descriptor_heap->vk_sets_mutex);
- descriptor_set->vk_descriptor_writes[0].dstArrayElement = dst + descriptor_set->vk_descriptor_writes[0].dstArrayElement = src - (const struct d3d12_desc *)descriptor_heap->descriptors; descriptor_set->vk_descriptor_writes[0].descriptorCount = 1; switch (src->vk_descriptor_type) @@ -2275,7 +2275,7 @@ static void d3d12_desc_write_vk_heap(const struct d3d12_desc *dst, const struct if (src->magic == VKD3D_DESCRIPTOR_MAGIC_UAV && src->u.view_info.view->vk_counter_view) { descriptor_set = &descriptor_heap->vk_descriptor_sets[VKD3D_SET_INDEX_UAV_COUNTER]; - descriptor_set->vk_descriptor_writes[0].dstArrayElement = dst + descriptor_set->vk_descriptor_writes[0].dstArrayElement = src - (const struct d3d12_desc *)descriptor_heap->descriptors; descriptor_set->vk_descriptor_writes[0].descriptorCount = 1; descriptor_set->vk_descriptor_writes[0].pTexelBufferView = &src->u.view_info.view->vk_counter_view; @@ -2285,60 +2285,22 @@ static void d3d12_desc_write_vk_heap(const struct d3d12_desc *dst, const struct vkd3d_mutex_unlock(&descriptor_heap->vk_sets_mutex); }
-static void d3d12_desc_write_atomic_d3d12_only(struct d3d12_desc *dst, const struct d3d12_desc *src, struct d3d12_device *device) -{ - struct vkd3d_view *defunct_view; - struct vkd3d_mutex *mutex; - - mutex = d3d12_device_get_descriptor_mutex(device, dst); - vkd3d_mutex_lock(mutex); - - if (!(dst->magic & VKD3D_DESCRIPTOR_MAGIC_HAS_VIEW) || InterlockedDecrement(&dst->u.view_info.view->refcount)) - { - *dst = *src; - vkd3d_mutex_unlock(mutex); - return; - } - - defunct_view = dst->u.view_info.view; - *dst = *src; - vkd3d_mutex_unlock(mutex); - - /* Destroy the view after unlocking to reduce wait time. */ - vkd3d_view_destroy(defunct_view, device); -} - -void d3d12_desc_write_atomic(struct d3d12_desc *dst, const struct d3d12_desc *src, +static void d3d12_desc_copy_d3d12_only(struct d3d12_desc *dst, const struct d3d12_desc *src, struct d3d12_device *device) { - struct vkd3d_view *defunct_view = NULL; - struct vkd3d_mutex *mutex; - - mutex = d3d12_device_get_descriptor_mutex(device, dst); - vkd3d_mutex_lock(mutex); + assert(dst != src);
- /* Nothing to do for VKD3D_DESCRIPTOR_MAGIC_CBV. */ - if ((dst->magic & VKD3D_DESCRIPTOR_MAGIC_HAS_VIEW) - && !InterlockedDecrement(&dst->u.view_info.view->refcount)) - defunct_view = dst->u.view_info.view; + d3d12_desc_destroy(dst, device);
*dst = *src; - - vkd3d_mutex_unlock(mutex); - - /* Destroy the view after unlocking to reduce wait time. */ - if (defunct_view) - vkd3d_view_destroy(defunct_view, device); - - if (device->use_vk_heaps && dst->magic) - d3d12_desc_write_vk_heap(dst, src, device); }
-static void d3d12_desc_destroy(struct d3d12_desc *descriptor, struct d3d12_device *device) +void d3d12_desc_destroy(struct d3d12_desc *descriptor, struct d3d12_device *device) { - static const struct d3d12_desc null_desc = {0}; + if (descriptor->magic & VKD3D_DESCRIPTOR_MAGIC_HAS_VIEW) + vkd3d_view_decref_descriptor(descriptor->u.view_info.view, device);
- d3d12_desc_write_atomic(descriptor, &null_desc, device); + memset(descriptor, 0, sizeof(*descriptor)); }
void d3d12_desc_copy_vk_heap_range(struct d3d12_desc_copy_location *locations, const struct d3d12_desc_copy_info *info, @@ -2353,7 +2315,7 @@ void d3d12_desc_copy_vk_heap_range(struct d3d12_desc_copy_location *locations, c
for (i = 0, write_count = 0; i < info->count; ++i) { - d3d12_desc_write_atomic_d3d12_only(locations[i].dst, &locations[i].src, device); + d3d12_desc_copy_d3d12_only(locations[i].dst, &locations[i].src, device);
if (i && locations[i].dst == locations[i - 1].dst + 1) { @@ -2394,24 +2356,17 @@ done: void d3d12_desc_copy(struct d3d12_desc *dst, const struct d3d12_desc *src, struct d3d12_device *device) { - struct d3d12_desc tmp; - struct vkd3d_mutex *mutex; - assert(dst != src);
- /* Shadow of the Tomb Raider and possibly other titles sometimes destroy - * and rewrite a descriptor in another thread while it is being copied. */ - mutex = d3d12_device_get_descriptor_mutex(device, src); - vkd3d_mutex_lock(mutex); + d3d12_desc_destroy(dst, device);
if (src->magic & VKD3D_DESCRIPTOR_MAGIC_HAS_VIEW) vkd3d_view_incref(src->u.view_info.view);
- tmp = *src; - - vkd3d_mutex_unlock(mutex); + *dst = *src;
- d3d12_desc_write_atomic(dst, &tmp, device); + if (device->use_vk_heaps && dst->magic) + d3d12_desc_write_vk_heap(dst, device); }
static VkDeviceSize vkd3d_get_required_texel_buffer_alignment(const struct d3d12_device *device, @@ -2813,6 +2768,8 @@ void d3d12_desc_create_cbv(struct d3d12_desc *descriptor, struct VkDescriptorBufferInfo *buffer_info; struct d3d12_resource *resource;
+ d3d12_desc_destroy(descriptor, device); + if (!desc) { WARN("Constant buffer desc is NULL.\n"); @@ -2988,6 +2945,8 @@ void d3d12_desc_create_srv(struct d3d12_desc *descriptor, struct vkd3d_texture_view_desc vkd3d_desc; struct vkd3d_view *view;
+ d3d12_desc_destroy(descriptor, device); + if (!resource) { vkd3d_create_null_srv(descriptor, device, desc); @@ -3286,6 +3245,8 @@ void d3d12_desc_create_uav(struct d3d12_desc *descriptor, struct d3d12_device *d struct d3d12_resource *resource, struct d3d12_resource *counter_resource, const D3D12_UNORDERED_ACCESS_VIEW_DESC *desc) { + d3d12_desc_destroy(descriptor, device); + if (!resource) { if (counter_resource) @@ -3416,6 +3377,8 @@ void d3d12_desc_create_sampler(struct d3d12_desc *sampler, { struct vkd3d_view *view;
+ d3d12_desc_destroy(sampler, device); + if (!desc) { WARN("NULL sampler desc.\n"); diff --git a/libs/vkd3d/vkd3d_private.h b/libs/vkd3d/vkd3d_private.h index 37bac159..e2ab8eac 100644 --- a/libs/vkd3d/vkd3d_private.h +++ b/libs/vkd3d/vkd3d_private.h @@ -746,7 +746,8 @@ void d3d12_desc_create_uav(struct d3d12_desc *descriptor, struct d3d12_device *d struct d3d12_resource *resource, struct d3d12_resource *counter_resource, const D3D12_UNORDERED_ACCESS_VIEW_DESC *desc); void d3d12_desc_create_sampler(struct d3d12_desc *sampler, struct d3d12_device *device, const D3D12_SAMPLER_DESC *desc); -void d3d12_desc_write_atomic(struct d3d12_desc *dst, const struct d3d12_desc *src, struct d3d12_device *device); +void d3d12_desc_write_vk_heap(const struct d3d12_desc *src, struct d3d12_device *device); +void d3d12_desc_destroy(struct d3d12_desc *descriptor, struct d3d12_device *device);
bool vkd3d_create_raw_buffer_view(struct d3d12_device *device, D3D12_GPU_VIRTUAL_ADDRESS gpu_address, VkBufferView *vk_buffer_view); @@ -1468,7 +1469,6 @@ struct d3d12_device struct vkd3d_gpu_va_allocator gpu_va_allocator;
struct vkd3d_mutex mutex; - struct vkd3d_mutex desc_mutex[8]; struct vkd3d_render_pass_cache render_pass_cache; VkPipelineCache vk_pipeline_cache;
@@ -1544,19 +1544,6 @@ static inline unsigned int d3d12_device_get_descriptor_handle_increment_size(str return ID3D12Device_GetDescriptorHandleIncrementSize(&device->ID3D12Device_iface, descriptor_type); }
-static inline struct vkd3d_mutex *d3d12_device_get_descriptor_mutex(struct d3d12_device *device, - const struct d3d12_desc *descriptor) -{ - STATIC_ASSERT(!(ARRAY_SIZE(device->desc_mutex) & (ARRAY_SIZE(device->desc_mutex) - 1))); - uintptr_t idx = (uintptr_t)descriptor; - - idx ^= idx >> 12; - idx ^= idx >> 6; - idx ^= idx >> 3; - - return &device->desc_mutex[idx & (ARRAY_SIZE(device->desc_mutex) - 1)]; -} - /* utils */ enum vkd3d_format_type {