[PATCH 3/6] drm: add support of syncobj timeline point wait v2
Zhou, David(ChunMing)
David1.Zhou at amd.com
Fri Sep 21 07:15:28 UTC 2018
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
> Christian K?nig
> Sent: Thursday, September 20, 2018 7:11 PM
> To: Zhou, David(ChunMing) <David1.Zhou at amd.com>; dri-
> devel at lists.freedesktop.org
> Cc: amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 3/6] drm: add support of syncobj timeline point wait v2
>
> Am 20.09.2018 um 13:03 schrieb Chunming Zhou:
> > points array is one-to-one match with syncobjs array.
> > v2:
> > add seperate ioctl for timeline point wait, otherwise break uapi.
> >
> > Signed-off-by: Chunming Zhou <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);
>
> That looks fishy to me. Why do we need to unlock
Cb func will call _search_fence, which will need to grab the lock, otherwise deadlock.
>and who guarantees that
> tmp is still valid when we grab the lock again?
Sorry for that, quickly fix deadlock and forget to care that when debug.
How about splice to a tmp list, and then list_for _xxx without lock?
Any other comment on patch series and libdrm patches? That one comment increases a patch set version seems be overcommit.
Thanks,
David Zhou
>
> Apart from that can't see anything obvious wrong, but I certainly need to
> take a closer look.
>
> Christian.
>
> > }
> > 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.
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the amd-gfx
mailing list