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 <[email protected]> On 18/03/25 12:54, Tvrtko Ursulin wrote:Helper which fails to consolidate the code and instead just forks into twocopies 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 <[email protected]> --- 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.cindex 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,
