[PATCH v2 0/2] MR131: vkd3d: Persistently map host-visible heaps on creation.

[Zebediah Figura (＠zfigura)]

--
  v2: tests: Add a test for readback map stability.
      vkd3d: Persistently map host-visible heaps on creation.
 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131

[PATCH v2 1/2] vkd3d: Persistently map host-visible heaps on creation.

[Philip Rebohle]

From: Philip Rebohle <philip.rebohle(a)tu-dortmund.de>

--

Based on 6643ab45b5d3fa6ec124dc6a04c8b61fc03118ae from vkd3d-proton, but
modified somewhat.
---
 libs/vkd3d/resource.c | 214 ++++++++++++++++++------------------------
 1 file changed, 92 insertions(+), 122 deletions(-)

diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c
index 8c050cfe..2faa717f 100644
--- a/libs/vkd3d/resource.c
+++ b/libs/vkd3d/resource.c
@@ -326,6 +326,9 @@ static void d3d12_heap_destroy(struct d3d12_heap *heap)
 
     vkd3d_private_store_destroy(&heap->private_store);
 
+    if (heap->map_ptr)
+        VK_CALL(vkUnmapMemory(device->vk_device, heap->vk_memory));
+
     VK_CALL(vkFreeMemory(device->vk_device, heap->vk_memory, NULL));
 
     vkd3d_mutex_destroy(&heap->mutex);
@@ -437,97 +440,6 @@ struct d3d12_heap *unsafe_impl_from_ID3D12Heap(ID3D12Heap *iface)
     return impl_from_ID3D12Heap(iface);
 }
 
