From: Philip Rebohle philip.rebohle@tu-dortmund.de
--
Based on 6643ab45b5d3fa6ec124dc6a04c8b61fc03118ae from vkd3d-proton, but modified somewhat. --- libs/vkd3d/resource.c | 209 ++++++++++++++++++------------------------ 1 file changed, 87 insertions(+), 122 deletions(-)
diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c index 8c050cfe..6e22931d 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,17 @@ 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) + { + ERR("Failed to map memory, vr %d.\n", vr); + d3d12_heap_destroy(heap); + return hresult_from_vk_result(hr); + } + } + return S_OK; }
@@ -1223,12 +1153,51 @@ 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; + + 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); + VK_CALL(vkInvalidateMappedMemoryRanges(resource->device->vk_device, 1, &vk_range)); +} + +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; + + 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); + VK_CALL(vkFlushMappedMemoryRanges(resource->device->vk_device, 1, &vk_range)); +} + 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 +1228,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, read_range->Begin, read_range->End - read_range->Begin); + else + d3d12_resource_invalidate(resource, 0, resource->desc.Width); + + return S_OK; }
static void STDMETHODCALLTYPE d3d12_resource_Unmap(ID3D12Resource *iface, UINT sub_resource, @@ -1286,9 +1258,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, written_range->Begin, written_range->End - written_range->Begin); + else + d3d12_resource_flush(resource, 0, resource->desc.Width); }
static D3D12_RESOURCE_DESC * STDMETHODCALLTYPE d3d12_resource_GetDesc(ID3D12Resource *iface, @@ -1320,10 +1293,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 +1354,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 +1377,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 +1438,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; }
From: Zebediah Figura zfigura@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); }
This merge request was approved by Zebediah Figura.
Do we need to worry about 32-bit address space considerations here along the lines of MAX_PERSISTENT_MAPPED_BYTES in wined3d?
This does seem unfortunate in the sense that it's somewhat counter to the whole "explicit API" concept; you'd hope that in an API like Direct3D 12 calling Unmap() actually unmapped things.
Resident Evil 2 accesses a resource immediately after unmapping it.
And presumably that's the last mapped resource on the heap, like in this test, or the heap would already stay mapped, right?
Giovanni Mascellani (@giomasce) commented about libs/vkd3d/resource.c:
MSDN hints that `End` might be (strictly) smaller than `Begin`. Maybe it would be better to prevent an underflow in that case?
Giovanni Mascellani (@giomasce) commented about libs/vkd3d/resource.c:
Same here.
Giovanni Mascellani (@giomasce) commented about libs/vkd3d/resource.c:
My reading of the specs is that `vkMapMemory()` is not required to leave `heap->map_ptr` untouched if it fails. Since `d3d12_heap_destroy()` relies on that value being `NULL` to avoid unmapping twice, maybe the pointer should be explicitly reset to `NULL` in the failure branch.
Giovanni Mascellani (@giomasce) commented about libs/vkd3d/resource.c:
Please check the returned value.
Giovanni Mascellani (@giomasce) commented about libs/vkd3d/resource.c:
Please check the returned value.
Do we need to worry about 32-bit address space considerations here along the lines of MAX_PERSISTENT_MAPPED_BYTES in wined3d?
Testing as in the attached patch [1] suggests that no.
This does seem unfortunate in the sense that it's somewhat counter to the whole "explicit API" concept; you'd hope that in an API like Direct3D 12 calling Unmap() actually unmapped things.
Yep!
Resident Evil 2 accesses a resource immediately after unmapping it.
And presumably that's the last mapped resource on the heap, like in this test, or the heap would already stay mapped, right?
Yes. It's a committed resource, so the only resource on its heap. I don't recall but I believe I tried to duplicate the conditions (so it is also a readback resource).
[1] [scratch.diff](/uploads/5fbb2d2ab1f28140be7f726f6ee650ef/scratch.diff)
On Tue Mar 21 15:28:52 2023 +0000, Giovanni Mascellani wrote:
Please check the returned value.
I tacitly assumed that vkInvalidateMappedMemoryRanges and vkFlushMappedMemoryRanges() returned void. Will change.
On Tue Mar 21 15:28:51 2023 +0000, Giovanni Mascellani wrote:
Makes sense, will do.
-
Giovanni Mascellani (＠giomasce) -
Henri Verbeet (＠hverbeet)
-
Philip Rebohle
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)