Re: [PATCH 1/1] drm/amdgpu: move bo_va ref counting to internal funcs

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

On 1/13/2022 1:12 PM, Christian König wrote:
> Am 13.01.22 um 13:06 schrieb Nirmoy Das:
> > GEM code should not deal with struct amdgpu_bo_va's ref_count.
> > Move ref counting to amdgpu_vm.c.
> >
> > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 +++----
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 38 +++++++++++++++++++++++--
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 ++
> >   3 files changed, 42 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > index 4a11a2f4fa73..691f0a879c90 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > @@ -176,12 +176,9 @@ static int amdgpu_gem_object_open(struct 
> > drm_gem_object *obj,
> >       if (r)
> >           return r;
> >   -    bo_va = amdgpu_vm_bo_find(vm, abo);
> > -    if (!bo_va) {
> > -        bo_va = amdgpu_vm_bo_add(adev, vm, abo);
> > -    } else {
> > -        ++bo_va->ref_count;
> > -    }
> > +    if (!amdgpu_vm_bo_get(vm, abo))
> > +        amdgpu_vm_bo_add(adev, vm, abo);
> > +
> >       amdgpu_bo_unreserve(abo);
> >       return 0;
> >   }
> > @@ -218,7 +215,7 @@ static void amdgpu_gem_object_close(struct 
> > drm_gem_object *obj,
> >           return;
> >       }
> >       bo_va = amdgpu_vm_bo_find(vm, bo);
> > -    if (!bo_va || --bo_va->ref_count)
> > +    if (!bo_va)
> >           goto out_unlock;
> >         amdgpu_vm_bo_rmv(adev, bo_va);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index b23cb463b106..9d60de6a6697 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -1290,16 +1290,49 @@ struct amdgpu_bo_va *amdgpu_vm_bo_find(struct 
> > amdgpu_vm *vm,
> >                          struct amdgpu_bo *bo)
> >   {
> >       struct amdgpu_vm_bo_base *base;
> > +    struct amdgpu_bo_va *bo_va = NULL;
> >         for (base = bo->vm_bo; base; base = base->next) {
> >           if (base->vm != vm)
> >               continue;
> >   -        return container_of(base, struct amdgpu_bo_va, base);
> > +        bo_va = container_of(base, struct amdgpu_bo_va, base);
> >       }
> > -    return NULL;
> > +
> > +    if (bo_va && bo_va->ref_count <= 0)
> > +        return NULL;
> > +
> > +    return bo_va;
> >   }
> >   +/**
> > + * amdgpu_vm_bo_get - find the bo_va for a specific vm & bo and 
> > increase
> > + * the ref_count
> > + *
> > + * @vm: requested vm
> > + * @bo: requested buffer object
> > + *
> > + * Find @bo inside the requested vm.
> > + * Search inside the @bos vm list for the requested vm
> > + * Returns the found bo_va with +1 ref_count or NULL if none is found
> > + *
> > + * Object has to be reserved!
> > + *
> > + * Returns:
> > + * Found bo_va or NULL.
> > + */
> > +struct amdgpu_bo_va *amdgpu_vm_bo_get(struct amdgpu_vm *vm,
> > +                      struct amdgpu_bo *bo)
> > +{
> > +    struct amdgpu_bo_va *bo_va = amdgpu_vm_bo_find(vm, bo);
> > +
> > +    if (bo_va)
> > +        ++bo_va->ref_count;
> > +
> > +    return bo_va;
> > +}
> > +
> > +
> >   /**
> >    * amdgpu_vm_map_gart - Resolve gart mapping of addr
> >    *
> > @@ -2704,6 +2737,7 @@ void amdgpu_vm_bo_rmv(struct amdgpu_device *adev,
> >       if (bo && bo_va->is_xgmi)
> >           amdgpu_xgmi_set_pstate(adev, AMDGPU_XGMI_PSTATE_MIN);
> >   +    --bo_va->ref_count;
> >       kfree(bo_va);
>
> That here won't work, you are removing and freeing the bo_va even if 
> the refcount is not zero yet.
>
> I suggest to have a matching amdgpu_vm_bo_put() function instead.


Right, let me resend v2.


Thanks,

Nirmoy

> Regards,
> Christian.
>
> >   }
> >   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > index 85fcfb8c5efd..6d936fb1b934 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > @@ -415,6 +415,8 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device 
> > *adev,
> >   uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t 
> > addr);
> >   struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
> >                          struct amdgpu_bo *bo);
> > +struct amdgpu_bo_va *amdgpu_vm_bo_get(struct amdgpu_vm *vm,
> > +                       struct amdgpu_bo *bo);
> >   struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
> >                         struct amdgpu_vm *vm,
> >                         struct amdgpu_bo *bo);