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

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

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@xxxxxxx>
> > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
> > ---
> >   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: