Am 21.09.2018 um 09:15 schrieb Zhou, David(ChunMing):
>
>> -----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?
Yeah, that should work. Alternative is to use something like "while
(!list_empty()) { e = list_first_entry(); list_del(e)....". But either
way should work.
> Any other comment on patch series and libdrm patches? That one comment increases a patch set version seems be overcommit.
On a first glance that stuff looked good, but give me till monday for
that. I'm currently in the middle of debugging issues.
Thanks,
Christian.
>
> 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
