Re: [PATCH 3/3] drm/amdgpu: Add CHANGE_ID option for USERQ ioctl

On 23/07/2026 19:50, David Francis wrote:
> Add a new option to the USERQ ioctl, which is called with
> the queue_id of an existing user queue and an unused queue_id,
> and changes that queue's id to the new value.
> 
> The operation is fully protected by userq_mutex to avoid races.
> 
> Performing this operation on a queue with signals or waits
> outstanding is fine, as those hold not the queue_id but a
> direct reference to the queue object.
> 
> Signed-off-by: David Francis <[email protected]>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 37 +++++++++++++++++++++++
>   include/uapi/drm/amdgpu_drm.h             | 17 +++++++++--
>   2 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index b13ed7f8be7f..ce084e2cfe53 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -852,6 +852,11 @@ static int amdgpu_userq_input_args_validate(struct drm_device *dev,
>   		break;
>   	case AMDGPU_USERQ_OP_LIST:
>   		break;
> +	case AMDGPU_USERQ_OP_CHANGE_ID:
> +		if (!args->change_in.new_queue_id ||
> +		    args->change_in.new_queue_id > AMDGPU_MAX_USERQ_COUNT)

Maybe also check they are not the same? Currently same id will fail in 
xa_insert with -EBUSY which is maybe not what you intended.

> +			return -EINVAL;
> +		break;
>   	default:
>   		return -EINVAL;
>   	}
> @@ -1011,6 +1016,33 @@ amdgpu_userq_list(struct drm_file *filp, union drm_amdgpu_userq *args)
>   	return ret;
>   }
>   
> +static int amdgpu_userq_change_id(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 amdgpu_usermode_queue *queue;
> +	int ret = 0;
> +
> +	mutex_lock(&uq_mgr->userq_mutex);
> +
> +	queue = amdgpu_userq_get(uq_mgr, args->change_in.queue_id);
> +	if (!queue) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	ret = xa_insert(&uq_mgr->userq_xa, args->change_in.new_queue_id, queue, GFP_KERNEL);
> +	if (ret)
> +		goto put;
> +
> +	xa_erase(&uq_mgr->userq_xa, args->change_in.queue_id);

I guess here the mutex makes it easier although it is probably doable 
without it by preallocating and using the internal lock and advanced api.

Hmm.. I will have to implement the same thing for ctx handle rename any 
day now, I was planning to respin the render node CRIU RFC this week. 
Not that I think taking the mutex is a problem at all, just that my 
initial reaction was that in 1/3 it did not fix the race fully so maybe 
we don't even need it. But maybe it is easier with it.

I'll see there how doing it with internal lock only pans out in context 
rename.

> +put:
> +	amdgpu_userq_put(queue);
> +unlock:
> +	mutex_unlock(&uq_mgr->userq_mutex);
> +	return ret;
> +}
> +
>   bool amdgpu_userq_enabled(struct drm_device *dev)
>   {
>   	struct amdgpu_device *adev = drm_to_adev(dev);
> @@ -1060,6 +1092,11 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
>   		if (r)
>   			drm_file_err(filp, "Failed to get list of usermode queues\n");
>   		break;
> +	case AMDGPU_USERQ_OP_CHANGE_ID:
> +		r = amdgpu_userq_change_id(filp, args);
> +		if (r)
> +			drm_file_err(filp, "Failed to change queue id\n");

Remove the logging as in the previous patch. Or downgrade to debug if 
must be.

Regards,

Tvrtko

> +		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 678f3d531df7..de9ae1296819 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -330,9 +330,10 @@ 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
> +#define AMDGPU_USERQ_OP_CREATE		1
> +#define AMDGPU_USERQ_OP_FREE		2
> +#define AMDGPU_USERQ_OP_LIST		3
> +#define AMDGPU_USERQ_OP_CHANGE_ID	4
>   
>   /* queue priority levels */
>   /* low < normal low < normal high < high */
> @@ -463,10 +464,20 @@ struct drm_amdgpu_userq_list_in_out {
>   	__u64   entries;
>   };
>   
> +struct drm_amdgpu_userq_change_id_in {
> +	/** AMDGPU_USERQ_OP_CHANGE_ID */
> +	__u32   op;
> +	/** Queue id of some queue */
> +	__u32   queue_id;
> +	/** Queue id to change that queue to */
> +	__u32   new_queue_id;
> +};
> +
>   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;
> +	struct drm_amdgpu_userq_change_id_in change_in;
>   };
>   
>   /* GFX V11 IP specific MQD parameters */