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

[Das, Nirmoy]

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 at amd.com>
>> ---
>>   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);
>