[PATCH 03/14] drm/amdgpu: Add helper to turn an existing VM into a compute VM

[Christian König]

Am 04.03.2018 um 03:34 schrieb Felix Kuehling:
> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 82 ++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  1 +
>   2 files changed, 83 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 5afbc5e..58153ef 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2350,6 +2350,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	INIT_KFIFO(vm->faults);
>   	vm->fault_credit = 16;
>   
> +	vm->vm_context = vm_context;

I think we should drop the vm_context parameter and all the related code 
in amdgpu_vm_init(). But that can be part of a later cleanup patch as well.

> +
>   	return 0;
>   
>   error_free_root:
> @@ -2364,6 +2366,86 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   }
>   
>   /**
> + * amdgpu_vm_make_compute - Turn a GFX VM into a compute VM
> + *
> + * This only works on GFX VMs that don't have any BOs added and no
> + * page tables allocated yet.
> + *
> + * Changes the following VM parameters:
> + * - vm_context
> + * - use_cpu_for_update
> + * - pte_supports_ats
> + * - pasid (old PASID is released, because compute manages its own PASIDs)
> + *
> + * Reinitializes the page directory to reflect the changed ATS
> + * setting. May leave behind an unused shadow BO for the page
> + * directory when switching from SDMA updates to CPU updates.
> + *
> + * Returns 0 for success, -errno for errors.
> + */
> +int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> +{
> +	bool pte_support_ats = (adev->asic_type == CHIP_RAVEN);
> +	int r;
> +
> +	r = amdgpu_bo_reserve(vm->root.base.bo, true);
> +	if (r)
> +		return r;
> +
> +	/* Sanity checks */
> +	if (vm->vm_context == AMDGPU_VM_CONTEXT_COMPUTE) {
> +		/* Can happen if ioctl is interrupted by a signal after
> +		 * this function already completed. Just return success.
> +		 */
> +		r = 0;
> +		goto error;
> +	}

Ok, that is actually a show stopper. An interrupted IOCTL should never 
have a visible effect.

Is that just a theoretical issue or did you run into this in practice?

Regards,
Christian.

> +	if (!RB_EMPTY_ROOT(&vm->va.rb_root) || vm->root.entries) {
> +		r = -EINVAL;
> +		goto error;
> +	}
> +
> +	/* Check if PD needs to be reinitialized and do it before
> +	 * changing any other state, in case it fails.
> +	 */
> +	if (pte_support_ats != vm->pte_support_ats) {
> +		/* TODO: This depends on a patch series by Christian.
> +		 * It's only needed for GFX9 GPUs, which aren't
> +		 * supported by upstream KFD yet.
> +		r = amdgpu_vm_clear_bo(adev, vm, vm->root.base.bo,
> +			       adev->vm_manager.root_level,
> +			       pte_support_ats);
> +		if (r)
> +			goto error;
> +		*/
> +	}
> +
> +	/* Update VM state */
> +	vm->vm_context = AMDGPU_VM_CONTEXT_COMPUTE;
> +	vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
> +				    AMDGPU_VM_USE_CPU_FOR_COMPUTE);
> +	vm->pte_support_ats = pte_support_ats;
> +	DRM_DEBUG_DRIVER("VM update mode is %s\n",
> +			 vm->use_cpu_for_update ? "CPU" : "SDMA");
> +	WARN_ONCE((vm->use_cpu_for_update & !amdgpu_vm_is_large_bar(adev)),
> +		  "CPU update of VM recommended only for large BAR system\n");
> +
> +	if (vm->pasid) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> +		idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
> +		spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> +
> +		vm->pasid = 0;
> +	}
> +
> +error:
> +	amdgpu_bo_unreserve(vm->root.base.bo);
> +	return r;
> +}
> +
> +/**
>    * amdgpu_vm_free_levels - free PD/PT levels
>    *
>    * @adev: amdgpu device structure
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 40b4e09..7f50a38 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -263,6 +263,7 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev);
>   void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
>   int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		   int vm_context, unsigned int pasid);
> +int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>   void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>   bool amdgpu_vm_pasid_fault_credit(struct amdgpu_device *adev,
>   				  unsigned int pasid);