[PATCH] drm/amdgpu: have bos for PDs/PTS cpu accessible when kfd uses cpu to update vm

[Christian König]

Am 29.06.23 um 18:09 schrieb Xiaogang.Chen:
> From: Xiaogang Chen <xiaogang.chen at amd.com>
>
> When kfd uses cpu to update vm iterates all current PDs/PTs bos, adds
> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag and kmap them to kernel virtual
> address space before kfd updates the vm that was created by gfx.
>
> Signed-off-by: Xiaogang Chen <Xiaogang.Chen at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c    | 11 ++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h    |  2 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 28 +++++++++++++++++++++++
>   3 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 291977b93b1d..dedf1bf44dc6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2278,17 +2278,14 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   		if (r)
>   			goto unreserve_bo;
>   
> +		r = amdgpu_vm_pt_cpu_access_root(adev, vm);
> +		if (r)
> +			goto unreserve_bo;
> +

Please call that after setting vm->update_funcs.

>   		vm->update_funcs = &amdgpu_vm_cpu_funcs;
>   	} else {
>   		vm->update_funcs = &amdgpu_vm_sdma_funcs;
>   	}
> -	/*
> -	 * Make sure root PD gets mapped. As vm_update_mode could be changed
> -	 * when turning a GFX VM into a compute VM.
> -	 */
> -	r = vm->update_funcs->map_table(to_amdgpu_bo_vm(vm->root.bo));
> -	if (r)
> -		goto unreserve_bo;
>   
>   	dma_fence_put(vm->last_update);
>   	vm->last_update = dma_fence_get_stub();
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 9c85d494f2a2..9b3e75de7c5c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -491,6 +491,8 @@ void amdgpu_vm_pt_free_work(struct work_struct *work);
>   void amdgpu_debugfs_vm_bo_info(struct amdgpu_vm *vm, struct seq_file *m);
>   #endif
>   
> +int amdgpu_vm_pt_cpu_access_root(struct amdgpu_device *adev, struct amdgpu_vm *vm);
> +
>   /**
>    * amdgpu_vm_tlb_seq - return tlb flush sequence number
>    * @vm: the amdgpu_vm structure to query
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> index dea1a64be44d..a08742191b7d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -1044,3 +1044,31 @@ int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params *params,
>   
>   	return 0;
>   }
> +
> +/**
> + * amdgpu_vm_pt_cpu_access_root - have bo of root PD cpu accessible
> + * @adev: amdgpu device structure
> + * @vm: amdgpu vm structure
> + *
> + * make root page directory and everything below it cpu accessible.
> + */
> +int amdgpu_vm_pt_cpu_access_root(struct amdgpu_device *adev, struct amdgpu_vm *vm)

Rename that to amdgpu_vm_pt_map_tables.

> +{
> +	struct amdgpu_vm_pt_cursor cursor;
> +	struct amdgpu_vm_bo_base *entry;
> +	int r;
> +	struct amdgpu_bo_vm *bo;

Please use reverse xmas tree for declarations and declare variables 
local to the block they are used in if possible.

> +
> +	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, NULL, cursor, entry) {
> +
> +		if (entry->bo) {
> +			bo = to_amdgpu_bo_vm(entry->bo);
> +			entry->bo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;

Probably best to move this into amdgpu_vm_cpu_map_table(). This should 
allow us to remove setting the flag while creating page tables as well.

> +			r = amdgpu_vm_cpu_funcs.map_table(bo);


Please use vm->update_funcs->map_table here.

Apart from those minor cleanups the patch looks good from the technical 
side.

Regards,
Christian.

> +			if (r)
> +				return r;
> +		}
> +	}
> +
> +	return 0;
> +}