From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Ekstrand Subject: Re: [PATCH 2/5] drm/syncobj: add sync obj wait interface. (v3) Date: Wed, 24 May 2017 10:33:08 -0700 Message-ID: References: <20170524070615.1634-1-airlied@gmail.com> <20170524070615.1634-3-airlied@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2045770448==" Return-path: Received: from mail-wr0-x244.google.com (mail-wr0-x244.google.com [IPv6:2a00:1450:400c:c0c::244]) by gabe.freedesktop.org (Postfix) with ESMTPS id DA0DC6E6F0 for ; Wed, 24 May 2017 17:33:10 +0000 (UTC) Received: by mail-wr0-x244.google.com with SMTP id j27so3957291wre.2 for ; Wed, 24 May 2017 10:33:10 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Dave Airlie Cc: amd-gfx mailing list , Maling list - DRI developers List-Id: dri-devel@lists.freedesktop.org --===============2045770448== Content-Type: multipart/alternative; boundary="001a11497cfe6f67440550487f1a" --001a11497cfe6f67440550487f1a Content-Type: text/plain; charset="UTF-8" On Wed, May 24, 2017 at 10:25 AM, Jason Ekstrand wrote: > On Wed, May 24, 2017 at 12:06 AM, Dave Airlie wrote: > >> From: Dave Airlie >> >> This interface will allow sync object to be used to back >> Vulkan fences. This API is pretty much the vulkan fence waiting >> API, and I've ported the code from amdgpu. >> >> v2: accept relative timeout, pass remaining time back >> to userspace. >> v3: return to absolute timeouts. >> >> Signed-off-by: Dave Airlie >> --- >> drivers/gpu/drm/drm_internal.h | 2 + >> drivers/gpu/drm/drm_ioctl.c | 2 + >> drivers/gpu/drm/drm_syncobj.c | 164 ++++++++++++++++++++++++++++++ >> +++++++++++ >> include/uapi/drm/drm.h | 14 ++++ >> 4 files changed, 182 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_internal.h >> b/drivers/gpu/drm/drm_internal.h >> index 3fdef2c..53e3f6b 100644 >> --- a/drivers/gpu/drm/drm_internal.h >> +++ b/drivers/gpu/drm/drm_internal.h >> @@ -156,3 +156,5 @@ int drm_syncobj_handle_to_fd_ioctl(struct drm_device >> *dev, void *data, >> struct drm_file *file_private); >> int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_private); >> +int drm_syncobj_wait_ioctl(struct drm_device *dev, void *data, >> + struct drm_file *file_private); >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >> index f1e5681..385ce74 100644 >> --- a/drivers/gpu/drm/drm_ioctl.c >> +++ b/drivers/gpu/drm/drm_ioctl.c >> @@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { >> DRM_UNLOCKED|DRM_RENDER_ALLOW), >> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, >> drm_syncobj_fd_to_handle_ioctl, >> DRM_UNLOCKED|DRM_RENDER_ALLOW), >> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_syncobj_wait_ioctl, >> + DRM_UNLOCKED|DRM_RENDER_ALLOW), >> }; >> >> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) >> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj. >> c >> index b611480..8b87594 100644 >> --- a/drivers/gpu/drm/drm_syncobj.c >> +++ b/drivers/gpu/drm/drm_syncobj.c >> @@ -1,5 +1,7 @@ >> /* >> * Copyright 2017 Red Hat >> + * Parts ported from amdgpu (fence wait code). >> + * Copyright 2016 Advanced Micro Devices, Inc. >> * >> * Permission is hereby granted, free of charge, to any person obtaining >> a >> * copy of this software and associated documentation files (the >> "Software"), >> @@ -31,6 +33,9 @@ >> * that contain an optional fence. The fence can be updated with a new >> * fence, or be NULL. >> * >> + * syncobj's can be waited upon, where it will wait for the underlying >> + * fence. >> + * >> * syncobj's can be export to fd's and back, these fd's are opaque and >> * have no other use case, except passing the syncobj between processes. >> * >> @@ -375,3 +380,162 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device >> *dev, void *data, >> return drm_syncobj_fd_to_handle(file_private, args->fd, >> &args->handle); >> } >> + >> + >> +/** >> + * drm_timeout_abs_to_jiffies - calculate jiffies timeout from absolute >> value >> + * >> + * @timeout_ns: timeout in ns >> + * >> + * Calculate the timeout in jiffies from an absolute timeout in ns. >> + */ >> +unsigned long drm_timeout_abs_to_jiffies(uint64_t timeout_ns) >> +{ >> + unsigned long timeout_jiffies; >> + ktime_t timeout; >> + >> + /* clamp timeout if it's to large */ >> + if (((int64_t)timeout_ns) < 0) >> + return MAX_SCHEDULE_TIMEOUT; >> + >> + timeout = ktime_sub(ns_to_ktime(timeout_ns), ktime_get()); >> + if (ktime_to_ns(timeout) < 0) >> + return 0; >> + >> + timeout_jiffies = nsecs_to_jiffies(ktime_to_ns(timeout)); >> + /* clamp timeout to avoid unsigned-> signed overflow */ >> + if (timeout_jiffies > MAX_SCHEDULE_TIMEOUT ) >> + return MAX_SCHEDULE_TIMEOUT - 1; >> + >> + return timeout_jiffies; >> +} >> + >> +static int drm_syncobj_wait_all_fences(struct drm_device *dev, >> + struct drm_file *file_private, >> + struct drm_syncobj_wait *wait, >> + uint32_t *handles) >> +{ >> + uint32_t i; >> + int ret; >> + unsigned long timeout = drm_timeout_abs_to_jiffies(wai >> t->timeout_ns); >> + >> + for (i = 0; i < wait->count_handles; i++) { >> + struct dma_fence *fence; >> + >> + ret = drm_syncobj_fence_get(file_private, handles[i], >> + &fence); >> + if (ret) >> + return ret; >> + >> + if (!fence) >> + continue; >> + >> + ret = dma_fence_wait_timeout(fence, true, timeout); >> + >> + dma_fence_put(fence); >> + if (ret < 0) >> + return ret; >> + if (ret == 0) >> + break; >> + timeout = ret; >> + } >> + >> + wait->out_timeout_ns = jiffies_to_nsecs(ret); >> + wait->out_status = (ret > 0); >> + wait->first_signaled = 0; >> + return 0; >> +} >> + >> +static int drm_syncobj_wait_any_fence(struct drm_device *dev, >> + struct drm_file *file_private, >> + struct drm_syncobj_wait *wait, >> + uint32_t *handles) >> +{ >> + unsigned long timeout = drm_timeout_abs_to_jiffies(wai >> t->timeout_ns); >> + struct dma_fence **array; >> + uint32_t i; >> + int ret; >> + uint32_t first = ~0; >> + >> + /* Prepare the fence array */ >> + array = kcalloc(wait->count_handles, >> + sizeof(struct dma_fence *), GFP_KERNEL); >> + >> + if (array == NULL) >> + return -ENOMEM; >> + >> + for (i = 0; i < wait->count_handles; i++) { >> + struct dma_fence *fence; >> + >> + ret = drm_syncobj_fence_get(file_private, handles[i], >> + &fence); >> + if (ret) >> + goto err_free_fence_array; >> + else if (fence) >> + array[i] = fence; >> + else { /* NULL, the fence has been already signaled */ >> + ret = 1; >> + goto out; >> + } >> + } >> > These two functions have unexpected and subtly different behaviors when drm_syncobj_fence_get fails. wait_all may fail in the middle of waiting whereas wait_any fails up-front and holds a reference to ensure it doesn't fail later. Given that freeing things is inherently racy, it seems less than ideal to wait some arbitrary amount of time governed by fence[0] before getting fence[1]. Would it make sense to move the array setup into _ioctl and make both hold a reference to every fence? > + >> + ret = dma_fence_wait_any_timeout(array, wait->count_handles, >> true, timeout, >> + &first); >> + if (ret < 0) >> + goto err_free_fence_array; >> +out: >> + wait->out_timeout_ns = jiffies_to_nsecs(ret); >> + wait->out_status = (ret > 0); >> + wait->first_signaled = first; >> + /* set return value 0 to indicate success */ >> + ret = 0; >> + >> +err_free_fence_array: >> + for (i = 0; i < wait->count_handles; i++) >> + dma_fence_put(array[i]); >> + kfree(array); >> + >> + return ret; >> +} >> + >> +int >> +drm_syncobj_wait_ioctl(struct drm_device *dev, void *data, >> + struct drm_file *file_private) >> +{ >> + struct drm_syncobj_wait *args = data; >> + uint32_t *handles; >> + int ret = 0; >> + >> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) >> + return -ENODEV; >> + >> + if (args->flags != 0 && args->flags != >> DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL) >> + return -EINVAL; >> + >> + if (args->count_handles == 0) >> + return 0; >> + >> + /* Get the handles from userspace */ >> + handles = kmalloc_array(args->count_handles, sizeof(uint32_t), >> + GFP_KERNEL); >> + if (handles == NULL) >> + return -ENOMEM; >> + >> + if (copy_from_user(handles, >> + (void __user *)(unsigned long)(args->handles), >> + sizeof(uint32_t) * args->count_handles)) { >> + ret = -EFAULT; >> + goto err_free_handles; >> + } >> + >> + if (args->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL) >> + ret = drm_syncobj_wait_all_fences(dev, file_private, >> + args, handles); >> + else >> + ret = drm_syncobj_wait_any_fence(dev, file_private, >> + args, handles); >> +err_free_handles: >> + kfree(handles); >> + >> + return ret; >> +} >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >> index 96c5c78..d6e2f62 100644 >> --- a/include/uapi/drm/drm.h >> +++ b/include/uapi/drm/drm.h >> @@ -716,6 +716,19 @@ struct drm_syncobj_handle { >> __u32 pad; >> }; >> >> +#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0) >> +struct drm_syncobj_wait { >> + __u64 handles; >> + /* absolute timeout */ >> + __u64 timeout_ns; >> + /* remaining timeout */ >> + __u64 out_timeout_ns; >> > > Any particular reason why we don't just make timeout_ns an in/out? I > don't really care and haven't given it much thought; mostly just curious. > > >> + __u32 count_handles; >> + __u32 flags; >> + __u32 out_status; >> + __u32 first_signaled; /* on valid when not waiting all */ >> +}; >> + >> #if defined(__cplusplus) >> } >> #endif >> @@ -838,6 +851,7 @@ extern "C" { >> #define DRM_IOCTL_SYNCOBJ_DESTROY DRM_IOWR(0xC0, struct >> drm_syncobj_destroy) >> #define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD DRM_IOWR(0xC1, struct >> drm_syncobj_handle) >> #define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE DRM_IOWR(0xC2, struct >> drm_syncobj_handle) >> +#define DRM_IOCTL_SYNCOBJ_WAIT DRM_IOWR(0xC3, struct >> drm_syncobj_wait) >> >> /** >> * Device specific ioctls should only be in their respective headers >> -- >> 2.9.4 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > > --001a11497cfe6f67440550487f1a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On W= ed, May 24, 2017 at 10:25 AM, Jason Ekstrand <jason@jlekstrand.net= > wrote:
On = Wed, May 24, 2017 at 12:06 AM, Dave Airlie <airlied@gmail.com> wrote:
From: Dave Airlie <airlied@redhat.com>=

