[PATCH vkd3d v2 1/2] tests: Add test for texture placement at 64kb alignment.
Signed-off-by: Conor McCarthy <cmccarthy(a)codeweavers.com> --- tests/d3d12.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/tests/d3d12.c b/tests/d3d12.c index ce6d6b36..82b1b45b 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -29766,6 +29766,106 @@ static void test_resource_allocation_info(void) ok(!refcount, "ID3D12Device has %u references left.\n", (unsigned int)refcount); } +static void test_64kb_texture_alignment(void) +{ + ID3D12GraphicsCommandList *command_list; + D3D12_RESOURCE_ALLOCATION_INFO info; + D3D12_SUBRESOURCE_DATA texture_data; + D3D12_RESOURCE_DESC resource_desc; + struct test_context context; + struct resource_readback rb; + ID3D12Resource *textures[2]; + D3D12_HEAP_DESC heap_desc; + ID3D12CommandQueue *queue; + uint32_t *upload_buffer; + unsigned int i, x, y; + ID3D12Device *device; + ID3D12Heap *heap; + HRESULT hr; + + if (!init_test_context(&context, NULL)) + return; + device = context.device; + command_list = context.list; + queue = context.queue; + + /* This results in an alignment of 0x20000 with RX580/RADV, but + * only with D3D12_TEXTURE_LAYOUT_UNKNOWN. Other cards/drivers may + * specify smaller alignments compatible with the 64kb D3D12 default. */ + resource_desc.Dimension = D3D12_RESOURCE_DIMENSION_TEXTURE2D; + resource_desc.Alignment = 0; + resource_desc.Width = 1024; + resource_desc.Height = 1024; + resource_desc.DepthOrArraySize = 1; + resource_desc.MipLevels = 1; + resource_desc.Format = DXGI_FORMAT_R8G8B8A8_UNORM; + resource_desc.SampleDesc.Count = 1; + resource_desc.SampleDesc.Quality = 0; + resource_desc.Layout = D3D12_TEXTURE_LAYOUT_UNKNOWN; + resource_desc.Flags = 0; + + info = ID3D12Device_GetResourceAllocationInfo(device, 0, 1, &resource_desc); + + heap_desc.SizeInBytes = info.SizeInBytes * 2 + D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT; + memset(&heap_desc.Properties, 0, sizeof(heap_desc.Properties)); + heap_desc.Properties.Type = D3D12_HEAP_TYPE_DEFAULT; + heap_desc.Alignment = 0; + heap_desc.Flags = D3D12_HEAP_FLAG_DENY_BUFFERS | D3D12_HEAP_FLAG_DENY_RT_DS_TEXTURES; + hr = ID3D12Device_CreateHeap(device, &heap_desc, &IID_ID3D12Heap, (void **)&heap); + ok(hr == S_OK, "Failed to create heap, hr %#x.\n", hr); + + /* Padding of info.SizeInBytes is (vulkan_alignment - d3d12_alignment), so this heap_offset calculation + * always results in a Vulkan-aligned offset which won't be adjusted upwards. */ + hr = ID3D12Device_CreatePlacedResource(device, heap, D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT + info.SizeInBytes, + &resource_desc, D3D12_RESOURCE_STATE_COPY_DEST, NULL, &IID_ID3D12Resource, (void **)&textures[1]); + ok(hr == S_OK, "Failed to create placed resource, hr %#x.\n", hr); + + /* Some apps, e.g. WoW, ignore the alignment returned by GetResourceAllocationInfo() and use + * D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT. D3D12 requires support for this alignment. */ + hr = ID3D12Device_CreatePlacedResource(device, heap, D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT, + &resource_desc, D3D12_RESOURCE_STATE_COPY_DEST, NULL, &IID_ID3D12Resource, (void **)&textures[0]); + ok(hr == S_OK, "Failed to create placed resource, hr %#x.\n", hr); + + upload_buffer = malloc(info.SizeInBytes); + ok(upload_buffer, "Failed to allocate memory.\n"); + + for (i = 0; i < resource_desc.Width * resource_desc.Height; ++i) + upload_buffer[i] = 0xcafef00d; + texture_data.pData = upload_buffer; + texture_data.RowPitch = resource_desc.Width * sizeof(uint32_t); + texture_data.SlicePitch = texture_data.RowPitch * resource_desc.Height; + upload_texture_data(textures[1], &texture_data, 1, queue, command_list); + + for (i = 0; i < resource_desc.Width * resource_desc.Height; ++i) + upload_buffer[i] = 0xdeadbeef; + texture_data.SlicePitch = texture_data.RowPitch * resource_desc.Height; + reset_command_list(command_list, context.allocator); + /* Write data to the first texture to check if the second is overwritten. + * Resource overlap may still go undetected depending on the actual layout + * used by the driver. */ + upload_texture_data(textures[0], &texture_data, 1, queue, command_list); + + reset_command_list(command_list, context.allocator); + transition_resource_state(command_list, textures[1], + D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_COPY_SOURCE); + get_texture_readback_with_command_list(textures[1], 0, &rb, queue, command_list); + for (y = 0; y < resource_desc.Height; ++y) + { + for (x = 0; x < resource_desc.Width; ++x) + { + i = get_readback_uint(&rb, x, y, 0); + ok(i == 0xcafef00d, "Expected 0xcafef00d, got %#x at %u, %u.\n", i, x, y); + } + } + release_resource_readback(&rb); + + free(upload_buffer); + ID3D12Resource_Release(textures[0]); + ID3D12Resource_Release(textures[1]); + ID3D12Heap_Release(heap); + destroy_test_context(&context); +} + static void test_suballocate_small_textures(void) { D3D12_GPU_VIRTUAL_ADDRESS gpu_address; @@ -33982,6 +34082,7 @@ START_TEST(d3d12) run_test(test_clip_distance); run_test(test_combined_clip_and_cull_distances); run_test(test_resource_allocation_info); + run_test(test_64kb_texture_alignment); run_test(test_suballocate_small_textures); run_test(test_command_list_initial_pipeline_state); run_test(test_blend_factor); -- 2.31.1
Support for D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT is required, and some apps, e.g. WoW, use it without checking the allocation info. Signed-of-by: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d/device.c | 15 ++++++++++++--- libs/vkd3d/resource.c | 6 ++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d/device.c b/libs/vkd3d/device.c index a63fc92b..159bc470 100644 --- a/libs/vkd3d/device.c +++ b/libs/vkd3d/device.c @@ -3221,7 +3221,7 @@ static D3D12_RESOURCE_ALLOCATION_INFO * STDMETHODCALLTYPE d3d12_device_GetResour if (desc->Dimension == D3D12_RESOURCE_DIMENSION_BUFFER) { - info->SizeInBytes = desc->Width; + info->SizeInBytes = align(desc->Width, D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT); info->Alignment = D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT; } else @@ -3235,9 +3235,18 @@ static D3D12_RESOURCE_ALLOCATION_INFO * STDMETHODCALLTYPE d3d12_device_GetResour requested_alignment = desc->Alignment ? desc->Alignment : D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT; info->Alignment = max(info->Alignment, requested_alignment); - } - info->SizeInBytes = align(info->SizeInBytes, info->Alignment); + info->SizeInBytes = align(info->SizeInBytes, info->Alignment); + + /* Pad by the maximum heap offset increase which may be needed to align to a higher + * Vulkan requirement an offset supplied by the calling application. This allows + * us to return the standard D3D12 alignment and adjust resource placement later. */ + if (info->Alignment > requested_alignment) + { + info->SizeInBytes += info->Alignment - requested_alignment; + info->Alignment = requested_alignment; + } + } TRACE("Size %#"PRIx64", alignment %#"PRIx64".\n", info->SizeInBytes, info->Alignment); diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c index 3a66bb15..3faa6794 100644 --- a/libs/vkd3d/resource.c +++ b/libs/vkd3d/resource.c @@ -1901,9 +1901,15 @@ static HRESULT vkd3d_bind_heap_memory(struct d3d12_device *device, VkResult vr; if (d3d12_resource_is_buffer(resource)) + { VK_CALL(vkGetBufferMemoryRequirements(vk_device, resource->u.vk_buffer, &requirements)); + } else + { VK_CALL(vkGetImageMemoryRequirements(vk_device, resource->u.vk_image, &requirements)); + /* Padding in d3d12_device_GetResourceAllocationInfo() leaves room to align the offset. */ + heap_offset = align(heap_offset, requirements.alignment); + } if (heap_offset % requirements.alignment) { -- 2.31.1
On Thu, 1 Jul 2021 at 14:37, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
+ for (i = 0; i < resource_desc.Width * resource_desc.Height; ++i) + upload_buffer[i] = 0xdeadbeef; + texture_data.SlicePitch = texture_data.RowPitch * resource_desc.Height; + reset_command_list(command_list, context.allocator); + /* Write data to the first texture to check if the second is overwritten. + * Resource overlap may still go undetected depending on the actual layout + * used by the driver. */ + upload_texture_data(textures[0], &texture_data, 1, queue, command_list); + + reset_command_list(command_list, context.allocator); + transition_resource_state(command_list, textures[1], + D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_COPY_SOURCE); + get_texture_readback_with_command_list(textures[1], 0, &rb, queue, command_list); + for (y = 0; y < resource_desc.Height; ++y) + { + for (x = 0; x < resource_desc.Width; ++x) + { + i = get_readback_uint(&rb, x, y, 0); + ok(i == 0xcafef00d, "Expected 0xcafef00d, got %#x at %u, %u.\n", i, x, y); + } + } + release_resource_readback(&rb); + We'll want to use check_sub_resource_uint() here.
Is this test missing a todo_if()? The returned alignment is 64 KiB here on all my systems, so I can't test the actual issue, but presumably the test is supposed to fail on setups that do require larger alignments. Or is this succeeding because we fall back to a separate allocation in vkd3d_bind_heap_memory()? In that case though: - Why is that a problem for the actual application? - We may be able to detect that by aliasing multiple resources to the same heap offset.
July 2, 2021 10:17 PM, "Henri Verbeet" <hverbeet(a)gmail.com> wrote:
Or is this succeeding because we fall back to a separate allocation in vkd3d_bind_heap_memory()? In that case though: - Why is that a problem for the actual application?
It's a problem not because of any failure but because heap memory is wasted. That's the only reason for the patch.
- We may be able to detect that by aliasing multiple resources to the same heap offset.
Would this work by placing a smaller resource at a larger alignment within the same heap area, and check if its data has already been set to the uploaded value?
On Fri, 2 Jul 2021 at 15:04, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
- We may be able to detect that by aliasing multiple resources to the same heap offset.
Would this work by placing a smaller resource at a larger alignment within the same heap area, and check if its data has already been set to the uploaded value?
It doesn't need to be smaller or have a different alignment. Creating two resources with the same properties at the same heap offset should work. If these share the same heap allocation, modifications to one should be visible in the other (after the appropriate barriers, etc.), if they get separate allocations modifications wouldn't be visible.
July 2, 2021 11:14 PM, "Henri Verbeet" <hverbeet(a)gmail.com> wrote:
It doesn't need to be smaller or have a different alignment. Creating two resources with the same properties at the same heap offset should work. If these share the same heap allocation, modifications to one should be visible in the other (after the appropriate barriers, etc.), if they get separate allocations modifications wouldn't be visible.
How should the 'ok' line be marked before the problem is patched? I see no way of using todo_if() because the Vulkan alignment requirement is unknown.
On Fri, 2 Jul 2021 at 16:51, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
July 2, 2021 11:14 PM, "Henri Verbeet" <hverbeet(a)gmail.com> wrote:
It doesn't need to be smaller or have a different alignment. Creating two resources with the same properties at the same heap offset should work. If these share the same heap allocation, modifications to one should be visible in the other (after the appropriate barriers, etc.), if they get separate allocations modifications wouldn't be visible.
How should the 'ok' line be marked before the problem is patched? I see no way of using todo_if() because the Vulkan alignment requirement is unknown.
Isn't it returned by GetResourceAllocationInfo()?
participants (2)
-
Conor McCarthy -
Henri Verbeet