[PATCH v2 1/2] drm/amdgpu: Add LIST option to USERQ ioctl

[PATCH v2 1/2] drm/amdgpu: Add LIST option to USERQ ioctl

[David Francis]

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.

This operation holds both userq_mutex and the userq_xa xa_lock for
its entire duration (including for copy_to_user, copy_from_user,
and kvmalloc).

Signed-off-by: David Francis <David.Francis@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 162 +++++++++++++++++++++-
 include/uapi/drm/amdgpu_drm.h             |  39 ++++++
 2 files changed, 198 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..3c930425c1bb 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,159 @@ 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;
+
+	mutex_lock(&uq_mgr->userq_mutex);
+	xa_lock(&uq_mgr->userq_xa);
+
+	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;
+		xa_unlock(&uq_mgr->userq_xa);
+		mutex_unlock(&uq_mgr->userq_mutex);
+		return 0;
+	}
+	if (num_queues == 0) {
+		xa_unlock(&uq_mgr->userq_xa);
+		mutex_unlock(&uq_mgr->userq_mutex);
+		return 0;
+	}
+
+	entries = kvmalloc_objs(typeof(*entries), num_queues, GFP_KERNEL);
+
+	if (!entries) {
+		xa_unlock(&uq_mgr->userq_xa);
+		mutex_unlock(&uq_mgr->userq_mutex);
+		return -ENOMEM;
+	}
+
+	ret = check_mul_overflow(num_queues, sizeof(*entries), &entry_buffer_size);
+
+	if (ret) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	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) {
+		/**
+		 * 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. 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 {
+			ret = -EINVAL;
+			goto exit;
+		}
+
+		if (mqd_size != entries[i].mqd_size) {
+			entries[i].mqd_size = mqd_size;
+			entries[i].ip_type = queue->queue_type;
+			i += 1;
+			continue;
+		}
+
+		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_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,
+					   entries[i].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,
+					   entries[i].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,
+					   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:
+	xa_unlock(&uq_mgr->userq_xa);
+	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);
@@ -891,7 +1046,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 +1055,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..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;
+	/**
+	 * 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 */
-- 
2.34.1

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

[David Francis]

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.

This operation holds userq_mutex and the userq_xa xa_lock for its
entire duration.

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 <David.Francis@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 43 +++++++++++++++++++++++
 include/uapi/drm/amdgpu_drm.h             | 17 +++++++--
 2 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 3c930425c1bb..b532ba0f4cef 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)
+			return -EINVAL;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -1012,6 +1017,41 @@ 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);
+	xa_lock(&uq_mgr->userq_xa);
+
+	queue = xa_load(&uq_mgr->userq_xa, args->change_in.queue_id);
+	if (!queue) {
+		ret = -ENOENT;
+		goto unlock;
+	}
+
+	if (args->change_in.new_queue_id == args->change_in.queue_id) {
+		ret = 0;
+		goto unlock;
+	}
+
+	ret = __xa_insert(&uq_mgr->userq_xa, args->change_in.new_queue_id, queue, GFP_KERNEL);
+	if (ret) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	__xa_erase(&uq_mgr->userq_xa, args->change_in.queue_id);
+
+unlock:
+	xa_unlock(&uq_mgr->userq_xa);
+	mutex_unlock(&uq_mgr->userq_mutex);
+	return ret;
+}
+
 bool amdgpu_userq_enabled(struct drm_device *dev)
 {
 	struct amdgpu_device *adev = drm_to_adev(dev);
@@ -1058,6 +1098,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 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 */
-- 
2.34.1

Re: [PATCH v2 1/2] drm/amdgpu: Add LIST option to USERQ ioctl

[Tvrtko Ursulin]

On 11/08/2026 15:12, 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.
> 
> This operation holds both userq_mutex and the userq_xa xa_lock for
> its entire duration (including for copy_to_user, copy_from_user,
> and kvmalloc).
> 
> Signed-off-by: David Francis <David.Francis@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 162 +++++++++++++++++++++-
>   include/uapi/drm/amdgpu_drm.h             |  39 ++++++
>   2 files changed, 198 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..3c930425c1bb 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,159 @@ 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;
> +
> +	mutex_lock(&uq_mgr->userq_mutex);
> +	xa_lock(&uq_mgr->userq_xa);
> +
> +	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.
> +		 */

I suggest changing to greater than. Caller is okay to allocate an 
opportunistically size buffer and have it work in one pass. Same as 
amdgpu_gem_list_handles_ioctl().

> +		args->list_in_out.num_entries = num_queues;
> +		xa_unlock(&uq_mgr->userq_xa);
> +		mutex_unlock(&uq_mgr->userq_mutex);
> +		return 0;
> +	}
> +	if (num_queues == 0) {
> +		xa_unlock(&uq_mgr->userq_xa);
> +		mutex_unlock(&uq_mgr->userq_mutex);
> +		return 0;
> +	}
> +
> +	entries = kvmalloc_objs(typeof(*entries), num_queues, GFP_KERNEL);