This interface will allow sync object to be used to back
Vulkan fences. This API is pretty much the vulkan fence waiting
API, and I've ported the code from amdgpu.

v2: accept relative timeout, pass remaining time back
to userspace.
v3: return to absolute timeouts.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
=C2=A0drivers/gpu/drm/drm_internal.h |=C2=A0 =C2=A02 +
=C2=A0drivers/gpu/drm/drm_ioctl.c=C2=A0 =C2=A0 |=C2=A0 =C2=A02 +
=C2=A0drivers/gpu/drm/drm_syncobj.c=C2=A0 | 164 ++++++++++++++++++++++= +++++++++++++++++++
=C2=A0include/uapi/drm/drm.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 14 ++= ++
=C2=A04 files changed, 182 insertions(+)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_inte= rnal.h
index 3fdef2c..53e3f6b 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -156,3 +156,5 @@ int drm_syncobj_handle_to_fd_ioctl(struct drm_devi= ce *dev, void *data,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct drm_file *file_p= rivate);
=C2=A0int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void = *data,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct drm_file *file_p= rivate);
+int drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 struct drm_file *file_private);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index f1e5681..385ce74 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -657,6 +657,8 @@ static const struct drm_ioctl_desc drm_ioctls[] =3D { =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 DRM_UNLOCKED|DRM_RENDER_ALLOW),
=C2=A0 =C2=A0 =C2=A0 =C2=A0 DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HAND= LE, drm_syncobj_fd_to_handle_ioctl,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 DRM_UNLOCKED|DRM_RENDER_ALLOW),
+=C2=A0 =C2=A0 =C2=A0 =C2=A0DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_= syncobj_wait_ioctl,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0DRM_UNLOCKED|DRM_RENDER_ALLOW),
=C2=A0};

