[PATCH 3/6] drm: add support of syncobj timeline point wait v2
Christian König
ckoenig.leichtzumerken at gmail.com
Tue Sep 25 09:49:48 UTC 2018
Am 25.09.2018 um 11:22 schrieb Jason Ekstrand:
> On Thu, Sep 20, 2018 at 6:04 AM Chunming Zhou <david1.zhou at amd.com
> <mailto:david1.zhou at amd.com>> wrote:
>
> points array is one-to-one match with syncobjs array.
> v2:
> add seperate ioctl for timeline point wait, otherwise break uapi.
>
>
> I think ioctl structs can be extended as long as fields aren't
> re-ordered. I'm not sure on the details of this though as I'm not a
> particularly experienced kernel developer.
Yeah, that is correct. The problem in this particular case is that we
don't change the direct IOCTL parameter, but rather the array it points to.
We could do something like keep the existing handles array and add a
separate optional one for the timeline points. That would also drop the
need for the padding of the structure.
> Another general comment (no good place to put it) is that I think we
> want two kinds of waits: Wait for time point to be completed and wait
> for time point to become available. The first is the usual CPU wait
> for completion while the second is for use by userspace drivers to
> wait until the first moment where they can submit work which depends
> on a given time point.
Oh, yeah that is a really good point as ell.
Christian.
> Signed-off-by: Chunming Zhou <david1.zhou at amd.com
> <mailto:david1.zhou at amd.com>>
> ---
> drivers/gpu/drm/drm_internal.h | 2 +
> drivers/gpu/drm/drm_ioctl.c | 2 +
> drivers/gpu/drm/drm_syncobj.c | 99
> +++++++++++++++++++++++++++++-----
> include/uapi/drm/drm.h | 14 +++++
> 4 files changed, 103 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_internal.h
> b/drivers/gpu/drm/drm_internal.h
> index 0c4eb4a9ab31..566d44e3c782 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -183,6 +183,8 @@ 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);
> +int drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void
> *data,
> + struct drm_file *file_private);
> int drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_private);
> int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 6b4a633b4240..c0891614f516 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -669,6 +669,8 @@ static const struct drm_ioctl_desc
> drm_ioctls[] = {
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_syncobj_wait_ioctl,
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT,
> drm_syncobj_timeline_wait_ioctl,
> + DRM_UNLOCKED|DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_RESET,
> drm_syncobj_reset_ioctl,
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL,
> drm_syncobj_signal_ioctl,
> diff --git a/drivers/gpu/drm/drm_syncobj.c
> b/drivers/gpu/drm/drm_syncobj.c
> index 67472bd77c83..a43de0e4616c 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -126,13 +126,14 @@ static void
> drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj,
> }
>
> static int drm_syncobj_fence_get_or_add_callback(struct
> drm_syncobj *syncobj,
> + u64 point,
> struct dma_fence
> **fence,
> struct
> drm_syncobj_cb *cb,
> drm_syncobj_func_t func)
> {
> int ret;
>
> - ret = drm_syncobj_search_fence(syncobj, 0, 0, fence);
> + ret = drm_syncobj_search_fence(syncobj, point, 0, fence);
> if (!ret)
> return 1;
>
> @@ -143,7 +144,7 @@ static int
> drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
> */
> if (!list_empty(&syncobj->signal_pt_list)) {
> spin_unlock(&syncobj->lock);
> - drm_syncobj_search_fence(syncobj, 0, 0, fence);
> + drm_syncobj_search_fence(syncobj, point, 0, fence);
> if (*fence)
> return 1;
> spin_lock(&syncobj->lock);
> @@ -358,7 +359,9 @@ void drm_syncobj_replace_fence(struct
> drm_syncobj *syncobj,
> spin_lock(&syncobj->lock);
> list_for_each_entry_safe(cur, tmp,
> &syncobj->cb_list, node) {
> list_del_init(&cur->node);
> + spin_unlock(&syncobj->lock);
> cur->func(syncobj, cur);
> + spin_lock(&syncobj->lock);
> }
> spin_unlock(&syncobj->lock);
> }
> @@ -856,6 +859,7 @@ struct syncobj_wait_entry {
> struct dma_fence *fence;
> struct dma_fence_cb fence_cb;
> struct drm_syncobj_cb syncobj_cb;
> + u64 point;
> };
>
> static void syncobj_wait_fence_func(struct dma_fence *fence,
> @@ -873,12 +877,13 @@ static void syncobj_wait_syncobj_func(struct
> drm_syncobj *syncobj,
> struct syncobj_wait_entry *wait =
> container_of(cb, struct syncobj_wait_entry,
> syncobj_cb);
>
> - drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
> + drm_syncobj_search_fence(syncobj, wait->point, 0,
> &wait->fence);
>
> wake_up_process(wait->task);
> }
>
> static signed long drm_syncobj_array_wait_timeout(struct
> drm_syncobj **syncobjs,
> + void __user
> *user_points,
> uint32_t count,
> uint32_t flags,
> signed long timeout,
> @@ -886,13 +891,27 @@ static signed long
> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> {
> struct syncobj_wait_entry *entries;
> struct dma_fence *fence;
> + uint64_t *points;
> signed long ret;
> uint32_t signaled_count, i;
>
> - entries = kcalloc(count, sizeof(*entries), GFP_KERNEL);
> - if (!entries)
> + points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
> + if (points == NULL)
> return -ENOMEM;
>
> + if (!user_points) {
> + memset(points, 0, count * sizeof(uint64_t));
> + } else if (copy_from_user(points, user_points,
> sizeof(uint64_t) * count)) {
> + ret = -EFAULT;
> + goto err_free_points;
> + }
> +
> +
> + entries = kcalloc(count, sizeof(*entries), GFP_KERNEL);
> + if (!entries) {
> + ret = -ENOMEM;
> + goto err_free_points;
> + }
> /* Walk the list of sync objects and initialize entries.
> We do
> * this up-front so that we can properly return -EINVAL if
> there is
> * a syncobj with a missing fence and then never have the
> chance of
> @@ -901,7 +920,8 @@ static signed long
> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> signaled_count = 0;
> for (i = 0; i < count; ++i) {
> entries[i].task = current;
> - ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
> + entries[i].point = points[i];
> + ret = drm_syncobj_search_fence(syncobjs[i],
> points[i], 0,
> &entries[i].fence);
> if (!entries[i].fence) {
> if (flags &
> DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
> @@ -940,6 +960,7 @@ static signed long
> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
> for (i = 0; i < count; ++i) {
> drm_syncobj_fence_get_or_add_callback(syncobjs[i],
> + entries[i].point,
> &entries[i].fence,
> &entries[i].syncobj_cb,
> syncobj_wait_syncobj_func);
> @@ -1003,6 +1024,9 @@ static signed long
> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> }
> kfree(entries);
>
> +err_free_points:
> + kfree(points);
> +
> return ret;
> }
>
> @@ -1041,20 +1065,33 @@ static signed long
> drm_timeout_abs_to_jiffies(int64_t timeout_nsec)
> static int drm_syncobj_array_wait(struct drm_device *dev,
> struct drm_file *file_private,
> struct drm_syncobj_wait *wait,
> - struct drm_syncobj **syncobjs)
> + struct drm_syncobj_timeline_wait
> *timeline_wait,
> + struct drm_syncobj **syncobjs,
> bool timeline)
> {
> - signed long timeout =
> drm_timeout_abs_to_jiffies(wait->timeout_nsec);
> + signed long timeout = 0;
> signed long ret = 0;
> uint32_t first = ~0;
>
> - ret = drm_syncobj_array_wait_timeout(syncobjs,
> - wait->count_handles,
> - wait->flags,
> - timeout, &first);
> + if (!timeline) {
> + timeout =
> drm_timeout_abs_to_jiffies(wait->timeout_nsec);
> + ret = drm_syncobj_array_wait_timeout(syncobjs,
> + NULL,
> + wait->count_handles,
> + wait->flags,
> + timeout, &first);
> + wait->first_signaled = first;
> + } else {
> + timeout =
> drm_timeout_abs_to_jiffies(timeline_wait->timeout_nsec);
> + ret = drm_syncobj_array_wait_timeout(syncobjs,
> + u64_to_user_ptr(timeline_wait->points),
> + timeline_wait->count_handles,
> + timeline_wait->flags,
> + timeout, &first);
> + timeline_wait->first_signaled = first;
> + }
> if (ret < 0)
> return ret;
>
> - wait->first_signaled = first;
> if (ret == 0)
> return -ETIME;
> return 0;
> @@ -1142,13 +1179,47 @@ drm_syncobj_wait_ioctl(struct drm_device
> *dev, void *data,
> return ret;
>
> ret = drm_syncobj_array_wait(dev, file_private,
> - args, syncobjs);
> + args, NULL, syncobjs, false);
>
> drm_syncobj_array_free(syncobjs, args->count_handles);
>
> return ret;
> }
>
> +int
> +drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private)
> +{
> + struct drm_syncobj_timeline_wait *args = data;
> + struct drm_syncobj **syncobjs;
> + int ret = 0;
> +
> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> + return -ENODEV;
> +
> + if (args->flags & ~(DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL |
> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
> + return -EINVAL;
> +
> + if (args->count_handles == 0)
> + return -EINVAL;
> +
> + ret = drm_syncobj_array_find(file_private,
> + u64_to_user_ptr(args->handles),
> + args->count_handles,
> + &syncobjs);
> + if (ret < 0)
> + return ret;
> +
> + ret = drm_syncobj_array_wait(dev, file_private,
> + NULL, args, syncobjs, true);
> +
> + drm_syncobj_array_free(syncobjs, args->count_handles);
> +
> + return ret;
> +}
> +
> +
> int
> drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_private)
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index cebdb2541eb7..501e86d81f47 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -748,6 +748,19 @@ struct drm_syncobj_wait {
> __u32 pad;
> };
>
> +struct drm_syncobj_timeline_wait {
> + __u64 handles;
> + /* wait on specific timeline point for every handles*/
> + __u64 points;
> + /* absolute timeout */
> + __s64 timeout_nsec;
> + __u32 count_handles;
> + __u32 flags;
> + __u32 first_signaled; /* only valid when not waiting all */
> + __u32 pad;
> +};
> +
> +
> struct drm_syncobj_array {
> __u64 handles;
> __u32 count_handles;
> @@ -910,6 +923,7 @@ extern "C" {
> #define DRM_IOCTL_MODE_GET_LEASE DRM_IOWR(0xC8, struct
> drm_mode_get_lease)
> #define DRM_IOCTL_MODE_REVOKE_LEASE DRM_IOWR(0xC9, struct
> drm_mode_revoke_lease)
>
> +#define DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT DRM_IOWR(0xCA, struct
> drm_syncobj_timeline_wait)
> /**
> * Device specific ioctls should only be in their respective headers
> * The device specific ioctl range is from 0x40 to 0x9f.
> --
> 2.17.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> <mailto:dri-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180925/735ab887/attachment-0001.html>
More information about the dri-devel
mailing list