[PATCH v4 0/2] MR131: vkd3d: Persistently map host-visible heaps on creation.
-- v4: 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
From: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> -- Based on 6643ab45b5d3fa6ec124dc6a04c8b61fc03118ae from vkd3d-proton, but modified somewhat. --- libs/vkd3d/resource.c | 216 ++++++++++++++++++------------------------ 1 file changed, 94 insertions(+), 122 deletions(-) diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c index 8c050cfe..1a3c59a0 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 flush 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,17 @@ 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.offset + vkd3d_format_get_data_offset(format, vk_layout.rowPitch, + vk_layout.depthPitch, dst_box->right, dst_box->bottom - 1, dst_box->back - 1) - dst_offset; 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 +1383,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 +1444,18 @@ 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.offset + vkd3d_format_get_data_offset(format, vk_layout.rowPitch, + vk_layout.depthPitch, src_box->right, src_box->bottom - 1, src_box->back - 1) - src_offset; + + 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
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
On Wed Mar 29 13:17:17 2023 +0000, Giovanni Mascellani wrote:
I'm pretty sleepy right now, but this doesn't look right either. For example, if you want to flush the whole resource the `(right, bottom, back)` point that is involved in that computation is a whole slice (plus a line, plus a point) past the end of the resource. Wouldn't you want something like `get_data_offset(right - 1, bottom - 1, back - 1) + 1`, at least for uncompressed formats? As I said, I might be missing something here. Y...es, you're right, almost. It should be get_data_offset(right, bottom - 1, back - 1), which is almost what you said except that it correctly accounts for pixel size. Fixed in v4.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131#note_28790
On Mon Apr 3 23:45:45 2023 +0000, Zebediah Figura wrote:
changed this line in [version 4 of the diff](/wine/vkd3d/-/merge_requests/131/diffs?diff_id=40496&start_sha=cc1d3e0a0a788046f6c06f3523696d4023da49a8#1de6db547ff05a31fe39212d8871a474ab65dcba_1198_1198) Thanks, fixed in v4.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131#note_28791
This needs a rebase after !127. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131#note_28837
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131
participants (5)
-
Giovanni Mascellani (＠giomasce) -
Henri Verbeet (＠hverbeet)
-
Philip Rebohle
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)