On Mon, Oct 8, 2018 at 12:53 AM Zhou, David(ChunMing) <david1.z...@amd.com>
wrote:
> >> 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.
>
>
>
> Hi Jason,
>
>
>
> How about adding two new wait flags?
>
> DRM_SYNCOBJ_WAIT_FLAGS_WAIT_COMPLETED
>
> DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE
>
Those seem like fine names to me. We should require that one of the two
flags be present when the sync object is a timeline.
--Jason
> Thanks,
>
> David
>
>
>
> *From:* Christian König <ckoenig.leichtzumer...@gmail.com>
> *Sent:* Tuesday, September 25, 2018 5:50 PM
> *To:* Jason Ekstrand <ja...@jlekstrand.net>; Zhou, David(ChunMing) <
> david1.z...@amd.com>
> *Cc:* amd-gfx mailing list <amd-...@lists.freedesktop.org>; Maling list -
> DRI developers <dri-devel@lists.freedesktop.org>
> *Subject:* Re: [PATCH 3/6] drm: add support of syncobj timeline point
> wait v2
>
>
>
> Am 25.09.2018 um 11:22 schrieb Jason Ekstrand:
>
> On Thu, Sep 20, 2018 at 6:04 AM Chunming Zhou <david1.z...@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.z...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
>
> _______________________________________________
>
> dri-devel mailing list
>
> dri-devel@lists.freedesktop.org
>
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
