Re: [PATCH 2/3] drm/amdgpu: amdgpu_vm_fini hold vm lock to access vm->va

[Philip Yang <yangp@amd.com>]

On 2025-05-15 10:40, Chen, Xiaogang wrote:
>
> On 5/14/2025 12:10 PM, Philip Yang wrote:
>> Move vm root bo unreserve after vm->va mapping free because we should
>> hold vm lock to access vm->va.
>>
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 3911c78f8282..fb5baa6ec32d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2740,10 +2740,6 @@ void amdgpu_vm_fini(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm)
>>       }
>>         amdgpu_vm_pt_free_root(adev, vm);
>> -    amdgpu_bo_unreserve(root);
>> -    amdgpu_bo_unref(&root);
>> -    WARN_ON(vm->root.bo);
>> -
>>       amdgpu_vm_fini_entities(vm);
>>         if (!RB_EMPTY_ROOT(&vm->va.rb_root)) {
>> @@ -2758,6 +2754,10 @@ void amdgpu_vm_fini(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm)
>>           kfree(mapping);
>>       }
>>   +    amdgpu_bo_unreserve(root);
>> +    amdgpu_bo_unref(&root);
>> +    WARN_ON(vm->root.bo);
>> +
>
> if the places of unresrved/unref vm root here do matter putting them 
> at end of this function may look more reasonable.

No, this change doesn't fix any issue, as Christian stated, we should 
not have concurrent using of vm inside this function, hold root bo lock 
is only required by assert in amdgpu_vm_pt_free_root. I will drop this 
patch.

Regards,

Philip

>
> Regards
>
> Xiaogang
>
>
>>       dma_fence_put(vm->last_update);
>>         for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {