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-id3d12device-createheap
Merge request reports
Activity
1027 1047 else 1028 1048 VK_CALL(vkDestroyImage(device->vk_device, resource->u.vk_image, NULL)); 1029 1049 1030 if (resource->flags & VKD3D_RESOURCE_DEDICATED_HEAP) 1031 d3d12_heap_destroy(resource->heap); 1050 if (resource->heap) 1051 d3d12_heap_decref(resource->heap); - Comment on lines -1030 to +1051
Unless I am missing something, this doesn't look correct. For example,
d3d12_committed_resource_create()
assignsresource->heap
(insidevkd3d_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 calld3d12_heap_incref()
close to whereresource->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). For example,
d3d12_committed_resource_create()
assignsresource->heap
(insidevkd3d_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().changed this line in version 3 of the diff
added 1 commit
- d51421b8 - 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-id3d12device-createheap
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?
added 4 commits
-
d51421b8...57d92a15 - 2 commits from branch
wine:master
- 3df0a540 - vkd3d: Do not destroy a heap until its resource count is zero.
- f27748e7 - tests: Release and then use a heap which contains resources.
-
d51421b8...57d92a15 - 2 commits from branch
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.
added 9 commits
-
f27748e7...7a9e393e - 7 commits from branch
wine:master
- 88667098 - vkd3d: Do not destroy a heap until its resource count is zero.
- c87a292f - tests: Release and then use a heap which contains resources.
-
f27748e7...7a9e393e - 7 commits from branch
mentioned in merge request !131 (merged)