[PATCH 0/3] MR94: Fix other synchronization problems
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> Otherwise it could be added more than once. Note that the deleted comment is wrong: between when d3d12_command_queue_flush_ops() returns and when the queue is added back to the blocked list, the queue might have been pushed to and flushed an arbitrary number of times. --- libs/vkd3d/command.c | 51 ++++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 30 deletions(-) diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index 158d0d9f..b7d6815c 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -26,7 +26,7 @@ static HRESULT d3d12_fence_signal(struct d3d12_fence *fence, uint64_t value, VkF static void d3d12_fence_signal_timeline_semaphore(struct d3d12_fence *fence, uint64_t timeline_value); static HRESULT d3d12_command_queue_signal(struct d3d12_command_queue *command_queue, struct d3d12_fence *fence, uint64_t value); -static bool d3d12_command_queue_flush_ops(struct d3d12_command_queue *queue, bool *flushed_any); +static HRESULT d3d12_command_queue_flush_ops(struct d3d12_command_queue *queue, bool *flushed_any); HRESULT vkd3d_queue_create(struct d3d12_device *device, uint32_t family_index, const VkQueueFamilyProperties *properties, struct vkd3d_queue **queue) @@ -636,26 +636,17 @@ static HRESULT d3d12_fence_update_pending_value(struct d3d12_fence *fence) return S_OK; } -static HRESULT d3d12_device_add_blocked_command_queues(struct d3d12_device *device, - struct d3d12_command_queue * const *command_queues, unsigned int count) +static HRESULT d3d12_command_queue_mark_as_blocked(struct d3d12_command_queue *command_queue) { + struct d3d12_device *device = command_queue->device; HRESULT hr = S_OK; - unsigned int i; - - if (count == 0) - return S_OK; vkd3d_mutex_lock(&device->blocked_queues_mutex); - if ((i = ARRAY_SIZE(device->blocked_queues) - device->blocked_queue_count) < count) - { - FIXME("Failed to add %u blocked command queue(s) to device %p.\n", count - i, device); - count = i; - hr = E_FAIL; - } - - for (i = 0; i < count; ++i) - device->blocked_queues[device->blocked_queue_count++] = command_queues[i]; + if (device->blocked_queue_count < ARRAY_SIZE(device->blocked_queues)) + device->blocked_queues[device->blocked_queue_count++] = command_queue; + else + WARN("Failed to add blocked command queue %p to device %p.\n", command_queue, device); vkd3d_mutex_unlock(&device->blocked_queues_mutex); return hr; @@ -665,6 +656,7 @@ static HRESULT d3d12_device_flush_blocked_queues_once(struct d3d12_device *devic { struct d3d12_command_queue *blocked_queues[VKD3D_MAX_DEVICE_BLOCKED_QUEUES]; unsigned int i, blocked_queue_count; + HRESULT hr = S_OK; *flushed_any = false; @@ -678,18 +670,17 @@ static HRESULT d3d12_device_flush_blocked_queues_once(struct d3d12_device *devic vkd3d_mutex_unlock(&device->blocked_queues_mutex); - i = 0; - while (i < blocked_queue_count) + for (i = 0; i < blocked_queue_count; ++i) { - if (d3d12_command_queue_flush_ops(blocked_queues[i], flushed_any)) - blocked_queues[i] = blocked_queues[--blocked_queue_count]; - else - ++i; + HRESULT new_hr; + + new_hr = d3d12_command_queue_flush_ops(blocked_queues[i], flushed_any); + + if (SUCCEEDED(hr)) + hr = new_hr; } - /* None of these queues could have been re-added during the above loop because - * blocked queues always have a nonzero op count. */ - return d3d12_device_add_blocked_command_queues(device, blocked_queues, blocked_queue_count); + return hr; } static HRESULT d3d12_device_flush_blocked_queues(struct d3d12_device *device) @@ -6639,7 +6630,7 @@ static HRESULT STDMETHODCALLTYPE d3d12_command_queue_Wait(ID3D12CommandQueue *if /* Add the queue to the blocked list after writing the op to ensure the queue isn't * removed again in another thread because it has no ops. */ if (command_queue->ops_count == 1) - hr = d3d12_device_add_blocked_command_queues(command_queue->device, &command_queue, 1); + hr = d3d12_command_queue_mark_as_blocked(command_queue); /* The fence must remain locked until the op is created and the queue is added to the blocked list, * because if an unblocking d3d12_fence_Signal() call occurs on another thread before the above @@ -6773,11 +6764,11 @@ static const struct ID3D12CommandQueueVtbl d3d12_command_queue_vtbl = }; /* flushed_any is initialised by the caller. */ -static bool d3d12_command_queue_flush_ops(struct d3d12_command_queue *queue, bool *flushed_any) +static HRESULT d3d12_command_queue_flush_ops(struct d3d12_command_queue *queue, bool *flushed_any) { struct vkd3d_cs_op_data *op; struct d3d12_fence *fence; - bool flushed_all = false; + HRESULT hr = S_OK; unsigned int i; if (!queue->ops_count) @@ -6807,6 +6798,7 @@ static bool d3d12_command_queue_flush_ops(struct d3d12_command_queue *queue, boo vkd3d_mutex_unlock(&fence->mutex); queue->ops_count -= i; memmove(queue->ops, op, queue->ops_count * sizeof(*op)); + hr = d3d12_command_queue_mark_as_blocked(queue); goto done; } d3d12_command_queue_wait_locked(queue, fence, op->u.wait.value); @@ -6830,13 +6822,12 @@ static bool d3d12_command_queue_flush_ops(struct d3d12_command_queue *queue, boo } queue->ops_count = 0; - flushed_all = true; done: queue->is_flushing = false; vkd3d_mutex_unlock(&queue->op_mutex); - return flushed_all; + return hr; } static HRESULT d3d12_command_queue_init(struct d3d12_command_queue *queue, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/94
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> The goal is to simplify the CS queue handling: with this change operations are always started by d3d12_command_queue_flush_ops(), in order to make further refactoring easier. Notice that while with this change executing an operation on an empty CS queue is a bit less efficient, it doesn't require more locking. On the other hand, this change paves the road for executing CS operations without holding the queue lock. --- libs/vkd3d/command.c | 138 ++++++++++++++++--------------------------- 1 file changed, 52 insertions(+), 86 deletions(-) diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index b7d6815c..f82401c6 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -27,6 +27,7 @@ static void d3d12_fence_signal_timeline_semaphore(struct d3d12_fence *fence, uin static HRESULT d3d12_command_queue_signal(struct d3d12_command_queue *command_queue, struct d3d12_fence *fence, uint64_t value); static HRESULT d3d12_command_queue_flush_ops(struct d3d12_command_queue *queue, bool *flushed_any); +static HRESULT d3d12_command_queue_flush_ops_locked(struct d3d12_command_queue *queue, bool *flushed_any); HRESULT vkd3d_queue_create(struct d3d12_device *device, uint32_t family_index, const VkQueueFamilyProperties *properties, struct vkd3d_queue **queue) @@ -6141,12 +6142,34 @@ static void d3d12_command_queue_execute(struct d3d12_command_queue *command_queu vkd3d_free(buffers); } +static HRESULT d3d12_command_queue_enqueue_op(struct d3d12_command_queue *queue, const struct vkd3d_cs_op_data *op) +{ + struct vkd3d_cs_op_data *new_op; + bool flushed_any = false; + + vkd3d_mutex_lock(&queue->op_mutex); + + if (!(new_op = d3d12_command_queue_require_space_locked(queue))) + { + vkd3d_mutex_unlock(&queue->op_mutex); + return E_OUTOFMEMORY; + } + + *new_op = *op; + + if (queue->ops_count == 1) + return d3d12_command_queue_flush_ops_locked(queue, &flushed_any); + + vkd3d_mutex_unlock(&queue->op_mutex); + return S_OK; +} + static void STDMETHODCALLTYPE d3d12_command_queue_ExecuteCommandLists(ID3D12CommandQueue *iface, UINT command_list_count, ID3D12CommandList * const *command_lists) { struct d3d12_command_queue *command_queue = impl_from_ID3D12CommandQueue(iface); struct d3d12_command_list *cmd_list; - struct vkd3d_cs_op_data *op; + struct vkd3d_cs_op_data op; VkCommandBuffer *buffers; unsigned int i; @@ -6177,26 +6200,12 @@ static void STDMETHODCALLTYPE d3d12_command_queue_ExecuteCommandLists(ID3D12Comm buffers[i] = cmd_list->vk_command_buffer; } - vkd3d_mutex_lock(&command_queue->op_mutex); - - if (!command_queue->ops_count) - { - d3d12_command_queue_execute(command_queue, buffers, command_list_count); - vkd3d_mutex_unlock(&command_queue->op_mutex); - return; - } - - if (!(op = d3d12_command_queue_require_space_locked(command_queue))) - { - ERR("Failed to add op.\n"); - return; - } - op->opcode = VKD3D_CS_OP_EXECUTE; - op->u.execute.buffers = buffers; - op->u.execute.buffer_count = command_list_count; + op.opcode = VKD3D_CS_OP_EXECUTE; + op.u.execute.buffers = buffers; + op.u.execute.buffer_count = command_list_count; - vkd3d_mutex_unlock(&command_queue->op_mutex); - return; + if (FAILED(d3d12_command_queue_enqueue_op(command_queue, &op))) + ERR("Cannot enqueue in command queue %p.\n", command_queue); } static void STDMETHODCALLTYPE d3d12_command_queue_SetMarker(ID3D12CommandQueue *iface, @@ -6253,33 +6262,17 @@ static HRESULT STDMETHODCALLTYPE d3d12_command_queue_Signal(ID3D12CommandQueue * { struct d3d12_command_queue *command_queue = impl_from_ID3D12CommandQueue(iface); struct d3d12_fence *fence = unsafe_impl_from_ID3D12Fence(fence_iface); - struct vkd3d_cs_op_data *op; - HRESULT hr = S_OK; + struct vkd3d_cs_op_data op; TRACE("iface %p, fence %p, value %#"PRIx64".\n", iface, fence_iface, value); - vkd3d_mutex_lock(&command_queue->op_mutex); - - if (!command_queue->ops_count) - { - hr = d3d12_command_queue_signal(command_queue, fence, value); - goto done; - } - - if (!(op = d3d12_command_queue_require_space_locked(command_queue))) - { - hr = E_OUTOFMEMORY; - goto done; - } - op->opcode = VKD3D_CS_OP_SIGNAL; - op->u.signal.fence = fence; - op->u.signal.value = value; + op.opcode = VKD3D_CS_OP_SIGNAL; + op.u.signal.fence = fence; + op.u.signal.value = value; d3d12_fence_incref(fence); -done: - vkd3d_mutex_unlock(&command_queue->op_mutex); - return hr; + return d3d12_command_queue_enqueue_op(command_queue, &op); } static HRESULT d3d12_command_queue_signal(struct d3d12_command_queue *command_queue, @@ -6595,51 +6588,17 @@ static HRESULT STDMETHODCALLTYPE d3d12_command_queue_Wait(ID3D12CommandQueue *if { struct d3d12_command_queue *command_queue = impl_from_ID3D12CommandQueue(iface); struct d3d12_fence *fence = unsafe_impl_from_ID3D12Fence(fence_iface); - struct vkd3d_cs_op_data *op; - HRESULT hr = S_OK; + struct vkd3d_cs_op_data op; TRACE("iface %p, fence %p, value %#"PRIx64".\n", iface, fence_iface, value); - vkd3d_mutex_lock(&command_queue->op_mutex); - vkd3d_mutex_lock(&fence->mutex); - - if (!command_queue->ops_count && value <= fence->max_pending_value) - { - hr = d3d12_command_queue_wait_locked(command_queue, fence, value); - goto done; - } - - /* This is the critical part required to support out-of-order signal. - * Normally we would be able to submit waits and signals out of order, but - * we don't have virtualized queues in Vulkan, so we need to handle the case - * where multiple queues alias over the same physical queue, so effectively, - * we need to manage out-of-order submits ourselves. */ - - if (!(op = d3d12_command_queue_require_space_locked(command_queue))) - { - vkd3d_mutex_unlock(&fence->mutex); - hr = E_OUTOFMEMORY; - goto done; - } - op->opcode = VKD3D_CS_OP_WAIT; - op->u.wait.fence = fence; - op->u.wait.value = value; + op.opcode = VKD3D_CS_OP_WAIT; + op.u.wait.fence = fence; + op.u.wait.value = value; d3d12_fence_incref(fence); - /* Add the queue to the blocked list after writing the op to ensure the queue isn't - * removed again in another thread because it has no ops. */ - if (command_queue->ops_count == 1) - hr = d3d12_command_queue_mark_as_blocked(command_queue); - - /* The fence must remain locked until the op is created and the queue is added to the blocked list, - * because if an unblocking d3d12_fence_Signal() call occurs on another thread before the above - * work is done, flushing will be delayed until the next signal, if one occurs at all. */ - vkd3d_mutex_unlock(&fence->mutex); - -done: - vkd3d_mutex_unlock(&command_queue->op_mutex); - return hr; + return d3d12_command_queue_enqueue_op(command_queue, &op); } static HRESULT STDMETHODCALLTYPE d3d12_command_queue_GetTimestampFrequency(ID3D12CommandQueue *iface, @@ -6763,14 +6722,10 @@ static const struct ID3D12CommandQueueVtbl d3d12_command_queue_vtbl = d3d12_command_queue_GetDesc, }; -/* flushed_any is initialised by the caller. */ +static HRESULT d3d12_command_queue_flush_ops_locked(struct d3d12_command_queue *queue, bool *flushed_any); + static HRESULT d3d12_command_queue_flush_ops(struct d3d12_command_queue *queue, bool *flushed_any) { - struct vkd3d_cs_op_data *op; - struct d3d12_fence *fence; - HRESULT hr = S_OK; - unsigned int i; - if (!queue->ops_count) return true; @@ -6782,6 +6737,17 @@ static HRESULT d3d12_command_queue_flush_ops(struct d3d12_command_queue *queue, vkd3d_mutex_lock(&queue->op_mutex); + return d3d12_command_queue_flush_ops_locked(queue, flushed_any); +} + +/* flushed_any is initialised by the caller. */ +static HRESULT d3d12_command_queue_flush_ops_locked(struct d3d12_command_queue *queue, bool *flushed_any) +{ + struct vkd3d_cs_op_data *op; + struct d3d12_fence *fence; + HRESULT hr = S_OK; + unsigned int i; + /* Currently only required for d3d12_command_queue_signal(), but set it here anyway. */ queue->is_flushing = true; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/94
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> d3d12_command_queue_flush_ops() can renter itself while processing signal events. Since we don't use recursive mutexes, we currently have to check some of the queue variables without holding the mutex, which is not safe. This is solved by allowing the queue to release its mutex while it is processing entries: when flushing, the queue is briefly locked, the is_flushing flag is set, the queue content is copied away and the queue is unlocked again. After having processed the entries, the queue is locked again to check is something else was added in the meantime. This is repeated until the queue is empty (or a wait operation is blocking it). This should also remove some latency when a thread pushes to the queue while another one is processing it, but I didn't try to measure any impact. While it is expected that with this patch the queue mutex will be locked and unlocked more frequently, it should also remain locked for less time, hopefully creating little contention. --- libs/vkd3d/command.c | 141 +++++++++++++++++++++++++++---------- libs/vkd3d/vkd3d_private.h | 9 +++ 2 files changed, 111 insertions(+), 39 deletions(-) diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index f82401c6..0a3a4214 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -6008,6 +6008,7 @@ static ULONG STDMETHODCALLTYPE d3d12_command_queue_Release(ID3D12CommandQueue *i vkd3d_mutex_destroy(&command_queue->op_mutex); vkd3d_free(command_queue->ops); + vkd3d_free(command_queue->aux_ops); vkd3d_private_store_destroy(&command_queue->private_store); @@ -6157,7 +6158,7 @@ static HRESULT d3d12_command_queue_enqueue_op(struct d3d12_command_queue *queue, *new_op = *op; - if (queue->ops_count == 1) + if (queue->ops_count == 1 && !queue->is_flushing) return d3d12_command_queue_flush_ops_locked(queue, &flushed_any); vkd3d_mutex_unlock(&queue->op_mutex); @@ -6722,20 +6723,70 @@ static const struct ID3D12CommandQueueVtbl d3d12_command_queue_vtbl = d3d12_command_queue_GetDesc, }; +static void d3d12_command_queue_swap_queues(struct d3d12_command_queue *queue) +{ + size_t size; + struct vkd3d_cs_op_data *ops; + + size = queue->ops_count; + queue->ops_count = queue->aux_ops_count; + queue->aux_ops_count = size; + + size = queue->ops_size; + queue->ops_size = queue->aux_ops_size; + queue->aux_ops_size = size; + + ops = queue->ops; + queue->ops = queue->aux_ops; + queue->aux_ops = ops; +} + +static HRESULT d3d12_command_queue_fixup_after_flush(struct d3d12_command_queue *queue, unsigned int done_count) +{ + HRESULT hr; + + queue->aux_ops_count -= done_count; + memmove(queue->aux_ops, &queue->aux_ops[done_count], queue->aux_ops_count * sizeof(*queue->ops)); + + vkd3d_mutex_lock(&queue->op_mutex); + + d3d12_command_queue_swap_queues(queue); + + if (!vkd3d_array_reserve((void **)&queue->ops, &queue->ops_size, + queue->ops_count + queue->aux_ops_count, sizeof(*queue->ops))) + { + ERR("Cannot reallocate, dropping %zu ops.\n", queue->aux_ops_count); + } + else + { + memcpy(&queue->ops[queue->ops_count], queue->aux_ops, queue->aux_ops_count * sizeof(*queue->ops)); + queue->ops_count += queue->aux_ops_count; + } + + queue->aux_ops_count = 0; + queue->is_flushing = false; + + hr = d3d12_command_queue_mark_as_blocked(queue); + + vkd3d_mutex_unlock(&queue->op_mutex); + + return hr; +} + static HRESULT d3d12_command_queue_flush_ops_locked(struct d3d12_command_queue *queue, bool *flushed_any); static HRESULT d3d12_command_queue_flush_ops(struct d3d12_command_queue *queue, bool *flushed_any) { - if (!queue->ops_count) - return true; + vkd3d_mutex_lock(&queue->op_mutex); /* This function may be re-entered during a call below to d3d12_command_queue_signal(). * We return true because the first caller is responsible for re-adding this queue to * the flush list if it ends up returning false. */ if (queue->is_flushing) + { + vkd3d_mutex_unlock(&queue->op_mutex); return true; - - vkd3d_mutex_lock(&queue->op_mutex); + } return d3d12_command_queue_flush_ops_locked(queue, flushed_any); } @@ -6745,55 +6796,61 @@ static HRESULT d3d12_command_queue_flush_ops_locked(struct d3d12_command_queue * { struct vkd3d_cs_op_data *op; struct d3d12_fence *fence; - HRESULT hr = S_OK; unsigned int i; - /* Currently only required for d3d12_command_queue_signal(), but set it here anyway. */ queue->is_flushing = true; - for (i = 0; i < queue->ops_count; ++i) + assert(queue->aux_ops_count == 0); + + while (queue->ops_count != 0) { - op = &queue->ops[i]; - switch (op->opcode) + d3d12_command_queue_swap_queues(queue); + + vkd3d_mutex_unlock(&queue->op_mutex); + + for (i = 0; i < queue->aux_ops_count; ++i) { - case VKD3D_CS_OP_WAIT: - fence = op->u.wait.fence; - vkd3d_mutex_lock(&fence->mutex); - if (op->u.wait.value > fence->max_pending_value) - { - vkd3d_mutex_unlock(&fence->mutex); - queue->ops_count -= i; - memmove(queue->ops, op, queue->ops_count * sizeof(*op)); - hr = d3d12_command_queue_mark_as_blocked(queue); - goto done; - } - d3d12_command_queue_wait_locked(queue, fence, op->u.wait.value); - d3d12_fence_decref(fence); - break; + op = &queue->aux_ops[i]; + switch (op->opcode) + { + case VKD3D_CS_OP_WAIT: + fence = op->u.wait.fence; + vkd3d_mutex_lock(&fence->mutex); + if (op->u.wait.value > fence->max_pending_value) + { + vkd3d_mutex_unlock(&fence->mutex); + return d3d12_command_queue_fixup_after_flush(queue, i); + } + d3d12_command_queue_wait_locked(queue, fence, op->u.wait.value); + d3d12_fence_decref(fence); + break; - case VKD3D_CS_OP_SIGNAL: - d3d12_command_queue_signal(queue, op->u.signal.fence, op->u.signal.value); - d3d12_fence_decref(op->u.signal.fence); - break; + case VKD3D_CS_OP_SIGNAL: + d3d12_command_queue_signal(queue, op->u.signal.fence, op->u.signal.value); + d3d12_fence_decref(op->u.signal.fence); + break; - case VKD3D_CS_OP_EXECUTE: - d3d12_command_queue_execute(queue, op->u.execute.buffers, op->u.execute.buffer_count); - break; + case VKD3D_CS_OP_EXECUTE: + d3d12_command_queue_execute(queue, op->u.execute.buffers, op->u.execute.buffer_count); + break; - default: - FIXME("Unhandled op type %u.\n", op->opcode); - break; + default: + vkd3d_unreachable(); + } + + *flushed_any |= true; } - *flushed_any |= true; - } - queue->ops_count = 0; + queue->aux_ops_count = 0; + + vkd3d_mutex_lock(&queue->op_mutex); + } -done: queue->is_flushing = false; vkd3d_mutex_unlock(&queue->op_mutex); - return hr; + + return S_OK; } static HRESULT d3d12_command_queue_init(struct d3d12_command_queue *queue, @@ -6819,6 +6876,10 @@ static HRESULT d3d12_command_queue_init(struct d3d12_command_queue *queue, queue->ops_size = 0; queue->is_flushing = false; + queue->aux_ops = NULL; + queue->aux_ops_count = 0; + queue->aux_ops_size = 0; + if (desc->Priority == D3D12_COMMAND_QUEUE_PRIORITY_GLOBAL_REALTIME) { FIXME("Global realtime priority is not implemented.\n"); @@ -6884,6 +6945,8 @@ VkQueue vkd3d_acquire_vk_queue(ID3D12CommandQueue *queue) if (d3d12_queue->ops_count) WARN("Acquired command queue %p with %zu remaining ops.\n", d3d12_queue, d3d12_queue->ops_count); + else if (d3d12_queue->is_flushing) + WARN("Acquired command queue %p which is flushing.\n", d3d12_queue); return vk_queue; } diff --git a/libs/vkd3d/vkd3d_private.h b/libs/vkd3d/vkd3d_private.h index 56677648..f2b519fa 100644 --- a/libs/vkd3d/vkd3d_private.h +++ b/libs/vkd3d/vkd3d_private.h @@ -1387,11 +1387,20 @@ struct d3d12_command_queue struct d3d12_device *device; struct vkd3d_mutex op_mutex; + + /* These fields are protected by op_mutex. */ struct vkd3d_cs_op_data *ops; size_t ops_count; size_t ops_size; bool is_flushing; + /* These fields are not protected by op_mutex, but can only be + * used by the thread that set is_flushing; when is_flushing is + * not set, aux_ops_count must be zero. */ + struct vkd3d_cs_op_data *aux_ops; + size_t aux_ops_count; + size_t aux_ops_size; + struct vkd3d_private_store private_store; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/94
I'm not well enough at the moment to examine this series thoroughly, but what issues is it designed to fix? I understand that in d3d12_device_flush_blocked_queues_once() a blocked queue which has been buffered in blocked_queues[] for re-adding may be unblocked and then re-added in another thread, then added again from blocked_queues[]. Can we just perform deduplication on the blocked queue array (maybe only if fills up), since flushing an unblocked queue is harmless, or would there still be an issue? It may be much better to create a worker thread for queue flushing, similar to how vkd3d-proton does it. Henri rejected the idea a while ago because it may fail badly on external acquisition, but if that issue is fixed then maybe a thread is the best solution to remaining problems with blocked queue handling. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/94#note_25302
I'm not well enough at the moment to examine this series thoroughly, but what issues is it designed to fix? I understand that in d3d12_device_flush_blocked_queues_once() a blocked queue which has been buffered in blocked_queues\[\] for re-adding may be unblocked and then re-added in another thread, then added again from blocked_queues\[\]. Can we just perform deduplication on the blocked queue array (maybe only if fills up), since flushing an unblocked queue is harmless, or would there still be an issue?
No, I don't think there would. But I think that at least there should be an actual deduplication procedure that happens every now and then. The corner case I am concerned about is that for some bad access pattern queues get added over and over to the blocked list and never removed. I'll think more about this. Anyway, the important patch is the third one, and the second one is basically a preparation for it. The goal is to avoid to access mutex protected fields without holding the mutex, specifically `is_flushing` (and also `ops_count`, but that is less critical, it could just be removed).
It may be much better to create a worker thread for queue flushing, similar to how vkd3d-proton does it. Henri rejected the idea a while ago because it may fail badly on external acquisition, but if that issue is fixed then maybe a thread is the best solution to remaining problems with blocked queue handling.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/94#note_25318
I'm not well enough at the moment to examine this series thoroughly, but what issues is it designed to fix? I understand that in d3d12_device_flush_blocked_queues_once() a blocked queue which has been buffered in blocked_queues\[\] for re-adding may be unblocked and then re-added in another thread, then added again from blocked_queues\[\]. Can we just perform deduplication on the blocked queue array (maybe only if fills up), since flushing an unblocked queue is harmless, or would there still be an issue?
On the other hand, why do you think that doing deduplication would be better? Keeping the blocked queues array deduplicated in the first place looks better to me because it saves time both when scanning it to look for unblocked queues and when deduplicating it if necessary. The cost of my implementation is basically having to lock and unlock the mutex more frequently, but all the critical sections are quite fast, so that should be rarely contended. The effect is probably mild if existing at all on either side, granted, so I can drop this patch without too much remorse, but I am interested in your perspective. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/94#note_25403
Apart form it being complex to review, my only concern is the chance of regression. I think after the external acquisition issue is fixed, the blocked queue implementation should be changed to use a worker thread, but @hverbeet may not agree. I haven't looked at it recently but I think the problem with the proton implementation was it tries to flush the queue up to the point where it's acquired, but a blocked wait could be in the queue, and if the same thread is supposed to submit the unblocking signal to another queue after the Present, it deadlocks. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/94#note_25405
Ok, let's say that for the moment I'll ignore this blocked queue deduplication thing and will resubmit only the last two patches. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/94#note_25406
participants (3)
-
Conor McCarthy (＠cmccarthy) -
Giovanni Mascellani
-
Giovanni Mascellani (＠giomasce)