GFP_KERNEL under xa_lock will not work.

If you look at amdgpu_gem_list_handles_ioctl() and also how I have done 
in amdgpu_gem_list_handles_ioctl(), you simply don't take the lock just 
yet. xa_for_each for counting is safe lockless and should the allocated 
array be too small during actually populating it, tough luck, caller is 
playing multi-threaded games so let it retry.

> +
> +	if (!entries) {
> +		xa_unlock(&uq_mgr->userq_xa);
> +		mutex_unlock(&uq_mgr->userq_mutex);
> +		return -ENOMEM;
> +	}
> +
> +	ret = check_mul_overflow(num_queues, sizeof(*entries), &entry_buffer_size);
> +
> +	if (ret) {

Nitpick here and elsewhere - I don't think a blank line between 
assignments and check is a typical pattern and for me it makes the code 
less readable but up to you.

> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	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) {
> +		/**
> +		 * 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. 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);

It's okay for now I guess but it really feels like a layering violation. 
One day this may need to be somehow queried from the queue.

> +		}
> +		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 {

} else {

> +			ret = -EINVAL;

This is probably a drm_err_once type of situation to flag the ioctl 
implementaion fell behind the actual userqueues code. Ie. someone forgot 
to update the ioctl after new queue type has been added to the kernel. 
Userspace alone would have to way to work past this -EINVAL.

> +			goto exit;
> +		}
> +
> +		if (mqd_size != entries[i].mqd_size) {
> +			entries[i].mqd_size = mqd_size;
> +			entries[i].ip_type = queue->queue_type;
> +			i += 1;
> +			continue;

Hmm how will userspace know the order of queues to correctly 
pre-allocate the correct sizes? In other words, wouldn't it work to 
allow for larger than required and only skip if too small?

> +		}
> +
> +		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_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,
> +					   entries[i].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,
> +					   entries[i].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,
> +					   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;

Could remove this goto.

> +	}
> +exit:
> +	xa_unlock(&uq_mgr->userq_xa);
> +	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);
> @@ -891,7 +1046,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 +1055,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..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;

There is a hole here so you could add a padding/mbz element here. I 
don't think it matters hugely in a write-mostly struct but in UAPI it is 
best to be 100% explicit.

> +	__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;

Hm, actually, you could make mqd_size __u32 and move it up as a 
different way to avoid holes and having to add explicit padding. I don't 
think mqd_size will ever need more than __u32, right?

Regards,

Tvrtko

> +};
> +
> +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 */

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

[Tvrtko Ursulin]

On 11/08/2026 15:12, 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.
> 
> This operation holds userq_mutex and the userq_xa xa_lock for its
> entire duration.
> 
> 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 <David.Francis@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 43 +++++++++++++++++++++++
>   include/uapi/drm/amdgpu_drm.h             | 17 +++++++--
>   2 files changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 3c930425c1bb..b532ba0f4cef 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)
> +			return -EINVAL;
> +		break;
>   	default:
>   		return -EINVAL;
>   	}
> @@ -1012,6 +1017,41 @@ 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);
> +	xa_lock(&uq_mgr->userq_xa);
> +
> +	queue = xa_load(&uq_mgr->userq_xa, args->change_in.queue_id);
> +	if (!queue) {
> +		ret = -ENOENT;
> +		goto unlock;
> +	}
> +
> +	if (args->change_in.new_queue_id == args->change_in.queue_id) {

I would move this outside the lock or even consider returning -EINVAL. 
Or you have a reason why returning success is handy? Probing what 
exists? Why?

> +		ret = 0;
> +		goto unlock;
> +	}
> +
> +	ret = __xa_insert(&uq_mgr->userq_xa, args->change_in.new_queue_id, queue, GFP_KERNEL);