=C2=A0#define DRM_CORE_IOCTL_COUNT=C2=A0 =C2=A0ARRAY_SIZE( drm_ioctls )
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_synco= bj.c
index b611480..8b87594 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1,5 +1,7 @@
=C2=A0/*
=C2=A0 * Copyright 2017 Red Hat
+ * Parts ported from amdgpu (fence wait code).
+ * Copyright 2016 Advanced Micro Devices, Inc.
=C2=A0 *
=C2=A0 * Permission is hereby granted, free of charge, to any person obtain= ing a
=C2=A0 * copy of this software and associated documentation files (the &quo= t;Software"),
@@ -31,6 +33,9 @@
=C2=A0 * that contain an optional fence. The fence can be updated with a ne= w
=C2=A0 * fence, or be NULL.
=C2=A0 *
+ * syncobj's can be waited upon, where it will wait for the underlying=
+ * fence.
+ *
=C2=A0 * syncobj's can be export to fd's and back, these fd's a= re opaque and
=C2=A0 * have no other use case, except passing the syncobj between process= es.
=C2=A0 *
@@ -375,3 +380,162 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device= *dev, void *data,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return drm_syncobj_fd_to_handle(file_priva= te, args->fd,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &arg= s->handle);
=C2=A0}
+
+
+/**
+ * drm_timeout_abs_to_jiffies - calculate jiffies timeout from absolute va= lue
+ *
+ * @timeout_ns: timeout in ns
+ *
+ * Calculate the timeout in jiffies from an absolute timeout in ns.
+ */
+unsigned long drm_timeout_abs_to_jiffies(uint64_t timeout_ns)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long timeout_jiffies;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0ktime_t timeout;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* clamp timeout if it's to large */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (((int64_t)timeout_ns) < 0)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return MAX_SCHEDULE= _TIMEOUT;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0timeout =3D ktime_sub(ns_to_ktime(timeout_= ns), ktime_get());
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (ktime_to_ns(timeout) < 0)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0timeout_jiffies =3D nsecs_to_jiffies(ktime_to_n= s(timeout));
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/*=C2=A0 clamp timeout to avoid unsigned-> s= igned overflow */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (timeout_jiffies > MAX_SCHEDULE_TIMEOUT )=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return MAX_SCHEDULE= _TIMEOUT - 1;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return timeout_jiffies;
+}
+
+static int drm_syncobj_wait_all_fences(struct drm_device *dev,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct drm_file= *file_private,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct drm_sync= obj_wait *wait,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 uint32_t *handl= es)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0uint32_t i;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0int ret;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long timeout =3D drm_timeout_abs_to_ji= ffies(wait->timeout_ns);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < wait->count_handles; i+= +) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct dma_fence *f= ence;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D drm_syncobj= _fence_get(file_private, handles[i],
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0&fence);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0return ret;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!fence)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0continue;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D dma_fence_w= ait_timeout(fence, true, timeout);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dma_fence_put(fence= );
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0return ret;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret =3D=3D 0) +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0break;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0timeout =3D ret; +=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0wait->out_timeout_ns =3D jiffies_to_nsecs(re= t);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0wait->out_status =3D (ret > 0);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0wait->first_signaled =3D 0;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
+}
+
+static int drm_syncobj_wait_any_fence(struct drm_device *dev,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct drm_file = *file_private,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct drm_synco= bj_wait *wait,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0uint32_t *handle= s)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long timeout =3D drm_timeout_abs_to_ji= ffies(wait->timeout_ns);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct dma_fence **array;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0uint32_t i;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0int ret;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0uint32_t first =3D ~0;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Prepare the fence array */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0array =3D kcalloc(wait->count_handles,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0sizeof(struct dma_fence *), GFP_KERNEL);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (array =3D=3D NULL)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOMEM;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < wait->count_handles; i+= +) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct dma_fence *f= ence;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D drm_syncobj= _fence_get(file_private, handles[i],
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0&fence);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0goto err_free_fence_array;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else if (fence)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0array[i] =3D fence;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else { /* NULL, the= fence has been already signaled */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0ret =3D 1;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0goto out;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}

