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

[Christian König]

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.

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);