From patch 2/4:
@@ -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_record_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;
This drops setting hr to E_FAIL when adding the queue fails. If that's in fact intentional, there's no longer much of a point in returning a HRESULT here; it'll always be S_OK.
From patch 3/4:
+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; +}
I don't like that d3d12_command_queue_enqueue_op() introduces an extra copy of "op". It's not the end of the world, but it seems unnecessary. The introduction of the helper also ends up obfuscating what's supposed to be the main change in the commit according to the commit message somewhat. (I.e., the removal of the direct paths in d3d12_command_queue_ExecuteCommandLists(), d3d12_command_queue_Signal(), and d3d12_command_queue_Wait(). Which could have been three separate commits, I suppose.)
@@ -6783,6 +6736,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;
It's perhaps not terribly obvious from the diff, but this moves releasing queue->op_mutex to d3d12_command_queue_flush_ops_locked(). I don't think that's desirable, and with the somewhat unfortunate exceptions of d3d12_command_queue_wait_locked() and d3d12_command_queue_wait_binary_semaphore_locked(), that's not what _locked means in vkd3d.
I haven't reviewed 4/4 in detail yet, though it doesn't look wrong at first sight.