Same as for the list ioctl, GFP_KERNEL under the xa_lock will not work.

You can have a look on how I've done it in "drm/amdgpu: Add context 
handle renaming operation" and see if you can punch some holes in my 
logic there?

If that works question will be do you really need both the userq_mutext 
and xa_lock or perhaps xa_lock would be enough throughout the series.

Regards,

Tvrtko

> +	if (ret) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	__xa_erase(&uq_mgr->userq_xa, args->change_in.queue_id);
> +
> +unlock:
> +	xa_unlock(&uq_mgr->userq_xa);
> +	mutex_unlock(&uq_mgr->userq_mutex);
> +	return ret;
> +}
> +
>   bool amdgpu_userq_enabled(struct drm_device *dev)
>   {
>   	struct amdgpu_device *adev = drm_to_adev(dev);
> @@ -1058,6 +1098,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 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 */

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

[Francis, David]

Thanks for the comments. Most of these are just mistakes on my part and will be fixed.

Regarding the locking,

on CHANGE,
> Same as for the list ioctl, GFP_KERNEL under the xa_lock will not work.
>
> You can have a look on how I've done it in "drm/amdgpu: Add context
> handle renaming operation" and see if you can punch some holes in my
> logic there?

I think the solution here is just to use idr_preload.
I'd rather hold the lock the whole time to avoid having to do the
dance from GEM_CHANGE_HANDLE.

in LIST
> GFP_KERNEL under xa_lock will not work.

This one is harder. I can shift around the allocations but the point
remains that if I don't hold the xa_lock the whole time
there's a chance that create / free / other modifications
of a queue's data will happen in the meantime.

I can avoid over-writing the end of the arrays / structs just by
re-checking that the array index never gets past the size of
the array, but that wouldn't protect about returning
corrupted data if LIST races another operation
(such as CHANGE_HANDLE).

I guess in that case it isn't a security risk and you could
say it's the user's fault for creating this race condition, but
I'd prefer that wasn't part of the interface.

Thanks,
David

________________________________________
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Sent: Thursday, August 13, 2026 4:48 AM
To: Francis, David; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 2/2] drm/amdgpu: Add CHANGE_ID option for USERQ ioctl


On 11/08/2026 15:12, 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.
>
> This operation holds userq_mutex and the userq_xa xa_lock for its
> entire duration.
>
> 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 <David.Francis@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 43 +++++++++++++++++++++++
>   include/uapi/drm/amdgpu_drm.h             | 17 +++++++--
>   2 files changed, 57 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 3c930425c1bb..b532ba0f4cef 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)
> +                     return -EINVAL;
> +             break;
>       default:
>               return -EINVAL;
>       }
> @@ -1012,6 +1017,41 @@ 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);
> +     xa_lock(&uq_mgr->userq_xa);
> +
> +     queue = xa_load(&uq_mgr->userq_xa, args->change_in.queue_id);
> +     if (!queue) {
> +             ret = -ENOENT;
> +             goto unlock;
> +     }
> +
> +     if (args->change_in.new_queue_id == args->change_in.queue_id) {

I would move this outside the lock or even consider returning -EINVAL.
Or you have a reason why returning success is handy? Probing what
exists? Why?

> +             ret = 0;
> +             goto unlock;
> +     }
> +
> +     ret = __xa_insert(&uq_mgr->userq_xa, args->change_in.new_queue_id, queue, GFP_KERNEL);

Same as for the list ioctl, GFP_KERNEL under the xa_lock will not work.

You can have a look on how I've done it in "drm/amdgpu: Add context
handle renaming operation" and see if you can punch some holes in my
logic there?

If that works question will be do you really need both the userq_mutext
and xa_lock or perhaps xa_lock would be enough throughout the series.

Regards,

Tvrtko

