Re: [PATCH v2] drm/amdgpu/userq: Add syncobj_points to signal ioctl

On 5/19/26 14:58, David Rosca wrote:
> Userspace patches:
> 
> * radeonsi NV_timeline_semaphore
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/37335
> 
> * RADV user queues
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/40808
> 
> Signed-off-by: David Rosca <[email protected]>
> ---
> v2: don't use struct amdgpu_cs_post_dep
>     fixed syncobj leak when dma_fence_chain_alloc fails
> 
>  .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c   | 50 ++++++++++++++++---
>  include/uapi/drm/amdgpu_drm.h                 |  5 ++
>  2 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index c0d68863fa17..f4366c5d1e35 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -460,9 +460,13 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>  
>  	struct drm_gem_object **gobj_write, **gobj_read;
>  	u32 *syncobj_handles, num_syncobj_handles;
> +	u64 *syncobj_points = NULL;

Don't NULL initialize that here.

>  	struct amdgpu_usermode_queue *queue;
>  	struct amdgpu_userq_fence *fence;
> -	struct drm_syncobj **syncobj;
> +	struct {
> +		struct drm_syncobj *syncobj;
> +		struct dma_fence_chain *chain;
> +	} *syncobj;
>  	struct drm_exec exec;
>  	void __user *ptr;
>  	int r, i, entry;
> @@ -482,19 +486,38 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>  	if (IS_ERR(syncobj_handles))
>  		return PTR_ERR(syncobj_handles);
>  
> +	if (args->syncobj_points) {
> +		ptr = u64_to_user_ptr(args->syncobj_points);
> +		syncobj_points = memdup_array_user(ptr, num_syncobj_handles,
> +					    sizeof(u64));
> +		if (IS_ERR(syncobj_points)) {
> +			r = PTR_ERR(syncobj_points);
> +			goto free_syncobj_handles;
> +		}
> +	}

Instead add an "} else { syncobj_points = NULL; }" here.

With that done Reviewed-by: Christian König <[email protected]>.

Regards,
Christian.

> +
>  	syncobj = kmalloc_array(num_syncobj_handles, sizeof(*syncobj),
>  				GFP_KERNEL);
>  	if (!syncobj) {
>  		r = -ENOMEM;
> -		goto free_syncobj_handles;
> +		goto free_syncobj_points;
>  	}
>  
>  	for (entry = 0; entry < num_syncobj_handles; entry++) {
> -		syncobj[entry] = drm_syncobj_find(filp, syncobj_handles[entry]);
> -		if (!syncobj[entry]) {
> +		syncobj[entry].chain = NULL;
> +		syncobj[entry].syncobj = drm_syncobj_find(filp, syncobj_handles[entry]);
> +		if (!syncobj[entry].syncobj) {
>  			r = -ENOENT;
>  			goto free_syncobj;
>  		}
> +		if (syncobj_points && syncobj_points[entry]) {
> +			syncobj[entry].chain = dma_fence_chain_alloc();
> +			if (!syncobj[entry].chain) {
> +				drm_syncobj_put(syncobj[entry].syncobj);
> +				r = -ENOMEM;
> +				goto free_syncobj;
> +			}
> +		}
>  	}
>  
>  	ptr = u64_to_user_ptr(args->bo_read_handles);
> @@ -561,8 +584,15 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>  		dma_resv_add_fence(gobj_write[i]->resv, &fence->base,
>  				   DMA_RESV_USAGE_WRITE);
>  
> -	for (i = 0; i < num_syncobj_handles; i++)
> -		drm_syncobj_replace_fence(syncobj[i], &fence->base);
> +	for (i = 0; i < num_syncobj_handles; i++) {
> +		if (syncobj[i].chain) {
> +			drm_syncobj_add_point(syncobj[i].syncobj, syncobj[i].chain,
> +					   &fence->base, syncobj_points[i]);
> +			syncobj[i].chain = NULL;
> +		} else {
> +			drm_syncobj_replace_fence(syncobj[i].syncobj, &fence->base);
> +		}
> +	}
>  
>  exec_fini:
>  	/* drop the reference acquired in fence creation function */
> @@ -580,9 +610,13 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>  		drm_gem_object_put(gobj_read[i]);
>  	kvfree(gobj_read);
>  free_syncobj:
> -	while (entry-- > 0)
> -		drm_syncobj_put(syncobj[entry]);
> +	while (entry-- > 0) {
> +		drm_syncobj_put(syncobj[entry].syncobj);
> +		dma_fence_chain_free(syncobj[entry].chain);
> +	}
>  	kfree(syncobj);
> +free_syncobj_points:
> +	kfree(syncobj_points);
>  free_syncobj_handles:
>  	kfree(syncobj_handles);
>  
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index 9f3090db2f16..ad643b41982c 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -502,6 +502,11 @@ struct drm_amdgpu_userq_signal {
>  	 * @bo_write_handles.
>  	 */
>  	__u32	num_bo_write_handles;
> +	/**
> +	 * @syncobj_points: The list of syncobj points submitted by the user queue job
> +	 * for the corresponding @syncobj_handles.
> +	 */
> +	__u64	syncobj_points;
>  };
>  
>  struct drm_amdgpu_userq_fence_info {