[PATCH 0/1] 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...

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/127

[PATCH 1/1] vkd3d: Add an internal reference to a heap when a resource is placed there.

[Conor McCarthy]

From: Conor McCarthy <cmccarthy(a)codeweavers.com>

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...
---
 libs/vkd3d/resource.c      | 29 ++++++++++++++++++++++++++---
 libs/vkd3d/vkd3d_private.h |  1 +
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c
index 8c050cfe..7eb46d83 100644
--- a/libs/vkd3d/resource.c
+++ b/libs/vkd3d/resource.c
@@ -339,6 +339,25 @@ static void d3d12_heap_destroy(struct d3d12_heap *heap)
         d3d12_device_release(device);
 }
 
+static ULONG d3d12_heap_incref(struct d3d12_heap *heap)
+{
+    ULONG refcount = InterlockedIncrement(&heap->internal_refcount);
+
+    TRACE("%p increasing refcount to %u.\n", heap, refcount);
+
+    return refcount;
+}
+
+static ULONG d3d12_heap_decref(struct d3d12_heap *heap)
+{
+    ULONG refcount = InterlockedDecrement(&heap->internal_refcount);
+
+    if (!refcount)
+        d3d12_heap_destroy(heap);
+
+    return refcount;
+}
+
 static ULONG STDMETHODCALLTYPE d3d12_heap_Release(ID3D12Heap *iface)
 {
     struct d3d12_heap *heap = impl_from_ID3D12Heap(iface);
@@ -347,7 +366,7 @@ static ULONG STDMETHODCALLTYPE d3d12_heap_Release(ID3D12Heap *iface)
     TRACE("%p decreasing refcount to %u.\n", heap, refcount);
 
     if (!refcount)
-        d3d12_heap_destroy(heap);
+        d3d12_heap_decref(heap);
 
     return refcount;
 }
@@ -561,6 +580,7 @@ static HRESULT d3d12_heap_init(struct d3d12_heap *heap,
 
     heap->ID3D12Heap_iface.lpVtbl = &d3d12_heap_vtbl;
     heap->refcount = 1;
+    heap->internal_refcount = 1;
 
     heap->is_private = !!resource;
 
@@ -1027,8 +1047,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_decref(resource->heap);
 }
 
 static ULONG d3d12_resource_incref(struct d3d12_resource *resource)
@@ -1971,6 +1991,9 @@ HRESULT d3d12_placed_resource_create(struct d3d12_device *device, struct d3d12_h
         return hr;
     }
 
+    /* Resources are required to keep a reference to their heap. */
+    d3d12_heap_incref(heap);
+
     TRACE("Created placed resource %p.\n", object);
 
     *resource = object;
diff --git a/libs/vkd3d/vkd3d_private.h b/libs/vkd3d/vkd3d_private.h
index 075b67c2..c5484e38 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 internal_refcount;
 
     bool is_private;
     D3D12_HEAP_DESC desc;
-- 
GitLab

https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/127

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d/resource.c:
> 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_decref(resource->heap);

Unless I am missing something, this doesn't look correct. For example, `d3d12_committed_resource_create()` assigns `resource->heap` (inside `vkd3d_allocate_resource_memory()`), but doesn't increment its reference count. So either that's wrong, or it's wrong here to decrement indiscriminately.

Even in `d3d12_placed_resource_create()` I'd suggest to call `d3d12_heap_incref()` close to where `resource->heap` is actually assigned, so that it's easier to check that assignments and increments are correctly paired (even if control flow might change in the future).

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/127#note_27151

[Conor McCarthy (＠cmccarthy)]

> For example, `d3d12_committed_resource_create()` assigns `resource->heap` (inside `vkd3d_allocate_resource_memory()`), but doesn't increment its reference count.

The heap is created with `internal_refcount = 1` so this decref is required to free it. Valgrind reports no leaks in test_create_committed_resource().

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/127#note_27165