Re: [PATCH v6 8/8] drm/amdgpu/userq: user array to store userq vas

["Christian König" <christian.koenig@amd.com>]

On 5/19/26 13:18, Sunil Khatri wrote:
> Add per queue array to store userq vas and keep
> size to accommodate vas of all types of queues
> i.e gfx, compute and sdma.

Yeah that's a start, but I think we should go a bit further.

> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 36 ++++++++---------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h |  4 ++-
>  2 files changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 7354c51ae83d..9ac7f18c903f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -218,18 +218,11 @@ void amdgpu_userq_process_fence_irq(struct amdgpu_device *adev, u32 doorbell)
>  static int amdgpu_userq_buffer_va_list_add(struct amdgpu_usermode_queue *queue,
>  					   struct amdgpu_bo_va_mapping *va_map, u64 addr)
>  {

Completely nuke that function and it's caller.

> -	struct amdgpu_userq_va_cursor *va_cursor;
> -	struct userq_va_list;
> -
> -	va_cursor = kzalloc(sizeof(*va_cursor), GFP_KERNEL);
> -	if (!va_cursor)
> +	if (queue->userq_va_count >= ARRAY_SIZE(queue->userq_va))
>  		return -ENOMEM;
>  
> -	INIT_LIST_HEAD(&va_cursor->list);
> -	va_cursor->gpu_addr = addr;
> +	queue->userq_va[queue->userq_va_count++] = addr;
>  	va_map->bo_va->userq_va_mapped = true;
> -	list_add(&va_cursor->list, &queue->userq_va_list);
> -
>  	return 0;
>  }
>  
> @@ -284,14 +277,13 @@ static bool amdgpu_userq_buffer_va_mapped(struct amdgpu_vm *vm, u64 addr)
>  
>  static bool amdgpu_userq_buffer_vas_mapped(struct amdgpu_usermode_queue *queue)
>  {
> -	struct amdgpu_userq_va_cursor *va_cursor, *tmp;
> -	int r = 0;
> +	int i, r = 0;
>  
> -	list_for_each_entry_safe(va_cursor, tmp, &queue->userq_va_list, list) {
> -		r += amdgpu_userq_buffer_va_mapped(queue->vm, va_cursor->gpu_addr);
> +	for (i = 0; i < queue->userq_va_count; i++) {
> +		r += amdgpu_userq_buffer_va_mapped(queue->vm, queue->userq_va[i]);
>  		dev_dbg(queue->userq_mgr->adev->dev,
>  			"validate the userq mapping:%p va:%llx r:%d\n",
> -			queue, va_cursor->gpu_addr, r);
> +			queue, queue->userq_va[i], r);
>  	}
>  
>  	if (r != 0)
> @@ -303,19 +295,19 @@ static bool amdgpu_userq_buffer_vas_mapped(struct amdgpu_usermode_queue *queue)
>  static void amdgpu_userq_buffer_vas_list_cleanup(struct amdgpu_device *adev,
>  						 struct amdgpu_usermode_queue *queue)

Drop that whole function.

>  {
> -	struct amdgpu_userq_va_cursor *va_cursor, *tmp;
>  	struct amdgpu_bo_va_mapping *mapping;
> +	int i;
>  
>  	/* Caller must hold vm->root.bo reservation */
>  	dma_resv_assert_held(queue->vm->root.bo->tbo.base.resv);
>  
> -	list_for_each_entry_safe(va_cursor, tmp, &queue->userq_va_list, list) {
> -		mapping = amdgpu_vm_bo_lookup_mapping(queue->vm, va_cursor->gpu_addr);
> -		if (mapping)
> +	for (i = 0; i < queue->userq_va_count; i++) {
> +		mapping = amdgpu_vm_bo_lookup_mapping(queue->vm, queue->userq_va[i]);
> +		if (mapping) {
> +			mapping->bo_va->userq_va_mapped = false;
>  			dev_dbg(adev->dev, "delete the userq:%p va:%llx\n",
> -				queue, va_cursor->gpu_addr);
> -		list_del(&va_cursor->list);
> -		kfree(va_cursor);
> +				queue, queue->userq_va[i]);
> +			}
>  	}
>  }
>  
> @@ -633,7 +625,6 @@ amdgpu_userq_destroy(struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode_que
>  	amdgpu_bo_reserve(vm->root.bo, true);
>  	amdgpu_userq_buffer_vas_list_cleanup(adev, queue);
>  	amdgpu_bo_unreserve(vm->root.bo);
> -	list_del(&queue->userq_va_list);
>  	queue->userq_mgr = NULL;
>  
>  	amdgpu_bo_reserve(queue->db_obj.obj, true);
> @@ -738,7 +729,6 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>  	}
>  
>  	kref_init(&queue->refcount);
> -	INIT_LIST_HEAD(&queue->userq_va_list);
>  	queue->doorbell_handle = args->in.doorbell_handle;
>  	queue->queue_type = args->in.ip_type;
>  	queue->vm = &fpriv->vm;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> index 033b8a0de6b1..fdf4d878c894 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> @@ -93,7 +93,9 @@ struct amdgpu_usermode_queue {
>  	struct delayed_work	hang_detect_work;
>  	struct kref		refcount;
>  
> -	struct list_head	userq_va_list;
> +	/* User to store core bo's va addresses */
> +	u64			userq_va[5];
> +	int			userq_va_count;

Make that look like this:

union {
	struct {
		u64 queue_rb;
		u64 wptr;
		u64 rptr;
		....
	} va_names;
	u64 va_array[];
};

Unused entries should simply be zero.

After amdgpu_userq_create() fills in the different VAs we just call amdgpu_userq_buffer_vas_mapped() to double check that they are valid before mapping the queue.

Regards,
Christian.

>  };
>  
>  struct amdgpu_userq_funcs {