[PATCH 1/2] vkd3d: Implement ID3D12Resource::ReadFromSubresource().
Signed-off-by: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d/command.c | 2 +- libs/vkd3d/resource.c | 93 ++++++++++++++++++++++++++++++++++++-- libs/vkd3d/vkd3d_private.h | 2 + 3 files changed, 93 insertions(+), 4 deletions(-) diff --git a/libs/vkd3d/command.c b/libs/vkd3d/command.c index bbd398c..16613e5 100644 --- a/libs/vkd3d/command.c +++ b/libs/vkd3d/command.c @@ -3126,7 +3126,7 @@ static void STDMETHODCALLTYPE d3d12_command_list_CopyBufferRegion(ID3D12Graphics src_resource->u.vk_buffer, dst_resource->u.vk_buffer, 1, &buffer_copy)); } -static void vk_image_subresource_layers_from_d3d12(VkImageSubresourceLayers *subresource, +void vk_image_subresource_layers_from_d3d12(VkImageSubresourceLayers *subresource, const struct vkd3d_format *format, unsigned int sub_resource_idx, unsigned int miplevel_count) { subresource->aspectMask = format->vk_aspect_mask; diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c index e0e1aad..35290f3 100644 --- a/libs/vkd3d/resource.c +++ b/libs/vkd3d/resource.c @@ -1205,12 +1205,99 @@ static HRESULT STDMETHODCALLTYPE d3d12_resource_ReadFromSubresource(ID3D12Resour void *dst_data, UINT dst_row_pitch, UINT dst_slice_pitch, UINT src_sub_resource, const D3D12_BOX *src_box) { - FIXME("iface %p, dst_data %p, dst_row_pitch %u, dst_slice_pitch %u, " - "src_sub_resource %u, src_box %p stub!\n", + struct d3d12_resource *resource = impl_from_ID3D12Resource(iface); + const struct vkd3d_vk_device_procs *vk_procs; + VkImageSubresourceLayers vk_sub_layers; + VkImageSubresource vk_sub_resource; + const struct vkd3d_format *format; + VkSubresourceLayout vk_layout; + struct d3d12_device *device; + void *src_map_ptr; + D3D12_BOX box; + HRESULT hr; + BYTE *dst; + UINT z; + + TRACE("iface %p, dst_data %p, dst_row_pitch %u, dst_slice_pitch %u, " + "src_sub_resource %u, src_box %p\n", iface, dst_data, dst_row_pitch, dst_slice_pitch, src_sub_resource, src_box); - return E_NOTIMPL; + if (src_box) + { + box = *src_box; + } + else + { + box.left = 0; + box.top = 0; + box.front = 0; + box.right = resource->desc.Width; + box.bottom = resource->desc.Height; + box.back = d3d12_resource_desc_get_depth(&resource->desc, 0); + } + if (box.right <= box.left || box.bottom <= box.top || box.back <= box.front) + return S_OK; + + if (d3d12_resource_is_buffer(resource)) + { + WARN("Buffers are not supported.\n"); + return E_INVALIDARG; + } + + if (!resource->heap) + { + FIXME("Not implemented for this resource type.\n"); + return E_NOTIMPL; + } + + device = resource->device; + vk_procs = &device->vk_procs; + + if (!(format = vkd3d_format_from_d3d12_resource_desc(device, &resource->desc, 0))) + { + WARN("Invalid DXGI format %#x.\n", resource->desc.Format); + return E_INVALIDARG; + } + + if (FAILED(hr = d3d12_heap_map(resource->heap, resource->heap_offset, resource, &src_map_ptr))) + { + WARN("Failed to map resource %p, hr %#x.\n", resource, hr); + return hr; + } + + if (resource->desc.Layout != D3D12_TEXTURE_LAYOUT_ROW_MAJOR) + FIXME_ONCE("Layouts other than D3D12_TEXTURE_LAYOUT_ROW_MAJOR are not supported and results are implementation-dependent.\n"); + + vk_image_subresource_layers_from_d3d12(&vk_sub_layers, format, src_sub_resource, resource->desc.MipLevels); + vk_sub_resource.arrayLayer = vk_sub_layers.baseArrayLayer; + vk_sub_resource.mipLevel = vk_sub_layers.mipLevel; + vk_sub_resource.aspectMask = vk_sub_layers.aspectMask; + + VK_CALL(vkGetImageSubresourceLayout(device->vk_device, resource->u.vk_image, &vk_sub_resource, &vk_layout)); + TRACE("offset %#"PRIx64", size %#"PRIx64", rowPitch %#"PRIx64", arrayPitch %#"PRIx64", depthPitch %#"PRIx64".\n", + vk_layout.offset, vk_layout.size, vk_layout.rowPitch, vk_layout.arrayPitch, vk_layout.depthPitch); + + src_map_ptr = (BYTE*)src_map_ptr + vk_layout.offset; + for (z = box.front; z < box.back; ++z) + { + UINT y; + dst = dst_data + (z - box.front) * dst_slice_pitch; + for (y = box.top; y < box.bottom; y += format->block_height) + { + SIZE_T size = (box.right - box.left) / format->block_width + * format->byte_count * format->block_byte_count; + const BYTE *src = (BYTE*)src_map_ptr + z * vk_layout.depthPitch + + y / format->block_height * vk_layout.rowPitch + + box.left / format->block_width * format->byte_count * format->block_byte_count; + memcpy(dst, src, size); + dst += dst_row_pitch; + } + } + + d3d12_heap_unmap(resource->heap, resource); + + return S_OK; } static HRESULT STDMETHODCALLTYPE d3d12_resource_GetHeapProperties(ID3D12Resource *iface, diff --git a/libs/vkd3d/vkd3d_private.h b/libs/vkd3d/vkd3d_private.h index 82d56c2..cd3f904 100644 --- a/libs/vkd3d/vkd3d_private.h +++ b/libs/vkd3d/vkd3d_private.h @@ -1176,6 +1176,8 @@ static inline unsigned int d3d12_resource_desc_get_sub_resource_count(const D3D1 } VkCompareOp vk_compare_op_from_d3d12(D3D12_COMPARISON_FUNC op) DECLSPEC_HIDDEN; +void vk_image_subresource_layers_from_d3d12(VkImageSubresourceLayers *subresource, + const struct vkd3d_format *format, unsigned int sub_resource_idx, unsigned int miplevel_count) DECLSPEC_HIDDEN; VkSampleCountFlagBits vk_samples_from_dxgi_sample_desc(const DXGI_SAMPLE_DESC *desc) DECLSPEC_HIDDEN; VkSampleCountFlagBits vk_samples_from_sample_count(unsigned int sample_count) DECLSPEC_HIDDEN; -- 2.22.0
Signed-off-by: Conor McCarthy <cmccarthy(a)codeweavers.com> --- tests/d3d12.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/tests/d3d12.c b/tests/d3d12.c index 6cee005..7308525 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -30119,7 +30119,7 @@ static void test_read_write_subresource(void) todo ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); hr = ID3D12Resource_ReadFromSubresource(rb_buffer, dst_buffer, row_pitch, slice_pitch, 0, &box); - todo ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); ID3D12Resource_Release(rb_buffer); @@ -30133,7 +30133,7 @@ static void test_read_write_subresource(void) resource_desc.Format = DXGI_FORMAT_R8G8B8A8_UNORM; resource_desc.SampleDesc.Count = 1; resource_desc.SampleDesc.Quality = 0; - resource_desc.Layout = D3D12_TEXTURE_LAYOUT_UNKNOWN; + resource_desc.Layout = D3D12_TEXTURE_LAYOUT_ROW_MAJOR; resource_desc.Flags = 0; memset(&heap_properties, 0, sizeof(heap_properties)); @@ -30153,7 +30153,7 @@ static void test_read_write_subresource(void) todo ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); hr = ID3D12Resource_ReadFromSubresource(src_texture, dst_buffer, row_pitch, slice_pitch, 0, NULL); - todo ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); /* Empty box */ set_box(&box, 0, 0, 0, 0, 0, 0); @@ -30161,7 +30161,7 @@ static void test_read_write_subresource(void) todo ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); hr = ID3D12Resource_ReadFromSubresource(src_texture, dst_buffer, row_pitch, slice_pitch, 0, &box); - todo ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); for (z = 0; z < 64; ++z) { @@ -30191,18 +30191,29 @@ static void test_read_write_subresource(void) row_pitch, slice_pitch); todo ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + /* WriteToSubresource() is not implemented so upload test data */ + transition_resource_state(command_list, src_texture, + D3D12_RESOURCE_STATE_COMMON, D3D12_RESOURCE_STATE_COPY_DEST); + texture_data.pData = dst_buffer; + texture_data.RowPitch = row_pitch; + texture_data.SlicePitch = slice_pitch; + upload_texture_data(src_texture, &texture_data, 1, queue, command_list); + reset_command_list(command_list, context.allocator); + transition_resource_state(command_list, src_texture, + D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_COMMON); + memset(dst_buffer, 0, buffer_size); /* Read region 1 */ set_box(&box, 0, 0, 0, 2, 2, 2); hr = ID3D12Resource_ReadFromSubresource(src_texture, dst_buffer, row_pitch, slice_pitch, 0, &box); - todo ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); /* Read region 2 */ set_box(&box, 2, 2, 2, 11, 13, 17); hr = ID3D12Resource_ReadFromSubresource(src_texture, &dst_buffer[2 * 128 * 100 + 2 * 128 + 2], row_pitch, slice_pitch, 0, &box); - todo ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); for (z = 0; z < 64; ++z) { @@ -30227,7 +30238,7 @@ static void test_read_write_subresource(void) if (got != expected) break; } - todo ok(got == expected, "Got unexpected value 0x%08x at (%u, %u, %u), expected 0x%08x.\n", got, x, y, z, expected); + ok(got == expected, "Got unexpected value 0x%08x at (%u, %u, %u), expected 0x%08x.\n", got, x, y, z, expected); /* Test layout is the same */ dst_texture = create_default_texture3d(device, 128, 100, 64, 1, DXGI_FORMAT_R8G8B8A8_UNORM, 0, -- 2.22.0
On Wed, Jul 31, 2019 at 1:24 PM Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
Signed-off-by: Conor McCarthy <cmccarthy(a)codeweavers.com> --- tests/d3d12.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/tests/d3d12.c b/tests/d3d12.c index 6cee005..7308525 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -30119,7 +30119,7 @@ static void test_read_write_subresource(void) todo ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr);
hr = ID3D12Resource_ReadFromSubresource(rb_buffer, dst_buffer, row_pitch, slice_pitch, 0, &box); - todo ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr);
todo() changes need to be made together with the implementation. The previous patch introduces test failures.
ID3D12Resource_Release(rb_buffer);
@@ -30133,7 +30133,7 @@ static void test_read_write_subresource(void) resource_desc.Format = DXGI_FORMAT_R8G8B8A8_UNORM; resource_desc.SampleDesc.Count = 1; resource_desc.SampleDesc.Quality = 0; - resource_desc.Layout = D3D12_TEXTURE_LAYOUT_UNKNOWN; + resource_desc.Layout = D3D12_TEXTURE_LAYOUT_ROW_MAJOR; resource_desc.Flags = 0;
I don't like this change. The main point of ReadFromSubresource() is to read textures with undefined layout on CPU. We should add additional test cases instead. Also, it should be possible to make ReadFromSubresource() work with D3D12_TEXTURE_LAYOUT_UNKNOWN in our implementations for some cases. We can try to use VK_IMAGE_TILING_LINEAR for textures with custom heaps. This isn't very nice but Vulkan doesn't allow us to read textures with optimal tiling. See also VK_IMAGE_TILING_LINEAR restrictions in the Vulkan spec.
+ /* WriteToSubresource() is not implemented so upload test data */ + transition_resource_state(command_list, src_texture, + D3D12_RESOURCE_STATE_COMMON, D3D12_RESOURCE_STATE_COPY_DEST); + texture_data.pData = dst_buffer; + texture_data.RowPitch = row_pitch; + texture_data.SlicePitch = slice_pitch; + upload_texture_data(src_texture, &texture_data, 1, queue, command_list); + reset_command_list(command_list, context.allocator); + transition_resource_state(command_list, src_texture, + D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_COMMON);
This makes a bunch of WriteToSubresource() and ID3D12Resource_ReadFromSubresource() tests much less interesting. Please try to extends tests instead and add this as a separate test case as suggested above.
Hi Józef, August 1, 2019 5:33 PM, "Józef Kucia" <joseph.kucia(a)gmail.com> wrote:
+ /* WriteToSubresource() is not implemented so upload test data */ + transition_resource_state(command_list, src_texture, + D3D12_RESOURCE_STATE_COMMON, D3D12_RESOURCE_STATE_COPY_DEST); + texture_data.pData = dst_buffer; + texture_data.RowPitch = row_pitch; + texture_data.SlicePitch = slice_pitch; + upload_texture_data(src_texture, &texture_data, 1, queue, command_list); + reset_command_list(command_list, context.allocator); + transition_resource_state(command_list, src_texture, + D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_COMMON);
This makes a bunch of WriteToSubresource() and ID3D12Resource_ReadFromSubresource() tests much less interesting. Please try to extends tests instead and add this as a separate test case as suggested above.
I'm not sure what you mean. The two ReadFromSubresource() tests which follow this do almost nothing useful without the test data. They return S_OK and junk data. This extra code can be deleted if/when WriteToSubresource() is implemented. Conor
On Thu, Aug 1, 2019 at 4:17 PM Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
Hi Józef,
August 1, 2019 5:33 PM, "Józef Kucia" <joseph.kucia(a)gmail.com> wrote:
+ /* WriteToSubresource() is not implemented so upload test data */ + transition_resource_state(command_list, src_texture, + D3D12_RESOURCE_STATE_COMMON, D3D12_RESOURCE_STATE_COPY_DEST); + texture_data.pData = dst_buffer; + texture_data.RowPitch = row_pitch; + texture_data.SlicePitch = slice_pitch; + upload_texture_data(src_texture, &texture_data, 1, queue, command_list); + reset_command_list(command_list, context.allocator); + transition_resource_state(command_list, src_texture, + D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_COMMON);
This makes a bunch of WriteToSubresource() and ID3D12Resource_ReadFromSubresource() tests much less interesting. Please try to extends tests instead and add this as a separate test case as suggested above.
I'm not sure what you mean. The two ReadFromSubresource() tests which follow this do almost nothing useful without the test data. They return S_OK and junk data. This extra code can be deleted if/when WriteToSubresource() is implemented.
The test are for WriteToSubresource() and ReadFromSubresource() interaction. They work as expected on Windows. After your change the test doesn't check the behavior of WriteToSubresource() anymore. If we are worried about junk data we can initialize the resource with data different than expected by the test. In other words, I would prefer to not add workarounds just so we can remove a todo. Keeping todo is perfectly fine if something is not implemented yet.
On Wed, Jul 31, 2019 at 1:24 PM Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c index e0e1aad..35290f3 100644 --- a/libs/vkd3d/resource.c +++ b/libs/vkd3d/resource.c @@ -1205,12 +1205,99 @@ static HRESULT STDMETHODCALLTYPE d3d12_resource_ReadFromSubresource(ID3D12Resour void *dst_data, UINT dst_row_pitch, UINT dst_slice_pitch, UINT src_sub_resource, const D3D12_BOX *src_box) { - FIXME("iface %p, dst_data %p, dst_row_pitch %u, dst_slice_pitch %u, " - "src_sub_resource %u, src_box %p stub!\n", + struct d3d12_resource *resource = impl_from_ID3D12Resource(iface); + const struct vkd3d_vk_device_procs *vk_procs; + VkImageSubresourceLayers vk_sub_layers; + VkImageSubresource vk_sub_resource; + const struct vkd3d_format *format; + VkSubresourceLayout vk_layout; + struct d3d12_device *device; + void *src_map_ptr; + D3D12_BOX box; + HRESULT hr; + BYTE *dst; + UINT z;
Please use "unsigned int" instead of "INT". We stay away from UINT, VOID and similar defines in d3d code. Prototypes and definitions of D3D API functions are an exception.
+ + TRACE("iface %p, dst_data %p, dst_row_pitch %u, dst_slice_pitch %u, " + "src_sub_resource %u, src_box %p\n",
Missing dot ad the end of TRACE() message.
+ if (src_box) + { + box = *src_box; + } + else + { + box.left = 0; + box.top = 0; + box.front = 0; + box.right = resource->desc.Width; + box.bottom = resource->desc.Height; + box.back = d3d12_resource_desc_get_depth(&resource->desc, 0);
We need to call d3d12_resource_desc_get_depth() with the correct mip-level index.
+ if (!resource->heap) + { + FIXME("Not implemented for this resource type.\n"); + return E_NOTIMPL; + }
We should probably use d3d12_resource_is_cpu_accessible() instead. See d3d12_resource_is_cpu_accessible() for reference.
+ if (resource->desc.Layout != D3D12_TEXTURE_LAYOUT_ROW_MAJOR) + FIXME_ONCE("Layouts other than D3D12_TEXTURE_LAYOUT_ROW_MAJOR are not supported and results are implementation-dependent.\n");
The function needs to exit at this point. It is invalid usage to call vkGetImageSubresourceLayout() for images with VK_IMAGE_TILING_OPTIMAL.
+ vk_image_subresource_layers_from_d3d12(&vk_sub_layers, format, src_sub_resource, resource->desc.MipLevels); + vk_sub_resource.arrayLayer = vk_sub_layers.baseArrayLayer; + vk_sub_resource.mipLevel = vk_sub_layers.mipLevel; + vk_sub_resource.aspectMask = vk_sub_layers.aspectMask;
It doesn't seem beneficial to reuse vk_image_subresource_layers_from_d3d12(). Please simply fill members of vk_sub_resource(): it's less code, it doesn't need a new prototype in the header, it's a bit awkward to fill other structure with vk_image_subresource_layers_from_d3d12() and then copy some fields to vk_sub_resource(). vk_sub_resource.aspectMask = format->vk_aspect_mask; vk_sub_resource.mipLevel = src_sub_resource % resource->desc.MipLevels; vk_sub_resource.baseArrayLayer = src_sub_resource / resource->desc.MipLevels;
+ + VK_CALL(vkGetImageSubresourceLayout(device->vk_device, resource->u.vk_image, &vk_sub_resource, &vk_layout)); + TRACE("offset %#"PRIx64", size %#"PRIx64", rowPitch %#"PRIx64", arrayPitch %#"PRIx64", depthPitch %#"PRIx64".\n", + vk_layout.offset, vk_layout.size, vk_layout.rowPitch, vk_layout.arrayPitch, vk_layout.depthPitch);
Please avoid camelCase in TRACE() messages: "Offset ..., size, row pitch ..., array pitch ..., depth pitch ..."
+ + src_map_ptr = (BYTE*)src_map_ptr + vk_layout.offset; + for (z = box.front; z < box.back; ++z) + { + UINT y;
unsigned int. It could also be moved to other declarations at the top of the function.
+ dst = dst_data + (z - box.front) * dst_slice_pitch;
Are you sure that src_box should be used to offset dst?
+ for (y = box.top; y < box.bottom; y += format->block_height) + { + SIZE_T size = (box.right - box.left) / format->block_width + * format->byte_count * format->block_byte_count;
size_t
participants (2)
-
Conor McCarthy -
Józef Kucia