Revert "drm/syncobj: use dma_fence_get_stub"

From: David Stevens <stevensd@chromium.org>

This reverts commit 86bbd89d5da66fe760049ad3f04adc407ec0c4d6.

Using the singleton stub fence in drm_syncobj_assign_null_handle means
that all syncobjs created in an already signaled state or any syncobjs
signaled by userspace will reference the singleton fence when exported
to a sync_file. If those sync_files are queried with SYNC_IOC_FILE_INFO,
then the timestamp_ns value returned will correspond to whenever the
singleton stub fence was first initialized. This can break the ability
of userspace to use timestamps of these fences, as the singleton stub
fence's timestamp bears no relationship to any meaningful event.

Signed-off-by: David Stevens <stevensd@chromium.org>
---
 drivers/gpu/drm/drm_syncobj.c | 58 ++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 14 deletions(-)

Am 08.04.21 um 11:30 schrieb David Stevens:
> On Thu, Apr 8, 2021 at 4:03 PM Christian König <christian.koenig@amd.com> wrote:
>> Am 08.04.21 um 06:59 schrieb David Stevens:
>>> From: David Stevens <stevensd@chromium.org>
>>>
>>> This reverts commit 86bbd89d5da66fe760049ad3f04adc407ec0c4d6.
>>>
>>> Using the singleton stub fence in drm_syncobj_assign_null_handle means
>>> that all syncobjs created in an already signaled state or any syncobjs
>>> signaled by userspace will reference the singleton fence when exported
>>> to a sync_file. If those sync_files are queried with SYNC_IOC_FILE_INFO,
>>> then the timestamp_ns value returned will correspond to whenever the
>>> singleton stub fence was first initialized. This can break the ability
>>> of userspace to use timestamps of these fences, as the singleton stub
>>> fence's timestamp bears no relationship to any meaningful event.
>> And why exactly is having the timestamp of the call to
>> drm_syncobj_assign_null_handle() better?
> The timestamp returned by SYNC_IOC_FILE_INFO is the "timestamp of
> status change in nanoseconds". If userspace signals the fence with
> DRM_IOCTL_SYNCOBJ_SIGNAL, then a timestamp from
> drm_syncobj_assign_null_handle corresponds to the status change. If
> userspace sets DRM_SYNCOBJ_CREATE_SIGNALED when creating a fence, then
> the status change happens immediately upon creation, which again
> corresponds to when drm_syncobj_assign_null_handle gets called.

Ok, that makes sense.

>
>> Additional if you really need that please don't revert the patch.
>> Instead provide a function which returns a newly initialized stub fence
>> in the dma_fence.c code.
> Ack.

Just add a something like dma_fence_get_new_stub() with kmalloc(), 
dma_fence_init() and dma_fence_signal().

Shouldn't be more than a six liner.

Thanks,
Christian.

>
> -David
>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>> ---
>>>    drivers/gpu/drm/drm_syncobj.c | 58 ++++++++++++++++++++++++++---------
>>>    1 file changed, 44 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>>> index 349146049849..7cc11f1a83f4 100644
>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>> @@ -211,6 +211,21 @@ struct syncobj_wait_entry {
>>>    static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
>>>                                      struct syncobj_wait_entry *wait);
>>>
>>> +struct drm_syncobj_stub_fence {
>>> +     struct dma_fence base;
>>> +     spinlock_t lock;
>>> +};
>>> +
>>> +static const char *drm_syncobj_stub_fence_get_name(struct dma_fence *fence)
>>> +{
>>> +     return "syncobjstub";
>>> +}
>>> +
>>> +static const struct dma_fence_ops drm_syncobj_stub_fence_ops = {
>>> +     .get_driver_name = drm_syncobj_stub_fence_get_name,
>>> +     .get_timeline_name = drm_syncobj_stub_fence_get_name,
>>> +};
>>> +
>>>    /**
>>>     * drm_syncobj_find - lookup and reference a sync object.
>>>     * @file_private: drm file private pointer
>>> @@ -344,18 +359,24 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>>>    }
>>>    EXPORT_SYMBOL(drm_syncobj_replace_fence);
>>>
>>> -/**
>>> - * drm_syncobj_assign_null_handle - assign a stub fence to the sync object
>>> - * @syncobj: sync object to assign the fence on
>>> - *
>>> - * Assign a already signaled stub fence to the sync object.
>>> - */
>>> -static void drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>>> +static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>>>    {
>>> -     struct dma_fence *fence = dma_fence_get_stub();
>>> +     struct drm_syncobj_stub_fence *fence;
>>>
>>> -     drm_syncobj_replace_fence(syncobj, fence);
>>> -     dma_fence_put(fence);
>>> +     fence = kzalloc(sizeof(*fence), GFP_KERNEL);
>>> +     if (fence == NULL)
>>> +             return -ENOMEM;
>>> +
>>> +     spin_lock_init(&fence->lock);
>>> +     dma_fence_init(&fence->base, &drm_syncobj_stub_fence_ops,
>>> +                    &fence->lock, 0, 0);
>>> +     dma_fence_signal(&fence->base);
>>> +
>>> +     drm_syncobj_replace_fence(syncobj, &fence->base);
>>> +
>>> +     dma_fence_put(&fence->base);
>>> +
>>> +     return 0;
>>>    }
>>>
>>>    /* 5s default for wait submission */
>>> @@ -469,6 +490,7 @@ EXPORT_SYMBOL(drm_syncobj_free);
>>>    int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
>>>                       struct dma_fence *fence)
>>>    {
>>> +     int ret;
>>>        struct drm_syncobj *syncobj;
>>>
>>>        syncobj = kzalloc(sizeof(struct drm_syncobj), GFP_KERNEL);
>>> @@ -479,8 +501,13 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
>>>        INIT_LIST_HEAD(&syncobj->cb_list);
>>>        spin_lock_init(&syncobj->lock);
>>>
>>> -     if (flags & DRM_SYNCOBJ_CREATE_SIGNALED)
>>> -             drm_syncobj_assign_null_handle(syncobj);
>>> +     if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
>>> +             ret = drm_syncobj_assign_null_handle(syncobj);
>>> +             if (ret < 0) {
>>> +                     drm_syncobj_put(syncobj);
>>> +                     return ret;
>>> +             }
>>> +     }
>>>
>>>        if (fence)
>>>                drm_syncobj_replace_fence(syncobj, fence);
>>> @@ -1322,8 +1349,11 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
>>>        if (ret < 0)
>>>                return ret;
>>>
>>> -     for (i = 0; i < args->count_handles; i++)
>>> -             drm_syncobj_assign_null_handle(syncobjs[i]);
>>> +     for (i = 0; i < args->count_handles; i++) {
>>> +             ret = drm_syncobj_assign_null_handle(syncobjs[i]);
>>> +             if (ret < 0)
>>> +                     break;
>>> +     }
>>>
>>>        drm_syncobj_array_free(syncobjs, args->count_handles);
>>>