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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 20.10.21 um 14:55 schrieb Das, Nirmoy:
>
> On 10/20/2021 1:51 PM, Christian König wrote:
>> 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_zhang@163.com>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@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.
>
>
> Shall I then remove amdgpu_test.c ?

Please double check if the amdgpu_stress utility gives you the same 
functionality, if yes we should probably remove this test here.

Thanks,
Christian.

>
>
> Nirmoy
>
>
>>
>> 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:
>>>
>>