These two functions have unexpected and s= ubtly different behaviors when drm_syncobj_fence_get fails.=C2=A0 wait_all = may fail in the middle of waiting whereas wait_any fails up-front and holds= a reference to ensure it doesn't fail later.=C2=A0 Given that freeing = things is inherently racy, it seems less than ideal to wait some arbitrary = amount of time governed by fence[0] before getting fence[1].=C2=A0 Would it= make sense to move the array setup into _ioctl and make both hold a refere= nce to every fence?
=C2=A0
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D dma_fence_wait_any_timeout(array, = wait->count_handles, true, timeout,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &fir= st);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto err_free_fence= _array;
+out:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0wait->out_timeout_ns =3D jiffies_to_nsecs(re= t);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0wait->out_status =3D (ret > 0);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0wait->first_signaled =3D first;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* set return value 0 to indicate success */ +=C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D 0;
+
+err_free_fence_array:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < wait->count_handles; i+= +)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dma_fence_put(array= [i]);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0kfree(array);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
+}
+
+int
+drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 struct drm_file *file_private)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct drm_syncobj_wait *args =3D data;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0uint32_t *handles;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0int ret =3D 0;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ= ))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENODEV;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (args->flags !=3D 0 && args->f= lags !=3D DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (args->count_handles =3D=3D 0)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Get the handles from userspace */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0handles =3D kmalloc_array(args->count_handles, sizeof(uint32_t),
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0GFP_KERNEL);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (handles =3D=3D NULL)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOMEM;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (copy_from_user(handles,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 (void __user *)(unsigned long)(args->handles),
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 sizeof(uint32_t) * args->count_handles)) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D -EFAULT; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto err_free_handl= es;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (args->flags & DRM_SYNCOBJ_WAIT_FLAGS= _WAIT_ALL)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D drm_syncobj= _wait_all_fences(dev, file_private,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0args, handles);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0else
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D drm_syncobj= _wait_any_fence(dev, file_private,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 args, handles);
+err_free_handles:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0kfree(handles);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
+}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 96c5c78..d6e2f62 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -716,6 +716,19 @@ struct drm_syncobj_handle {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 __u32 pad;
=C2=A0};

