On 25.09.25 12:32, Jesse.Zhang wrote:
> As KFD no longer uses a separate PASID, the global
> amdgpu_vm_set_pasid()function is no longer necessary.
> Merge its functionality directly intoamdgpu_vm_init() to simplify code flow
> and eliminate redundant locking.
>
> Suggested-by: Christian König <[email protected]>
> Signed-off-by: Jesse Zhang <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 10 ++-------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 30 +++++++++++++++++++++++--
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 5 +----
> 3 files changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 8676400834fc..a9327472c651 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1421,14 +1421,10 @@ int amdgpu_driver_open_kms(struct drm_device *dev,
> struct drm_file *file_priv)
>
> amdgpu_debugfs_vm_init(file_priv);
>
> - r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id);
> + r = amdgpu_vm_init(adev, &fpriv->vm, fpriv->xcp_id, pasid);
> if (r)
> goto error_pasid;
>
> - r = amdgpu_vm_set_pasid(adev, &fpriv->vm, pasid);
> - if (r)
> - goto error_vm;
> -
> fpriv->prt_va = amdgpu_vm_bo_add(adev, &fpriv->vm, NULL);
> if (!fpriv->prt_va) {
> r = -ENOMEM;
> @@ -1468,10 +1464,8 @@ int amdgpu_driver_open_kms(struct drm_device *dev,
> struct drm_file *file_priv)
> amdgpu_vm_fini(adev, &fpriv->vm);
>
> error_pasid:
> - if (pasid) {
> + if (pasid)
> amdgpu_pasid_free(pasid);
> - amdgpu_vm_set_pasid(adev, &fpriv->vm, 0);
> - }
>
> kfree(fpriv);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 108d2a838ef0..6a4902c972cf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -149,7 +149,7 @@ static void amdgpu_vm_assert_locked(struct amdgpu_vm *vm)
> * pasid by passing in zero.
> *
> */
> -int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> +static int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm
> *vm,
> u32 pasid)
> {
> int r;
> @@ -2552,6 +2552,7 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
> * @adev: amdgpu_device pointer
> * @vm: requested vm
> * @xcp_id: GPU partition selection id
> + * @pasid: the pasid the VM is using on this GPU
> *
> * Init @vm fields.
> *
> @@ -2559,7 +2560,7 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
> * 0 for success, error for failure.
> */
> int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> - int32_t xcp_id)
> + int32_t xcp_id, uint32_t pasid)
> {
> struct amdgpu_bo *root_bo;
> struct amdgpu_bo_vm *root;
> @@ -2636,12 +2637,37 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct
> amdgpu_vm *vm,
> if (r)
> dev_dbg(adev->dev, "Failed to create task info for VM\n");
>
> + if (vm->pasid != pasid) {
That check is superflous and maybe even a bit dangerous.
When amdgpu_vm_init() is called the memory backing the VM is freshly allocated.
> + /* Erase old PASID from XArray (if non-zero) */
> + if (vm->pasid != 0) {
> + r = xa_err(xa_erase_irq(&adev->vm_manager.pasids,
> vm->pasid));
> + if (r < 0)
> + goto error_free_root;
> +
> + vm->pasid = 0;
> + }
> +
> + /* Store new PASID in XArray (if non-zero) */
> + if (pasid != 0) {
> + r = xa_err(xa_store_irq(&adev->vm_manager.pasids,
> pasid, vm, GFP_KERNEL));
> + if (r < 0)
> + goto error_free_root;
> +
> + vm->pasid = pasid;
Just this chunk here is needed, maybe with the "if (pasid != 0) check around it.
You should probably also adjust amdgpu_vm_fini() and remove
amdgpu_vm_set_pasid().
Apart from that looks good to me,
Christian.
> + }
> + }
> +
> amdgpu_bo_unreserve(vm->root.bo);
> amdgpu_bo_unref(&root_bo);
>
> return 0;
>
> error_free_root:
> + /* If PASID was partially set, erase it from XArray before failing */
> + if (vm->pasid != 0) {
> + xa_erase_irq(&adev->vm_manager.pasids, vm->pasid);
> + vm->pasid = 0;
> + }
> amdgpu_vm_pt_free_root(adev, vm);
> amdgpu_bo_unreserve(vm->root.bo);
> amdgpu_bo_unref(&root_bo);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 3b9d583358b0..556d0483c6bc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -503,11 +503,8 @@ extern const struct amdgpu_vm_update_funcs
> amdgpu_vm_sdma_funcs;
> void amdgpu_vm_manager_init(struct amdgpu_device *adev);
> void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
>
> -int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> - u32 pasid);
> -
> long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout);
> -int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t
> xcp_id);
> +int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, int32_t
> xcp_id, uint32_t 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);
> int amdgpu_vm_lock_pd(struct amdgpu_vm *vm, struct drm_exec *exec,
