On 19-06-2026 12:59 pm, Christian König wrote:
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.
Let me work on that and push another patch set on this.
Regards
Sunil Khatri
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);
