On Wed, Jul 31, 2019 at 1:24 PM Conor McCarthy cmccarthy@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