[PATCH 0/4] MR260: vkd3d: Prepare for buffering of command list commands.
From: Conor McCarthy <cmccarthy(a)codeweavers.com> This is the simplest way to gather this information when command lists are executed later. --- libs/vkd3d/command.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index 1fc6c00d..1eaf60ea 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -3186,6 +3186,20 @@ static void command_list_flush_vk_heap_updates(struct d3d12_command_list *list) } } +static void command_list_add_descriptor_heap(struct d3d12_command_list *list, struct d3d12_descriptor_heap *heap) +{ + if (!contains_heap(list->descriptor_heaps, list->descriptor_heap_count, heap)) + { + if (list->descriptor_heap_count == ARRAY_SIZE(list->descriptor_heaps)) + { + /* Descriptors can be written after binding. */ + FIXME("Flushing descriptor updates while list %p is not closed.\n", list); + command_list_flush_vk_heap_updates(list); + } + list->descriptor_heaps[list->descriptor_heap_count++] = heap; + } +} + static void d3d12_command_list_bind_descriptor_heap(struct d3d12_command_list *list, enum vkd3d_pipeline_bind_point bind_point, struct d3d12_descriptor_heap *heap) { @@ -3210,18 +3224,6 @@ static void d3d12_command_list_bind_descriptor_heap(struct d3d12_command_list *l bindings->sampler_heap_id = heap->serial_id; } - if (!contains_heap(list->descriptor_heaps, list->descriptor_heap_count, heap)) - { - if (list->descriptor_heap_count == ARRAY_SIZE(list->descriptor_heaps)) - { - /* Descriptors can be written after binding. */ - FIXME("Flushing descriptor updates while list %p is not closed.\n", list); - command_list_flush_vk_heap_updates(list); - list->descriptor_heap_count = 0; - } - list->descriptor_heaps[list->descriptor_heap_count++] = heap; - } - vkd3d_mutex_lock(&heap->vk_sets_mutex); for (set = 0; set < ARRAY_SIZE(heap->vk_descriptor_sets); ++set) @@ -4419,6 +4421,8 @@ static void d3d12_command_list_set_descriptor_table(struct d3d12_command_list *l if (bindings->descriptor_tables[index] == desc) return; + command_list_add_descriptor_heap(list, d3d12_desc_get_descriptor_heap(desc)); + bindings->descriptor_tables[index] = desc; bindings->descriptor_table_dirty_mask |= (uint64_t)1 << index; bindings->descriptor_table_active_mask |= (uint64_t)1 << index; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/260
From: Conor McCarthy <cmccarthy(a)codeweavers.com> For correct function it cannot be freed until the command allocator is reset, and d3d12_command_allocator_free_command_buffer() normally does not free it. --- libs/vkd3d/command.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index 1eaf60ea..caf9964d 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -1316,32 +1316,26 @@ static HRESULT d3d12_command_allocator_allocate_command_buffer(struct d3d12_comm return hr; } - allocator->current_command_list = list; - - return S_OK; -} - -static void d3d12_command_allocator_free_command_buffer(struct d3d12_command_allocator *allocator, - struct d3d12_command_list *list) -{ - struct d3d12_device *device = allocator->device; - const struct vkd3d_vk_device_procs *vk_procs = &device->vk_procs; - - TRACE("allocator %p, list %p.\n", allocator, list); - - if (allocator->current_command_list == list) - allocator->current_command_list = NULL; - if (!vkd3d_array_reserve((void **)&allocator->command_buffers, &allocator->command_buffers_size, allocator->command_buffer_count + 1, sizeof(*allocator->command_buffers))) { WARN("Failed to add command buffer.\n"); VK_CALL(vkFreeCommandBuffers(device->vk_device, allocator->vk_command_pool, 1, &list->vk_command_buffer)); - return; + return E_OUTOFMEMORY; } - allocator->command_buffers[allocator->command_buffer_count++] = list->vk_command_buffer; + + allocator->current_command_list = list; + + return S_OK; +} + +static void d3d12_command_allocator_remove_command_list(struct d3d12_command_allocator *allocator, + const struct d3d12_command_list *list) +{ + if (allocator->current_command_list == list) + allocator->current_command_list = NULL; } static bool d3d12_command_allocator_add_render_pass(struct d3d12_command_allocator *allocator, VkRenderPass pass) @@ -2314,7 +2308,7 @@ static ULONG STDMETHODCALLTYPE d3d12_command_list_Release(ID3D12GraphicsCommandL /* When command pool is destroyed, all command buffers are implicitly freed. */ if (list->allocator) - d3d12_command_allocator_free_command_buffer(list->allocator, list); + d3d12_command_allocator_remove_command_list(list->allocator, list); vkd3d_pipeline_bindings_cleanup(&list->pipeline_bindings[VKD3D_PIPELINE_BIND_POINT_COMPUTE]); vkd3d_pipeline_bindings_cleanup(&list->pipeline_bindings[VKD3D_PIPELINE_BIND_POINT_GRAPHICS]); @@ -2412,7 +2406,7 @@ static HRESULT STDMETHODCALLTYPE d3d12_command_list_Close(ID3D12GraphicsCommandL if (list->allocator) { - d3d12_command_allocator_free_command_buffer(list->allocator, list); + d3d12_command_allocator_remove_command_list(list->allocator, list); list->allocator = NULL; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/260
From: Conor McCarthy <cmccarthy(a)codeweavers.com> D3D12 docs are silent on a required lifetime and immediate release works in Windows. --- tests/d3d12.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/d3d12.c b/tests/d3d12.c index 2f1c905f..3948a491 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -24328,6 +24328,7 @@ static void test_execute_indirect(void) ID3D12CommandQueue *queue; ID3D12Resource *vb, *ib; unsigned int i; + ULONG refcount; D3D12_BOX box; HRESULT hr; @@ -24507,13 +24508,15 @@ static void test_execute_indirect(void) ID3D12GraphicsCommandList_ExecuteIndirect(command_list, command_signature, 4, argument_buffer, 0, count_buffer, 0); + refcount = ID3D12CommandSignature_Release(command_signature); + ok(!refcount, "ID3D12CommandSignature has %u references left.\n", refcount); + transition_resource_state(command_list, context.render_target, D3D12_RESOURCE_STATE_RENDER_TARGET, D3D12_RESOURCE_STATE_COPY_SOURCE); check_sub_resource_uint(context.render_target, 0, queue, command_list, 0xff00ff00, 0); reset_command_list(command_list, context.allocator); - ID3D12CommandSignature_Release(command_signature); command_signature = create_command_signature(context.device, D3D12_INDIRECT_ARGUMENT_TYPE_DISPATCH); uav = create_default_buffer(context.device, 2 * 3 * 4 * sizeof(UINT), @@ -24540,6 +24543,9 @@ static void test_execute_indirect(void) ID3D12GraphicsCommandList_ExecuteIndirect(command_list, command_signature, 1, argument_buffer, offsetof(struct argument_data, dispatch), NULL, 0); + refcount = ID3D12CommandSignature_Release(command_signature); + ok(!refcount, "ID3D12CommandSignature has %u references left.\n", refcount); + transition_sub_resource_state(command_list, uav, 0, D3D12_RESOURCE_STATE_UNORDERED_ACCESS, D3D12_RESOURCE_STATE_COPY_SOURCE); get_buffer_readback_with_command_list(uav, DXGI_FORMAT_R32_UINT, &rb, queue, command_list); @@ -24554,7 +24560,6 @@ static void test_execute_indirect(void) transition_resource_state(command_list, context.render_target, D3D12_RESOURCE_STATE_COPY_SOURCE, D3D12_RESOURCE_STATE_RENDER_TARGET); - ID3D12CommandSignature_Release(command_signature); command_signature = create_command_signature(context.device, D3D12_INDIRECT_ARGUMENT_TYPE_DRAW_INDEXED); ID3D12GraphicsCommandList_ClearRenderTargetView(command_list, context.rtv, white, 0, NULL); @@ -24600,6 +24605,9 @@ static void test_execute_indirect(void) ARRAY_SIZE(argument_data.indexed_draws), argument_buffer, offsetof(struct argument_data, indexed_draws), count_buffer, sizeof(uint32_t)); + refcount = ID3D12CommandSignature_Release(command_signature); + ok(!refcount, "ID3D12CommandSignature has %u references left.\n", refcount); + transition_resource_state(command_list, context.render_target, D3D12_RESOURCE_STATE_RENDER_TARGET, D3D12_RESOURCE_STATE_COPY_SOURCE); check_sub_resource_uint(context.render_target, 0, queue, command_list, 0xffffff00, 0); @@ -24609,7 +24617,6 @@ static void test_execute_indirect(void) ID3D12Resource_Release(ib); ID3D12Resource_Release(uav); ID3D12Resource_Release(vb); - ID3D12CommandSignature_Release(command_signature); ID3D12Resource_Release(argument_buffer); ID3D12Resource_Release(count_buffer); destroy_test_context(&context); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/260
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Buffering of command list commands will depend upon command signatures persisting until the buffer is flushed. --- libs/vkd3d/command.c | 42 ++++++++++++++++++++++++++++---------- libs/vkd3d/vkd3d_private.h | 15 ++++++++++++++ 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index caf9964d..c8d1b5d4 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -1905,6 +1905,29 @@ HRESULT d3d12_command_allocator_create(struct d3d12_device *device, return S_OK; } +static void d3d12_command_signature_destroy(struct d3d12_command_signature *signature) +{ + struct d3d12_device *device = signature->device; + + vkd3d_private_store_destroy(&signature->private_store); + + vkd3d_free((void *)signature->desc.pArgumentDescs); + vkd3d_free(signature); + + d3d12_device_release(device); +} + +static void d3d12_command_signature_add_pending(struct d3d12_command_signature *signature) +{ + vkd3d_atomic_increment(&signature->pending_count); +} + +static void d3d12_command_signature_release_pending(struct d3d12_command_signature *signature) +{ + if (!vkd3d_atomic_decrement(&signature->pending_count) && !signature->refcount) + d3d12_command_signature_destroy(signature); +} + /* ID3D12CommandList */ static inline struct d3d12_command_list *impl_from_ID3D12GraphicsCommandList2(ID3D12GraphicsCommandList2 *iface) { @@ -5713,6 +5736,8 @@ static void STDMETHODCALLTYPE d3d12_command_list_ExecuteIndirect(ID3D12GraphicsC return; } + d3d12_command_signature_add_pending(sig_impl); + signature_desc = &sig_impl->desc; for (i = 0; i < signature_desc->NumArgumentDescs; ++i) { @@ -5775,6 +5800,7 @@ static void STDMETHODCALLTYPE d3d12_command_list_ExecuteIndirect(ID3D12GraphicsC if (!d3d12_command_list_update_compute_state(list)) { WARN("Failed to update compute state, ignoring dispatch.\n"); + d3d12_command_signature_release_pending(sig_impl); return; } @@ -5787,6 +5813,8 @@ static void STDMETHODCALLTYPE d3d12_command_list_ExecuteIndirect(ID3D12GraphicsC break; } } + + d3d12_command_signature_release_pending(sig_impl); } static void STDMETHODCALLTYPE d3d12_command_list_AtomicCopyBufferUINT(ID3D12GraphicsCommandList2 *iface, @@ -7252,17 +7280,8 @@ static ULONG STDMETHODCALLTYPE d3d12_command_signature_Release(ID3D12CommandSign TRACE("%p decreasing refcount to %u.\n", signature, refcount); - if (!refcount) - { - struct d3d12_device *device = signature->device; - - vkd3d_private_store_destroy(&signature->private_store); - - vkd3d_free((void *)signature->desc.pArgumentDescs); - vkd3d_free(signature); - - d3d12_device_release(device); - } + if (!refcount && !signature->pending_count) + d3d12_command_signature_destroy(signature); return refcount; } @@ -7369,6 +7388,7 @@ HRESULT d3d12_command_signature_create(struct d3d12_device *device, const D3D12_ object->ID3D12CommandSignature_iface.lpVtbl = &d3d12_command_signature_vtbl; object->refcount = 1; + object->pending_count = 0; object->desc = *desc; if (!(object->desc.pArgumentDescs = vkd3d_calloc(desc->NumArgumentDescs, sizeof(*desc->pArgumentDescs)))) diff --git a/libs/vkd3d/vkd3d_private.h b/libs/vkd3d/vkd3d_private.h index c5259420..645cbb18 100644 --- a/libs/vkd3d/vkd3d_private.h +++ b/libs/vkd3d/vkd3d_private.h @@ -253,6 +253,11 @@ static inline void vkd3d_cond_destroy(struct vkd3d_cond *cond) { } +static inline unsigned int vkd3d_atomic_increment(unsigned int volatile *x) +{ + return InterlockedIncrement((LONG volatile *)x); +} + static inline unsigned int vkd3d_atomic_decrement(unsigned int volatile *x) { return InterlockedDecrement((LONG volatile *)x); @@ -387,6 +392,15 @@ static inline unsigned int vkd3d_atomic_decrement(unsigned int volatile *x) } # else # error "vkd3d_atomic_decrement() not implemented for this platform" +# endif /* HAVE_SYNC_SUB_AND_FETCH */ + +# if HAVE_SYNC_ADD_AND_FETCH +static inline unsigned int vkd3d_atomic_increment(unsigned int volatile *x) +{ + return __sync_add_and_fetch(x, 1); +} +# else +# error "vkd3d_atomic_increment() not implemented for this platform" # endif /* HAVE_SYNC_ADD_AND_FETCH */ # if HAVE_SYNC_BOOL_COMPARE_AND_SWAP @@ -1575,6 +1589,7 @@ struct d3d12_command_signature { ID3D12CommandSignature ID3D12CommandSignature_iface; LONG refcount; + unsigned int pending_count; D3D12_COMMAND_SIGNATURE_DESC desc; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/260
Why did we want to buffer these in vkd3d again? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/260#note_38535
On Tue Jul 11 00:59:58 2023 +0000, Henri Verbeet wrote:
Why did we want to buffer these in vkd3d again? The main reason is to handle the sequence:
Resource is used in command list 1, so its initial transition is added there. Resource is then used in command list 2. Command list 2 is executed on a queue. Command list 1 is executed on a queue. This occurs in AC:Valhalla. It also makes available accurate layout information so we can stop all the validation failures on layout mismatches, lets us correctly implement d3d12 state promotion, and maybe handle specific performance issues which could arise from non-optimal layouts. It fixes the todo in `test_resolve_query_data_in_reordered_command_list()`. And it makes it much simpler to move initial transitions to the front so they don't end render passes, if that should prove useful. We currently ignore [state promotion](https://learn.microsoft.com/en-us/windows/win32/direct3d12/using-resource-ba...). I think for simultaneous access textures we should just leave them in `VK_IMAGE_LAYOUT_GENERAL`, but normal texures in common state can be used as a shader resource, copy source or copy dest without a state transition. I think we need to transition the layout for the latter two, and for shader resource just leave it in `GENERAL` because checking for such use is impractical. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/260#note_38614
```diff + refcount = ID3D12CommandSignature_Release(command_signature); + ok(!refcount, "ID3D12CommandSignature has %u references left.\n", refcount); ```
This introduces warnings with "make crosstest": ``` <vkd3d>/tests/d3d12.c:24512:19: warning: format ‘%u’ expects argument of type ‘unsigned int’, but argument 4 has type ‘ULONG’ {aka ‘long unsigned int’} [-Wformat=] ok(!refcount, "ID3D12CommandSignature has %u references left.\n", refcount); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~ ``` Similar tests typically have explicit casts to "unsigned int". -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/260#note_39317
Evan Tang (@etang-cw) commented about libs/vkd3d/command.c:
+ vkd3d_private_store_destroy(&signature->private_store); + + vkd3d_free((void *)signature->desc.pArgumentDescs); + vkd3d_free(signature); + + d3d12_device_release(device); +} + +static void d3d12_command_signature_add_pending(struct d3d12_command_signature *signature) +{ + vkd3d_atomic_increment(&signature->pending_count); +} + +static void d3d12_command_signature_release_pending(struct d3d12_command_signature *signature) +{ + if (!vkd3d_atomic_decrement(&signature->pending_count) && !signature->refcount) Having two separate refcounts seems race-prone
I think you can double-free if one thread decrements the pending count while another decrements the refcount, then both see the other as zero -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/260#note_39331
participants (4)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Evan Tang (＠etang-cw)
-
Henri Verbeet (＠hverbeet)