| Message ID | 20250318155424.78552-2-tvrtko.ursulin@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | A few drm_syncobj optimisations | expand |
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@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@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,
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@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@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, >
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; 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), + &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,
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@igalia.com> --- drivers/gpu/drm/drm_syncobj.c | 98 ++++++++++++++++------------------- 1 file changed, 44 insertions(+), 54 deletions(-)