[PATCH v2 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...

--
  v2: vkd3d: Add an internal reference to a heap when a resource is placed there.
 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/127

[PATCH v2 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 (if SM 6.0 support is added).

The docs state it is required:
https://learn.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-id3d12dev...
---
 libs/vkd3d/resource.c      | 28 +++++++++++++++++++++++++---
 libs/vkd3d/vkd3d_private.h |  1 +
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/libs/vkd3d/resource.c b/libs/vkd3d/resource.c
index 8c050cfe..3f4ebb9d 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)
@@ -1941,6 +1961,8 @@ static HRESULT vkd3d_bind_heap_memory(struct d3d12_device *device,
     {
         resource->heap = heap;
         resource->heap_offset = heap_offset;
+        /* Resources are required to keep a reference to their heap. */
+        d3d12_heap_incref(heap);
     }
     else
     {
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)]

On Fri Mar 17 13:46:14 2023 +0000, Conor McCarthy wrote:
> > 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().
> > Right. Anyway, now it feels better.

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

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

[Giovanni Mascellani (＠giomasce)]

This merge request was approved by Giovanni Mascellani.
-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/127

[Henri Verbeet (＠hverbeet)]

> 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...

What I'm missing in this patch/MR is:
  - Why do we need an internal reference count here instead of e.g. using ID3D12Heap_AddRef()? It turns out that we have a test that shows creating a placed resource doesn't increment the publicly visible reference count, but I had to go look that up.
  - Why can't we have a test for this?

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