Re: [PATCH 1/1] drm/amdgpu: fix BO leak after successful move test

[Christian König]

Am 20.10.21 um 13:50 schrieb Christian König:


Am 13.10.21 um 17:09 schrieb Nirmoy Das:
GTT BO cleanup code is with in the test for loop and
we would skip cleaning up GTT BO on success.

Reported-by: zhang <botton_zh...@163.com>
Signed-off-by: Nirmoy Das <nirmoy....@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 25 ++++++++++++------------
  1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
index 909d830b513e..5fe7ff680c29 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
@@ -35,6 +35,7 @@ static void amdgpu_do_test_moves(struct 
amdgpu_device *adev)
      struct amdgpu_bo *vram_obj = NULL;
      struct amdgpu_bo **gtt_obj = NULL;
      struct amdgpu_bo_param bp;
+    struct dma_fence *fence = NULL;
      uint64_t gart_addr, vram_addr;
      unsigned n, size;
      int i, r;
@@ -82,7 +83,6 @@ static void amdgpu_do_test_moves(struct 
amdgpu_device *adev)
          void *gtt_map, *vram_map;
          void **gart_start, **gart_end;
          void **vram_start, **vram_end;
-        struct dma_fence *fence = NULL;
            bp.domain = AMDGPU_GEM_DOMAIN_GTT;
          r = amdgpu_bo_create(adev, &bp, gtt_obj + i);
@@ -212,24 +212,23 @@ static void amdgpu_do_test_moves(struct 
amdgpu_device *adev)
            DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT 
offset 0x%llx\n",
               gart_addr - adev->gmc.gart_start);
-        continue;
+    }
  +    --i;
  out_lclean_unpin:
-        amdgpu_bo_unpin(gtt_obj[i]);
+    amdgpu_bo_unpin(gtt_obj[i]);
  out_lclean_unres:
-        amdgpu_bo_unreserve(gtt_obj[i]);
+    amdgpu_bo_unreserve(gtt_obj[i]);
  out_lclean_unref:
-        amdgpu_bo_unref(&gtt_obj[i]);
+    amdgpu_bo_unref(&gtt_obj[i]);
  out_lclean:
-        for (--i; i >= 0; --i) {
-            amdgpu_bo_unpin(gtt_obj[i]);
-            amdgpu_bo_unreserve(gtt_obj[i]);
-            amdgpu_bo_unref(&gtt_obj[i]);
-        }
-        if (fence)
-            dma_fence_put(fence);
-        break;
+    for (--i; i >= 0; --i) {

The usual idiom for cleanups like that is "while (i--)..." because 
that also works with an unsigned i.

Apart from that looks good to me.

But I'm not sure that we would want to keep the in kernel tests around 
anyway.

We now have my amdgpu_stress tool to test memory bandwidth and mesa has 
an option for that for a long time as well.

Christian.


Christian.

+        amdgpu_bo_unpin(gtt_obj[i]);
+        amdgpu_bo_unreserve(gtt_obj[i]);
+        amdgpu_bo_unref(&gtt_obj[i]);
      }
+    if (fence)
+        dma_fence_put(fence);
        amdgpu_bo_unpin(vram_obj);
  out_unres: