[PATCH v3 0/2] MR127: vkd3d: Add an internal reference to a heap when a resource is placed there.

[Conor McCarthy (＠cmccarthy)]

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

[PATCH v3 1/2] vkd3d: Do not destroy a heap until its resource count is zero.

[Conor McCarthy]

From: Conor McCarthy

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;

[PATCH v3 2/2] tests: Release and then use a heap which contains resources.

[Conor McCarthy]

From: Conor McCarthy

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)
     {

[Conor McCarthy (＠cmccarthy)]

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.

[PATCH v3 0/2] MR127: vkd3d: Add an internal reference to a heap when a resource is placed there. - approved

[Henri Verbeet (＠hverbeet)]

This merge request was approved by Henri Verbeet.