Re: [PATCH 1/9] drm/amdgpu: lock both VM and BO in amdgpu_gem_object_open

["Christian König" <christian.koenig@amd.com>]

On 2/6/26 11:21, Khatri, Sunil wrote:
> 
> On 02-02-2026 06:21 pm, Christian König wrote:
>> The VM was not locked in the past since we initially only cleared the
>> linked list element and not added it to any VM state.
>>
>> But this has changed quite some time ago, we just never realized this
>> problem because the VM state lock was masking it.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 19 +++++++++++-----
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       | 22 ++++++++++++++-----
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c       | 10 +++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        |  2 ++
>>   4 files changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index 768998c82b43..ec5130497743 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -878,6 +878,7 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>>       struct amdgpu_bo *bo[2] = {NULL, NULL};
>>       struct amdgpu_bo_va *bo_va;
>>       bool same_hive = false;
>> +    struct drm_exec exec;
>>       int i, ret;
>>         if (!va) {
>> @@ -958,19 +959,25 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>>               goto unwind;
>>           }
>>   -        /* Add BO to VM internal data structures */
>> -        ret = amdgpu_bo_reserve(bo[i], false);
>> -        if (ret) {
>> -            pr_debug("Unable to reserve BO during memory attach");
>> -            goto unwind;
>> +        drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT, 0);
>> +        drm_exec_until_all_locked(&exec) {
>> +            ret = amdgpu_vm_lock_pd(vm, &exec, 0);
>> +            drm_exec_retry_on_contention(&exec);
>> +            if (unlikely(ret))
>> +                goto unwind;
>> +            ret = drm_exec_lock_obj(&exec, &bo[i]->tbo.base);
>> +            drm_exec_retry_on_contention(&exec);
>> +            if (unlikely(ret))
>> +                goto unwind;
>>           }
>> +
>>           bo_va = amdgpu_vm_bo_find(vm, bo[i]);
>>           if (!bo_va)
>>               bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]);
>>           else
>>               ++bo_va->ref_count;
>>           attachment[i]->bo_va = bo_va;
>> -        amdgpu_bo_unreserve(bo[i]);
>> +        drm_exec_fini(&exec);
>>           if (unlikely(!attachment[i]->bo_va)) {
>>               ret = -ENOMEM;
>>               pr_err("Failed to add BO object to VM. ret == %d\n",
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 5f9fa2140f09..5c90de58cc28 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -232,6 +232,7 @@ static int amdgpu_gem_object_open(struct drm_gem_object *obj,
>>       struct amdgpu_vm *vm = &fpriv->vm;
>>       struct amdgpu_bo_va *bo_va;
>>       struct mm_struct *mm;
>> +    struct drm_exec exec;
>>       int r;
>>         mm = amdgpu_ttm_tt_get_usermm(abo->tbo.ttm);
>> @@ -242,9 +243,18 @@ static int amdgpu_gem_object_open(struct drm_gem_object *obj,
>>           !amdgpu_vm_is_bo_always_valid(vm, abo))
>>           return -EPERM;
>>   -    r = amdgpu_bo_reserve(abo, false);
>> -    if (r)
>> -        return r;
>> +    drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES, 0);
>> +    drm_exec_until_all_locked(&exec) {
>> +        r = drm_exec_prepare_obj(&exec, &abo->tbo.base, 1);
>> +        drm_exec_retry_on_contention(&exec);
>> +        if (unlikely(r))
>> +            goto out_unlock;
>> +
>> +        r = amdgpu_vm_lock_pd(vm, &exec, 0);
>> +        drm_exec_retry_on_contention(&exec);
>> +        if (unlikely(r))
>> +            goto out_unlock;
>> +    }
>>         amdgpu_vm_bo_update_shared(abo);
>>       bo_va = amdgpu_vm_bo_find(vm, abo);
>> @@ -260,8 +270,7 @@ static int amdgpu_gem_object_open(struct drm_gem_object *obj,
>>           amdgpu_bo_unreserve(abo);
>>           return r;
>>       }
>> -
>> -    amdgpu_bo_unreserve(abo);
>> +    drm_exec_fini(&exec);
>>         /* Validate and add eviction fence to DMABuf imports with dynamic
>>        * attachment in compute VMs. Re-validation will be done by
>> @@ -294,7 +303,10 @@ static int amdgpu_gem_object_open(struct drm_gem_object *obj,
>>           }
>>       }
>>       mutex_unlock(&vm->process_info->lock);
>> +    return r;
>>   +out_unlock:
>> +    drm_exec_fini(&exec);
>>       return r;
>>   }
>>   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 1878e0faa722..f69332eed051 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -1445,6 +1445,7 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>>   {
>>       struct amdgpu_device *adev = drm_to_adev(dev);
>>       struct amdgpu_fpriv *fpriv;
>> +    struct drm_exec exec;
>>       int r, pasid;
>>         /* Ensure IB tests are run on ring */
>> @@ -1484,7 +1485,16 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>>       if (r)
>>           goto error_pasid;
>>   +    drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES, 0);
>> +    drm_exec_until_all_locked(&exec) {
>> +        r = amdgpu_vm_lock_pd(&fpriv->vm, &exec, 0);
> Do we really need to do this here? We are in the process of creating a new VM and new PD so at this time, no one would be able to use the VM and PD till it returns from the function.
> 
> Looking at it with that understanding it feels we should be fine without locking the pd here

The PD is already on TTM BO eviction list at that point and might be locked concurrently.

So yes, we absolutely need that.

Regards,
Christian.

> 
> Regards
> Sunil Khatri
> 
>> +        drm_exec_retry_on_contention(&exec);
>> +        if (unlikely(r))
>> +            goto error_vm;
>> +    }
>> +
>>       fpriv->prt_va = amdgpu_vm_bo_add(adev, &fpriv->vm, NULL);
>> +    drm_exec_fini(&exec);
>>       if (!fpriv->prt_va) {
>>           r = -ENOMEM;
>>           goto error_vm;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 6a2ea200d90c..b4bf1b7c214f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1735,6 +1735,8 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct amdgpu_device *adev,
>>   {
>>       struct amdgpu_bo_va *bo_va;
>>   +    amdgpu_vm_assert_locked(vm);
>> +
>>       bo_va = kzalloc(sizeof(struct amdgpu_bo_va), GFP_KERNEL);
>>       if (bo_va == NULL) {
>>           return NULL;