On 8/28/2026 9:08 PM, Christian König wrote: > On 8/28/26 11:53, Zhu Lingshan wrote: >> This commit introduces a new helper >> amdgpu_lookup_queue_by_doorbell which helps >> look up a user queue with the given doorbell id >> in a xarray. >> >> This function takes a kref of the user space queue.
Hello Christian Thanks for your comments. > Well absolutely clear NAK to the whole approach. > > This is the nonsense Sunil and I have worked quite hard to remove and we > certainly shouldn't repeat such mistakes. > > When the userq needs to be used from interrupt context we need to hold the > xa_lock_irqsave() or otherwise we don't have any guarantee that the userq, > userq_mgr or associated fpriv went out of scope. Holding the spin lock by xa_lock_irqsave() can surely avoid racing with the destruction process, however, it does not apply to all scenarios, for example, you can not hold spin lock in mes_userq_reset_queue(), because it calls either amdgpu_mes_reset_queue_mmio or amdgpu_mes_reset_queue_mmio, both of them acquire the MES mutex through amdgpu_mes_lock. Another thing, out of the topic is, holding xa_lock does not guarantee fpriv/userq_mgr alive, for example, when drm_device->unplugged is true, all amdgpu teardown paths in amdgpu_drm_release are skipped, and the fpriv/userq_mgr is freed, no matter whether holding the xa spin lock. So IMHO since we have userq->kref, lets use it to maintain the lifecycle of the queues. > > Grabbing references from this side would obviously result in circle > dependencies. I am not sure, we should use the lock/unlock and kref_put/get in pairs in sequence, can you name some circle dependencies or AB-BA lockings as examples? Thanks Lingshan > > Regards, > Christian. > >> Signed-off-by: Zhu Lingshan <[email protected]> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 30 +++++++++++++++++++++++ >> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h | 2 ++ >> 2 files changed, 32 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >> index 0a816b3c5ff9..e0639f844a8e 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c >> @@ -609,6 +609,36 @@ struct amdgpu_usermode_queue *amdgpu_userq_get(struct >> amdgpu_userq_mgr *uq_mgr, >> return queue; >> } >> >> +/** >> + * amdgpu_lookup_queue_by_doorbell - look up a user queue by doorbell >> + * @xa: user queue XArray indexed by doorbell >> + * @doorbell: doorbell index >> + * >> + * Return: A queue with the doorbell indexed, or NULL if no such a queue >> found. >> + * >> + * This function increases kref of the queue, the caller >> + * must release the reference with amdgpu_userq_put(). >> + */ >> +struct amdgpu_usermode_queue * >> +amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell) >> +{ >> + struct amdgpu_usermode_queue *queue; >> + unsigned long flags; >> + >> + xa_lock_irqsave(xa, flags); >> + queue = xa_load(xa, doorbell); >> + if (!queue) >> + goto out_unlock; >> + >> + if (!kref_get_unless_zero(&queue->refcount)) >> + queue = NULL; >> + >> +out_unlock: >> + xa_unlock_irqrestore(xa, flags); >> + >> + return queue; >> +} >> + >> void amdgpu_userq_put(struct amdgpu_usermode_queue *queue) >> { >> if (queue) >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h >> index 6412a7f7b6ef..8fc73862f64e 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h >> @@ -151,6 +151,8 @@ struct amdgpu_db_info { >> }; >> >> struct amdgpu_usermode_queue *amdgpu_userq_get(struct amdgpu_userq_mgr >> *uq_mgr, u32 qid); >> +struct amdgpu_usermode_queue * >> +amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell); >> void amdgpu_userq_put(struct amdgpu_usermode_queue *queue); >> >> int amdgpu_userq_ioctl(struct drm_device *dev, void *data, struct drm_file >> *filp);
