Re: [PATCH v2 03/08] drm/amdgpu: Add wait IOCTL timeline syncobj support

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 25.09.24 um 21:59 schrieb Arunpravin Paneer Selvam:
> Add user fence wait IOCTL timeline syncobj support.
>
> v2:(Christian)
>    - handle dma_fence_wait() return value.
>    - shorten the variable name syncobj_timeline_points a bit.
>    - move num_points up to avoid padding issues.
>
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c   | 82 +++++++++++++++++--
>   include/uapi/drm/amdgpu_drm.h                 | 16 +++-
>   2 files changed, 88 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index 97b1af574407..2465ca307644 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -474,11 +474,11 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>   int amdgpu_userq_wait_ioctl(struct drm_device *dev, void *data,
>   			    struct drm_file *filp)
>   {
> +	u32 *syncobj_handles, *timeline_points, *timeline_handles, *bo_handles;
> +	u32 num_syncobj, num_bo_handles, num_points;
>   	struct drm_amdgpu_userq_fence_info *fence_info = NULL;
>   	struct drm_amdgpu_userq_wait *wait_info = data;
> -	u32 *syncobj_handles, *bo_handles;
>   	struct dma_fence **fences = NULL;
> -	u32 num_syncobj, num_bo_handles;
>   	struct drm_gem_object **gobj;
>   	struct drm_exec exec;
>   	int r, i, entry, cnt;
> @@ -498,11 +498,26 @@ int amdgpu_userq_wait_ioctl(struct drm_device *dev, void *data,
>   		goto free_bo_handles;
>   	}
>   
> +	num_points = wait_info->num_points;
> +	timeline_handles = memdup_user(u64_to_user_ptr(wait_info->syncobj_timeline_handles),
> +				       sizeof(u32) * num_points);
> +	if (IS_ERR(timeline_handles)) {
> +		r = PTR_ERR(timeline_handles);
> +		goto free_syncobj_handles;
> +	}
> +
> +	timeline_points = memdup_user(u64_to_user_ptr(wait_info->syncobj_timeline_points),
> +				      sizeof(u32) * num_points);
> +	if (IS_ERR(timeline_points)) {
> +		r = PTR_ERR(timeline_points);
> +		goto free_timeline_handles;
> +	}
> +
>   	/* Array of GEM object handles */
>   	gobj = kmalloc_array(num_bo_handles, sizeof(*gobj), GFP_KERNEL);
>   	if (!gobj) {
>   		r = -ENOMEM;
> -		goto free_syncobj_handles;
> +		goto free_timeline_points;
>   	}
>   
>   	for (entry = 0; entry < num_bo_handles; entry++) {
> @@ -524,17 +539,34 @@ int amdgpu_userq_wait_ioctl(struct drm_device *dev, void *data,
>   	}
>   
>   	if (!wait_info->num_fences) {
> +		if (num_points) {
> +			struct dma_fence *fence;
> ++
> +			for (i = 0; i < num_points; i++) {
> +				r = drm_syncobj_find_fence(filp, timeline_handles[i],
> +							   timeline_points[i],
> +							   DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
> +							   &fence);
> +				if (r || !fence)

You can't simply ignore errors here. That needs some goto error handling 
and cleanup.

> +					continue;
> +
> +				dma_fence_put(fence);
> +				num_fences++;

There can be more than one fence in the timeline we need to wait for. 
You should probably use dma_fence_unwrap_for_each() here.

> +			}
> +		}
> +
>   		/* Count syncobj's fence */
>   		for (i = 0; i < num_syncobj; i++) {
>   			struct dma_fence *fence;
>   
>   			r = drm_syncobj_find_fence(filp, syncobj_handles[i],
> -						   0, 0, &fence);
> -			dma_fence_put(fence);
> -
> +						   0,
> +						   DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
> +						   &fence);
>   			if (r || !fence)
>   				continue;
>   
> +			dma_fence_put(fence);
>   			num_fences++;
>   		}
>   
> @@ -589,12 +621,34 @@ int amdgpu_userq_wait_ioctl(struct drm_device *dev, void *data,
>   			}
>   		}
>   
> +		if (num_points) {
> +			struct dma_fence *fence;
> +
> +			for (i = 0; i < num_points; i++) {
> +				r = drm_syncobj_find_fence(filp, timeline_handles[i],
> +							   timeline_points[i],
> +							   DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
> +							   &fence);
> +				if (r || !fence)

Same here, just ignoring errors is a no-go.

Regards,
Christian.

> +					continue;
> +
> +				if (WARN_ON_ONCE(num_fences >= wait_info->num_fences)) {
> +					r = -EINVAL;
> +					goto free_fences;
> +				}
> +
> +				fences[num_fences++] = fence;
> +			}
> +		}
> +
>   		/* Retrieve syncobj's fence */
>   		for (i = 0; i < num_syncobj; i++) {
>   			struct dma_fence *fence;
>   
>   			r = drm_syncobj_find_fence(filp, syncobj_handles[i],
> -						   0, 0, &fence);
> +						   0,
> +						   DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
> +						   &fence);
>   			if (r || !fence)
>   				continue;
>   
> @@ -617,9 +671,13 @@ int amdgpu_userq_wait_ioctl(struct drm_device *dev, void *data,
>   				 * Just waiting on other driver fences should
>   				 * be good for now
>   				 */
> -				dma_fence_wait(fences[i], false);
> -				dma_fence_put(fences[i]);
> +				r = dma_fence_wait(fences[i], true);
> +				if (r) {
> +					dma_fence_put(fences[i]);
> +					goto free_fences;
> +				}
>   
> +				dma_fence_put(fences[i]);
>   				continue;
>   			}
>   
> @@ -665,6 +723,8 @@ int amdgpu_userq_wait_ioctl(struct drm_device *dev, void *data,
>   		drm_gem_object_put(gobj[i]);
>   	kfree(gobj);
>   
> +	kfree(timeline_points);
> +	kfree(timeline_handles);
>   	kfree(syncobj_handles);
>   	kfree(bo_handles);
>   
> @@ -682,6 +742,10 @@ int amdgpu_userq_wait_ioctl(struct drm_device *dev, void *data,
>   	while (entry-- > 0)
>   		drm_gem_object_put(gobj[entry]);
>   	kfree(gobj);
> +free_timeline_points:
> +	kfree(timeline_points);
> +free_timeline_handles:
> +	kfree(timeline_handles);
>   free_syncobj_handles:
>   	kfree(syncobj_handles);
>   free_bo_handles:
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index af42798e901d..3b24e0cb1b51 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -521,12 +521,26 @@ struct drm_amdgpu_userq_wait {
>   	 * matching fence wait info pair in @userq_fence_info.
>   	 */
>   	__u32	bo_wait_flags;
> -	__u32	pad;
> +	/**
> +	 * @num_points: A count that represents the number of timeline syncobj handles in
> +	 * syncobj_handles_array.
> +	 */
> +	__u32	num_points;
>   	/**
>   	 * @syncobj_handles_array: An array of syncobj handles defined to get the
>   	 * fence wait information of every syncobj handles in the array.
>   	 */
>   	__u64	syncobj_handles_array;
> +	/**
> +	 * @syncobj_timeline_handles: An array of timeline syncobj handles defined to get the
> +	 * fence wait information of every timeline syncobj handles in the array.
> +	 */
> +	__u64   syncobj_timeline_handles;
> +	/**
> +	 * @syncobj_timeline_points: An array of timeline syncobj points defined to get the
> +	 * fence wait points of every timeline syncobj handles in the syncobj_handles_array.
> +	 */
> +	__u64	syncobj_timeline_points;
>   	/**
>   	 * @bo_handles_array: An array of GEM BO handles defined to fetch the fence
>   	 * wait information of every BO handles in the array.