(Mistyped mailing list) ________________________________________ From: Tvrtko Ursulin <[email protected]> Sent: Friday, September 25, 2026 7:37 AM To: Francis, David; [email protected] Subject: Re: [PATCH v4 1/2] drm/amdgpu: Add LIST option to USERQ ioctl
On 23/09/2026 20:30, 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. Device or file? > 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 can guess at the number > of queues required; if insufficiently large, the correct > value will be returned. > The same is done for the mqd sizes of each entry. > > This operation holds userq_mutex for its entire duration > to prevent the queues from being freed or modified under it. Stale paragraph. > v3: Don't acquire xa_lock and other misc fixes > v4: Don't acquire mutex either and other misc fixes > > Signed-off-by: David Francis <[email protected]> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 145 +++++++++++++++++++++- > include/uapi/drm/amdgpu_drm.h | 40 ++++++ > 2 files changed, 182 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > index 3fe10d6af757..9338f6f4cc5f 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,142 @@ 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; > + unsigned long queue_id; > + size_t mqd_size, entry_buffer_size; > + uint32_t num_queues = 0; > + int ret; > + int i = 0; Nit - for me it reads nicer if the declaration block is sorted by width where possible. Although TBF I do not know if kernel coding style explicitly calls this out. > + > + xa_for_each(&uq_mgr->userq_xa, queue_id, queue) { > + num_queues += 1; > + } Can drop the curlies if you want. > + > + args->list_in_out.num_entries = num_queues; > + /** > + * If the num_entries buffer is too small, > + * return the correct size. User should > + * try again with the right space allocated. > + */ Question and a nit - what criteria do you use to decide on the column width for the comments? Like this one is narrower thatn the one below which is even indented so has less space to begin with. It just looks a bit wonky in my mind when this narrow. > + if (num_queues > args->list_in_out.num_entries) > + return 0; > + > + if (num_queues == 0) > + return 0; > + > + ret = check_mul_overflow(num_queues, sizeof(*entries), > &entry_buffer_size); > + if (ret) { > + ret = -E2BIG; > + goto exit; > + } Can return straight away here. > + > + entries = kvmalloc_objs(typeof(*entries), num_queues, GFP_KERNEL); > + Spurious blank line. > + if (!entries) > + return -ENOMEM; > + > + ret = copy_from_user(entries, > u64_to_user_ptr(args->list_in_out.entries), > + entry_buffer_size); > + if (ret) { > + ret = -EFAULT; > + goto exit; > + } > + > + xa_for_each(&uq_mgr->userq_xa, queue_id, queue) { > + /** > + * The xa may have added or removed queues since we counted > + * them, so break if there are now more queues than entries. > + */ > + if (i >= num_queues) > + break; > + /** > + * Check mqd size. As with num_entries, return the right sizes > + * if they are too small. These sizes also serve as > + * versioning for the mqd. Despite the names, these are the > + * mqd sizes for both gfx11 and gfx12. > + */ > + if (queue->queue_type == AMDGPU_HW_IP_COMPUTE) { > + mqd_size = sizeof(struct > drm_amdgpu_userq_mqd_compute_gfx11); > + } 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); > + } else { > + drm_err_once(adev_to_drm(uq_mgr->adev), "Unrecognized > mqd size in USERQ_OP_LIST"); > + ret = -EINVAL; > + goto exit; > + } > + > + entries[i].mqd_size = mqd_size; > + entries[i].ip_type = queue->queue_type; > + if (mqd_size > entries[i].mqd_size) { I'm afraid I gave you bad advice here and now the condition can never be true. :( Sorry about that. You will need to go back to your previous version, or a hybrid approach of only doing ip_type before the check. > + i += 1; > + continue; > + } > + > + entries[i].queue_id = queue_id; > + /* 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 > are the only flags 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_doorbell_index_on_bar > + * doorbell_index = db_bo_offset / sizeof(u32) > + * + doorbell_offset * DIV_ROUND_UP(db_size, 4) > + * db_size is always sizeof(u64) = 8 > + */ > + entries[i].doorbell_offset = > + (queue->doorbell_index - > amdgpu_bo_gpu_offset_no_check(queue->db_obj.obj) / 4) / 2; > + > + if (queue->queue_type == AMDGPU_HW_IP_COMPUTE) { > + struct drm_amdgpu_userq_mqd_compute_gfx11 compute_mqd = > {0}; > + > + compute_mqd.eop_va = queue->userq_prop->eop_gpu_addr; > + > + ret = copy_to_user(u64_to_user_ptr(entries[i].mqd_data), > + &compute_mqd, mqd_size); > + } 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, mqd_size); > + } 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, 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)); entry_buffer_size instead of the multiplication. The rest looks good to me. Regards, Tvrtko > + if (ret) > + ret = -EFAULT; > +exit: > + kvfree(entries); > + return ret; > +} > + > bool amdgpu_userq_enabled(struct drm_device *dev) > { > struct amdgpu_device *adev = drm_to_adev(dev); > @@ -891,7 +1029,7 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void > *data, > drm_file_err(filp, "Failed to create usermode queue\n"); > break; > > - case AMDGPU_USERQ_OP_FREE: { > + case AMDGPU_USERQ_OP_FREE: > xa_lock(&fpriv->userq_mgr.userq_xa); > queue = __xa_erase(&fpriv->userq_mgr.userq_xa, > args->in.queue_id); > xa_unlock(&fpriv->userq_mgr.userq_xa); > @@ -900,8 +1038,9 @@ 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); > + 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..92738d630eea 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,48 @@ struct drm_amdgpu_userq_out { > __u32 _pad; > }; > > +struct drm_amdgpu_userq_list_entry { > + /** 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. > + */ > + __u32 mqd_size; > + > + /** 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; > +}; > + > +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; > + /** > + * 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; > }; > > /* GFX V11 IP specific MQD parameters */
