Fix D3D12 device from leaking. (#925)

Use of GetDevice would increment but never decrement which caused the device to leak.
intel · Aug 29, 2023 · 8c3e9d6 · 8c3e9d6
1 parent 0b3ce5d
commit 8c3e9d6
Show file tree

Hide file tree

Showing 6 changed files with 21 additions and 30 deletions.
diff --git a/src/gpgmm/d3d12/FenceD3D12.cpp b/src/gpgmm/d3d12/FenceD3D12.cpp
@@ -54,7 +54,7 @@ namespace gpgmm::d3d12 {
     HRESULT Fence::WaitFor(uint64_t fenceValue) {
         if (!IsCompleted(fenceValue)) {
             GPGMM_RETURN_IF_FAILED(mFence->SetEventOnCompletion(fenceValue, mCompletionEvent),
-                                   GetDevice(mFence.Get()));
+                                   GetDevice(mFence.Get()).Get());
 
             // Wait for the event to complete (it will automatically reset).
             const uint32_t result = WaitForSingleObject(mCompletionEvent, INFINITE);
@@ -83,7 +83,7 @@ namespace gpgmm::d3d12 {
     HRESULT Fence::Signal(ID3D12CommandQueue* pCommandQueue) {
         ASSERT(mLastSignaledFence != mCurrentFence);
         GPGMM_RETURN_IF_FAILED(pCommandQueue->Signal(mFence.Get(), mCurrentFence),
-                               GetDevice(pCommandQueue));
+                               GetDevice(pCommandQueue).Get());
         mLastSignaledFence = mCurrentFence;
         mCurrentFence++;
         return S_OK;

diff --git a/src/gpgmm/d3d12/ResidencyHeapD3D12.cpp b/src/gpgmm/d3d12/ResidencyHeapD3D12.cpp
@@ -43,7 +43,7 @@ namespace gpgmm::d3d12 {
             ComPtr<ID3D12Resource> committedResource;
             if (SUCCEEDED(pageable.As(&committedResource))) {
                 GPGMM_RETURN_IF_FAILED(committedResource->GetHeapProperties(nullptr, heapFlags),
-                                       GetDevice(committedResource.Get()));
+                                       GetDevice(committedResource.Get()).Get());
                 return S_OK;
             }
 
@@ -188,17 +188,17 @@ namespace gpgmm::d3d12 {
             // residency cache.
             if (heap->GetInfo().Status != RESIDENCY_HEAP_STATUS_UNKNOWN) {
                 GPGMM_RETURN_IF_FAILED(residencyManager->InsertHeap(heap.get()),
-                                       GetDevice(pPageable));
+                                       GetDevice(pPageable).Get());
             } else {
                 if (newDescriptor.Flags & RESIDENCY_HEAP_FLAG_CREATE_RESIDENT) {
-                    GPGMM_RETURN_IF_FAILED(heap->Lock(), GetDevice(pPageable));
-                    GPGMM_RETURN_IF_FAILED(heap->Unlock(), GetDevice(pPageable));
+                    GPGMM_RETURN_IF_FAILED(heap->Lock(), GetDevice(pPageable).Get());
+                    GPGMM_RETURN_IF_FAILED(heap->Unlock(), GetDevice(pPageable).Get());
                     ASSERT(heap->GetInfo().Status == RESIDENCY_HEAP_STATUS_RESIDENT);
                 }
             }
 
             if (descriptor.Flags & RESIDENCY_HEAP_FLAG_CREATE_LOCKED) {
-                GPGMM_RETURN_IF_FAILED(heap->Lock(), GetDevice(pPageable));
+                GPGMM_RETURN_IF_FAILED(heap->Lock(), GetDevice(pPageable).Get());
             }
 
         } else {
@@ -217,7 +217,8 @@ namespace gpgmm::d3d12 {
             }
         }
 
-        GPGMM_RETURN_IF_FAILED(heap->SetDebugName(newDescriptor.DebugName), GetDevice(pPageable));
+        GPGMM_RETURN_IF_FAILED(heap->SetDebugName(newDescriptor.DebugName),
+                               GetDevice(pPageable).Get());
         GPGMM_TRACE_EVENT_OBJECT_SNAPSHOT(heap.get(), newDescriptor);
 
         DebugLog(MessageId::kObjectCreated, heap.get())
@@ -283,7 +284,7 @@ namespace gpgmm::d3d12 {
 
         ComPtr<ID3D12Pageable> pageable;
         GPGMM_RETURN_IF_FAILED(createHeapFn(pCreateHeapContext, &pageable),
-                               GetDevice(pageable.Get()));
+                               GetDevice(pageable.Get()).Get());
 
         return CreateResidencyHeap(descriptor, pResidencyManager, pageable.Get(),
                                    ppResidencyHeapOut);

diff --git a/src/gpgmm/d3d12/ResourceAllocationD3D12.cpp b/src/gpgmm/d3d12/ResourceAllocationD3D12.cpp
@@ -106,7 +106,7 @@ namespace gpgmm::d3d12 {
 
         ResidencyHeap* residencyHeap = static_cast<ResidencyHeap*>(GetMemory());
         if (SUCCEEDED(residencyHeap->GetResidencyManager(nullptr))) {
-            GPGMM_RETURN_IF_FAILED(residencyHeap->Lock(), GetDevice(mResource.Get()));
+            GPGMM_RETURN_IF_FAILED(residencyHeap->Lock(), GetDevice(mResource.Get()).Get());
         }
 
         // Range coordinates are always subresource-relative so the range should only be
@@ -122,7 +122,7 @@ namespace gpgmm::d3d12 {
 
         void* mappedData = nullptr;
         GPGMM_RETURN_IF_FAILED(mResource->Map(subresource, newReadRangePtr, &mappedData),
-                               GetDevice(mResource.Get()));
+                               GetDevice(mResource.Get()).Get());
 
         mMappedCount.Ref();
 

diff --git a/src/gpgmm/d3d12/UtilsD3D12.cpp b/src/gpgmm/d3d12/UtilsD3D12.cpp
@@ -14,6 +14,7 @@
 
 #include "gpgmm/d3d12/UtilsD3D12.h"
 
+#include "gpgmm/d3d12/ErrorD3D12.h"
 #include "gpgmm/utils/Math.h"
 
 namespace gpgmm::d3d12 {
@@ -466,13 +467,16 @@ namespace gpgmm::d3d12 {
         }
     }
 
-    ID3D12Device* GetDevice(ID3D12DeviceChild* pDeviceChild) {
+    ComPtr<ID3D12Device> GetDevice(ID3D12DeviceChild* pDeviceChild) {
         if (pDeviceChild == nullptr) {
             return {};
         }
-        ID3D12Device* pDevice = nullptr;
-        pDeviceChild->GetDevice(IID_PPV_ARGS(&pDevice));
-        return pDevice;
+        // ID3D12DeviceChild::GetDevice will AddRef the underlying device so return
+        // a COM pointer to ensure the caller must release it once finished.
+        ComPtr<ID3D12Device> device;
+        HRESULT hr = pDeviceChild->GetDevice(IID_PPV_ARGS(&device));
+        ASSERT(SUCCEEDED(hr));
+        return device;
     }
 
     bool IsTexture(const D3D12_RESOURCE_DESC& resourceDescriptor) {

diff --git a/src/gpgmm/d3d12/UtilsD3D12.h b/src/gpgmm/d3d12/UtilsD3D12.h
@@ -29,7 +29,7 @@ namespace gpgmm::d3d12 {
     HRESULT SetDebugObjectName(ID3D12Object* object, LPCWSTR name);
     DXGI_MEMORY_SEGMENT_GROUP GetHeapSegment(D3D12_MEMORY_POOL memoryPool, bool isUMA);
     const char* GetMemorySegmentName(DXGI_MEMORY_SEGMENT_GROUP heapSegment, bool isUMA);
-    ID3D12Device* GetDevice(ID3D12DeviceChild* pDeviceChild);
+    ComPtr<ID3D12Device> GetDevice(ID3D12DeviceChild* pDeviceChild);
     bool IsTexture(const D3D12_RESOURCE_DESC& resourceDescriptor);
     bool IsBuffer(const D3D12_RESOURCE_DESC& resourceDescriptor);
 

diff --git a/src/tests/end2end/D3D12ResourceAllocatorTests.cpp b/src/tests/end2end/D3D12ResourceAllocatorTests.cpp
@@ -739,20 +739,6 @@ TEST_F(D3D12ResourceAllocatorTests, GetResourceAllocator) {
     EXPECT_REFCOUNT_EQ(resourceAllocator.Get(), 1);
 }
 
-TEST_F(D3D12ResourceAllocatorTests, CreateBufferLeaked) {
-    ComPtr<IResourceAllocator> resourceAllocator;
-    ASSERT_SUCCEEDED(CreateResourceAllocator(CreateBasicAllocatorDesc(), mDevice.Get(),
-                                             mAdapter.Get(), &resourceAllocator, nullptr));
-    ASSERT_NE(resourceAllocator, nullptr);
-
-    ComPtr<IResourceAllocation> allocation;
-    ASSERT_SUCCEEDED(
-        resourceAllocator->CreateResource({}, CreateBasicBufferDesc(kBufferOf4MBAllocationSize),
-                                          D3D12_RESOURCE_STATE_COMMON, nullptr, &allocation));
-
-    allocation.Detach();  // leaked!
-}
-
 // Verifies there are no attribution of heaps when UMA + no read-back.
 TEST_F(D3D12ResourceAllocatorTests, CreateBufferUMA) {
     GPGMM_SKIP_TEST_IF(!mCaps->IsAdapterCacheCoherentUMA());