[PATCH v2 0/3] MR425: tests: Immediately transition resources after a resource readback.
The resource could be destructed before the command list left open is executed; instead, we immediately perform the transition. -- v2: tests: Immediately transition buffers after creation in the shader runner. tests: Immediately transition textures after creation in the shader runner. tests: Immediately transition resources after readback in the shader runner. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/425
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> The resource could be destructed before the command list left open is executed; instead, we immediately perform the transition. --- tests/d3d12_test_utils.h | 21 +++++++++++++++++++-- tests/shader_runner_d3d12.c | 8 ++------ 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/tests/d3d12_test_utils.h b/tests/d3d12_test_utils.h index 2c7b10f45..f0c4b4118 100644 --- a/tests/d3d12_test_utils.h +++ b/tests/d3d12_test_utils.h @@ -415,8 +415,11 @@ struct d3d12_resource_readback ID3D12Resource *resource; }; -static void get_resource_readback_with_command_list(ID3D12Resource *resource, unsigned int sub_resource, - struct d3d12_resource_readback *rb, ID3D12CommandQueue *queue, ID3D12GraphicsCommandList *command_list) +#define RESOURCE_STATE_DO_NOT_CHANGE (~0u) + +static void get_resource_readback_with_command_list_and_states(ID3D12Resource *resource, unsigned int sub_resource, + struct d3d12_resource_readback *rb, ID3D12CommandQueue *queue, ID3D12GraphicsCommandList *command_list, + D3D12_RESOURCE_STATES initial_state, D3D12_RESOURCE_STATES final_state) { D3D12_HEAP_PROPERTIES heap_properties; D3D12_RESOURCE_DESC resource_desc; @@ -444,6 +447,9 @@ static void get_resource_readback_with_command_list(ID3D12Resource *resource, un rb->rb.row_pitch = align(rb->rb.row_pitch, D3D12_TEXTURE_DATA_PITCH_ALIGNMENT); rb->rb.data = NULL; + if (initial_state != RESOURCE_STATE_DO_NOT_CHANGE) + transition_sub_resource_state(command_list, resource, sub_resource, initial_state, D3D12_RESOURCE_STATE_COPY_SOURCE); + src_resource = resource; if (resource_desc.Dimension != D3D12_RESOURCE_DIMENSION_BUFFER && resource_desc.SampleDesc.Count > 1) { @@ -493,6 +499,10 @@ static void get_resource_readback_with_command_list(ID3D12Resource *resource, un ID3D12GraphicsCommandList_CopyTextureRegion(command_list, &dst_location, 0, 0, 0, &src_location, NULL); } + + if (final_state != RESOURCE_STATE_DO_NOT_CHANGE) + transition_sub_resource_state(command_list, resource, sub_resource, D3D12_RESOURCE_STATE_COPY_SOURCE, final_state); + hr = ID3D12GraphicsCommandList_Close(command_list); assert_that(hr == S_OK, "Failed to close command list, hr %#x.\n", hr); @@ -509,6 +519,13 @@ static void get_resource_readback_with_command_list(ID3D12Resource *resource, un assert_that(hr == S_OK, "Failed to map readback buffer, hr %#x.\n", hr); } +static void get_resource_readback_with_command_list(ID3D12Resource *resource, unsigned int sub_resource, + struct d3d12_resource_readback *rb, ID3D12CommandQueue *queue, ID3D12GraphicsCommandList *command_list) +{ + return get_resource_readback_with_command_list_and_states(resource, sub_resource, rb, queue, command_list, + RESOURCE_STATE_DO_NOT_CHANGE, RESOURCE_STATE_DO_NOT_CHANGE); +} + static unsigned int get_readback_uint(struct resource_readback *rb, unsigned int x, unsigned int y, unsigned int z) { diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index 8453617ef..79d637e6b 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -546,13 +546,9 @@ static struct resource_readback *d3d12_runner_get_resource_readback(struct shade else state = D3D12_RESOURCE_STATE_UNORDERED_ACCESS; - transition_resource_state(test_context->list, resource->resource, - state, D3D12_RESOURCE_STATE_COPY_SOURCE); - get_resource_readback_with_command_list(resource->resource, 0, rb, - test_context->queue, test_context->list); + get_resource_readback_with_command_list_and_states(resource->resource, 0, rb, + test_context->queue, test_context->list, state, state); reset_command_list(test_context->list, test_context->allocator); - transition_resource_state(test_context->list, resource->resource, - D3D12_RESOURCE_STATE_COPY_SOURCE, state); return &rb->rb; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/425
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> The resource could be destructed before the command list left open is executed; instead, we immediately perform the transition. --- tests/d3d12_test_utils.h | 28 +++++++++++++++++++++++++--- tests/shader_runner_d3d12.c | 15 +++++++-------- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/tests/d3d12_test_utils.h b/tests/d3d12_test_utils.h index f0c4b4118..3c3e50ca0 100644 --- a/tests/d3d12_test_utils.h +++ b/tests/d3d12_test_utils.h @@ -671,10 +671,11 @@ static void copy_sub_resource_data(const D3D12_MEMCPY_DEST *dst, const D3D12_SUB } } -#define upload_texture_data(a, b, c, d, e) upload_texture_data_(__LINE__, a, b, c, d, e) -static inline void upload_texture_data_(unsigned int line, ID3D12Resource *texture, +#define upload_texture_data_with_states(a, b, c, d, e, f, g) upload_texture_data_with_states_(__LINE__, a, b, c, d, e, f, g) +static inline void upload_texture_data_with_states_(unsigned int line, ID3D12Resource *texture, const D3D12_SUBRESOURCE_DATA *data, unsigned int sub_resource_count, - ID3D12CommandQueue *queue, ID3D12GraphicsCommandList *command_list) + ID3D12CommandQueue *queue, ID3D12GraphicsCommandList *command_list, + D3D12_RESOURCE_STATES initial_state, D3D12_RESOURCE_STATES final_state) { D3D12_TEXTURE_COPY_LOCATION dst_location, src_location; D3D12_PLACED_SUBRESOURCE_FOOTPRINT *layouts; @@ -718,7 +719,13 @@ static inline void upload_texture_data_(unsigned int line, ID3D12Resource *textu if (resource_desc.Dimension == D3D12_RESOURCE_DIMENSION_BUFFER) { + if (initial_state != RESOURCE_STATE_DO_NOT_CHANGE) + transition_resource_state(command_list, texture, initial_state, D3D12_RESOURCE_STATE_COPY_DEST); + ID3D12GraphicsCommandList_CopyResource(command_list, texture, upload_buffer); + + if (final_state != RESOURCE_STATE_DO_NOT_CHANGE) + transition_resource_state(command_list, texture, D3D12_RESOURCE_STATE_COPY_DEST, final_state); } else { @@ -732,8 +739,14 @@ static inline void upload_texture_data_(unsigned int line, ID3D12Resource *textu src_location.Type = D3D12_TEXTURE_COPY_TYPE_PLACED_FOOTPRINT; src_location.PlacedFootprint = layouts[i]; + if (initial_state != RESOURCE_STATE_DO_NOT_CHANGE) + transition_sub_resource_state(command_list, texture, i, initial_state, D3D12_RESOURCE_STATE_COPY_DEST); + ID3D12GraphicsCommandList_CopyTextureRegion(command_list, &dst_location, 0, 0, 0, &src_location, NULL); + + if (final_state != RESOURCE_STATE_DO_NOT_CHANGE) + transition_sub_resource_state(command_list, texture, i, D3D12_RESOURCE_STATE_COPY_DEST, final_state); } } @@ -751,6 +764,15 @@ static inline void upload_texture_data_(unsigned int line, ID3D12Resource *textu free(row_sizes); } +#define upload_texture_data(a, b, c, d, e) upload_texture_data_(__LINE__, a, b, c, d, e) +static inline void upload_texture_data_(unsigned int line, ID3D12Resource *texture, + const D3D12_SUBRESOURCE_DATA *data, unsigned int sub_resource_count, + ID3D12CommandQueue *queue, ID3D12GraphicsCommandList *command_list) +{ + return upload_texture_data_with_states_(line, texture, data, sub_resource_count, queue, command_list, + RESOURCE_STATE_DO_NOT_CHANGE, RESOURCE_STATE_DO_NOT_CHANGE); +} + #define upload_buffer_data(a, b, c, d, e, f) upload_buffer_data_(__LINE__, a, b, c, d, e, f) static inline void upload_buffer_data_(unsigned int line, ID3D12Resource *buffer, size_t offset, size_t size, const void *data, ID3D12CommandQueue *queue, ID3D12GraphicsCommandList *command_list) diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index 79d637e6b..6c71b90f8 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -147,11 +147,11 @@ static struct resource *d3d12_runner_create_resource(struct shader_runner *r, co resource->resource = create_default_texture2d(device, params->width, params->height, 1, params->level_count, params->format, 0, D3D12_RESOURCE_STATE_COPY_DEST); - upload_texture_data(resource->resource, resource_data, - params->level_count, 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_DEST, + upload_texture_data_with_states(resource->resource, resource_data, + params->level_count, test_context->queue, test_context->list, + RESOURCE_STATE_DO_NOT_CHANGE, D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE | D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE); + reset_command_list(test_context->list, test_context->allocator); ID3D12Device_CreateShaderResourceView(device, resource->resource, NULL, get_cpu_descriptor_handle(test_context, runner->heap, resource->r.slot)); break; @@ -163,11 +163,10 @@ static struct resource *d3d12_runner_create_resource(struct shader_runner *r, co resource->resource = create_default_texture2d(device, params->width, params->height, 1, params->level_count, params->format, D3D12_RESOURCE_FLAG_ALLOW_UNORDERED_ACCESS, D3D12_RESOURCE_STATE_COPY_DEST); - upload_texture_data(resource->resource, resource_data, - params->level_count, test_context->queue, test_context->list); + upload_texture_data_with_states(resource->resource, resource_data, + params->level_count, test_context->queue, test_context->list, + RESOURCE_STATE_DO_NOT_CHANGE, D3D12_RESOURCE_STATE_UNORDERED_ACCESS); reset_command_list(test_context->list, test_context->allocator); - transition_resource_state(test_context->list, resource->resource, - D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_UNORDERED_ACCESS); ID3D12Device_CreateUnorderedAccessView(device, resource->resource, NULL, NULL, get_cpu_descriptor_handle(test_context, runner->heap, resource->r.slot + MAX_RESOURCES)); break; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/425
From: Giovanni Mascellani <gmascellani(a)codeweavers.com> The resource could be destructed before the command list left open is executed; instead, we immediately perform the transition. --- tests/d3d12_test_utils.h | 21 ++++++++++++++++++--- tests/shader_runner_d3d12.c | 7 +++---- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/tests/d3d12_test_utils.h b/tests/d3d12_test_utils.h index 3c3e50ca0..7473bbc43 100644 --- a/tests/d3d12_test_utils.h +++ b/tests/d3d12_test_utils.h @@ -773,9 +773,10 @@ static inline void upload_texture_data_(unsigned int line, ID3D12Resource *textu RESOURCE_STATE_DO_NOT_CHANGE, RESOURCE_STATE_DO_NOT_CHANGE); } -#define upload_buffer_data(a, b, c, d, e, f) upload_buffer_data_(__LINE__, a, b, c, d, e, f) -static inline void upload_buffer_data_(unsigned int line, ID3D12Resource *buffer, size_t offset, - size_t size, const void *data, ID3D12CommandQueue *queue, ID3D12GraphicsCommandList *command_list) +#define upload_buffer_data_with_states(a, b, c, d, e, f, g, h) upload_buffer_data_with_states_(__LINE__, a, b, c, d, e, f, g, h) +static inline void upload_buffer_data_with_states_(unsigned int line, ID3D12Resource *buffer, size_t offset, + size_t size, const void *data, ID3D12CommandQueue *queue, ID3D12GraphicsCommandList *command_list, + D3D12_RESOURCE_STATES initial_state, D3D12_RESOURCE_STATES final_state) { ID3D12Resource *upload_buffer; ID3D12Device *device; @@ -786,9 +787,15 @@ static inline void upload_buffer_data_(unsigned int line, ID3D12Resource *buffer upload_buffer = create_upload_buffer_(line, device, size, data); + if (initial_state != RESOURCE_STATE_DO_NOT_CHANGE) + transition_resource_state(command_list, buffer, initial_state, D3D12_RESOURCE_STATE_COPY_DEST); + ID3D12GraphicsCommandList_CopyBufferRegion(command_list, buffer, offset, upload_buffer, 0, size); + if (final_state != RESOURCE_STATE_DO_NOT_CHANGE) + transition_resource_state(command_list, buffer, D3D12_RESOURCE_STATE_COPY_DEST, final_state); + hr = ID3D12GraphicsCommandList_Close(command_list); ok_(line)(SUCCEEDED(hr), "Failed to close command list, hr %#x.\n", hr); exec_command_list(queue, command_list); @@ -798,6 +805,14 @@ static inline void upload_buffer_data_(unsigned int line, ID3D12Resource *buffer ID3D12Device_Release(device); } +#define upload_buffer_data(a, b, c, d, e, f) upload_buffer_data_(__LINE__, a, b, c, d, e, f) +static inline void upload_buffer_data_(unsigned int line, ID3D12Resource *buffer, size_t offset, + size_t size, const void *data, ID3D12CommandQueue *queue, ID3D12GraphicsCommandList *command_list) +{ + return upload_buffer_data_with_states_(line, buffer, offset, size, data, queue, command_list, + RESOURCE_STATE_DO_NOT_CHANGE, RESOURCE_STATE_DO_NOT_CHANGE); +} + static HRESULT create_root_signature(ID3D12Device *device, const D3D12_ROOT_SIGNATURE_DESC *desc, ID3D12RootSignature **root_signature) { diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index 6c71b90f8..a05dfd059 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -181,11 +181,10 @@ static struct resource *d3d12_runner_create_resource(struct shader_runner *r, co resource->resource = create_default_buffer(device, params->data_size, D3D12_RESOURCE_FLAG_ALLOW_UNORDERED_ACCESS, D3D12_RESOURCE_STATE_COPY_DEST); - upload_buffer_data(resource->resource, 0, params->data_size, resource_data, - test_context->queue, test_context->list); + upload_buffer_data_with_states(resource->resource, 0, params->data_size, resource_data, + test_context->queue, test_context->list, + RESOURCE_STATE_DO_NOT_CHANGE, D3D12_RESOURCE_STATE_UNORDERED_ACCESS); reset_command_list(test_context->list, test_context->allocator); - transition_resource_state(test_context->list, resource->resource, - D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_UNORDERED_ACCESS); uav_desc.Format = params->format; uav_desc.ViewDimension = D3D12_UAV_DIMENSION_BUFFER; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/425
But yes, keeping track of the resource state might ultimately be nicer.
Not sure if what I did is what you intended: what do you think of the version I just pushed? In the future more users could switch to the new `*_with_states()` functions and spare some boilerplate. In theory with this solution it is possible that some GPU time is wasted transitioning resources that will never be used again, but that looks like something we can afford. At least no new synchronization points should be added, I think. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/425#note_50308
But yes, keeping track of the resource state might ultimately be nicer.
Not sure if what I did is what you intended: what do you think of the version I just pushed?
I think that works, yeah. An alternative could be to store the "current" resource state in struct d3d12_resource, and then transition to the desired state as needed before draws, dispatches, readbacks, etc. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/425#note_50527
This merge request was approved by Henri Verbeet. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/425
participants (3)
-
Giovanni Mascellani -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)