[PATCH 1/7] drm/syncobj: Remove unhelpful helper

[Maíra Canal]

Hi Tvrtko,

Some nits inline, but feel free to ignore them if you don't think they
are reasonable. Apart from that,

Reviewed-by: Maíra Canal <mcanal at igalia.com>

On 18/03/25 12:54, Tvrtko Ursulin wrote:
> Helper which fails to consolidate the code and instead just forks into two
> copies of the code based on a boolean parameter is not very helpful or
> readable. Lets just remove it and proof in the pudding is the net smaller
> code.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
> ---
>   drivers/gpu/drm/drm_syncobj.c | 98 ++++++++++++++++-------------------
>   1 file changed, 44 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 4f2ab8a7b50f..d0d60c331df8 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -1221,42 +1221,6 @@ signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec)
>   }
>   EXPORT_SYMBOL(drm_timeout_abs_to_jiffies);
>   
> -static int drm_syncobj_array_wait(struct drm_device *dev,
> -				  struct drm_file *file_private,
> -				  struct drm_syncobj_wait *wait,
> -				  struct drm_syncobj_timeline_wait *timeline_wait,
> -				  struct drm_syncobj **syncobjs, bool timeline,
> -				  ktime_t *deadline)
> -{
> -	signed long timeout = 0;
> -	uint32_t first = ~0;
> -
> -	if (!timeline) {
> -		timeout = drm_timeout_abs_to_jiffies(wait->timeout_nsec);
> -		timeout = drm_syncobj_array_wait_timeout(syncobjs,
> -							 NULL,
> -							 wait->count_handles,
> -							 wait->flags,
> -							 timeout, &first,
> -							 deadline);
> -		if (timeout < 0)
> -			return timeout;
> -		wait->first_signaled = first;
> -	} else {
> -		timeout = drm_timeout_abs_to_jiffies(timeline_wait->timeout_nsec);
> -		timeout = drm_syncobj_array_wait_timeout(syncobjs,
> -							 u64_to_user_ptr(timeline_wait->points),
> -							 timeline_wait->count_handles,
> -							 timeline_wait->flags,
> -							 timeout, &first,
> -							 deadline);
> -		if (timeout < 0)
> -			return timeout;
> -		timeline_wait->first_signaled = first;
> -	}
> -	return 0;
> -}
> -
>   static int drm_syncobj_array_find(struct drm_file *file_private,
>   				  void __user *user_handles,
>   				  uint32_t count_handles,
> @@ -1319,9 +1283,12 @@ drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
>   		       struct drm_file *file_private)
>   {
>   	struct drm_syncobj_wait *args = data;
> +	ktime_t deadline, *pdeadline = NULL;
> +	u32 count = args->count_handles;

 From my point of view, this variable didn't make the code more readable.
I'd prefer to keep using `args->count_handles` in the code.

>   	struct drm_syncobj **syncobjs;
>   	unsigned int possible_flags;
> -	ktime_t t, *tp = NULL;
> +	u32 first = ~0;
> +	long timeout;
>   	int ret = 0;
>   
>   	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> @@ -1334,27 +1301,37 @@ drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
>   	if (args->flags & ~possible_flags)
>   		return -EINVAL;
>   
> -	if (args->count_handles == 0)
> +	if (count == 0)
>   		return 0;
>   
>   	ret = drm_syncobj_array_find(file_private,
>   				     u64_to_user_ptr(args->handles),
> -				     args->count_handles,
> +				     count,
>   				     &syncobjs);
>   	if (ret < 0)
>   		return ret;
>   
>   	if (args->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE) {
> -		t = ns_to_ktime(args->deadline_nsec);
> -		tp = &t;
> +		deadline = ns_to_ktime(args->deadline_nsec);
> +		pdeadline = &deadline;
>   	}
>   
> -	ret = drm_syncobj_array_wait(dev, file_private,
> -				     args, NULL, syncobjs, false, tp);
> +	timeout = drm_syncobj_array_wait_timeout(syncobjs,
> +						 NULL,
> +						 count,
> +						 args->flags,
> +						 drm_timeout_abs_to_jiffies(args->timeout_nsec),

Could you create a variable for the timeout instead of adding the
function as a parameter?

Same comments for `drm_syncobj_timeline_wait_ioctl()`.

Best Regards,
- Maíra

> +						 &first,
> +						 pdeadline);
>   
> -	drm_syncobj_array_free(syncobjs, args->count_handles);
> +	drm_syncobj_array_free(syncobjs, count);
>   
> -	return ret;
> +	if (timeout < 0)
> +		return timeout;
> +
> +	args->first_signaled = first;
> +
> +	return 0;
>   }
>   
>   int
> @@ -1362,9 +1339,12 @@ drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
>   				struct drm_file *file_private)
>   {
>   	struct drm_syncobj_timeline_wait *args = data;
> +	ktime_t deadline, *pdeadline = NULL;
> +	u32 count = args->count_handles;
>   	struct drm_syncobj **syncobjs;
>   	unsigned int possible_flags;
> -	ktime_t t, *tp = NULL;
> +	u32 first = ~0;
> +	long timeout;
>   	int ret = 0;
>   
>   	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
> @@ -1378,27 +1358,37 @@ drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
>   	if (args->flags & ~possible_flags)
>   		return -EINVAL;
>   
> -	if (args->count_handles == 0)
> +	if (count == 0)
>   		return 0;
>   
>   	ret = drm_syncobj_array_find(file_private,
>   				     u64_to_user_ptr(args->handles),
> -				     args->count_handles,
> +				     count,
>   				     &syncobjs);
>   	if (ret < 0)
>   		return ret;
>   
>   	if (args->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE) {
> -		t = ns_to_ktime(args->deadline_nsec);
> -		tp = &t;
> +		deadline = ns_to_ktime(args->deadline_nsec);
> +		pdeadline = &deadline;
>   	}
>   
> -	ret = drm_syncobj_array_wait(dev, file_private,
> -				     NULL, args, syncobjs, true, tp);
> +	timeout = drm_syncobj_array_wait_timeout(syncobjs,
> +						 u64_to_user_ptr(args->points),
> +						 count,
> +						 args->flags,
> +						 drm_timeout_abs_to_jiffies(args->timeout_nsec),
> +						 &first,
> +						 pdeadline);
>   
> -	drm_syncobj_array_free(syncobjs, args->count_handles);
> +	drm_syncobj_array_free(syncobjs, count);
>   
> -	return ret;
> +	if (timeout < 0)
> +		return timeout;
> +
> +	args->first_signaled = first;
> +
> +	return 0;
>   }
>   
>   static void syncobj_eventfd_entry_fence_func(struct dma_fence *fence,