On 6/11/25 16:00, Tvrtko Ursulin wrote:
> We can avoid one of the two temporary allocations if we read the userspace
> supplied timeline points as we go along.
The problem with that is that calling copy_from_user multiple times is really
inefficient.
So that improves performance with few entries and decreases performance with
many entries.
Not sure if having many entries is really a valid use case here.
Regards,
Christian.
>
> The only new complication is to unwind unused fence chains on the error
> path, but even that code was already present in the function.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursu...@igalia.com>
> Reviewed-by: Maíra Canal <mca...@igalia.com> #v1
> ---
> v2:
> * Change back to copy_from_user due 32-bit ARM not implementing 64-bit
> get_user.
>
> v3:
> * Fix argument order mixup.
> ---
> drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++---------------------
> 1 file changed, 17 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 497009fc66f8..9968d9429d90 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -1579,10 +1579,10 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device
> *dev, void *data,
> struct drm_file *file_private)
> {
> struct drm_syncobj_timeline_array *args = data;
> + uint64_t __user *points = u64_to_user_ptr(args->points);
> + uint32_t i, j, count = args->count_handles;
> struct drm_syncobj **syncobjs;
> struct dma_fence_chain **chains;
> - uint64_t *points;
> - uint32_t i, j;
> int ret;
>
> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
> @@ -1596,31 +1596,17 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device
> *dev, void *data,
>
> ret = drm_syncobj_array_find(file_private,
> u64_to_user_ptr(args->handles),
> - args->count_handles,
> + count,
> &syncobjs);
> if (ret < 0)
> return ret;
>
> - points = kmalloc_array(args->count_handles, sizeof(*points),
> - GFP_KERNEL);
> - if (!points) {
> - ret = -ENOMEM;
> - goto out;
> - }
> - if (!u64_to_user_ptr(args->points)) {
> - memset(points, 0, args->count_handles * sizeof(uint64_t));
> - } else if (copy_from_user(points, u64_to_user_ptr(args->points),
> - sizeof(uint64_t) * args->count_handles)) {
> - ret = -EFAULT;
> - goto err_points;
> - }
> -
> - chains = kmalloc_array(args->count_handles, sizeof(void *), GFP_KERNEL);
> + chains = kmalloc_array(count, sizeof(void *), GFP_KERNEL);
> if (!chains) {
> ret = -ENOMEM;
> - goto err_points;
> + goto out;
> }
> - for (i = 0; i < args->count_handles; i++) {
> + for (i = 0; i < count; i++) {
> chains[i] = dma_fence_chain_alloc();
> if (!chains[i]) {
> for (j = 0; j < i; j++)
> @@ -1630,19 +1616,24 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device
> *dev, void *data,
> }
> }
>
> - for (i = 0; i < args->count_handles; i++) {
> + for (i = 0; i < count; i++) {
> struct dma_fence *fence = dma_fence_get_stub();
> + u64 point = 0;
>
> - drm_syncobj_add_point(syncobjs[i], chains[i],
> - fence, points[i]);
> + if (points && copy_from_user(&point, points++, sizeof(point))) {
> + ret = -EFAULT;
> + for (j = i; j < count; j++)
> + dma_fence_chain_free(chains[j]);
> + goto err_chains;
> + }
> +
> + drm_syncobj_add_point(syncobjs[i], chains[i], fence, point);
> dma_fence_put(fence);
> }
> err_chains:
> kfree(chains);
> -err_points:
> - kfree(points);
> out:
> - drm_syncobj_array_free(syncobjs, args->count_handles);
> + drm_syncobj_array_free(syncobjs, count);
>
> return ret;
> }
