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

["Das, Nirmoy" <nirmoy.das@amd.com>]

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 ?


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