[PATCH 1/7] drm/syncobj: Remove unhelpful helper
Tvrtko Ursulin
tvrtko.ursulin at igalia.com
Tue Mar 25 09:12:39 UTC 2025
On 24/03/2025 21:38, Maíra Canal wrote:
> 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.
I think this one is 50-50. I made it a local since it used four times in
both functions. Since you are not strongly against it I opted to keep it
as is.
>> 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()`.
Good suggestion, done.
Regards,
Tvrtko
>> + &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,
>
More information about the dri-devel
mailing list