See comments inline ...
Regards,
 Felix
On 2018-08-22 05:10 PM, Oak Zeng wrote:
> Signed-off-by: Oak Zeng <Oak.Zeng at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 4 ++--
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8 ++++----
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 20 +++++++++++++++++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 +-
> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 4 ++--
> drivers/gpu/drm/amd/include/kgd_kfd_interface.h | 4 ++--
> 6 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index a8418a3..8939f54 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -157,11 +157,11 @@ uint64_t amdgpu_amdkfd_get_vram_usage(struct kgd_dev *kgd);
> /* GPUVM API */
> int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, void **vm,
> void **process_info,
> - struct dma_fence **ef);
> + struct dma_fence **ef, unsigned int pasid);
vm, process_info and ef are output parameters. pasid is an input
parameter. I'd add that before the output parameters.
> int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
> struct file *filp,
> void **vm, void **process_info,
> - struct dma_fence **ef);
> + struct dma_fence **ef, unsigned int pasid);
Same as above.
> void amdgpu_amdkfd_gpuvm_destroy_cb(struct amdgpu_device *adev,
> struct amdgpu_vm *vm);
> void amdgpu_amdkfd_gpuvm_destroy_process_vm(struct kgd_dev *kgd, void *vm);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 7eadc58..659c397 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1005,7 +1005,7 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void **process_info,
>
> int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, void **vm,
> void **process_info,
> - struct dma_fence **ef)
> + struct dma_fence **ef, unsigned int pasid)
> {
> struct amdgpu_device *adev = get_amdgpu_device(kgd);
> struct amdgpu_vm *new_vm;
> @@ -1016,7 +1016,7 @@ int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, void **vm,
> return -ENOMEM;
>
> /* Initialize AMDGPU part of the VM */
> - ret = amdgpu_vm_init(adev, new_vm, AMDGPU_VM_CONTEXT_COMPUTE, 0);
> + ret = amdgpu_vm_init(adev, new_vm, AMDGPU_VM_CONTEXT_COMPUTE, pasid);
> if (ret) {
> pr_err("Failed init vm ret %d\n", ret);
> goto amdgpu_vm_init_fail;
> @@ -1041,7 +1041,7 @@ int amdgpu_amdkfd_gpuvm_create_process_vm(struct kgd_dev *kgd, void **vm,
> int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
> struct file *filp,
> void **vm, void **process_info,
> - struct dma_fence **ef)
> + struct dma_fence **ef, unsigned int pasid)
> {
> struct amdgpu_device *adev = get_amdgpu_device(kgd);
> struct drm_file *drm_priv = filp->private_data;
> @@ -1054,7 +1054,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
> return -EINVAL;
>
> /* Convert VM into a compute VM */
> - ret = amdgpu_vm_make_compute(adev, avm);
> + ret = amdgpu_vm_make_compute(adev, avm, pasid);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 662aec5..0f6b304 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2690,7 +2690,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> * Returns:
> * 0 for success, -errno for errors.
> */
> -int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> +int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, unsigned int pasid)
> {
> bool pte_support_ats = (adev->asic_type == CHIP_RAVEN);
> int r;
> @@ -2705,6 +2705,18 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> goto error;
> }
>
> + if (vm->pasid) {
This condition should be if (pasid). Or if we always expect a valid
pasid, then you don't need the condition at all.
> + unsigned long flags;
> +
> + spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> + r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid + 1,
> + GFP_ATOMIC);
> + spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> +
> + if (r < 0)
> + goto error;
> + }
> +
> /* Check if PD needs to be reinitialized and do it before
> * changing any other state, in case it fails.
> */
> @@ -2713,7 +2725,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> adev->vm_manager.root_level,
> pte_support_ats);
> if (r)
> - goto error;
> + goto error_idr_free;
> }
>
> /* Update VM state */
> @@ -2732,9 +2744,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
> spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>
> - vm->pasid = 0;
> + vm->pasid = pasid;
This is in the wrong place, because you only assign vm->pasid if the VM
had a pasid to begin with.
> }
>
> +error_idr_free:
> + idr_remove(&adev->vm_manager.pasid_idr, pasid);
> error:
> amdgpu_bo_unreserve(vm->root.base.bo);
> return r;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 1162c2b..c68945d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -294,7 +294,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);
> +int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, unsigned int pasid);
> 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);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 1d80b4f..19541b2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -686,10 +686,10 @@ int kfd_process_device_init_vm(struct kfd_process_device *pdd,
> if (drm_file)
> ret = dev->kfd2kgd->acquire_process_vm(
> dev->kgd, drm_file,
> - &pdd->vm, &p->kgd_process_info, &p->ef);
> + &pdd->vm, &p->kgd_process_info, &p->ef, p->pasid);
> else
> ret = dev->kfd2kgd->create_process_vm(
> - dev->kgd, &pdd->vm, &p->kgd_process_info, &p->ef);
> + dev->kgd, &pdd->vm, &p->kgd_process_info, &p->ef, p->pasid);
> if (ret) {
> pr_err("Failed to create process VM object\n");
> return ret;
> diff --git a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> index 5733fbe..0171533 100644
> --- a/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_kfd_interface.h
> @@ -347,9 +347,9 @@ struct kfd2kgd_calls {
> uint64_t (*get_vram_usage)(struct kgd_dev *kgd);
>
> int (*create_process_vm)(struct kgd_dev *kgd, void **vm,
> - void **process_info, struct dma_fence **ef);
> + void **process_info, struct dma_fence **ef, unsigned int pasid);
> int (*acquire_process_vm)(struct kgd_dev *kgd, struct file *filp,
> - void **vm, void **process_info, struct dma_fence **ef);
> + void **vm, void **process_info, struct dma_fence **ef, unsigned int pasid);
> void (*destroy_process_vm)(struct kgd_dev *kgd, void *vm);
> uint32_t (*get_process_page_dir)(void *vm);
> void (*set_vm_context_page_table_base)(struct kgd_dev *kgd,
