On 6/19/26 07:51, Khatri, Sunil wrote: > > On 18-06-2026 06:59 pm, Christian König wrote: >> On 6/18/26 15:18, Khatri, Sunil wrote: >>> On 18-06-2026 06:12 pm, Christian König wrote: >>>> On 6/18/26 12:51, Zhu Lingshan wrote: >>>>> In amdgpu_userq_restore_all(), when failed to reserve >>>>> a bo, it should return a meaningful error code other than >>>>> "false" that means SUCCESS, which is wrong. >>>>> >>>>> The caller should not ignore the return code of >>>>> amdgpu_userq_restore_all as well >>>>> >>>>> Signed-off-by: Zhu Lingshan <[email protected]> >>>>> --- >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 10 ++++++---- >>>>> 1 file changed, 6 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >>>>> index 95b680fc88c5..8b14870afbf5 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >>>>> @@ -894,9 +894,9 @@ amdgpu_userq_restore_all(struct amdgpu_userq_mgr >>>>> *uq_mgr) >>>>> unsigned long queue_id; >>>>> int ret = 0, r; >>>>> >>>>> - >>>>> - if (amdgpu_bo_reserve(vm->root.bo, false)) >>>>> - return false; >>>>> + r = amdgpu_bo_reserve(vm->root.bo, false); >>>>> + if (r) >>>>> + return r; >>>> Good catch, but that amdgpu_bo_reserve() is called here is a bug in the >>>> first place. >>> I think i probably missed that. There is no return value check for >>> amdgpu_userq_restore_all and this cant fail. We should wait uninterruptible >>> here i.e change false->true and drop the if condition. >> No, dropping and re-acquiring the lock is a broken approach to begin with. >> >>>> The call to amdgpu_userq_vm_validate() must be moved into >>>> amdgpu_userq_vm_validate(), right before we call drm_exec_fini() and the >>>> manual call to amdgpu_bo_reserve() here dropped. >>>> >>>> Otherwise we have a small windows where we drop the BO locks before >>>> starting the queues which could make the VM invalid again and cause all >>>> kind of issues. >>> we need to maintain order of locking First reserve root bo and then take >>> mutex else we had mutex deadlocks.... this is why we have added root bo >>> locking first before taking mutex in next line. >> As long as we do that in amdgpu_userq_vm_validate() that should be >> unproblematic. The VM root BO and all other BOs are still locked and >> acquiring the userq_mutex shouldn't be much of a problem. > > Are you suggesting to move amdgpu_userq_restore_all inside the > amdgpu_userq_vm_validate? Current order is we amdgpu_userq_vm_validate and > then amdgpu_userq_restore_all. So if i get it correct, while we have the exec > locks just before exec_fini, after amdgpu_evf_mgr_rearm, take mutex and call > amdgpu_userq_restore_all?
Yes, exactly that. Christian. > > Regards > Sunil Khatri > >> Regards, >> Christian. >> >>> Regards >>> >>> Sunil Khatri >>> >>>> Regards, >>>> Christian. >>>> >>>>> >>>>> mutex_lock(&uq_mgr->userq_mutex); >>>>> /* Resume all the queues for this process */ >>>>> @@ -1133,7 +1133,9 @@ static void amdgpu_userq_restore_worker(struct >>>>> work_struct *work) >>>>> goto put_fence; >>>>> } >>>>> >>>>> - amdgpu_userq_restore_all(uq_mgr); >>>>> + ret = amdgpu_userq_restore_all(uq_mgr); >>>>> + if (ret) >>>>> + drm_file_err(uq_mgr->file, "Failed to restore user queues, >>>>> ret=%d\n", ret); >>>>> >>>>> put_fence: >>>>> dma_fence_put(ev_fence);