> +     if (ret) {
> +             ret = -EINVAL;
> +             goto unlock;
> +     }
> +
> +     __xa_erase(&uq_mgr->userq_xa, args->change_in.queue_id);
> +
> +unlock:
> +     xa_unlock(&uq_mgr->userq_xa);
> +     mutex_unlock(&uq_mgr->userq_mutex);
> +     return ret;
> +}
> +
>   bool amdgpu_userq_enabled(struct drm_device *dev)
>   {
>       struct amdgpu_device *adev = drm_to_adev(dev);
> @@ -1058,6 +1098,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 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 */

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

[Francis, David]

Wait, not idr_preload. Is there an xa_preload?

David

________________________________________
From: Francis, David <David.Francis@amd.com>
Sent: Friday, August 14, 2026 1:19 PM
To: Tvrtko Ursulin; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 2/2] drm/amdgpu: Add CHANGE_ID option for USERQ ioctl

Thanks for the comments. Most of these are just mistakes on my part and will be fixed.

Regarding the locking,

on CHANGE,
> Same as for the list ioctl, GFP_KERNEL under the xa_lock will not work.
>
> You can have a look on how I've done it in "drm/amdgpu: Add context
> handle renaming operation" and see if you can punch some holes in my
> logic there?

I think the solution here is just to use idr_preload.
I'd rather hold the lock the whole time to avoid having to do the
dance from GEM_CHANGE_HANDLE.

in LIST
> GFP_KERNEL under xa_lock will not work.

This one is harder. I can shift around the allocations but the point
remains that if I don't hold the xa_lock the whole time
there's a chance that create / free / other modifications
of a queue's data will happen in the meantime.

I can avoid over-writing the end of the arrays / structs just by
re-checking that the array index never gets past the size of
the array, but that wouldn't protect about returning
corrupted data if LIST races another operation
(such as CHANGE_HANDLE).

I guess in that case it isn't a security risk and you could
say it's the user's fault for creating this race condition, but
I'd prefer that wasn't part of the interface.

Thanks,
David

________________________________________
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Sent: Thursday, August 13, 2026 4:48 AM
To: Francis, David; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 2/2] drm/amdgpu: Add CHANGE_ID option for USERQ ioctl


On 11/08/2026 15:12, 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.
>
> This operation holds userq_mutex and the userq_xa xa_lock for its
> entire duration.
>
> 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 <David.Francis@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 43 +++++++++++++++++++++++
>   include/uapi/drm/amdgpu_drm.h             | 17 +++++++--
>   2 files changed, 57 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 3c930425c1bb..b532ba0f4cef 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)
> +                     return -EINVAL;
> +             break;
>       default:
>               return -EINVAL;
>       }
> @@ -1012,6 +1017,41 @@ 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);
> +     xa_lock(&uq_mgr->userq_xa);
> +
> +     queue = xa_load(&uq_mgr->userq_xa, args->change_in.queue_id);
> +     if (!queue) {
> +             ret = -ENOENT;
> +             goto unlock;
> +     }
> +
> +     if (args->change_in.new_queue_id == args->change_in.queue_id) {

I would move this outside the lock or even consider returning -EINVAL.
Or you have a reason why returning success is handy? Probing what
exists? Why?

> +             ret = 0;
> +             goto unlock;
> +     }
> +
> +     ret = __xa_insert(&uq_mgr->userq_xa, args->change_in.new_queue_id, queue, GFP_KERNEL);

Same as for the list ioctl, GFP_KERNEL under the xa_lock will not work.

You can have a look on how I've done it in "drm/amdgpu: Add context
handle renaming operation" and see if you can punch some holes in my
logic there?

If that works question will be do you really need both the userq_mutext
and xa_lock or perhaps xa_lock would be enough throughout the series.

Regards,

Tvrtko

> +     if (ret) {
> +             ret = -EINVAL;
> +             goto unlock;
> +     }
> +
> +     __xa_erase(&uq_mgr->userq_xa, args->change_in.queue_id);
> +
> +unlock:
> +     xa_unlock(&uq_mgr->userq_xa);
> +     mutex_unlock(&uq_mgr->userq_mutex);
> +     return ret;
> +}
> +
>   bool amdgpu_userq_enabled(struct drm_device *dev)
>   {
>       struct amdgpu_device *adev = drm_to_adev(dev);
> @@ -1058,6 +1098,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 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 */

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

[Francis, David]

Right, it would be just doing the __xa_alloc with GFP_NOWAIT.
In that case, it can -ENOMEM, which I'd either need to retry or just fail
with -EBUSY or something.

I think that's still preferable to having to do the gem_change_handle dance

David

________________________________________
From: Francis, David <David.Francis@amd.com>
Sent: Friday, August 14, 2026 1:29 PM
To: Tvrtko Ursulin; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 2/2] drm/amdgpu: Add CHANGE_ID option for USERQ ioctl

