[PATCH 1/2] vkd3d: Implement ID3D12Resource::ReadFromSubresource().

[Józef Kucia]

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