Skip to content
Snippets Groups Projects

vkd3d: Add an internal reference to a heap when a resource is placed there.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:heap_ref into master
1 unresolved thread

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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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() 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).

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

  • Conor McCarthy changed this line in version 3 of the diff

    changed this line in version 3 of the diff

  • Please register or sign in to reply
  • Conor McCarthy added 1 commit

    added 1 commit

    • d51421b8 - vkd3d: Add an internal reference to a heap when a resource is placed there.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • 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?
  • Conor McCarthy added 4 commits

    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.

    Compare with previous version

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

  • Henri Verbeet approved this merge request

    approved this merge request

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

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Henri Verbeet mentioned in merge request !131 (merged)

    mentioned in merge request !131 (merged)

  • Please register or sign in to reply
    Loading