[PATCH v3 0/2] MR131: vkd3d: Persistently map host-visible heaps on creation.
-- v3: 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..92861f7f 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,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, dst_box->back) - 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, src_box->back) - 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 Tue Mar 28 21:47:08 2023 +0000, Zebediah Figura wrote:
changed this line in [version 3 of the diff](/wine/vkd3d/-/merge_requests/131/diffs?diff_id=39521&start_sha=9bb9a5efe9656e8ce53b45399cc3e976f5e5f064#1de6db547ff05a31fe39212d8871a474ab65dcba_1365_1365) Yes, thanks, I don't know how I thought that code was correct. Should be fixed in v3.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131#note_28281
On Tue Mar 28 21:47:08 2023 +0000, Zebediah Figura wrote:
Yes, thanks, I don't know how I thought that code was correct. Should be fixed in v3. 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. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131#note_28323
Giovanni Mascellani (@giomasce) commented about libs/vkd3d/resource.c:
+ 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); Minor, but we're flushing here, not invalidating.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/131#note_28324
participants (4)
-
Giovanni Mascellani (＠giomasce) -
Philip Rebohle
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)