-static HRESULT d3d12_heap_map(struct d3d12_heap *heap, uint64_t offset,
-        struct d3d12_resource *resource, void **data)
-{
-    struct d3d12_device *device = heap->device;
-    HRESULT hr = S_OK;
-    VkResult vr;
-
-    vkd3d_mutex_lock(&heap->mutex);
-
-    assert(!resource->map_count || heap->map_ptr);
-
-    if (!resource->map_count)
-    {
-        if (!heap->map_ptr)
-        {
-            const struct vkd3d_vk_device_procs *vk_procs = &device->vk_procs;
-
-            TRACE("Mapping heap %p.\n", heap);
-
-            assert(!heap->map_count);
-
-            if ((vr = VK_CALL(vkMapMemory(device->vk_device, heap->vk_memory,
-                    0, VK_WHOLE_SIZE, 0, &heap->map_ptr))) < 0)
-            {
-                WARN("Failed to map device memory, vr %d.\n", vr);
-                heap->map_ptr = NULL;
-            }
-
-            hr = hresult_from_vk_result(vr);
-        }
-
-        if (heap->map_ptr)
-            ++heap->map_count;
-    }
-
-    if (hr == S_OK)
-    {
-        assert(heap->map_ptr);
-        if (data)
-            *data = (BYTE *)heap->map_ptr + offset;
-        ++resource->map_count;
-    }
-    else
-    {
-        assert(!heap->map_ptr);
-        if (data)
-            *data = NULL;
-    }
-
-    vkd3d_mutex_unlock(&heap->mutex);
-
-    return hr;
-}
-
-static void d3d12_heap_unmap(struct d3d12_heap *heap, struct d3d12_resource *resource)
-{
-    struct d3d12_device *device = heap->device;
-
-    vkd3d_mutex_lock(&heap->mutex);
-
-    if (!resource->map_count)
-    {
-        WARN("Resource %p is not mapped.\n", resource);
-        goto done;
-    }
-
-    --resource->map_count;
-    if (resource->map_count)
-        goto done;
-
-    if (!heap->map_count)
-    {
-        ERR("Heap %p is not mapped.\n", heap);
-        goto done;
-    }
-
-    --heap->map_count;
-    if (!heap->map_count)
-    {
-        const struct vkd3d_vk_device_procs *vk_procs = &device->vk_procs;
-
-        TRACE("Unmapping heap %p, ptr %p.\n", heap, heap->map_ptr);
-
-        VK_CALL(vkUnmapMemory(device->vk_device, heap->vk_memory));
-        heap->map_ptr = NULL;
-    }
-
-done:
-    vkd3d_mutex_unlock(&heap->mutex);
-}
-
 static HRESULT validate_heap_desc(const D3D12_HEAP_DESC *desc, const struct d3d12_resource *resource)
 {
     if (!resource && !desc->SizeInBytes)
@@ -552,11 +464,18 @@ static HRESULT validate_heap_desc(const D3D12_HEAP_DESC *desc, const struct d3d1
     return S_OK;
 }
 
+static VkMemoryPropertyFlags d3d12_heap_get_memory_property_flags(const struct d3d12_heap *heap)
+{
+    return heap->device->memory_properties.memoryTypes[heap->vk_memory_type].propertyFlags;
+}
+
 static HRESULT d3d12_heap_init(struct d3d12_heap *heap,
         struct d3d12_device *device, const D3D12_HEAP_DESC *desc, const struct d3d12_resource *resource)
 {
+    const struct vkd3d_vk_device_procs *vk_procs = &device->vk_procs;
     VkMemoryRequirements memory_requirements;
     VkDeviceSize vk_memory_size;
+    VkResult vr;
     HRESULT hr;
 
     heap->ID3D12Heap_iface.lpVtbl = &d3d12_heap_vtbl;
@@ -629,6 +548,18 @@ static HRESULT d3d12_heap_init(struct d3d12_heap *heap,
     if (!heap->is_private)
         d3d12_device_add_ref(heap->device);
 
+    if (d3d12_heap_get_memory_property_flags(heap) & VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT)
+    {
+        if ((vr = VK_CALL(vkMapMemory(device->vk_device,
+                heap->vk_memory, 0, VK_WHOLE_SIZE, 0, &heap->map_ptr))) < 0)
+        {
+            heap->map_ptr = NULL;
+            ERR("Failed to map memory, vr %d.\n", vr);
+            d3d12_heap_destroy(heap);
+            return hresult_from_vk_result(hr);
+        }
+    }
+
     return S_OK;
 }
 
@@ -1223,12 +1154,55 @@ static HRESULT STDMETHODCALLTYPE d3d12_resource_GetDevice(ID3D12Resource *iface,
     return d3d12_device_query_interface(resource->device, iid, device);
 }
 
+static void *d3d12_resource_get_map_ptr(struct d3d12_resource *resource)
+{
+    assert(resource->heap->map_ptr);
+    return (uint8_t *)resource->heap->map_ptr + resource->heap_offset;
+}
+
+static void d3d12_resource_get_vk_range(struct d3d12_resource *resource,
+        uint64_t offset, uint64_t size, VkMappedMemoryRange *vk_range)
+{
+    vk_range->sType = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE;
+    vk_range->pNext = NULL;
+    vk_range->memory = resource->heap->vk_memory;
+    vk_range->offset = resource->heap_offset + offset;
+    vk_range->size = size;
+}
+
+static void d3d12_resource_invalidate(struct d3d12_resource *resource, uint64_t offset, uint64_t size)
+{
+    const struct vkd3d_vk_device_procs *vk_procs = &resource->device->vk_procs;
+    VkMappedMemoryRange vk_range;
+    VkResult vr;
+
+    if (d3d12_heap_get_memory_property_flags(resource->heap) & VK_MEMORY_PROPERTY_HOST_COHERENT_BIT)
+        return;
+
+    d3d12_resource_get_vk_range(resource, offset, size, &vk_range);
+    if ((vr = VK_CALL(vkInvalidateMappedMemoryRanges(resource->device->vk_device, 1, &vk_range))) < 0)
+        ERR("Failed to invalidate memory, vr %d.\n", vr);
+}
+
+static void d3d12_resource_flush(struct d3d12_resource *resource, uint64_t offset, uint64_t size)
+{
+    const struct vkd3d_vk_device_procs *vk_procs = &resource->device->vk_procs;
+    VkMappedMemoryRange vk_range;
+    VkResult vr;
+
+    if (d3d12_heap_get_memory_property_flags(resource->heap) & VK_MEMORY_PROPERTY_HOST_COHERENT_BIT)
+        return;
+
+    d3d12_resource_get_vk_range(resource, offset, size, &vk_range);
+    if ((vr = VK_CALL(vkFlushMappedMemoryRanges(resource->device->vk_device, 1, &vk_range))) < 0)
+        ERR("Failed to invalidate memory, vr %d.\n", vr);
+}
+
 static HRESULT STDMETHODCALLTYPE d3d12_resource_Map(ID3D12Resource *iface, UINT sub_resource,
         const D3D12_RANGE *read_range, void **data)
 {
     struct d3d12_resource *resource = impl_from_ID3D12Resource(iface);
     unsigned int sub_resource_count;
-    HRESULT hr;
 
     TRACE("iface %p, sub_resource %u, read_range %p, data %p.\n",
             iface, sub_resource, read_range, data);
@@ -1259,15 +1233,18 @@ static HRESULT STDMETHODCALLTYPE d3d12_resource_Map(ID3D12Resource *iface, UINT
         return E_NOTIMPL;
     }
 
-    WARN("Ignoring read range %p.\n", read_range);
-
-    if (FAILED(hr = d3d12_heap_map(resource->heap, resource->heap_offset, resource, data)))
-        WARN("Failed to map resource %p, hr %#x.\n", resource, hr);
-
     if (data)
+    {
+        *data = d3d12_resource_get_map_ptr(resource);
         TRACE("Returning pointer %p.\n", *data);
+    }
 
-    return hr;
+    if (!read_range)
+        d3d12_resource_invalidate(resource, 0, resource->desc.Width);
+    else if (read_range->End > read_range->Begin)
+        d3d12_resource_invalidate(resource, read_range->Begin, read_range->End - read_range->Begin);
+
+    return S_OK;
 }
 
 static void STDMETHODCALLTYPE d3d12_resource_Unmap(ID3D12Resource *iface, UINT sub_resource,
@@ -1286,9 +1263,10 @@ static void STDMETHODCALLTYPE d3d12_resource_Unmap(ID3D12Resource *iface, UINT s
         return;
     }
 
-    WARN("Ignoring written range %p.\n", written_range);
-
-    d3d12_heap_unmap(resource->heap, resource);
+    if (!written_range)
+        d3d12_resource_flush(resource, 0, resource->desc.Width);
+    else if (written_range->End > written_range->Begin)
+        d3d12_resource_flush(resource, written_range->Begin, written_range->End - written_range->Begin);
 }
 
 static D3D12_RESOURCE_DESC * STDMETHODCALLTYPE d3d12_resource_GetDesc(ID3D12Resource *iface,
@@ -1320,10 +1298,10 @@ static HRESULT STDMETHODCALLTYPE d3d12_resource_WriteToSubresource(ID3D12Resourc
     VkImageSubresource vk_sub_resource;
     const struct vkd3d_format *format;
     VkSubresourceLayout vk_layout;
+    uint64_t dst_offset, dst_size;
     struct d3d12_device *device;
     uint8_t *dst_data;
     D3D12_BOX box;
-    HRESULT hr;
 
     TRACE("iface %p, src_data %p, src_row_pitch %u, src_slice_pitch %u, "
             "dst_sub_resource %u, dst_box %s.\n",
@@ -1381,20 +1359,16 @@ static HRESULT STDMETHODCALLTYPE d3d12_resource_WriteToSubresource(ID3D12Resourc
     TRACE("Offset %#"PRIx64", size %#"PRIx64", row pitch %#"PRIx64", depth pitch %#"PRIx64".\n",
             vk_layout.offset, vk_layout.size, vk_layout.rowPitch, vk_layout.depthPitch);
 
-    if (FAILED(hr = d3d12_heap_map(resource->heap, resource->heap_offset, resource, (void **)&dst_data)))
-    {
-        WARN("Failed to map resource %p, hr %#x.\n", resource, hr);
-        return hr;
-    }
-
-    dst_data += vk_layout.offset + vkd3d_format_get_data_offset(format, vk_layout.rowPitch,
+    dst_data = d3d12_resource_get_map_ptr(resource);
+    dst_offset = vk_layout.offset + vkd3d_format_get_data_offset(format, vk_layout.rowPitch,
             vk_layout.depthPitch, dst_box->left, dst_box->top, dst_box->front);
+    dst_size = vk_layout.depthPitch * (dst_box->back - dst_box->front);
 
     vkd3d_format_copy_data(format, src_data, src_row_pitch, src_slice_pitch,
-            dst_data, vk_layout.rowPitch, vk_layout.depthPitch, dst_box->right - dst_box->left,
+            dst_data + dst_offset, vk_layout.rowPitch, vk_layout.depthPitch, dst_box->right - dst_box->left,
             dst_box->bottom - dst_box->top, dst_box->back - dst_box->front);
 
-    d3d12_heap_unmap(resource->heap, resource);
+    d3d12_resource_flush(resource, dst_offset, dst_size);
 
     return S_OK;
 }
@@ -1408,10 +1382,10 @@ static HRESULT STDMETHODCALLTYPE d3d12_resource_ReadFromSubresource(ID3D12Resour
     VkImageSubresource vk_sub_resource;
     const struct vkd3d_format *format;
     VkSubresourceLayout vk_layout;
+    uint64_t src_offset, src_size;
     struct d3d12_device *device;
     uint8_t *src_data;
     D3D12_BOX box;
-    HRESULT hr;
 
     TRACE("iface %p, dst_data %p, dst_row_pitch %u, dst_slice_pitch %u, "
             "src_sub_resource %u, src_box %s.\n",
@@ -1469,21 +1443,17 @@ static HRESULT STDMETHODCALLTYPE d3d12_resource_ReadFromSubresource(ID3D12Resour
     TRACE("Offset %#"PRIx64", size %#"PRIx64", row pitch %#"PRIx64", depth pitch %#"PRIx64".\n",
             vk_layout.offset, vk_layout.size, vk_layout.rowPitch, vk_layout.depthPitch);
 
-    if (FAILED(hr = d3d12_heap_map(resource->heap, resource->heap_offset, resource, (void **)&src_data)))
-    {
-        WARN("Failed to map resource %p, hr %#x.\n", resource, hr);
-        return hr;
-    }
-
-    src_data += vk_layout.offset + vkd3d_format_get_data_offset(format, vk_layout.rowPitch,
+    src_data = d3d12_resource_get_map_ptr(resource);
+    src_offset = vk_layout.offset + vkd3d_format_get_data_offset(format, vk_layout.rowPitch,
             vk_layout.depthPitch, src_box->left, src_box->top, src_box->front);
+    src_size = vk_layout.depthPitch * (src_box->back - src_box->front);
+
+    d3d12_resource_invalidate(resource, src_offset, src_size);
 
-    vkd3d_format_copy_data(format, src_data, vk_layout.rowPitch, vk_layout.depthPitch,
+    vkd3d_format_copy_data(format, src_data + src_offset, vk_layout.rowPitch, vk_layout.depthPitch,
             dst_data, dst_row_pitch, dst_slice_pitch, src_box->right - src_box->left,
             src_box->bottom - src_box->top, src_box->back - src_box->front);
 
-    d3d12_heap_unmap(resource->heap, resource);
-
     return S_OK;
 }
 
-- 
GitLab


https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131

[PATCH v2 2/2] tests: Add a test for readback map stability.

[Zebediah Figura]

From: Zebediah Figura <zfigura(a)codeweavers.com>

Resident Evil 2 accesses a resource immediately after unmapping it.
---
 tests/d3d12.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/tests/d3d12.c b/tests/d3d12.c
index 405c9419..79deeb2f 100644
--- a/tests/d3d12.c
+++ b/tests/d3d12.c
@@ -36106,6 +36106,105 @@ static void test_clock_calibration(void)
     destroy_test_context(&context);
 }
 
+static void test_readback_map_stability(void)
+{
+    D3D12_TEXTURE_COPY_LOCATION dst_location, src_location;
+    ID3D12GraphicsCommandList *command_list;
+    unsigned int width, height, row_pitch;
+    D3D12_RESOURCE_DESC resource_desc;
+    struct test_context context;
+    ID3D12CommandQueue *queue;
+    ID3D12Resource *buffer;
+    D3D12_RANGE read_range;
+    ID3D12Device *device;
+    void *data, *data2;
+    uint32_t colour;
+    HRESULT hr;
+
+    static const float green[] = {0.0f, 1.0f, 0.0f, 1.0f};
+    static const float blue[] = {0.0f, 0.0f, 1.0f, 1.0f};
+
+    if (!init_test_context(&context, NULL))
+        return;
+    device = context.device;
+    queue = context.queue;
+    command_list = context.list;
+
+    resource_desc = ID3D12Resource_GetDesc(context.render_target);
+    width = align(resource_desc.Width, format_block_width(resource_desc.Format));
+    height = align(resource_desc.Height, format_block_height(resource_desc.Format));
+    row_pitch = align(width * format_size(resource_desc.Format), D3D12_TEXTURE_DATA_PITCH_ALIGNMENT);
+    buffer = create_readback_buffer(device, row_pitch * height);
+
+    dst_location.pResource = buffer;
+    dst_location.Type = D3D12_TEXTURE_COPY_TYPE_PLACED_FOOTPRINT;
+    dst_location.PlacedFootprint.Offset = 0;
+    dst_location.PlacedFootprint.Footprint.Format = resource_desc.Format;
+    dst_location.PlacedFootprint.Footprint.Width = width;
+    dst_location.PlacedFootprint.Footprint.Height = height;
+    dst_location.PlacedFootprint.Footprint.Depth = 1;
+    dst_location.PlacedFootprint.Footprint.RowPitch = row_pitch;
+
+    src_location.pResource = context.render_target;
+    src_location.Type = D3D12_TEXTURE_COPY_TYPE_SUBRESOURCE_INDEX;
+    src_location.SubresourceIndex = 0;
+
+    read_range.Begin = 0;
+    read_range.End = row_pitch * height;
+
+    ID3D12GraphicsCommandList_ClearRenderTargetView(context.list, context.rtv, green, 0, NULL);
+    transition_resource_state(command_list, context.render_target,
+            D3D12_RESOURCE_STATE_RENDER_TARGET, D3D12_RESOURCE_STATE_COPY_SOURCE);
+
+    ID3D12GraphicsCommandList_CopyTextureRegion(command_list, &dst_location, 0, 0, 0, &src_location, NULL);
+
+    hr = ID3D12GraphicsCommandList_Close(command_list);
+    assert_that(hr == S_OK, "Failed to close command list, hr %#x.\n", hr);
+    exec_command_list(queue, command_list);
+    wait_queue_idle(device, queue);
+
+    hr = ID3D12Resource_Map(buffer, 0, &read_range, &data);
+    assert_that(hr == S_OK, "Failed to map readback buffer, hr %#x.\n", hr);
+
+    colour = *(uint32_t *)data;
+    ok(colour == 0xff00ff00, "Got colour %08x.\n", colour);
+
+    ID3D12Resource_Unmap(buffer, 0, NULL);
+
+    colour = *(uint32_t *)data;
+    ok(colour == 0xff00ff00, "Got colour %08x.\n", colour);
+
+    reset_command_list(command_list, context.allocator);
+    transition_resource_state(command_list, context.render_target,
+            D3D12_RESOURCE_STATE_COPY_SOURCE, D3D12_RESOURCE_STATE_RENDER_TARGET);
+
+    ID3D12GraphicsCommandList_ClearRenderTargetView(context.list, context.rtv, blue, 0, NULL);
+    transition_resource_state(command_list, context.render_target,
+            D3D12_RESOURCE_STATE_RENDER_TARGET, D3D12_RESOURCE_STATE_COPY_SOURCE);
+
+    ID3D12GraphicsCommandList_CopyTextureRegion(command_list, &dst_location, 0, 0, 0, &src_location, NULL);
+
+    hr = ID3D12GraphicsCommandList_Close(command_list);
+    assert_that(hr == S_OK, "Failed to close command list, hr %#x.\n", hr);
+    exec_command_list(queue, command_list);
+    wait_queue_idle(device, queue);
+
+    colour = *(uint32_t *)data;
+    ok(colour == 0xffff0000, "Got colour %08x.\n", colour);
+
+    hr = ID3D12Resource_Map(buffer, 0, &read_range, &data2);
+    assert_that(hr == S_OK, "Failed to map readback buffer, hr %#x.\n", hr);
+
+    ok(data2 == data, "Expected map pointer to be stable.\n");
+    colour = *(uint32_t *)data2;
+    ok(colour == 0xffff0000, "Got colour %08x.\n", colour);
+
+    ID3D12Resource_Unmap(buffer, 0, NULL);
+
+    ID3D12Resource_Release(buffer);
+    destroy_test_context(&context);
+}
+
 START_TEST(d3d12)
 {
     parse_args(argc, argv);
@@ -36283,4 +36382,5 @@ START_TEST(d3d12)
     run_test(test_unbounded_resource_arrays);
     run_test(test_unbounded_samplers);
     run_test(test_clock_calibration);
+    run_test(test_readback_map_stability);
 }
-- 
GitLab

https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131

[Zebediah Figura (＠zfigura)]

On Fri Mar 24 21:54:53 2023 +0000, Zebediah Figura wrote:
> changed this line in [version 2 of the diff](/wine/vkd3d/-/merge_requests/131/diffs?diff_id=38981&start_sha=698ae1e710d2346a563ff4952d14f4c20681bbbc#1de6db547ff05a31fe39212d8871a474ab65dcba_1238_1242)
> Ah, indeed. Fixed in v2.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131#note_27846

[Conor McCarthy (＠cmccarthy)]

Conor McCarthy (@cmccarthy) commented about libs/vkd3d/resource.c:
> TRACE("Offset %#"PRIx64", size %#"PRIx64", row pitch %#"PRIx64", depth pitch %#"PRIx64".\n",
>              vk_layout.offset, vk_layout.size, vk_layout.rowPitch, vk_layout.depthPitch);
>
> -    if (FAILED(hr = d3d12_heap_map(resource->heap, resource->heap_offset, resource, (void **)&dst_data)))
> -    {
> -        WARN("Failed to map resource %p, hr %#x.\n", resource, hr);
> -        return hr;
> -    }
> -
> -    dst_data += vk_layout.offset + vkd3d_format_get_data_offset(format, vk_layout.rowPitch,
> +    dst_data = d3d12_resource_get_map_ptr(resource);
> +    dst_offset = vk_layout.offset + vkd3d_format_get_data_offset(format, vk_layout.rowPitch,
>              vk_layout.depthPitch, dst_box->left, dst_box->top, dst_box->front);
> +    dst_size = vk_layout.depthPitch * (dst_box->back - dst_box->front);

This can be beyond the end, for example for a 2D resource it's the entire resource size. Should it be:

`dst_size = vk_layout.offset + vkd3d_format_get_data_offset(format, vk_layout.rowPitch, vk_layout.depthPitch, dst_box->right, dst_box->bottom, dst_box->back) - dst_offset`? Ditto for `src_size` in `ReadFromSubresource()`.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131#note_27951