The resource could be destructed before the command list left open is executed; instead, we immediately perform the transition.
From: Giovanni Mascellani gmascellani@codeweavers.com
The resource could be destructed before the command list left open is executed; instead, we immediately perform the transition. --- tests/shader_runner_d3d12.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index 8453617ef..8abcdbc41 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -540,6 +540,7 @@ static struct resource_readback *d3d12_runner_get_resource_readback(struct shade struct d3d12_resource_readback *rb = malloc(sizeof(*rb)); struct d3d12_resource *resource = d3d12_resource(res); D3D12_RESOURCE_STATES state; + HRESULT hr;
if (resource->r.type == RESOURCE_TYPE_RENDER_TARGET) state = D3D12_RESOURCE_STATE_RENDER_TARGET; @@ -551,8 +552,14 @@ static struct resource_readback *d3d12_runner_get_resource_readback(struct shade get_resource_readback_with_command_list(resource->resource, 0, rb, test_context->queue, test_context->list); reset_command_list(test_context->list, test_context->allocator); + transition_resource_state(test_context->list, resource->resource, D3D12_RESOURCE_STATE_COPY_SOURCE, state); + hr = ID3D12GraphicsCommandList_Close(test_context->list); + assert_that(hr == S_OK, "Failed to close command list, hr %#x.\n", hr); + exec_command_list(test_context->queue, test_context->list); + wait_queue_idle(test_context->device, test_context->queue); + reset_command_list(test_context->list, test_context->allocator);
return &rb->rb; }
reset_command_list(test_context->list, test_context->allocator); + transition_resource_state(test_context->list, resource->resource, D3D12_RESOURCE_STATE_COPY_SOURCE, state); + hr = ID3D12GraphicsCommandList_Close(test_context->list); + assert_that(hr == S_OK, "Failed to close command list, hr %#x.\n", hr); + exec_command_list(test_context->queue, test_context->list); + wait_queue_idle(test_context->device, test_context->queue); + reset_command_list(test_context->list, test_context->allocator);
We're probably not overly concerned with the performance of the shader runner, but this doesn't seem ideal. It also seems like there might be a similar issue in d3d12_runner_create_resource(). Is there any chance we could instead do something like flushing the current command list in d3d12_runner_destroy_resource() if needed?
Is there any chance we could instead do something like flushing the current command list in d3d12_runner_destroy_resource() if needed?
In principle yes, of course, but I'm not sure it's worth the effort. Notice that we're already doing something similar for dispatch and draw operations. If we decide that we never want to execute and flush the command list until we need to get back data from the GPU, we'd have to track all the operations that are outstanding at any given moment so we can reply them when flushing as you propose. Or we'd have to keep many lists around and choose which one to fire and when, and possibly also add synchronization between them. My feeling is that both are overkill for a test program which we'd rather try to keep simple and explicit.
It also seems like there might be a similar issue in d3d12_runner_create_resource().
Good point, I'll add a patch for that too.
On Tue Oct 31 11:50:46 2023 +0000, Giovanni Mascellani wrote:
Is there any chance we could instead do something like flushing the
current command list in d3d12_runner_destroy_resource() if needed? In principle yes, of course, but I'm not sure it's worth the effort. Notice that we're already doing something similar for dispatch and draw operations. If we decide that we never want to execute and flush the command list until we need to get back data from the GPU, we'd have to track all the operations that are outstanding at any given moment so we can reply them when flushing as you propose. Or we'd have to keep many lists around and choose which one to fire and when, and possibly also add synchronization between them. My feeling is that both are overkill for a test program which we'd rather try to keep simple and explicit.
It also seems like there might be a similar issue in d3d12_runner_create_resource().
Good point, I'll add a patch for that too.
It just occurred to me that maybe a better alternative would be to make `get_resource_readback_with_command_list()` and `upload_texture_data()` aware of the states the resources they touch are at the beginning and should be at the end. So they can directly care about doing the required transitions in the command list they already execute.
Is there any chance we could instead do something like flushing the current command list in d3d12_runner_destroy_resource() if needed?
In principle yes, of course, but I'm not sure it's worth the effort. Notice that we're already doing something similar for dispatch and draw operations. If we decide that we never want to execute and flush the command list until we need to get back data from the GPU, we'd have to track all the operations that are outstanding at any given moment so we can reply them when flushing as you propose. Or we'd have to keep many lists around and choose which one to fire and when, and possibly also add synchronization between them. My feeling is that both are overkill for a test program which we'd rather try to keep simple and explicit.
What I had in mind would be to set something like "runner->pending_list = GRAPHICS | COMPUTE;" at the end of things like d3d12_runner_dispatch()/d3d12_runner_draw()/d3d12_runner_get_resource_readback(), and then flush as appropriate; it wouldn't need to be terribly complicated.
But yes, keeping track of the resource state might ultimately be nicer.
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)