[PATCH vkd3d 1/2] vkd3d: Delay adding a command queue to the blocked list until after the op is written.
Otherwise the following sequence can occur: 1. A command queue is added to the blocked list during a Wait() call. 2. An unblocking Signal() occurs on the CPU in another thread, flushing the blocked ops, but as no op has been written, the queue is removed from the blocked list. 3. The blocked op is written. 3. Another op is queued and the queue is not re-added to the blocked list because this only happens for the first op. World of Warcraft triggers this issue. Signed-off-by: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d/command.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index d0782e5a..7452128c 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -6812,12 +6812,6 @@ static HRESULT STDMETHODCALLTYPE d3d12_command_queue_Wait(ID3D12CommandQueue *if * where multiple queues alias over the same physical queue, so effectively, * we need to manage out-of-order submits ourselves. */ - if (!command_queue->ops_count) - hr = d3d12_device_add_blocked_command_queues(command_queue->device, &command_queue, 1); - - if (FAILED(hr)) - goto done; - if (!(op = d3d12_command_queue_require_space_locked(command_queue))) { hr = E_OUTOFMEMORY; @@ -6829,6 +6823,11 @@ static HRESULT STDMETHODCALLTYPE d3d12_command_queue_Wait(ID3D12CommandQueue *if 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_device_add_blocked_command_queues(command_queue->device, &command_queue, 1); + done: vkd3d_mutex_unlock(&command_queue->op_mutex); return hr; -- 2.36.1
An unblocking Signal() on the CPU must be handled after the blocked op is written, or the op will not be flushed until the next signal. The device is locked while the fence is already locked, so the fence must never be locked after locking the device. Currently this never occurs. Signed-off-by: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d/command.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index 7452128c..4bcab2b1 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -6804,8 +6804,6 @@ static HRESULT STDMETHODCALLTYPE d3d12_command_queue_Wait(ID3D12CommandQueue *if goto done; } - vkd3d_mutex_unlock(&fence->mutex); - /* 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 @@ -6814,6 +6812,7 @@ static HRESULT STDMETHODCALLTYPE d3d12_command_queue_Wait(ID3D12CommandQueue *if if (!(op = d3d12_command_queue_require_space_locked(command_queue))) { + vkd3d_mutex_unlock(&fence->mutex); hr = E_OUTOFMEMORY; goto done; } @@ -6828,6 +6827,11 @@ static HRESULT STDMETHODCALLTYPE d3d12_command_queue_Wait(ID3D12CommandQueue *if if (command_queue->ops_count == 1) hr = d3d12_device_add_blocked_command_queues(command_queue->device, &command_queue, 1); + /* 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; -- 2.36.1
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
On Mon, 18 Jul 2022 at 05:08, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
Otherwise the following sequence can occur: 1. A command queue is added to the blocked list during a Wait() call. 2. An unblocking Signal() occurs on the CPU in another thread, flushing the blocked ops, but as no op has been written, the queue is removed from the blocked list. 3. The blocked op is written. 3. Another op is queued and the queue is not re-added to the blocked list because this only happens for the first op.
There are two "3."'s in that sequence. In a certain sense that's fitting, but it still seems unintentional.
participants (3)
-
Conor McCarthy -
Henri Verbeet -
Henri Verbeet