[PATCH 1/4] [RFC]drm: add syncobj timeline support v6

Christian König christian.koenig at amd.com
Wed Sep 19 07:45:05 UTC 2018


Am 19.09.2018 um 09:32 schrieb zhoucm1:
>
>
> On 2018年09月19日 15:18, Christian König wrote:
>> Am 19.09.2018 um 06:26 schrieb Chunming Zhou:
> [snip]
>>>           *fence = NULL;
>>>           drm_syncobj_add_callback_locked(syncobj, cb, func);
>>> @@ -164,6 +177,153 @@ void drm_syncobj_remove_callback(struct 
>>> drm_syncobj *syncobj,
>>>       spin_unlock(&syncobj->lock);
>>>   }
>>>   +static void drm_syncobj_timeline_init(struct drm_syncobj *syncobj)
>>
>> We still have _timeline_ in the name here.
> the func is relevant to timeline members, or which name is proper?

Yeah, but we now use the timeline implementation for the individual 
syncobj as well.

Not a big issue, but I would just name it 
drm_syncobj_init()/drm_syncobj_fini.

>
>>
>>> +{
>>> +    spin_lock(&syncobj->lock);
>>> +    syncobj->timeline_context = dma_fence_context_alloc(1);
> [snip]
>>> +}
>>> +
>>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
>>> +               struct dma_fence **fence) {
>>> +
>>> +    return drm_syncobj_search_fence(syncobj, point,
>>> +                    DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
>>
>> I still have a bad feeling setting that flag as default cause it 
>> might change the behavior for the UAPI.
>>
>> Maybe export drm_syncobj_search_fence directly? E.g. with the flags 
>> parameter.
> previous v5 indeed do this, you let me wrap it, need change back?

No, the problem is that drm_syncobj_find_fence() is still using 
drm_syncobj_lookup_fence() which sets the flag instead of 
drm_syncobj_search_fence() without the flag.

That changes the UAPI behavior because previously we would have returned 
an error code and now we block for a fence to appear.

So I think the right solution would be to add the flags parameter to 
drm_syncobj_find_fence() and let the driver decide if we need to block 
or get -ENOENT.

Regards,
Christian.

>
> Regards,
> David Zhou
>>
>> Regards,
>> Christian.
>>
>>> +                    fence);
>>> +}
>>> +EXPORT_SYMBOL(drm_syncobj_lookup_fence);
>>> +
>>>   /**
>>>    * drm_syncobj_find_fence - lookup and reference the fence in a 
>>> sync object
>>>    * @file_private: drm file private pointer
>>> @@ -228,7 +443,7 @@ static int drm_syncobj_assign_null_handle(struct 
>>> drm_syncobj *syncobj)
>>>    * @fence: out parameter for the fence
>>>    *
>>>    * This is just a convenience function that combines 
>>> drm_syncobj_find() and
>>> - * drm_syncobj_fence_get().
>>> + * drm_syncobj_lookup_fence().
>>>    *
>>>    * Returns 0 on success or a negative error value on failure. On 
>>> success @fence
>>>    * contains a reference to the fence, which must be released by 
>>> calling
>>> @@ -236,18 +451,11 @@ static int 
>>> drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>>>    */
>>>   int drm_syncobj_find_fence(struct drm_file *file_private,
>>>                  u32 handle, u64 point,
>>> -               struct dma_fence **fence)
>>> -{
>>> +               struct dma_fence **fence) {
>>>       struct drm_syncobj *syncobj = drm_syncobj_find(file_private, 
>>> handle);
>>> -    int ret = 0;
>>> -
>>> -    if (!syncobj)
>>> -        return -ENOENT;
>>> +    int ret;
>>>   -    *fence = drm_syncobj_fence_get(syncobj);
>>> -    if (!*fence) {
>>> -        ret = -EINVAL;
>>> -    }
>>> +    ret = drm_syncobj_lookup_fence(syncobj, point, fence);
>>>       drm_syncobj_put(syncobj);
>>>       return ret;
>>>   }
>>> @@ -264,7 +472,7 @@ void drm_syncobj_free(struct kref *kref)
>>>       struct drm_syncobj *syncobj = container_of(kref,
>>>                              struct drm_syncobj,
>>>                              refcount);
>>> -    drm_syncobj_replace_fence(syncobj, 0, NULL);
>>> +    drm_syncobj_timeline_fini(syncobj);
>>>       kfree(syncobj);
>>>   }
>>>   EXPORT_SYMBOL(drm_syncobj_free);
>>> @@ -294,6 +502,11 @@ int drm_syncobj_create(struct drm_syncobj 
>>> **out_syncobj, uint32_t flags,
>>>       kref_init(&syncobj->refcount);
>>>       INIT_LIST_HEAD(&syncobj->cb_list);
>>>       spin_lock_init(&syncobj->lock);
>>> +    if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
>>> +        syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
>>> +    else
>>> +        syncobj->type = DRM_SYNCOBJ_TYPE_INDIVIDUAL;
>>> +    drm_syncobj_timeline_init(syncobj);
>>>         if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
>>>           ret = drm_syncobj_assign_null_handle(syncobj);
>>> @@ -576,7 +789,8 @@ drm_syncobj_create_ioctl(struct drm_device *dev, 
>>> void *data,
>>>           return -ENODEV;
>>>         /* no valid flags yet */
>>> -    if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
>>> +    if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
>>> +                DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
>>>           return -EINVAL;
>>>         return drm_syncobj_create_as_handle(file_private,
>>> @@ -669,9 +883,8 @@ static void syncobj_wait_syncobj_func(struct 
>>> drm_syncobj *syncobj,
>>>       struct syncobj_wait_entry *wait =
>>>           container_of(cb, struct syncobj_wait_entry, syncobj_cb);
>>>   -    /* This happens inside the syncobj lock */
>>> -    wait->fence = 
>>> dma_fence_get(rcu_dereference_protected(syncobj->fence,
>>> - lockdep_is_held(&syncobj->lock)));
>>> +    drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
>>> +
>>>       wake_up_process(wait->task);
>>>   }
>>>   @@ -698,7 +911,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;
>>> -        entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
>>> +        ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
>>> +                           &entries[i].fence);
>>>           if (!entries[i].fence) {
>>>               if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>>>                   continue;
>>> @@ -970,12 +1184,19 @@ drm_syncobj_reset_ioctl(struct drm_device 
>>> *dev, void *data,
>>>       if (ret < 0)
>>>           return ret;
>>>   -    for (i = 0; i < args->count_handles; i++)
>>> -        drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
>>> -
>>> +    for (i = 0; i < args->count_handles; i++) {
>>> +        if (syncobjs[i]->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
>>> +            DRM_ERROR("timeline syncobj cannot reset!\n");
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +        drm_syncobj_timeline_fini(syncobjs[i]);
>>> +        drm_syncobj_timeline_init(syncobjs[i]);
>>> +    }
>>> +out:
>>>       drm_syncobj_array_free(syncobjs, args->count_handles);
>>>   -    return 0;
>>> +    return ret;
>>>   }
>>>     int
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c 
>>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> index 0a8d2d64f380..a040fa3281df 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
>>>           if (!(flags & I915_EXEC_FENCE_WAIT))
>>>               continue;
>>>   -        fence = drm_syncobj_fence_get(syncobj);
>>> +        drm_syncobj_lookup_fence(syncobj, 0, &fence);
>>>           if (!fence)
>>>               return -EINVAL;
>>>   diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
>>> index 425432b85a87..6dac479fedfe 100644
>>> --- a/include/drm/drm_syncobj.h
>>> +++ b/include/drm/drm_syncobj.h
>>> @@ -30,6 +30,11 @@
>>>     struct drm_syncobj_cb;
>>>   +enum drm_syncobj_type {
>>> +    DRM_SYNCOBJ_TYPE_INDIVIDUAL,
>>> +    DRM_SYNCOBJ_TYPE_TIMELINE
>>> +};
>>> +
>>>   /**
>>>    * struct drm_syncobj - sync object.
>>>    *
>>> @@ -41,19 +46,36 @@ struct drm_syncobj {
>>>        */
>>>       struct kref refcount;
>>>       /**
>>> -     * @fence:
>>> -     * NULL or a pointer to the fence bound to this object.
>>> -     *
>>> -     * This field should not be used directly. Use 
>>> drm_syncobj_fence_get()
>>> -     * and drm_syncobj_replace_fence() instead.
>>> +     * @type: indicate syncobj type
>>> +     */
>>> +    enum drm_syncobj_type type;
>>> +    /**
>>> +     * @wq: wait signal operation work queue
>>> +     */
>>> +    wait_queue_head_t    wq;
>>> +    /**
>>> +     * @timeline_context: fence context used by timeline
>>>        */
>>> -    struct dma_fence __rcu *fence;
>>> +    u64 timeline_context;
>>>       /**
>>> -     * @cb_list: List of callbacks to call when the &fence gets 
>>> replaced.
>>> +     * @timeline: syncobj timeline value, which indicates point is 
>>> signaled.
>>>        */
>>> +    u64 timeline;
>>> +    /**
>>> +     * @signal_point: which indicates the latest signaler point.
>>> +     */
>>> +    u64 signal_point;
>>> +    /**
>>> +     * @signal_pt_list: signaler point list.
>>> +     */
>>> +    struct list_head signal_pt_list;
>>> +
>>> +    /**
>>> +         * @cb_list: List of callbacks to call when the &fence gets 
>>> replaced.
>>> +         */
>>>       struct list_head cb_list;
>>>       /**
>>> -     * @lock: Protects &cb_list and write-locks &fence.
>>> +     * @lock: Protects syncobj list and write-locks &fence.
>>>        */
>>>       spinlock_t lock;
>>>       /**
>>> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct 
>>> drm_syncobj *syncobj,
>>>   /**
>>>    * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
>>>    * @node: used by drm_syncob_add_callback to append this struct to
>>> - *      &drm_syncobj.cb_list
>>> + *       &drm_syncobj.cb_list
>>>    * @func: drm_syncobj_func_t to call
>>>    *
>>>    * This struct will be initialized by drm_syncobj_add_callback, 
>>> additional
>>> @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj *obj)
>>>       kref_put(&obj->refcount, drm_syncobj_free);
>>>   }
>>>   -/**
>>> - * drm_syncobj_fence_get - get a reference to a fence in a sync object
>>> - * @syncobj: sync object.
>>> - *
>>> - * This acquires additional reference to &drm_syncobj.fence 
>>> contained in @obj,
>>> - * if not NULL. It is illegal to call this without already holding 
>>> a reference.
>>> - * No locks required.
>>> - *
>>> - * Returns:
>>> - * Either the fence of @obj or NULL if there's none.
>>> - */
>>> -static inline struct dma_fence *
>>> -drm_syncobj_fence_get(struct drm_syncobj *syncobj)
>>> -{
>>> -    struct dma_fence *fence;
>>> -
>>> -    rcu_read_lock();
>>> -    fence = dma_fence_get_rcu_safe(&syncobj->fence);
>>> -    rcu_read_unlock();
>>> -
>>> -    return fence;
>>> -}
>>> -
>>>   struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>>>                        u32 handle);
>>>   void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 
>>> point,
>>> @@ -142,5 +141,7 @@ int drm_syncobj_create(struct drm_syncobj 
>>> **out_syncobj, uint32_t flags,
>>>   int drm_syncobj_get_handle(struct drm_file *file_private,
>>>                  struct drm_syncobj *syncobj, u32 *handle);
>>>   int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
>>> +int drm_syncobj_lookup_fence(struct drm_syncobj *syncobj, u64 point,
>>> +                 struct dma_fence **fence);
>>>     #endif
>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>> index 300f336633f2..cebdb2541eb7 100644
>>> --- a/include/uapi/drm/drm.h
>>> +++ b/include/uapi/drm/drm.h
>>> @@ -717,6 +717,7 @@ struct drm_prime_handle {
>>>   struct drm_syncobj_create {
>>>       __u32 handle;
>>>   #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
>>> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
>>>       __u32 flags;
>>>   };
>>
>



More information about the amd-gfx mailing list