+#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
+struct drm_syncobj_wait {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0__u64 handles;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* absolute timeout */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0__u64 timeout_ns;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/* remaining timeout */
+=C2=A0 =C2=A0 =C2=A0 =C2=A0__u64 out_timeout_ns;

=
Any particular reason why we don't just make tim= eout_ns an in/out?=C2=A0 I don't really care and haven't given it m= uch thought; mostly just curious.
=C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0__u32 count_handles;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0__u32 flags;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0__u32 out_status;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0__u32 first_signaled; /* on valid when not wait= ing all */
+};
+
=C2=A0#if defined(__cplusplus)
=C2=A0}
=C2=A0#endif
@@ -838,6 +851,7 @@ extern "C" {
=C2=A0#define DRM_IOCTL_SYNCOBJ_DESTROY=C2=A0 =C2=A0 =C2=A0 DRM_IOWR(0xC0, = struct drm_syncobj_destroy)
=C2=A0#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD DRM_IOWR(0xC1, struct drm_sync= obj_handle)
=C2=A0#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE DRM_IOWR(0xC2, struct drm_sync= obj_handle)
+#define DRM_IOCTL_SYNCOBJ_WAIT=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DRM_IOWR(0= xC3, struct drm_syncobj_wait)

=C2=A0/**
=C2=A0 * Device specific ioctls should only be in their respective headers<= br> --
2.9.4

_______________________________________________
dri-devel mailing list
dri-de= vel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/l= istinfo/dri-devel


--001a11497cfe6f67440550487f1a-- --===============2045770448== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============2045770448==--