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

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

On 27/08/2026 19:59, 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.
> 
> Calling with an invalid new handle will fail. Calling with new_handle
> = handle will succeed if that queue exists but not do anything.
> 
> A poison queue object is inserted at the old handle during the
> rekey to prevent concurrent FREE from destroying the queue or the
> old id being reused mid-operation.

Looks complicated. Have you looked at how I've done the rename with no 
external lock?

https://lore.kernel.org/amd-gfx/20260806134712.91816-7-tvrtko.ursulin@igalia.com/

I'll have a read through your approach but I can't say I am a fan.

> 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.
> 
> v3: Poison queue and misc fixes
> 
> Signed-off-by: David Francis <David.Francis@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 60 +++++++++++++++++++++++
>   include/uapi/drm/amdgpu_drm.h             | 17 +++++--
>   2 files changed, 74 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 32d1787aa7e2..699838042bda 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -35,6 +35,8 @@
>   #include "amdgpu_userq_fence.h"
>   #include "amdgpu_trace.h"
>   
> +struct amdgpu_usermode_queue poison_queue;
> +
>   u32 amdgpu_userq_get_supported_ip_mask(struct amdgpu_device *adev)
>   {
>   	int i;
> @@ -852,6 +854,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)
> +			return -EINVAL;
> +		break;
>   	default:
>   		return -EINVAL;
>   	}
> @@ -1010,6 +1017,51 @@ 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;
> +
> +	if (args->change_in.new_queue_id == args->change_in.queue_id) {
> +		if (xa_load(&uq_mgr->userq_xa, args->change_in.queue_id))
> +			return 0;
> +		return -ENOENT;

Why is is important to allow users to query for existence of an id like 
this ie. why wouldn't be be EINVAL if new == old?

> +	}
> +
> +	/* Poison the old handle so it doesn't get freed or reused. */
> +	queue = xa_store(&uq_mgr->userq_xa, args->change_in.queue_id, &poison_queue, GFP_KERNEL);

What if userspace is silly and looks up this handle from a racing 
thread? It gets the poison queue and things explode?

> +	if (!queue) {
> +		xa_erase(&uq_mgr->userq_xa, args->change_in.queue_id);
> +		return -ENOENT;
> +	}
> +	if (queue == &poison_queue)
> +		return -EINVAL;

Why is this EINVAL? Userspace races with itself I get it, but I think it 
shows the weakness of the poison entry multi-stage approach.

> +
> +	ret = xa_insert(&uq_mgr->userq_xa, args->change_in.new_queue_id, queue, GFP_KERNEL);
> +	if (ret == -EBUSY) {
> +		queue = xa_store(&uq_mgr->userq_xa, args->change_in.queue_id, queue, GFP_KERNEL);
> +		if (queue != &poison_queue) {
> +			drm_err_once(adev_to_drm(uq_mgr->adev),
> +				     "Expected poison queue to remain untouched during userqueue change id");

Do you expect drm_err to be reachable by silly userspace or just 
unexpected internal error?

> +			return -EINVAL;
> +		}
> +	}
> +	if (ret == -ENOMEM)
> +		return -ENOMEM;

Is there another possibily from xa_insert other than EBUSY and ENOMEM?

> +
> +	/* remove the poison */
> +	queue = xa_erase(&uq_mgr->userq_xa, args->change_in.queue_id);
> +	if (queue != &poison_queue) {
> +		drm_err_once(adev_to_drm(uq_mgr->adev),
> +			     "Expected poison queue to remain untouched during userqueue change id");

Same question as the previous drm_err_once.

Sorry I don't like this at all. I would much rather you try to punch a 
hole in my approach or confirm it works fine.

I adapted that from some existing driver.. can't remember which now 
after more than a month. But the approach sounds sane to me - do the 
rename atomically under the lock using GFP_NOWAIT first and if that 
fails drop the lock to pre-allocate space and retry.

Regards,

Tvrtko

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>   bool amdgpu_userq_enabled(struct drm_device *dev)
>   {
>   	struct amdgpu_device *adev = drm_to_adev(dev);
> @@ -1046,6 +1098,11 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
>   
>   	case AMDGPU_USERQ_OP_FREE:
>   		xa_lock(&fpriv->userq_mgr.userq_xa);
> +		queue = xa_load(&fpriv->userq_mgr.userq_xa, args->in.queue_id);
> +		if (queue == &poison_queue) {
> +			xa_unlock(&fpriv->userq_mgr.userq_xa);
> +			return -EINVAL;
> +		}
>   		queue = __xa_erase(&fpriv->userq_mgr.userq_xa, args->in.queue_id);
>   		xa_unlock(&fpriv->userq_mgr.userq_xa);
>   		if (!queue)
> @@ -1056,6 +1113,9 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
>   	case AMDGPU_USERQ_OP_LIST:
>   		r = amdgpu_userq_list(filp, args);
>   		break;
> +	case AMDGPU_USERQ_OP_CHANGE_ID:
> +		r = amdgpu_userq_change_id(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 92738d630eea..37d0efd1eda4 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 */
> @@ -464,10 +465,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 */