Re: [PATCH 3/5] drm/amdgpu: create shadow bo using amdgpu_bo_create_shadow()

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 22.04.21 um 14:35 schrieb Nirmoy Das:
> Shadow BOs are only needed for vm code so call amdgpu_bo_create_shadow()
> directly instead of depending on amdgpu_bo_create().
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 75 +++++++++++++++++---------
>   1 file changed, 49 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 6f0a6011cb3d..0e1d08a88f54 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -850,35 +850,64 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>   }
>   
>   /**
> - * amdgpu_vm_bo_param - fill in parameters for PD/PT allocation
> + * amdgpu_vm_bo_create - create bo for PD/PT

Better name that amdgpu_vm_pt_create.

>    *
>    * @adev: amdgpu_device pointer
>    * @vm: requesting vm
>    * @level: the page table level
>    * @immediate: use a immediate update
> - * @bp: resulting BO allocation parameters
> + * @bo: pointer to the buffer object pointer
>    */
> -static void amdgpu_vm_bo_param(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> -			       int level, bool immediate,
> -			       struct amdgpu_bo_param *bp)
> +static int amdgpu_vm_bo_create(struct amdgpu_device *adev,
> +				struct amdgpu_vm *vm,
> +				int level, bool immediate,
> +				struct amdgpu_bo **bo)
>   {
> -	memset(bp, 0, sizeof(*bp));
> +	struct amdgpu_bo_param bp;
> +	bool create_shadow = false;
> +	int r;
>   
> -	bp->size = amdgpu_vm_bo_size(adev, level);
> -	bp->byte_align = AMDGPU_GPU_PAGE_SIZE;
> -	bp->domain = AMDGPU_GEM_DOMAIN_VRAM;
> -	bp->domain = amdgpu_bo_get_preferred_pin_domain(adev, bp->domain);
> -	bp->flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
> +	memset(&bp, 0, sizeof(bp));
> +
> +	bp.size = amdgpu_vm_bo_size(adev, level);
> +	bp.byte_align = AMDGPU_GPU_PAGE_SIZE;
> +	bp.domain = AMDGPU_GEM_DOMAIN_VRAM;
> +	bp.domain = amdgpu_bo_get_preferred_pin_domain(adev, bp.domain);
> +	bp.flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
>   		AMDGPU_GEM_CREATE_CPU_GTT_USWC;
> -	bp->bo_ptr_size = sizeof(struct amdgpu_bo);
> +	bp.bo_ptr_size = sizeof(struct amdgpu_bo);
>   	if (vm->use_cpu_for_update)
> -		bp->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> +		bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>   	else if (!vm->root.base.bo || vm->root.base.bo->shadow)
> -		bp->flags |= AMDGPU_GEM_CREATE_SHADOW;
> -	bp->type = ttm_bo_type_kernel;
> -	bp->no_wait_gpu = immediate;
> +		create_shadow = true;
> +
> +	bp.type = ttm_bo_type_kernel;
> +	bp.no_wait_gpu = immediate;
>   	if (vm->root.base.bo)
> -		bp->resv = vm->root.base.bo->tbo.base.resv;
> +		bp.resv = vm->root.base.bo->tbo.base.resv;
> +
> +
> +	r = amdgpu_bo_create(adev, &bp, bo);
> +	if (r)
> +		return r;
> +	if (!vm->is_compute_context &&
> +	    !(adev->flags & AMD_IS_APU) &&
> +	    create_shadow) {

Better drop the create_show flag and just always check it like this:

if (vm->is_compute_context || adev->flags & AMD_IS_APU)
     return 0;

Apart from that looks good to me.

Christian.

> +		if (!bp.resv)
> +			WARN_ON(dma_resv_lock((*bo)->tbo.base.resv,
> +					      NULL));
> +		r = amdgpu_bo_create_shadow(adev, bp.size, *bo);
> +
> +		if (!bp.resv)
> +			dma_resv_unlock((*bo)->tbo.base.resv);
> +
> +		if (r) {
> +			amdgpu_bo_unref(bo);
> +			return r;
> +		}
> +	}
> +
> +	return 0;
>   }
>   
>   /**
> @@ -901,7 +930,6 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   			       bool immediate)
>   {
>   	struct amdgpu_vm_pt *entry = cursor->entry;
> -	struct amdgpu_bo_param bp;
>   	struct amdgpu_bo *pt;
>   	int r;
>   
> @@ -919,9 +947,7 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   	if (entry->base.bo)
>   		return 0;
>   
> -	amdgpu_vm_bo_param(adev, vm, cursor->level, immediate, &bp);
> -
> -	r = amdgpu_bo_create(adev, &bp, &pt);
> +	r = amdgpu_vm_bo_create(adev, vm, cursor->level, immediate, &pt);
>   	if (r)
>   		return r;
>   
> @@ -2785,7 +2811,6 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout)
>   int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		   int vm_context, u32 pasid)
>   {
> -	struct amdgpu_bo_param bp;
>   	struct amdgpu_bo *root;
>   	int r, i;
>   
> @@ -2843,10 +2868,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	mutex_init(&vm->eviction_lock);
>   	vm->evicting = false;
>   
> -	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, &bp);
> -	if (vm->is_compute_context)
> -		bp.flags &= ~AMDGPU_GEM_CREATE_SHADOW;
> -	r = amdgpu_bo_create(adev, &bp, &root);
> +	r = amdgpu_vm_bo_create(adev, vm, adev->vm_manager.root_level,
> +				false, &root);
>   	if (r)
>   		goto error_free_delayed;
>   

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx