>> + size_t mqd_size;
>> + uint32_t num_queues = 0;
>> + int ret = 0;
>> + int i = 0;
> On a glance ret does not need to be initialized and i would potentially
> be better done so above the first block which uses it, in order to match
> the pattern of the second user.
Thanks, fixed
>> +
>> + mutex_lock(&uq_mgr->userq_mutex);
> Design question is whether there is any benefit from the mutex.
> Alternative could be to simply abort the attempt if the number of queues
> changed between counting to filling the data. For CRIU that's fine since
> everything will be idled. And for non-CRIU it doesn't matter since the
> by the time the query returns the data may be stale anyway.
> In which case not extending the scope of the mutex would simplify the
> previous patch and this one.
On reconsideration, I'm having both LIST and CHANGE_IF hold the
xa_lock for their entire duration, to avoid any shenanigans with
things being removed or created under them.
I am also having them hold the userq_mutex. I don't think this is
strictly necessary since a userq cannot be freed while it is in the
xa_array but no reason not to.
>> + mutex_unlock(&uq_mgr->userq_mutex);
>> + if (args->list_in_out.num_entries == 0)
>> + return 0;
>> + return -EINVAL;
> Why -EINVAL? It differs from the established practice from
> amdgpu_gem_list_handles_ioctl for example.
Thanks, Fixed.
>> + entries = kvmalloc_array(num_queues, sizeof(*entries), GFP_KERNEL);
> kvmalloc_objs is preferred these days.
Thanks, fixed
>> + ret = copy_from_user(entries,
>> u64_to_user_ptr(args->list_in_out.entries),
>> + num_queues * sizeof(*entries));
> Multiplication can overflow on 32-bit so best would be to use
> check_mul_oveflow and stash the total size into a size_t local.
Thanks, fixed
>> + xa_for_each(&uq_mgr->userq_xa, queue_id, queue) {
>> + mqd_size = 0;
> Move to else?
> ...
> Emit a warning and fail if unknown queue type is encountered?
> ...
> Why not break out?
>
> Or could you even merge this into the main xa_for_each below? Simply
> walk it and branch between good and bad mqd_sizes, each filling their
> respecive block.
I think there's enough shared handling for two ifs, but definitely close
Making invalid mqd type an error.
>> + if (queue->queue_type == AMDGPU_HW_IP_COMPUTE)
>> + mqd_size = sizeof(struct
>> drm_amdgpu_userq_mqd_compute_gfx11);
> Mention of gfx11 feels a bit out of place - how will this work for
future extensions?
Despite the names these are currently the structs for both gfx11 and gfx12
If gfx13 is different this will need to check for it somehow.
>> + ret = -EFAULT;
>> + goto exit;
> So userspace gets a success but entries[] has only been partially
> filled? How is userspace expected to detect and handle that? By looking
> if any entry mqd_size returned different than the one it passed in?
Reconsidering this interface. Now, each entry is handled independently.
Some might have correct mqd and get filled while others have incorrect
mqd size and get just the size.
Seems the most consistent way.
>> + (queue->doorbell_index -
>> amdgpu_bo_gpu_offset_no_check(queue->db_obj.obj) / sizeof(u32))
>> + / DIV_ROUND_UP(sizeof(u64), 4);
> What is the point of DIV_ROUND_UP here?
Whoops, fixed (and made this comment clearer)
>> + if (queue->queue_type == AMDGPU_HW_IP_COMPUTE) {
>> + struct drm_amdgpu_userq_mqd_compute_gfx11 compute_mqd
>> = {0};
> = {} is preferred I think, or maybe better, all initializers in this
> blow and the ones below can be made at declaration time which would be a
> bit more compact.
Near as I can tell amdgpu uses {0}. They're the same thing in C as far as I
know.
>> + if (ret) {
>> + ret = -EFAULT;
>> + goto exit;
>> + }
> if (ret) etc block could be moved to a single one after the if-elseif-
ladder.
Thanks, fixed
>> + if (r)
>> + drm_file_err(filp, "Failed to get list of usermode
>> queues\n");
> Drop this - we don't want to allow naughty userspace spam the kernel log
at will.
Thanks, fixed.
>> + /** AMDGPU_USERQ_OP_LIST */
>> + __u32 op;
> At first I wanted to ask what is this for, that it seems unused, but
> then I realized it is for..
> .. the union! And that the list_in_out.op is needed to match the exact
> position of in.op. A bit evil but I guess it works.
Agreed that this is slightly evil but it's what needs to be done.
________________________________________
From: Tvrtko Ursulin <[email protected]>
Sent: Wednesday, August 5, 2026 10:11 AM
To: Francis, David; [email protected]
Subject: Re: [PATCH 2/3] drm/amdgpu: Add LIST option to USERQ ioctl
On 23/07/2026 19:50, David Francis wrote:
> Add a new option to ioctl USERQ which provides information
> to a process about their own user queues on the queried
> device.
>
> The returned data is in the same format as that used to
> create the queue in the first place.
>
> The interface uses retries; the user first sends a request with
> num_entries 0, then receives the right number of entries.
> The same is done for the mqd sizes of each entry.
>
> Signed-off-by: David Francis <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 159 ++++++++++++++++++++++
> include/uapi/drm/amdgpu_drm.h | 39 ++++++
> 2 files changed, 198 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index cfee7466ef44..b13ed7f8be7f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -850,6 +850,8 @@ static int amdgpu_userq_input_args_validate(struct
> drm_device *dev,
> args->in.mqd_size)
> return -EINVAL;
> break;
> + case AMDGPU_USERQ_OP_LIST:
> + break;
> default:
> return -EINVAL;
> }
> @@ -857,6 +859,158 @@ static int amdgpu_userq_input_args_validate(struct
> drm_device *dev,
> return 0;
> }
>
> +static int
> +amdgpu_userq_list(struct drm_file *filp, union drm_amdgpu_userq *args)
> +{
> + struct amdgpu_fpriv *fpriv = filp->driver_priv;
> + struct amdgpu_userq_mgr *uq_mgr = &fpriv->userq_mgr;
> + struct drm_amdgpu_userq_list_entry *entries;
> + struct amdgpu_usermode_queue *queue;
> + bool mqd_sizes_correct = true;
> + unsigned long queue_id;
> + size_t mqd_size;
> + uint32_t num_queues = 0;
> + int ret = 0;
> + int i = 0;
On a glance ret does not need to be initialized and i would potentially
be better done so above the first block which uses it, in order to match
the pattern of the second user.
> +
> + mutex_lock(&uq_mgr->userq_mutex);
Design question is whether there is any benefit from the mutex.
Alternative could be to simply abort the attempt if the number of queues
changed between counting to filling the data. For CRIU that's fine since
everything will be idled. And for non-CRIU it doesn't matter since the
by the time the query returns the data may be stale anyway.
In which case not extending the scope of the mutex would simplify the
previous patch and this one.
> +
> + xa_for_each(&uq_mgr->userq_xa, queue_id, queue) {
> + num_queues += 1;
> + }
> +
> + if (num_queues != args->list_in_out.num_entries) {
> + /**
> + * If the num_entries is not the number of queues,
> + * return the correct number. User should
> + * try again with the right space allocated.
> + */
> + args->list_in_out.num_entries = num_queues;
> + mutex_unlock(&uq_mgr->userq_mutex);
> + if (args->list_in_out.num_entries == 0)
> + return 0;
> + return -EINVAL;
Why -EINVAL? It differs from the established practice from
amdgpu_gem_list_handles_ioctl for example.
> + }
> + if (num_queues == 0) {
> + mutex_unlock(&uq_mgr->userq_mutex);
> + return 0;
> + }
> +
> + entries = kvmalloc_array(num_queues, sizeof(*entries), GFP_KERNEL);
kvmalloc_objs is preferred these days.
> +
> + if (!entries) {
> + mutex_unlock(&uq_mgr->userq_mutex);
> + return -ENOMEM;
> + }
> +
> + ret = copy_from_user(entries,
> u64_to_user_ptr(args->list_in_out.entries),
> + num_queues * sizeof(*entries));
Multiplication can overflow on 32-bit so best would be to use
check_mul_oveflow and stash the total size into a size_t local.
> +
> + if (ret) {
> + ret = -EFAULT;
> + goto exit;
> + }
> +
> + xa_for_each(&uq_mgr->userq_xa, queue_id, queue) {
> + mqd_size = 0;
Move to else?
> + /**
> + * Check mqd size. As with num_entries, return the right sizes
> + * if they are not correct. These sizes also serve as
> + * versioning for the mqd.
> + */
> + if (queue->queue_type == AMDGPU_HW_IP_COMPUTE)
> + mqd_size = sizeof(struct
> drm_amdgpu_userq_mqd_compute_gfx11);
Mention of gfx11 feels a bit out of place - how will this work for
future extensions?
> + else if (queue->queue_type == AMDGPU_HW_IP_GFX)
> + mqd_size = sizeof(struct drm_amdgpu_userq_mqd_gfx11);
> + else if (queue->queue_type == AMDGPU_HW_IP_DMA)
> + mqd_size = sizeof(struct
> drm_amdgpu_userq_mqd_sdma_gfx11);
Emit a warning and fail if unknown queue type is encountered?
> +
> + if (mqd_size != entries[i].mqd_size) {
> + mqd_sizes_correct = false;
> + entries[i].mqd_size = mqd_size;
> + entries[i].ip_type = queue->queue_type;
Why not break out?
Or could you even merge this into the main xa_for_each below? Simply
walk it and branch between good and bad mqd_sizes, each filling their
respecive block.
> + }
> + i += 1;
> + }
> + if (!mqd_sizes_correct) {
> + ret = copy_to_user(u64_to_user_ptr(args->list_in_out.entries),
> entries,
> + num_queues * sizeof(*entries));
> + if (ret)
> + ret = -EFAULT;
> + goto exit;
So userspace gets a success but entries[] has only been partially
filled? How is userspace expected to detect and handle that? By looking
if any entry mqd_size returned different than the one it passed in?
> + }
> + i = 0;
> + xa_for_each(&uq_mgr->userq_xa, queue_id, queue) {
> + entries[i].queue_id = queue_id;
> + entries[i].ip_type = queue->queue_type;
> + /* userq prop handling */
> + entries[i].queue_va = queue->userq_prop->hqd_base_gpu_addr;
> + entries[i].queue_size = queue->userq_prop->queue_size;
> + entries[i].rptr_va = queue->userq_prop->rptr_gpu_addr;
> + entries[i].wptr_va = queue->userq_prop->wptr_gpu_addr;
> + /* flag handling (AMDGPU_USERQ_CREATE_FLAGS_QUEUE_PRIORITY_MASK
> is the only flag in use) */
> + entries[i].flags = (queue->priority <<
> AMDGPU_USERQ_CREATE_FLAGS_QUEUE_PRIORITY_SHIFT)
> + & AMDGPU_USERQ_CREATE_FLAGS_QUEUE_PRIORITY_MASK;
> + /* doorbell handling */
> + entries[i].doorbell_handle = queue->doorbell_handle;
> + /* This is the inverse of the calculation used in
> amdgpu_userq_get_doorbell_index */
> + entries[i].doorbell_offset =
> + (queue->doorbell_index -
> amdgpu_bo_gpu_offset_no_check(queue->db_obj.obj) / sizeof(u32))
> + / DIV_ROUND_UP(sizeof(u64), 4);
What is the point of DIV_ROUND_UP here?
> +
> + if (queue->queue_type == AMDGPU_HW_IP_COMPUTE) {
> + struct drm_amdgpu_userq_mqd_compute_gfx11 compute_mqd =
> {0};
= {} is preferred I think, or maybe better, all initializers in this
blow and the ones below can be made at declaration time which would be a
bit more compact.
> +
> + compute_mqd.eop_va = queue->userq_prop->eop_gpu_addr;
> +
> + ret = copy_to_user(u64_to_user_ptr(entries[i].mqd_data),
> + &compute_mqd,
> + entries[i].mqd_size);
> +
> + if (ret) {
> + ret = -EFAULT;
> + goto exit;
> + }
if (ret) etc block could be moved to a single one after the if-elseif-
ladder.
> + } else if (queue->queue_type == AMDGPU_HW_IP_GFX) {
> + struct drm_amdgpu_userq_mqd_gfx11 mqd_gfx_v11 = {0};
> +
> + mqd_gfx_v11.shadow_va = queue->userq_prop->shadow_addr;
> + mqd_gfx_v11.csa_va = queue->userq_prop->csa_addr;
> +
> + ret = copy_to_user(u64_to_user_ptr(entries[i].mqd_data),
> + &mqd_gfx_v11,
> + entries[i].mqd_size);
> + if (ret) {
> + ret = -EFAULT;
> + goto exit;
> + }
> + } else if (queue->queue_type == AMDGPU_HW_IP_DMA) {
> + struct drm_amdgpu_userq_mqd_sdma_gfx11 mqd_sdma_v11 =
> {0};
> +
> + mqd_sdma_v11.csa_va = queue->userq_prop->csa_addr;
> +
> + ret = copy_to_user(u64_to_user_ptr(entries[i].mqd_data),
> + &mqd_sdma_v11,
> + entries[i].mqd_size);
> + if (ret) {
> + ret = -EFAULT;
> + goto exit;
> + }
> + }
> + i += 1;
> + }
> + ret = copy_to_user(u64_to_user_ptr(args->list_in_out.entries), entries,
> + num_queues * sizeof(*entries));
> + if (ret) {
> + ret = -EFAULT;
> + goto exit;
> + }
> +exit:
> + mutex_unlock(&uq_mgr->userq_mutex);
> + kvfree(entries);
> + return ret;
> +}
> +
> bool amdgpu_userq_enabled(struct drm_device *dev)
> {
> struct amdgpu_device *adev = drm_to_adev(dev);
> @@ -901,6 +1055,11 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void
> *data,
>
> amdgpu_userq_put(queue);
> break;
> + case AMDGPU_USERQ_OP_LIST:
> + r = amdgpu_userq_list(filp, args);
> + if (r)
> + drm_file_err(filp, "Failed to get list of usermode
> queues\n");
Drop this - we don't want to allow naughty userspace spam the kernel log
at will.
> + break;
> default:
> drm_dbg_driver(dev, "Invalid user queue op specified: %d\n",
> args->in.op);
> return -EINVAL;
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index b32c72a662b6..678f3d531df7 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -332,6 +332,7 @@ union drm_amdgpu_ctx {
> /* user queue IOCTL operations */
> #define AMDGPU_USERQ_OP_CREATE 1
> #define AMDGPU_USERQ_OP_FREE 2
> +#define AMDGPU_USERQ_OP_LIST 3
>
> /* queue priority levels */
> /* low < normal low < normal high < high */
> @@ -425,9 +426,47 @@ struct drm_amdgpu_userq_out {
> __u32 _pad;
> };
>
> +struct drm_amdgpu_userq_list_entry {
> + /** Definitions same as drm_amdgpu_userq_in */
> + __u32 queue_id;
> + __u32 ip_type;
> + __u32 doorbell_handle;
> + __u32 doorbell_offset;
> + __u32 flags;
> + __u64 queue_va;
> + __u64 queue_size;
> + __u64 rptr_va;
> + __u64 wptr_va;
> + /** Userspace pointer to buffer holding mqd */
> + __u64 mqd_data;
> + /**
> + * In: Size of mqd_data user-allocated buffer.
> + * Out: If mqd_data was insufficiently large, the
> + * size it needs to be.
> + */
> + __u64 mqd_size;
> +};
> +
> +struct drm_amdgpu_userq_list_in_out {
> + /**
> + * For operation AMDGPU_USERQ_OP_LIST: User will provide a buffer,
> which the
> + * driver will fill with information about all of that process's queues
> on this device.
> + */
> + /** AMDGPU_USERQ_OP_LIST */
> + __u32 op;
At first I wanted to ask what is this for, that it seems unused, but
then I realized it is for..
> + /**
> + * Size of entries buffer / Number of handles in process
> + * (if larger than size of buffer, must retry)
> + */
> + __u32 num_entries;
> + /* User pointer to array of drm_amdgpu_userq_list_entry */
> + __u64 entries;
> +};
> +
> union drm_amdgpu_userq {
> struct drm_amdgpu_userq_in in;
> struct drm_amdgpu_userq_out out;
> + struct drm_amdgpu_userq_list_in_out list_in_out;
.. the union! And that the list_in_out.op is needed to match the exact
position of in.op. A bit evil but I guess it works.
Regards,
Tvrtko
> };
>
> /* GFX V11 IP specific MQD parameters */
