[PATCH v2 0/2] MR292: vkd3d: Write Vulkan descriptors in a worker thread.
-- v2: vkd3d: Write Vulkan descriptors in a worker thread. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/292
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Fixes a race condition where the descriptor is modified between getting its object and resetting the `next` index. The new object would never be written. While it is invalid for the app to write descriptors used by a command list which has been submitted to a queue, unused descriptors may be written. This change also supports writing descriptors in a worker thread. --- libs/vkd3d/resource.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c index f2fe1250..dffe2f06 100644 --- a/libs/vkd3d/resource.c +++ b/libs/vkd3d/resource.c @@ -2389,8 +2389,11 @@ void d3d12_desc_flush_vk_heap_updates_locked(struct d3d12_descriptor_heap *descr for (; i != UINT_MAX; i = next) { src = &descriptors[i]; - next = (int)src->next >> 1; + next = vkd3d_atomic_exchange(&src->next, 0); + next = (int)next >> 1; + /* A race exists here between updating src->next and getting the current object. The best + * we can do is get the object last, which may result in a harmless rewrite later. */ u.object = d3d12_desc_get_object_ref(src, device); if (!u.object) @@ -2401,8 +2404,6 @@ void d3d12_desc_flush_vk_heap_updates_locked(struct d3d12_descriptor_heap *descr writes.held_refs[writes.held_ref_count++] = u.object; d3d12_desc_write_vk_heap(descriptor_heap, i, &writes, u.object, device); - - vkd3d_atomic_exchange(&src->next, 0); } /* Avoid thunk calls wherever possible. */ -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/292
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Raises framerate in Horizon Zero Dawn by about 5-10%. --- libs/vkd3d/command.c | 17 +++++++- libs/vkd3d/resource.c | 83 ++++++++++++++++++++++++++++++++++++++ libs/vkd3d/vkd3d_private.h | 11 +++++ 3 files changed, 109 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index 24fbbce9..3cccd361 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -2623,10 +2623,21 @@ static bool d3d12_command_list_update_current_framebuffer(struct d3d12_command_l return true; } +static void command_list_signal_descriptor_heaps(struct d3d12_command_list *list) +{ + unsigned int i; + + for (i = 0; i < list->descriptor_heap_count; ++i) + if (list->descriptor_heaps[i]->dirty_list_head != UINT_MAX) + vkd3d_cond_signal(&list->descriptor_heaps[i]->worker.cond); +} + static bool d3d12_command_list_update_compute_pipeline(struct d3d12_command_list *list) { const struct vkd3d_vk_device_procs *vk_procs = &list->device->vk_procs; + command_list_signal_descriptor_heaps(list); + if (list->current_pipeline != VK_NULL_HANDLE) return true; @@ -2648,6 +2659,8 @@ static bool d3d12_command_list_update_graphics_pipeline(struct d3d12_command_lis VkRenderPass vk_render_pass; VkPipeline vk_pipeline; + command_list_signal_descriptor_heaps(list); + if (list->current_pipeline != VK_NULL_HANDLE) return true; @@ -3196,9 +3209,9 @@ static void command_list_flush_vk_heap_updates(struct d3d12_command_list *list) for (i = 0; i < list->descriptor_heap_count; ++i) { - vkd3d_mutex_lock(&list->descriptor_heaps[i]->vk_sets_mutex); + vkd3d_mutex_lock(&list->descriptor_heaps[i]->worker.mutex); d3d12_desc_flush_vk_heap_updates_locked(list->descriptor_heaps[i], device); - vkd3d_mutex_unlock(&list->descriptor_heaps[i]->vk_sets_mutex); + vkd3d_mutex_unlock(&list->descriptor_heaps[i]->worker.mutex); } } diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c index dffe2f06..6e2b7cab 100644 --- a/libs/vkd3d/resource.c +++ b/libs/vkd3d/resource.c @@ -2412,6 +2412,80 @@ void d3d12_desc_flush_vk_heap_updates_locked(struct d3d12_descriptor_heap *descr descriptor_writes_free_object_refs(&writes, device); } +static void *descriptor_write_worker_main(void *arg) +{ + struct descriptor_write_worker *worker = arg; + struct d3d12_descriptor_heap *heap; + struct d3d12_device *device; + + vkd3d_set_thread_name("descriptor_write"); + + heap = worker->heap; + device = heap->device; + + vkd3d_mutex_lock(&worker->mutex); + + for (;;) + { + while (heap->dirty_list_head == UINT_MAX && !worker->should_exit) + vkd3d_cond_wait(&worker->cond, &worker->mutex); + + if (worker->should_exit) + break; + + d3d12_desc_flush_vk_heap_updates_locked(heap, device); + } + + vkd3d_mutex_unlock(&worker->mutex); + + return NULL; +} + +static HRESULT descriptor_write_worker_start(struct descriptor_write_worker *worker, + struct d3d12_descriptor_heap *descriptor_heap) +{ + struct d3d12_device *device = descriptor_heap->device; + HRESULT hr; + + TRACE("worker %p.\n", worker); + + worker->should_exit = false; + worker->heap = descriptor_heap; + vkd3d_mutex_init(&worker->mutex); + vkd3d_cond_init(&worker->cond); + + if (FAILED(hr = vkd3d_create_thread(device->vkd3d_instance, + descriptor_write_worker_main, worker, &worker->thread))) + { + vkd3d_cond_destroy(&worker->cond); + } + + return hr; +} + +static HRESULT descriptor_write_worker_stop(struct descriptor_write_worker *worker) +{ + struct d3d12_device *device = worker->heap->device; + HRESULT hr; + + TRACE("worker %p.\n", worker); + + vkd3d_mutex_lock(&worker->mutex); + + worker->should_exit = true; + vkd3d_cond_signal(&worker->cond); + + vkd3d_mutex_unlock(&worker->mutex); + + if (FAILED(hr = vkd3d_join_thread(device->vkd3d_instance, &worker->thread))) + return hr; + + vkd3d_mutex_destroy(&worker->mutex); + vkd3d_cond_destroy(&worker->cond); + + return S_OK; +} + static void d3d12_desc_mark_as_modified(struct d3d12_desc *dst, struct d3d12_descriptor_heap *descriptor_heap) { unsigned int i, head; @@ -3737,6 +3811,9 @@ static ULONG STDMETHODCALLTYPE d3d12_descriptor_heap_Release(ID3D12DescriptorHea { struct d3d12_desc *descriptors = (struct d3d12_desc *)heap->descriptors; + if (heap->use_vk_heaps) + descriptor_write_worker_stop(&heap->worker); + for (i = 0; i < heap->desc.NumDescriptors; ++i) { d3d12_desc_destroy(&descriptors[i], device); @@ -4060,6 +4137,12 @@ HRESULT d3d12_descriptor_heap_create(struct d3d12_device *device, dst[i].next = 0; } object->dirty_list_head = UINT_MAX; + + if (object->use_vk_heaps && FAILED(hr = descriptor_write_worker_start(&object->worker, object))) + { + vkd3d_free(object); + return hr; + } } else { diff --git a/libs/vkd3d/vkd3d_private.h b/libs/vkd3d/vkd3d_private.h index 3be12be7..5d8926d1 100644 --- a/libs/vkd3d/vkd3d_private.h +++ b/libs/vkd3d/vkd3d_private.h @@ -1005,6 +1005,16 @@ struct d3d12_descriptor_heap_vk_set VkDescriptorType vk_type; }; +struct descriptor_write_worker +{ + union vkd3d_thread_handle thread; + struct vkd3d_mutex mutex; + struct vkd3d_cond cond; + bool should_exit; + + struct d3d12_descriptor_heap *heap; +}; + /* ID3D12DescriptorHeap */ struct d3d12_descriptor_heap { @@ -1022,6 +1032,7 @@ struct d3d12_descriptor_heap VkDescriptorPool vk_descriptor_pool; struct d3d12_descriptor_heap_vk_set vk_descriptor_sets[VKD3D_SET_INDEX_COUNT]; struct vkd3d_mutex vk_sets_mutex; + struct descriptor_write_worker worker; unsigned int volatile dirty_list_head; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/292
Evan Tang (@etang-cw) commented about libs/vkd3d/resource.c:
for (; i != UINT_MAX; i = next) { src = &descriptors[i]; - next = (int)src->next >> 1; + next = vkd3d_atomic_exchange(&src->next, 0); + next = (int)next >> 1;
+ /* A race exists here between updating src->next and getting the current object. The best + * we can do is get the object last, which may result in a harmless rewrite later. */ u.object = d3d12_desc_get_object_ref(src, device);
if (!u.object) { vkd3d_atomic_exchange(&src->next, 0);
Should this be deleted as well? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/292#note_42376
participants (3)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Evan Tang (＠etang-cw)