[PATCH v3 0/2] MR127: vkd3d: Add an internal reference to a heap when a resource is placed there.
Fixes a crash on exit in Horizon Zero Dawn (with SM 6.0 support added). The docs state it is required: https://learn.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-id3d12dev... -- v3: tests: Release and then use a heap which contains resources. vkd3d: Do not destroy a heap until its resource count is zero. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/127
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Fixes a crash on exit in Horizon Zero Dawn (which requres added SM 6.0 support). Placed resources should hold a reference to their heap: https://learn.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-id3d12dev... --- libs/vkd3d/resource.c | 17 ++++++++++++++--- libs/vkd3d/vkd3d_private.h | 1 + 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c index 8c050cfe..79db6666 100644 --- a/libs/vkd3d/resource.c +++ b/libs/vkd3d/resource.c @@ -346,12 +346,19 @@ static ULONG STDMETHODCALLTYPE d3d12_heap_Release(ID3D12Heap *iface) TRACE("%p decreasing refcount to %u.\n", heap, refcount); - if (!refcount) + /* A heap must not be destroyed until all contained resources are destroyed. */ + if (!refcount && !heap->resource_count) d3d12_heap_destroy(heap); return refcount; } +static void d3d12_heap_resource_destroyed(struct d3d12_heap *heap) +{ + if (!InterlockedDecrement(&heap->resource_count) && (!heap->refcount || heap->is_private)) + d3d12_heap_destroy(heap); +} + static HRESULT STDMETHODCALLTYPE d3d12_heap_GetPrivateData(ID3D12Heap *iface, REFGUID guid, UINT *data_size, void *data) { @@ -561,6 +568,7 @@ static HRESULT d3d12_heap_init(struct d3d12_heap *heap, heap->ID3D12Heap_iface.lpVtbl = &d3d12_heap_vtbl; heap->refcount = 1; + heap->resource_count = 0; heap->is_private = !!resource; @@ -628,6 +636,8 @@ static HRESULT d3d12_heap_init(struct d3d12_heap *heap, heap->device = device; if (!heap->is_private) d3d12_device_add_ref(heap->device); + else + heap->resource_count = 1; return S_OK; } @@ -1027,8 +1037,8 @@ static void d3d12_resource_destroy(struct d3d12_resource *resource, struct d3d12 else VK_CALL(vkDestroyImage(device->vk_device, resource->u.vk_image, NULL)); - if (resource->flags & VKD3D_RESOURCE_DEDICATED_HEAP) - d3d12_heap_destroy(resource->heap); + if (resource->heap) + d3d12_heap_resource_destroyed(resource->heap); } static ULONG d3d12_resource_incref(struct d3d12_resource *resource) @@ -1941,6 +1951,7 @@ static HRESULT vkd3d_bind_heap_memory(struct d3d12_device *device, { resource->heap = heap; resource->heap_offset = heap_offset; + InterlockedIncrement(&heap->resource_count); } else { diff --git a/libs/vkd3d/vkd3d_private.h b/libs/vkd3d/vkd3d_private.h index 075b67c2..1a277a47 100644 --- a/libs/vkd3d/vkd3d_private.h +++ b/libs/vkd3d/vkd3d_private.h @@ -563,6 +563,7 @@ struct d3d12_heap { ID3D12Heap ID3D12Heap_iface; LONG refcount; + LONG resource_count; bool is_private; D3D12_HEAP_DESC desc; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/127
From: Conor McCarthy <cmccarthy(a)codeweavers.com> The expected use case where a heap is freed before its contained resources is not reasonably testable, so the ability to place a new resource is tested instead. --- tests/d3d12.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/tests/d3d12.c b/tests/d3d12.c index 405c9419..890a65d5 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -2081,12 +2081,12 @@ done: static void test_create_placed_resource(void) { D3D12_GPU_VIRTUAL_ADDRESS gpu_address; + ID3D12Resource *resource, *resource2; D3D12_RESOURCE_DESC resource_desc; ID3D12Device *device, *tmp_device; D3D12_CLEAR_VALUE clear_value; D3D12_RESOURCE_STATES state; D3D12_HEAP_DESC heap_desc; - ID3D12Resource *resource; ID3D12Heap *heap; unsigned int i; ULONG refcount; @@ -2112,7 +2112,7 @@ static void test_create_placed_resource(void) return; } - heap_desc.SizeInBytes = D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT; + heap_desc.SizeInBytes = D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT * 2; memset(&heap_desc.Properties, 0, sizeof(heap_desc.Properties)); heap_desc.Properties.Type = D3D12_HEAP_TYPE_DEFAULT; heap_desc.Alignment = 0; @@ -2181,7 +2181,21 @@ static void test_create_placed_resource(void) &IID_ID3D12Resource, (void **)&resource); ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + /* Test heap peristence when its resource count is non-zero. */ + hr = ID3D12Device_CreatePlacedResource(device, heap, 0, + &resource_desc, D3D12_RESOURCE_STATE_COMMON, NULL, &IID_ID3D12Resource, (void **)&resource); + ok(hr == S_OK, "Failed to create placed resource, hr %#x.\n", hr); + ID3D12Heap_Release(heap); + refcount = get_refcount(heap); + ok(!refcount, "Got unexpected refcount %u.\n", (unsigned int)refcount); + + hr = ID3D12Device_CreatePlacedResource(device, heap, D3D12_DEFAULT_RESOURCE_PLACEMENT_ALIGNMENT, + &resource_desc, D3D12_RESOURCE_STATE_COMMON, NULL, &IID_ID3D12Resource, (void **)&resource2); + ok(hr == S_OK, "Failed to create placed resource, hr %#x.\n", hr); + + ID3D12Resource_Release(resource); + ID3D12Resource_Release(resource2); for (i = 0; i < ARRAY_SIZE(invalid_buffer_desc_tests); ++i) { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/127
The expected use case can't really be tested, but it works in Windows if a heap is dec'd to refcount 0 and then a new resource is placed on it. I changed the implementation to use a resource count because it doesn't break when the refcount is checked by bumping to 1 and releasing to 0. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/127#note_27944
This merge request was approved by Henri Verbeet. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/127
participants (3)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Henri Verbeet (＠hverbeet)