Wait, not idr_preload. Is there an xa_preload?

David

________________________________________
From: Francis, David <David.Francis@amd.com>
Sent: Friday, August 14, 2026 1:19 PM
To: Tvrtko Ursulin; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 2/2] drm/amdgpu: Add CHANGE_ID option for USERQ ioctl

Thanks for the comments. Most of these are just mistakes on my part and will be fixed.

Regarding the locking,

on CHANGE,
> Same as for the list ioctl, GFP_KERNEL under the xa_lock will not work.
>
> You can have a look on how I've done it in "drm/amdgpu: Add context
> handle renaming operation" and see if you can punch some holes in my
> logic there?

I think the solution here is just to use idr_preload.
I'd rather hold the lock the whole time to avoid having to do the
dance from GEM_CHANGE_HANDLE.

in LIST
> GFP_KERNEL under xa_lock will not work.

This one is harder. I can shift around the allocations but the point
remains that if I don't hold the xa_lock the whole time
there's a chance that create / free / other modifications
of a queue's data will happen in the meantime.

I can avoid over-writing the end of the arrays / structs just by
re-checking that the array index never gets past the size of
the array, but that wouldn't protect about returning
corrupted data if LIST races another operation
(such as CHANGE_HANDLE).

I guess in that case it isn't a security risk and you could
say it's the user's fault for creating this race condition, but
I'd prefer that wasn't part of the interface.

Thanks,
David

________________________________________
From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Sent: Thursday, August 13, 2026 4:48 AM
To: Francis, David; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 2/2] drm/amdgpu: Add CHANGE_ID option for USERQ ioctl


On 11/08/2026 15:12, 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.
>
> This operation holds userq_mutex and the userq_xa xa_lock for its
> entire duration.
>
> 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 <David.Francis@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 43 +++++++++++++++++++++++
>   include/uapi/drm/amdgpu_drm.h             | 17 +++++++--
>   2 files changed, 57 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 3c930425c1bb..b532ba0f4cef 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)
> +                     return -EINVAL;
> +             break;
>       default:
>               return -EINVAL;
>       }
> @@ -1012,6 +1017,41 @@ 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);
> +     xa_lock(&uq_mgr->userq_xa);
> +
> +     queue = xa_load(&uq_mgr->userq_xa, args->change_in.queue_id);
> +     if (!queue) {
> +             ret = -ENOENT;
> +             goto unlock;
> +     }
> +
> +     if (args->change_in.new_queue_id == args->change_in.queue_id) {

I would move this outside the lock or even consider returning -EINVAL.
Or you have a reason why returning success is handy? Probing what
exists? Why?

> +             ret = 0;
> +             goto unlock;
> +     }
> +
> +     ret = __xa_insert(&uq_mgr->userq_xa, args->change_in.new_queue_id, queue, GFP_KERNEL);

Same as for the list ioctl, GFP_KERNEL under the xa_lock will not work.

You can have a look on how I've done it in "drm/amdgpu: Add context
handle renaming operation" and see if you can punch some holes in my
logic there?

If that works question will be do you really need both the userq_mutext
and xa_lock or perhaps xa_lock would be enough throughout the series.

Regards,

Tvrtko

> +     if (ret) {
> +             ret = -EINVAL;
> +             goto unlock;
> +     }
> +
> +     __xa_erase(&uq_mgr->userq_xa, args->change_in.queue_id);
> +
> +unlock:
> +     xa_unlock(&uq_mgr->userq_xa);
> +     mutex_unlock(&uq_mgr->userq_mutex);
> +     return ret;
> +}
> +
>   bool amdgpu_userq_enabled(struct drm_device *dev)
>   {
>       struct amdgpu_device *adev = drm_to_adev(dev);
> @@ -1058,6 +1098,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 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 */