| Message ID | 20181019092711.20144-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Revert "add syncobj timeline support v9" | expand |
On Fri, Oct 19, 2018 at 11:27:11AM +0200, Christian König wrote: > From: Christian König <easy2remember.chk@googlemail.com> > > Still contains some bugs. > > This reverts commit 48197bc564c7a1888c86024a1ba4f956e0ec2300. Thanks for the super-quick handling, much appreciated. > > Bugs: https://bugs.freedesktop.org/show_bug.cgi?id=108490 > Signed-off-by: Christian König <Christian.Koenig@amd.com> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cheers, Daniel > --- > drivers/gpu/drm/drm_syncobj.c | 306 +++------------------ > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- > include/drm/drm_syncobj.h | 67 +++-- > include/uapi/drm/drm.h | 1 - > 4 files changed, 75 insertions(+), 301 deletions(-) > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 57bf6006394d..e2c5b3ca4824 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -56,9 +56,6 @@ > #include "drm_internal.h" > #include <drm/drm_syncobj.h> > > -/* merge normal syncobj to timeline syncobj, the point interval is 1 */ > -#define DRM_SYNCOBJ_BINARY_POINT 1 > - > struct drm_syncobj_stub_fence { > struct dma_fence base; > spinlock_t lock; > @@ -74,11 +71,6 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops = { > .get_timeline_name = drm_syncobj_stub_fence_get_name, > }; > > -struct drm_syncobj_signal_pt { > - struct dma_fence_array *fence_array; > - u64 value; > - struct list_head list; > -}; > > /** > * drm_syncobj_find - lookup and reference a sync object. > @@ -121,8 +113,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, > { > int ret; > > - ret = drm_syncobj_search_fence(syncobj, 0, 0, fence); > - if (!ret) > + *fence = drm_syncobj_fence_get(syncobj); > + if (*fence) > return 1; > > spin_lock(&syncobj->lock); > @@ -130,12 +122,10 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, > * have the lock, try one more time just to be sure we don't add a > * callback when a fence has already been set. > */ > - if (!list_empty(&syncobj->signal_pt_list)) { > - spin_unlock(&syncobj->lock); > - drm_syncobj_search_fence(syncobj, 0, 0, fence); > - if (*fence) > - return 1; > - spin_lock(&syncobj->lock); > + if (syncobj->fence) { > + *fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, > + lockdep_is_held(&syncobj->lock))); > + ret = 1; > } else { > *fence = NULL; > drm_syncobj_add_callback_locked(syncobj, cb, func); > @@ -163,160 +153,6 @@ void drm_syncobj_remove_callback(struct drm_syncobj *syncobj, > spin_unlock(&syncobj->lock); > } > > -static void drm_syncobj_init(struct drm_syncobj *syncobj) > -{ > - spin_lock(&syncobj->lock); > - syncobj->timeline_context = dma_fence_context_alloc(1); > - syncobj->timeline = 0; > - syncobj->signal_point = 0; > - init_waitqueue_head(&syncobj->wq); > - > - INIT_LIST_HEAD(&syncobj->signal_pt_list); > - spin_unlock(&syncobj->lock); > -} > - > -static void drm_syncobj_fini(struct drm_syncobj *syncobj) > -{ > - struct drm_syncobj_signal_pt *signal_pt = NULL, *tmp; > - > - spin_lock(&syncobj->lock); > - list_for_each_entry_safe(signal_pt, tmp, > - &syncobj->signal_pt_list, list) { > - list_del(&signal_pt->list); > - dma_fence_put(&signal_pt->fence_array->base); > - kfree(signal_pt); > - } > - spin_unlock(&syncobj->lock); > -} > - > -static struct dma_fence > -*drm_syncobj_find_signal_pt_for_point(struct drm_syncobj *syncobj, > - uint64_t point) > -{ > - struct drm_syncobj_signal_pt *signal_pt; > - > - if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) && > - (point <= syncobj->timeline)) { > - struct drm_syncobj_stub_fence *fence = > - kzalloc(sizeof(struct drm_syncobj_stub_fence), > - GFP_KERNEL); > - > - if (!fence) > - return NULL; > - spin_lock_init(&fence->lock); > - dma_fence_init(&fence->base, > - &drm_syncobj_stub_fence_ops, > - &fence->lock, > - syncobj->timeline_context, > - point); > - > - dma_fence_signal(&fence->base); > - return &fence->base; > - } > - > - list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) { > - if (point > signal_pt->value) > - continue; > - if ((syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) && > - (point != signal_pt->value)) > - continue; > - return dma_fence_get(&signal_pt->fence_array->base); > - } > - return NULL; > -} > - > -static int drm_syncobj_create_signal_pt(struct drm_syncobj *syncobj, > - struct dma_fence *fence, > - u64 point) > -{ > - struct drm_syncobj_signal_pt *signal_pt = > - kzalloc(sizeof(struct drm_syncobj_signal_pt), GFP_KERNEL); > - struct drm_syncobj_signal_pt *tail_pt; > - struct dma_fence **fences; > - int num_fences = 0; > - int ret = 0, i; > - > - if (!signal_pt) > - return -ENOMEM; > - if (!fence) > - goto out; > - > - fences = kmalloc_array(sizeof(void *), 2, GFP_KERNEL); > - if (!fences) { > - ret = -ENOMEM; > - goto out; > - } > - fences[num_fences++] = dma_fence_get(fence); > - /* timeline syncobj must take this dependency */ > - if (syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) { > - spin_lock(&syncobj->lock); > - if (!list_empty(&syncobj->signal_pt_list)) { > - tail_pt = list_last_entry(&syncobj->signal_pt_list, > - struct drm_syncobj_signal_pt, list); > - fences[num_fences++] = > - dma_fence_get(&tail_pt->fence_array->base); > - } > - spin_unlock(&syncobj->lock); > - } > - signal_pt->fence_array = dma_fence_array_create(num_fences, fences, > - syncobj->timeline_context, > - point, false); > - if (!signal_pt->fence_array) { > - ret = -ENOMEM; > - goto fail; > - } > - > - spin_lock(&syncobj->lock); > - if (syncobj->signal_point >= point) { > - DRM_WARN("A later signal is ready!"); > - spin_unlock(&syncobj->lock); > - goto exist; > - } > - signal_pt->value = point; > - list_add_tail(&signal_pt->list, &syncobj->signal_pt_list); > - syncobj->signal_point = point; > - spin_unlock(&syncobj->lock); > - wake_up_all(&syncobj->wq); > - > - return 0; > -exist: > - dma_fence_put(&signal_pt->fence_array->base); > -fail: > - for (i = 0; i < num_fences; i++) > - dma_fence_put(fences[i]); > - kfree(fences); > -out: > - kfree(signal_pt); > - return ret; > -} > - > -static void drm_syncobj_garbage_collection(struct drm_syncobj *syncobj) > -{ > - struct drm_syncobj_signal_pt *signal_pt, *tmp, *tail_pt; > - > - spin_lock(&syncobj->lock); > - tail_pt = list_last_entry(&syncobj->signal_pt_list, > - struct drm_syncobj_signal_pt, > - list); > - list_for_each_entry_safe(signal_pt, tmp, > - &syncobj->signal_pt_list, list) { > - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY && > - signal_pt == tail_pt) > - continue; > - if (dma_fence_is_signaled(&signal_pt->fence_array->base)) { > - syncobj->timeline = signal_pt->value; > - list_del(&signal_pt->list); > - dma_fence_put(&signal_pt->fence_array->base); > - kfree(signal_pt); > - } else { > - /*signal_pt is in order in list, from small to big, so > - * the later must not be signal either */ > - break; > - } > - } > - > - spin_unlock(&syncobj->lock); > -} > /** > * drm_syncobj_replace_fence - replace fence in a sync object. > * @syncobj: Sync object to replace fence in > @@ -329,29 +165,28 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, > u64 point, > struct dma_fence *fence) > { > - u64 pt_value = point; > - > - drm_syncobj_garbage_collection(syncobj); > - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) { > - if (!fence) { > - drm_syncobj_fini(syncobj); > - drm_syncobj_init(syncobj); > - return; > - } > - pt_value = syncobj->signal_point + > - DRM_SYNCOBJ_BINARY_POINT; > - } > - drm_syncobj_create_signal_pt(syncobj, fence, pt_value); > - if (fence) { > - struct drm_syncobj_cb *cur, *tmp; > + struct dma_fence *old_fence; > + struct drm_syncobj_cb *cur, *tmp; > + > + if (fence) > + dma_fence_get(fence); > > - spin_lock(&syncobj->lock); > + spin_lock(&syncobj->lock); > + > + old_fence = rcu_dereference_protected(syncobj->fence, > + lockdep_is_held(&syncobj->lock)); > + rcu_assign_pointer(syncobj->fence, fence); > + > + if (fence != old_fence) { > list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) { > list_del_init(&cur->node); > cur->func(syncobj, cur); > } > - spin_unlock(&syncobj->lock); > } > + > + spin_unlock(&syncobj->lock); > + > + dma_fence_put(old_fence); > } > EXPORT_SYMBOL(drm_syncobj_replace_fence); > > @@ -374,64 +209,6 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) > return 0; > } > > -static int > -drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags, > - struct dma_fence **fence) > -{ > - int ret = 0; > - > - if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { > - ret = wait_event_interruptible(syncobj->wq, > - point <= syncobj->signal_point); > - if (ret < 0) > - return ret; > - } > - spin_lock(&syncobj->lock); > - *fence = drm_syncobj_find_signal_pt_for_point(syncobj, point); > - if (!*fence) > - ret = -EINVAL; > - spin_unlock(&syncobj->lock); > - return ret; > -} > - > -/** > - * drm_syncobj_search_fence - lookup and reference the fence in a sync object or > - * in a timeline point > - * @syncobj: sync object pointer > - * @point: timeline point > - * @flags: DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT or not > - * @fence: out parameter for the fence > - * > - * if flags is DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT, the function will block > - * here until specific timeline points is reached. > - * if not, you need a submit thread and block in userspace until all future > - * timeline points have materialized, only then you can submit to the kernel, > - * otherwise, function will fail to return 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 > - * dma_fence_put(). > - */ > -int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point, > - u64 flags, struct dma_fence **fence) > -{ > - u64 pt_value = point; > - > - if (!syncobj) > - return -ENOENT; > - > - drm_syncobj_garbage_collection(syncobj); > - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) { > - /*BINARY syncobj always wait on last pt */ > - pt_value = syncobj->signal_point; > - > - if (pt_value == 0) > - pt_value += DRM_SYNCOBJ_BINARY_POINT; > - } > - return drm_syncobj_point_get(syncobj, pt_value, flags, fence); > -} > -EXPORT_SYMBOL(drm_syncobj_search_fence); > - > /** > * drm_syncobj_find_fence - lookup and reference the fence in a sync object > * @file_private: drm file private pointer > @@ -441,7 +218,7 @@ EXPORT_SYMBOL(drm_syncobj_search_fence); > * @fence: out parameter for the fence > * > * This is just a convenience function that combines drm_syncobj_find() and > - * drm_syncobj_lookup_fence(). > + * drm_syncobj_fence_get(). > * > * 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 > @@ -452,9 +229,15 @@ int drm_syncobj_find_fence(struct drm_file *file_private, > struct dma_fence **fence) > { > struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle); > - int ret; > + int ret = 0; > > - ret = drm_syncobj_search_fence(syncobj, point, flags, fence); > + if (!syncobj) > + return -ENOENT; > + > + *fence = drm_syncobj_fence_get(syncobj); > + if (!*fence) { > + ret = -EINVAL; > + } > drm_syncobj_put(syncobj); > return ret; > } > @@ -471,7 +254,7 @@ void drm_syncobj_free(struct kref *kref) > struct drm_syncobj *syncobj = container_of(kref, > struct drm_syncobj, > refcount); > - drm_syncobj_fini(syncobj); > + drm_syncobj_replace_fence(syncobj, 0, NULL); > kfree(syncobj); > } > EXPORT_SYMBOL(drm_syncobj_free); > @@ -501,11 +284,6 @@ 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_BINARY; > - drm_syncobj_init(syncobj); > > if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { > ret = drm_syncobj_assign_null_handle(syncobj); > @@ -788,8 +566,7 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data, > return -EOPNOTSUPP; > > /* no valid flags yet */ > - if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED | > - DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)) > + if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED) > return -EINVAL; > > return drm_syncobj_create_as_handle(file_private, > @@ -882,8 +659,9 @@ 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); > - > + /* This happens inside the syncobj lock */ > + wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, > + lockdep_is_held(&syncobj->lock))); > wake_up_process(wait->task); > } > > @@ -909,8 +687,7 @@ 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; > - drm_syncobj_search_fence(syncobjs[i], 0, 0, > - &entries[i].fence); > + entries[i].fence = drm_syncobj_fence_get(syncobjs[i]); > if (!entries[i].fence) { > if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { > continue; > @@ -1172,13 +949,12 @@ 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_fini(syncobjs[i]); > - drm_syncobj_init(syncobjs[i]); > - } > + for (i = 0; i < args->count_handles; i++) > + drm_syncobj_replace_fence(syncobjs[i], 0, NULL); > + > drm_syncobj_array_free(syncobjs, args->count_handles); > > - return ret; > + return 0; > } > > int > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 6d99651c6c4b..22b4cb775576 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -2152,7 +2152,7 @@ await_fence_array(struct i915_execbuffer *eb, > if (!(flags & I915_EXEC_FENCE_WAIT)) > continue; > > - drm_syncobj_search_fence(syncobj, 0, 0, &fence); > + fence = drm_syncobj_fence_get(syncobj); > if (!fence) > return -EINVAL; > > diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h > index 5e8c5c027e09..2eda44def639 100644 > --- a/include/drm/drm_syncobj.h > +++ b/include/drm/drm_syncobj.h > @@ -30,15 +30,10 @@ > > struct drm_syncobj_cb; > > -enum drm_syncobj_type { > - DRM_SYNCOBJ_TYPE_BINARY, > - DRM_SYNCOBJ_TYPE_TIMELINE > -}; > - > /** > * struct drm_syncobj - sync object. > * > - * This structure defines a generic sync object which is timeline based. > + * This structure defines a generic sync object which wraps a &dma_fence. > */ > struct drm_syncobj { > /** > @@ -46,36 +41,19 @@ struct drm_syncobj { > */ > struct kref refcount; > /** > - * @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 > + * @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. > */ > - u64 timeline_context; > + struct dma_fence __rcu *fence; > /** > - * @timeline: syncobj timeline value, which indicates point is signaled. > + * @cb_list: List of callbacks to call when the &fence gets replaced. > */ > - 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 syncobj list and write-locks &fence. > + * @lock: Protects &cb_list and write-locks &fence. > */ > spinlock_t lock; > /** > @@ -90,7 +68,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 > @@ -128,6 +106,29 @@ 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, > @@ -141,7 +142,5 @@ 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_search_fence(struct drm_syncobj *syncobj, u64 point, u64 flags, > - struct dma_fence **fence); > > #endif > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index cebdb2541eb7..300f336633f2 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -717,7 +717,6 @@ 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; > }; > > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Oct 19, 2018 at 11:29:49AM +0200, Daniel Vetter wrote: > On Fri, Oct 19, 2018 at 11:27:11AM +0200, Christian König wrote: > > From: Christian König <easy2remember.chk@googlemail.com> > > > > Still contains some bugs. > > > > This reverts commit 48197bc564c7a1888c86024a1ba4f956e0ec2300. > > Thanks for the super-quick handling, much appreciated. > > > > > Bugs: https://bugs.freedesktop.org/show_bug.cgi?id=108490 > > Signed-off-by: Christian König <Christian.Koenig@amd.com> > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Just quick question: Why did we not push the revert right away, but instead spend a few more days letting CI crash all over for a real fix? Seems a bit backwards to me ... -Daniel > > Cheers, Daniel > > > --- > > drivers/gpu/drm/drm_syncobj.c | 306 +++------------------ > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- > > include/drm/drm_syncobj.h | 67 +++-- > > include/uapi/drm/drm.h | 1 - > > 4 files changed, 75 insertions(+), 301 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > > index 57bf6006394d..e2c5b3ca4824 100644 > > --- a/drivers/gpu/drm/drm_syncobj.c > > +++ b/drivers/gpu/drm/drm_syncobj.c > > @@ -56,9 +56,6 @@ > > #include "drm_internal.h" > > #include <drm/drm_syncobj.h> > > > > -/* merge normal syncobj to timeline syncobj, the point interval is 1 */ > > -#define DRM_SYNCOBJ_BINARY_POINT 1 > > - > > struct drm_syncobj_stub_fence { > > struct dma_fence base; > > spinlock_t lock; > > @@ -74,11 +71,6 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops = { > > .get_timeline_name = drm_syncobj_stub_fence_get_name, > > }; > > > > -struct drm_syncobj_signal_pt { > > - struct dma_fence_array *fence_array; > > - u64 value; > > - struct list_head list; > > -}; > > > > /** > > * drm_syncobj_find - lookup and reference a sync object. > > @@ -121,8 +113,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, > > { > > int ret; > > > > - ret = drm_syncobj_search_fence(syncobj, 0, 0, fence); > > - if (!ret) > > + *fence = drm_syncobj_fence_get(syncobj); > > + if (*fence) > > return 1; > > > > spin_lock(&syncobj->lock); > > @@ -130,12 +122,10 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, > > * have the lock, try one more time just to be sure we don't add a > > * callback when a fence has already been set. > > */ > > - if (!list_empty(&syncobj->signal_pt_list)) { > > - spin_unlock(&syncobj->lock); > > - drm_syncobj_search_fence(syncobj, 0, 0, fence); > > - if (*fence) > > - return 1; > > - spin_lock(&syncobj->lock); > > + if (syncobj->fence) { > > + *fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, > > + lockdep_is_held(&syncobj->lock))); > > + ret = 1; > > } else { > > *fence = NULL; > > drm_syncobj_add_callback_locked(syncobj, cb, func); > > @@ -163,160 +153,6 @@ void drm_syncobj_remove_callback(struct drm_syncobj *syncobj, > > spin_unlock(&syncobj->lock); > > } > > > > -static void drm_syncobj_init(struct drm_syncobj *syncobj) > > -{ > > - spin_lock(&syncobj->lock); > > - syncobj->timeline_context = dma_fence_context_alloc(1); > > - syncobj->timeline = 0; > > - syncobj->signal_point = 0; > > - init_waitqueue_head(&syncobj->wq); > > - > > - INIT_LIST_HEAD(&syncobj->signal_pt_list); > > - spin_unlock(&syncobj->lock); > > -} > > - > > -static void drm_syncobj_fini(struct drm_syncobj *syncobj) > > -{ > > - struct drm_syncobj_signal_pt *signal_pt = NULL, *tmp; > > - > > - spin_lock(&syncobj->lock); > > - list_for_each_entry_safe(signal_pt, tmp, > > - &syncobj->signal_pt_list, list) { > > - list_del(&signal_pt->list); > > - dma_fence_put(&signal_pt->fence_array->base); > > - kfree(signal_pt); > > - } > > - spin_unlock(&syncobj->lock); > > -} > > - > > -static struct dma_fence > > -*drm_syncobj_find_signal_pt_for_point(struct drm_syncobj *syncobj, > > - uint64_t point) > > -{ > > - struct drm_syncobj_signal_pt *signal_pt; > > - > > - if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) && > > - (point <= syncobj->timeline)) { > > - struct drm_syncobj_stub_fence *fence = > > - kzalloc(sizeof(struct drm_syncobj_stub_fence), > > - GFP_KERNEL); > > - > > - if (!fence) > > - return NULL; > > - spin_lock_init(&fence->lock); > > - dma_fence_init(&fence->base, > > - &drm_syncobj_stub_fence_ops, > > - &fence->lock, > > - syncobj->timeline_context, > > - point); > > - > > - dma_fence_signal(&fence->base); > > - return &fence->base; > > - } > > - > > - list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) { > > - if (point > signal_pt->value) > > - continue; > > - if ((syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) && > > - (point != signal_pt->value)) > > - continue; > > - return dma_fence_get(&signal_pt->fence_array->base); > > - } > > - return NULL; > > -} > > - > > -static int drm_syncobj_create_signal_pt(struct drm_syncobj *syncobj, > > - struct dma_fence *fence, > > - u64 point) > > -{ > > - struct drm_syncobj_signal_pt *signal_pt = > > - kzalloc(sizeof(struct drm_syncobj_signal_pt), GFP_KERNEL); > > - struct drm_syncobj_signal_pt *tail_pt; > > - struct dma_fence **fences; > > - int num_fences = 0; > > - int ret = 0, i; > > - > > - if (!signal_pt) > > - return -ENOMEM; > > - if (!fence) > > - goto out; > > - > > - fences = kmalloc_array(sizeof(void *), 2, GFP_KERNEL); > > - if (!fences) { > > - ret = -ENOMEM; > > - goto out; > > - } > > - fences[num_fences++] = dma_fence_get(fence); > > - /* timeline syncobj must take this dependency */ > > - if (syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) { > > - spin_lock(&syncobj->lock); > > - if (!list_empty(&syncobj->signal_pt_list)) { > > - tail_pt = list_last_entry(&syncobj->signal_pt_list, > > - struct drm_syncobj_signal_pt, list); > > - fences[num_fences++] = > > - dma_fence_get(&tail_pt->fence_array->base); > > - } > > - spin_unlock(&syncobj->lock); > > - } > > - signal_pt->fence_array = dma_fence_array_create(num_fences, fences, > > - syncobj->timeline_context, > > - point, false); > > - if (!signal_pt->fence_array) { > > - ret = -ENOMEM; > > - goto fail; > > - } > > - > > - spin_lock(&syncobj->lock); > > - if (syncobj->signal_point >= point) { > > - DRM_WARN("A later signal is ready!"); > > - spin_unlock(&syncobj->lock); > > - goto exist; > > - } > > - signal_pt->value = point; > > - list_add_tail(&signal_pt->list, &syncobj->signal_pt_list); > > - syncobj->signal_point = point; > > - spin_unlock(&syncobj->lock); > > - wake_up_all(&syncobj->wq); > > - > > - return 0; > > -exist: > > - dma_fence_put(&signal_pt->fence_array->base); > > -fail: > > - for (i = 0; i < num_fences; i++) > > - dma_fence_put(fences[i]); > > - kfree(fences); > > -out: > > - kfree(signal_pt); > > - return ret; > > -} > > - > > -static void drm_syncobj_garbage_collection(struct drm_syncobj *syncobj) > > -{ > > - struct drm_syncobj_signal_pt *signal_pt, *tmp, *tail_pt; > > - > > - spin_lock(&syncobj->lock); > > - tail_pt = list_last_entry(&syncobj->signal_pt_list, > > - struct drm_syncobj_signal_pt, > > - list); > > - list_for_each_entry_safe(signal_pt, tmp, > > - &syncobj->signal_pt_list, list) { > > - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY && > > - signal_pt == tail_pt) > > - continue; > > - if (dma_fence_is_signaled(&signal_pt->fence_array->base)) { > > - syncobj->timeline = signal_pt->value; > > - list_del(&signal_pt->list); > > - dma_fence_put(&signal_pt->fence_array->base); > > - kfree(signal_pt); > > - } else { > > - /*signal_pt is in order in list, from small to big, so > > - * the later must not be signal either */ > > - break; > > - } > > - } > > - > > - spin_unlock(&syncobj->lock); > > -} > > /** > > * drm_syncobj_replace_fence - replace fence in a sync object. > > * @syncobj: Sync object to replace fence in > > @@ -329,29 +165,28 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, > > u64 point, > > struct dma_fence *fence) > > { > > - u64 pt_value = point; > > - > > - drm_syncobj_garbage_collection(syncobj); > > - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) { > > - if (!fence) { > > - drm_syncobj_fini(syncobj); > > - drm_syncobj_init(syncobj); > > - return; > > - } > > - pt_value = syncobj->signal_point + > > - DRM_SYNCOBJ_BINARY_POINT; > > - } > > - drm_syncobj_create_signal_pt(syncobj, fence, pt_value); > > - if (fence) { > > - struct drm_syncobj_cb *cur, *tmp; > > + struct dma_fence *old_fence; > > + struct drm_syncobj_cb *cur, *tmp; > > + > > + if (fence) > > + dma_fence_get(fence); > > > > - spin_lock(&syncobj->lock); > > + spin_lock(&syncobj->lock); > > + > > + old_fence = rcu_dereference_protected(syncobj->fence, > > + lockdep_is_held(&syncobj->lock)); > > + rcu_assign_pointer(syncobj->fence, fence); > > + > > + if (fence != old_fence) { > > list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) { > > list_del_init(&cur->node); > > cur->func(syncobj, cur); > > } > > - spin_unlock(&syncobj->lock); > > } > > + > > + spin_unlock(&syncobj->lock); > > + > > + dma_fence_put(old_fence); > > } > > EXPORT_SYMBOL(drm_syncobj_replace_fence); > > > > @@ -374,64 +209,6 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) > > return 0; > > } > > > > -static int > > -drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags, > > - struct dma_fence **fence) > > -{ > > - int ret = 0; > > - > > - if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { > > - ret = wait_event_interruptible(syncobj->wq, > > - point <= syncobj->signal_point); > > - if (ret < 0) > > - return ret; > > - } > > - spin_lock(&syncobj->lock); > > - *fence = drm_syncobj_find_signal_pt_for_point(syncobj, point); > > - if (!*fence) > > - ret = -EINVAL; > > - spin_unlock(&syncobj->lock); > > - return ret; > > -} > > - > > -/** > > - * drm_syncobj_search_fence - lookup and reference the fence in a sync object or > > - * in a timeline point > > - * @syncobj: sync object pointer > > - * @point: timeline point > > - * @flags: DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT or not > > - * @fence: out parameter for the fence > > - * > > - * if flags is DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT, the function will block > > - * here until specific timeline points is reached. > > - * if not, you need a submit thread and block in userspace until all future > > - * timeline points have materialized, only then you can submit to the kernel, > > - * otherwise, function will fail to return 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 > > - * dma_fence_put(). > > - */ > > -int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point, > > - u64 flags, struct dma_fence **fence) > > -{ > > - u64 pt_value = point; > > - > > - if (!syncobj) > > - return -ENOENT; > > - > > - drm_syncobj_garbage_collection(syncobj); > > - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) { > > - /*BINARY syncobj always wait on last pt */ > > - pt_value = syncobj->signal_point; > > - > > - if (pt_value == 0) > > - pt_value += DRM_SYNCOBJ_BINARY_POINT; > > - } > > - return drm_syncobj_point_get(syncobj, pt_value, flags, fence); > > -} > > -EXPORT_SYMBOL(drm_syncobj_search_fence); > > - > > /** > > * drm_syncobj_find_fence - lookup and reference the fence in a sync object > > * @file_private: drm file private pointer > > @@ -441,7 +218,7 @@ EXPORT_SYMBOL(drm_syncobj_search_fence); > > * @fence: out parameter for the fence > > * > > * This is just a convenience function that combines drm_syncobj_find() and > > - * drm_syncobj_lookup_fence(). > > + * drm_syncobj_fence_get(). > > * > > * 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 > > @@ -452,9 +229,15 @@ int drm_syncobj_find_fence(struct drm_file *file_private, > > struct dma_fence **fence) > > { > > struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle); > > - int ret; > > + int ret = 0; > > > > - ret = drm_syncobj_search_fence(syncobj, point, flags, fence); > > + if (!syncobj) > > + return -ENOENT; > > + > > + *fence = drm_syncobj_fence_get(syncobj); > > + if (!*fence) { > > + ret = -EINVAL; > > + } > > drm_syncobj_put(syncobj); > > return ret; > > } > > @@ -471,7 +254,7 @@ void drm_syncobj_free(struct kref *kref) > > struct drm_syncobj *syncobj = container_of(kref, > > struct drm_syncobj, > > refcount); > > - drm_syncobj_fini(syncobj); > > + drm_syncobj_replace_fence(syncobj, 0, NULL); > > kfree(syncobj); > > } > > EXPORT_SYMBOL(drm_syncobj_free); > > @@ -501,11 +284,6 @@ 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_BINARY; > > - drm_syncobj_init(syncobj); > > > > if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { > > ret = drm_syncobj_assign_null_handle(syncobj); > > @@ -788,8 +566,7 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data, > > return -EOPNOTSUPP; > > > > /* no valid flags yet */ > > - if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED | > > - DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)) > > + if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED) > > return -EINVAL; > > > > return drm_syncobj_create_as_handle(file_private, > > @@ -882,8 +659,9 @@ 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); > > - > > + /* This happens inside the syncobj lock */ > > + wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, > > + lockdep_is_held(&syncobj->lock))); > > wake_up_process(wait->task); > > } > > > > @@ -909,8 +687,7 @@ 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; > > - drm_syncobj_search_fence(syncobjs[i], 0, 0, > > - &entries[i].fence); > > + entries[i].fence = drm_syncobj_fence_get(syncobjs[i]); > > if (!entries[i].fence) { > > if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { > > continue; > > @@ -1172,13 +949,12 @@ 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_fini(syncobjs[i]); > > - drm_syncobj_init(syncobjs[i]); > > - } > > + for (i = 0; i < args->count_handles; i++) > > + drm_syncobj_replace_fence(syncobjs[i], 0, NULL); > > + > > drm_syncobj_array_free(syncobjs, args->count_handles); > > > > - return ret; > > + return 0; > > } > > > > int > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index 6d99651c6c4b..22b4cb775576 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -2152,7 +2152,7 @@ await_fence_array(struct i915_execbuffer *eb, > > if (!(flags & I915_EXEC_FENCE_WAIT)) > > continue; > > > > - drm_syncobj_search_fence(syncobj, 0, 0, &fence); > > + fence = drm_syncobj_fence_get(syncobj); > > if (!fence) > > return -EINVAL; > > > > diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h > > index 5e8c5c027e09..2eda44def639 100644 > > --- a/include/drm/drm_syncobj.h > > +++ b/include/drm/drm_syncobj.h > > @@ -30,15 +30,10 @@ > > > > struct drm_syncobj_cb; > > > > -enum drm_syncobj_type { > > - DRM_SYNCOBJ_TYPE_BINARY, > > - DRM_SYNCOBJ_TYPE_TIMELINE > > -}; > > - > > /** > > * struct drm_syncobj - sync object. > > * > > - * This structure defines a generic sync object which is timeline based. > > + * This structure defines a generic sync object which wraps a &dma_fence. > > */ > > struct drm_syncobj { > > /** > > @@ -46,36 +41,19 @@ struct drm_syncobj { > > */ > > struct kref refcount; > > /** > > - * @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 > > + * @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. > > */ > > - u64 timeline_context; > > + struct dma_fence __rcu *fence; > > /** > > - * @timeline: syncobj timeline value, which indicates point is signaled. > > + * @cb_list: List of callbacks to call when the &fence gets replaced. > > */ > > - 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 syncobj list and write-locks &fence. > > + * @lock: Protects &cb_list and write-locks &fence. > > */ > > spinlock_t lock; > > /** > > @@ -90,7 +68,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 > > @@ -128,6 +106,29 @@ 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, > > @@ -141,7 +142,5 @@ 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_search_fence(struct drm_syncobj *syncobj, u64 point, u64 flags, > > - struct dma_fence **fence); > > > > #endif > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > index cebdb2541eb7..300f336633f2 100644 > > --- a/include/uapi/drm/drm.h > > +++ b/include/uapi/drm/drm.h > > @@ -717,7 +717,6 @@ 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; > > }; > > > > -- > > 2.17.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Am 07.11.18 um 21:33 schrieb Daniel Vetter: > On Fri, Oct 19, 2018 at 11:29:49AM +0200, Daniel Vetter wrote: >> On Fri, Oct 19, 2018 at 11:27:11AM +0200, Christian König wrote: >>> From: Christian König <easy2remember.chk@googlemail.com> >>> >>> Still contains some bugs. >>> >>> This reverts commit 48197bc564c7a1888c86024a1ba4f956e0ec2300. >> Thanks for the super-quick handling, much appreciated. >> >>> Bugs: https://bugs.freedesktop.org/show_bug.cgi?id=108490 >>> Signed-off-by: Christian König <Christian.Koenig@amd.com> >> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Just quick question: Why did we not push the revert right away, but > instead spend a few more days letting CI crash all over for a real fix? > Seems a bit backwards to me ... Well to find more bugs. I mean that's what we have all this unit testing for. It turned out to be the right decision because it not only pointed out some more problems in the patch itself, but also not handled corner cases in drivers as well as a problem in dma-fence-array. Except for one false positive warning from lockdep all of those should be fixed by now and Chunming is already working on that last item. BTW: I've intentionally holding back most of the UAPI because we haven't got any anv/radv reviews for that. The only UAPI currently exposed is the DRM_SYNCOBJ_CREATE_TYPE_TIMELINE flag. Christian. > -Daniel > >> Cheers, Daniel >> >>> --- >>> drivers/gpu/drm/drm_syncobj.c | 306 +++------------------ >>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- >>> include/drm/drm_syncobj.h | 67 +++-- >>> include/uapi/drm/drm.h | 1 - >>> 4 files changed, 75 insertions(+), 301 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c >>> index 57bf6006394d..e2c5b3ca4824 100644 >>> --- a/drivers/gpu/drm/drm_syncobj.c >>> +++ b/drivers/gpu/drm/drm_syncobj.c >>> @@ -56,9 +56,6 @@ >>> #include "drm_internal.h" >>> #include <drm/drm_syncobj.h> >>> >>> -/* merge normal syncobj to timeline syncobj, the point interval is 1 */ >>> -#define DRM_SYNCOBJ_BINARY_POINT 1 >>> - >>> struct drm_syncobj_stub_fence { >>> struct dma_fence base; >>> spinlock_t lock; >>> @@ -74,11 +71,6 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops = { >>> .get_timeline_name = drm_syncobj_stub_fence_get_name, >>> }; >>> >>> -struct drm_syncobj_signal_pt { >>> - struct dma_fence_array *fence_array; >>> - u64 value; >>> - struct list_head list; >>> -}; >>> >>> /** >>> * drm_syncobj_find - lookup and reference a sync object. >>> @@ -121,8 +113,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, >>> { >>> int ret; >>> >>> - ret = drm_syncobj_search_fence(syncobj, 0, 0, fence); >>> - if (!ret) >>> + *fence = drm_syncobj_fence_get(syncobj); >>> + if (*fence) >>> return 1; >>> >>> spin_lock(&syncobj->lock); >>> @@ -130,12 +122,10 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, >>> * have the lock, try one more time just to be sure we don't add a >>> * callback when a fence has already been set. >>> */ >>> - if (!list_empty(&syncobj->signal_pt_list)) { >>> - spin_unlock(&syncobj->lock); >>> - drm_syncobj_search_fence(syncobj, 0, 0, fence); >>> - if (*fence) >>> - return 1; >>> - spin_lock(&syncobj->lock); >>> + if (syncobj->fence) { >>> + *fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, >>> + lockdep_is_held(&syncobj->lock))); >>> + ret = 1; >>> } else { >>> *fence = NULL; >>> drm_syncobj_add_callback_locked(syncobj, cb, func); >>> @@ -163,160 +153,6 @@ void drm_syncobj_remove_callback(struct drm_syncobj *syncobj, >>> spin_unlock(&syncobj->lock); >>> } >>> >>> -static void drm_syncobj_init(struct drm_syncobj *syncobj) >>> -{ >>> - spin_lock(&syncobj->lock); >>> - syncobj->timeline_context = dma_fence_context_alloc(1); >>> - syncobj->timeline = 0; >>> - syncobj->signal_point = 0; >>> - init_waitqueue_head(&syncobj->wq); >>> - >>> - INIT_LIST_HEAD(&syncobj->signal_pt_list); >>> - spin_unlock(&syncobj->lock); >>> -} >>> - >>> -static void drm_syncobj_fini(struct drm_syncobj *syncobj) >>> -{ >>> - struct drm_syncobj_signal_pt *signal_pt = NULL, *tmp; >>> - >>> - spin_lock(&syncobj->lock); >>> - list_for_each_entry_safe(signal_pt, tmp, >>> - &syncobj->signal_pt_list, list) { >>> - list_del(&signal_pt->list); >>> - dma_fence_put(&signal_pt->fence_array->base); >>> - kfree(signal_pt); >>> - } >>> - spin_unlock(&syncobj->lock); >>> -} >>> - >>> -static struct dma_fence >>> -*drm_syncobj_find_signal_pt_for_point(struct drm_syncobj *syncobj, >>> - uint64_t point) >>> -{ >>> - struct drm_syncobj_signal_pt *signal_pt; >>> - >>> - if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) && >>> - (point <= syncobj->timeline)) { >>> - struct drm_syncobj_stub_fence *fence = >>> - kzalloc(sizeof(struct drm_syncobj_stub_fence), >>> - GFP_KERNEL); >>> - >>> - if (!fence) >>> - return NULL; >>> - spin_lock_init(&fence->lock); >>> - dma_fence_init(&fence->base, >>> - &drm_syncobj_stub_fence_ops, >>> - &fence->lock, >>> - syncobj->timeline_context, >>> - point); >>> - >>> - dma_fence_signal(&fence->base); >>> - return &fence->base; >>> - } >>> - >>> - list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) { >>> - if (point > signal_pt->value) >>> - continue; >>> - if ((syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) && >>> - (point != signal_pt->value)) >>> - continue; >>> - return dma_fence_get(&signal_pt->fence_array->base); >>> - } >>> - return NULL; >>> -} >>> - >>> -static int drm_syncobj_create_signal_pt(struct drm_syncobj *syncobj, >>> - struct dma_fence *fence, >>> - u64 point) >>> -{ >>> - struct drm_syncobj_signal_pt *signal_pt = >>> - kzalloc(sizeof(struct drm_syncobj_signal_pt), GFP_KERNEL); >>> - struct drm_syncobj_signal_pt *tail_pt; >>> - struct dma_fence **fences; >>> - int num_fences = 0; >>> - int ret = 0, i; >>> - >>> - if (!signal_pt) >>> - return -ENOMEM; >>> - if (!fence) >>> - goto out; >>> - >>> - fences = kmalloc_array(sizeof(void *), 2, GFP_KERNEL); >>> - if (!fences) { >>> - ret = -ENOMEM; >>> - goto out; >>> - } >>> - fences[num_fences++] = dma_fence_get(fence); >>> - /* timeline syncobj must take this dependency */ >>> - if (syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) { >>> - spin_lock(&syncobj->lock); >>> - if (!list_empty(&syncobj->signal_pt_list)) { >>> - tail_pt = list_last_entry(&syncobj->signal_pt_list, >>> - struct drm_syncobj_signal_pt, list); >>> - fences[num_fences++] = >>> - dma_fence_get(&tail_pt->fence_array->base); >>> - } >>> - spin_unlock(&syncobj->lock); >>> - } >>> - signal_pt->fence_array = dma_fence_array_create(num_fences, fences, >>> - syncobj->timeline_context, >>> - point, false); >>> - if (!signal_pt->fence_array) { >>> - ret = -ENOMEM; >>> - goto fail; >>> - } >>> - >>> - spin_lock(&syncobj->lock); >>> - if (syncobj->signal_point >= point) { >>> - DRM_WARN("A later signal is ready!"); >>> - spin_unlock(&syncobj->lock); >>> - goto exist; >>> - } >>> - signal_pt->value = point; >>> - list_add_tail(&signal_pt->list, &syncobj->signal_pt_list); >>> - syncobj->signal_point = point; >>> - spin_unlock(&syncobj->lock); >>> - wake_up_all(&syncobj->wq); >>> - >>> - return 0; >>> -exist: >>> - dma_fence_put(&signal_pt->fence_array->base); >>> -fail: >>> - for (i = 0; i < num_fences; i++) >>> - dma_fence_put(fences[i]); >>> - kfree(fences); >>> -out: >>> - kfree(signal_pt); >>> - return ret; >>> -} >>> - >>> -static void drm_syncobj_garbage_collection(struct drm_syncobj *syncobj) >>> -{ >>> - struct drm_syncobj_signal_pt *signal_pt, *tmp, *tail_pt; >>> - >>> - spin_lock(&syncobj->lock); >>> - tail_pt = list_last_entry(&syncobj->signal_pt_list, >>> - struct drm_syncobj_signal_pt, >>> - list); >>> - list_for_each_entry_safe(signal_pt, tmp, >>> - &syncobj->signal_pt_list, list) { >>> - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY && >>> - signal_pt == tail_pt) >>> - continue; >>> - if (dma_fence_is_signaled(&signal_pt->fence_array->base)) { >>> - syncobj->timeline = signal_pt->value; >>> - list_del(&signal_pt->list); >>> - dma_fence_put(&signal_pt->fence_array->base); >>> - kfree(signal_pt); >>> - } else { >>> - /*signal_pt is in order in list, from small to big, so >>> - * the later must not be signal either */ >>> - break; >>> - } >>> - } >>> - >>> - spin_unlock(&syncobj->lock); >>> -} >>> /** >>> * drm_syncobj_replace_fence - replace fence in a sync object. >>> * @syncobj: Sync object to replace fence in >>> @@ -329,29 +165,28 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, >>> u64 point, >>> struct dma_fence *fence) >>> { >>> - u64 pt_value = point; >>> - >>> - drm_syncobj_garbage_collection(syncobj); >>> - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) { >>> - if (!fence) { >>> - drm_syncobj_fini(syncobj); >>> - drm_syncobj_init(syncobj); >>> - return; >>> - } >>> - pt_value = syncobj->signal_point + >>> - DRM_SYNCOBJ_BINARY_POINT; >>> - } >>> - drm_syncobj_create_signal_pt(syncobj, fence, pt_value); >>> - if (fence) { >>> - struct drm_syncobj_cb *cur, *tmp; >>> + struct dma_fence *old_fence; >>> + struct drm_syncobj_cb *cur, *tmp; >>> + >>> + if (fence) >>> + dma_fence_get(fence); >>> >>> - spin_lock(&syncobj->lock); >>> + spin_lock(&syncobj->lock); >>> + >>> + old_fence = rcu_dereference_protected(syncobj->fence, >>> + lockdep_is_held(&syncobj->lock)); >>> + rcu_assign_pointer(syncobj->fence, fence); >>> + >>> + if (fence != old_fence) { >>> list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) { >>> list_del_init(&cur->node); >>> cur->func(syncobj, cur); >>> } >>> - spin_unlock(&syncobj->lock); >>> } >>> + >>> + spin_unlock(&syncobj->lock); >>> + >>> + dma_fence_put(old_fence); >>> } >>> EXPORT_SYMBOL(drm_syncobj_replace_fence); >>> >>> @@ -374,64 +209,6 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) >>> return 0; >>> } >>> >>> -static int >>> -drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags, >>> - struct dma_fence **fence) >>> -{ >>> - int ret = 0; >>> - >>> - if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { >>> - ret = wait_event_interruptible(syncobj->wq, >>> - point <= syncobj->signal_point); >>> - if (ret < 0) >>> - return ret; >>> - } >>> - spin_lock(&syncobj->lock); >>> - *fence = drm_syncobj_find_signal_pt_for_point(syncobj, point); >>> - if (!*fence) >>> - ret = -EINVAL; >>> - spin_unlock(&syncobj->lock); >>> - return ret; >>> -} >>> - >>> -/** >>> - * drm_syncobj_search_fence - lookup and reference the fence in a sync object or >>> - * in a timeline point >>> - * @syncobj: sync object pointer >>> - * @point: timeline point >>> - * @flags: DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT or not >>> - * @fence: out parameter for the fence >>> - * >>> - * if flags is DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT, the function will block >>> - * here until specific timeline points is reached. >>> - * if not, you need a submit thread and block in userspace until all future >>> - * timeline points have materialized, only then you can submit to the kernel, >>> - * otherwise, function will fail to return 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 >>> - * dma_fence_put(). >>> - */ >>> -int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point, >>> - u64 flags, struct dma_fence **fence) >>> -{ >>> - u64 pt_value = point; >>> - >>> - if (!syncobj) >>> - return -ENOENT; >>> - >>> - drm_syncobj_garbage_collection(syncobj); >>> - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) { >>> - /*BINARY syncobj always wait on last pt */ >>> - pt_value = syncobj->signal_point; >>> - >>> - if (pt_value == 0) >>> - pt_value += DRM_SYNCOBJ_BINARY_POINT; >>> - } >>> - return drm_syncobj_point_get(syncobj, pt_value, flags, fence); >>> -} >>> -EXPORT_SYMBOL(drm_syncobj_search_fence); >>> - >>> /** >>> * drm_syncobj_find_fence - lookup and reference the fence in a sync object >>> * @file_private: drm file private pointer >>> @@ -441,7 +218,7 @@ EXPORT_SYMBOL(drm_syncobj_search_fence); >>> * @fence: out parameter for the fence >>> * >>> * This is just a convenience function that combines drm_syncobj_find() and >>> - * drm_syncobj_lookup_fence(). >>> + * drm_syncobj_fence_get(). >>> * >>> * 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 >>> @@ -452,9 +229,15 @@ int drm_syncobj_find_fence(struct drm_file *file_private, >>> struct dma_fence **fence) >>> { >>> struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle); >>> - int ret; >>> + int ret = 0; >>> >>> - ret = drm_syncobj_search_fence(syncobj, point, flags, fence); >>> + if (!syncobj) >>> + return -ENOENT; >>> + >>> + *fence = drm_syncobj_fence_get(syncobj); >>> + if (!*fence) { >>> + ret = -EINVAL; >>> + } >>> drm_syncobj_put(syncobj); >>> return ret; >>> } >>> @@ -471,7 +254,7 @@ void drm_syncobj_free(struct kref *kref) >>> struct drm_syncobj *syncobj = container_of(kref, >>> struct drm_syncobj, >>> refcount); >>> - drm_syncobj_fini(syncobj); >>> + drm_syncobj_replace_fence(syncobj, 0, NULL); >>> kfree(syncobj); >>> } >>> EXPORT_SYMBOL(drm_syncobj_free); >>> @@ -501,11 +284,6 @@ 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_BINARY; >>> - drm_syncobj_init(syncobj); >>> >>> if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { >>> ret = drm_syncobj_assign_null_handle(syncobj); >>> @@ -788,8 +566,7 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data, >>> return -EOPNOTSUPP; >>> >>> /* no valid flags yet */ >>> - if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED | >>> - DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)) >>> + if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED) >>> return -EINVAL; >>> >>> return drm_syncobj_create_as_handle(file_private, >>> @@ -882,8 +659,9 @@ 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); >>> - >>> + /* This happens inside the syncobj lock */ >>> + wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, >>> + lockdep_is_held(&syncobj->lock))); >>> wake_up_process(wait->task); >>> } >>> >>> @@ -909,8 +687,7 @@ 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; >>> - drm_syncobj_search_fence(syncobjs[i], 0, 0, >>> - &entries[i].fence); >>> + entries[i].fence = drm_syncobj_fence_get(syncobjs[i]); >>> if (!entries[i].fence) { >>> if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { >>> continue; >>> @@ -1172,13 +949,12 @@ 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_fini(syncobjs[i]); >>> - drm_syncobj_init(syncobjs[i]); >>> - } >>> + for (i = 0; i < args->count_handles; i++) >>> + drm_syncobj_replace_fence(syncobjs[i], 0, NULL); >>> + >>> drm_syncobj_array_free(syncobjs, args->count_handles); >>> >>> - return ret; >>> + return 0; >>> } >>> >>> int >>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >>> index 6d99651c6c4b..22b4cb775576 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >>> @@ -2152,7 +2152,7 @@ await_fence_array(struct i915_execbuffer *eb, >>> if (!(flags & I915_EXEC_FENCE_WAIT)) >>> continue; >>> >>> - drm_syncobj_search_fence(syncobj, 0, 0, &fence); >>> + fence = drm_syncobj_fence_get(syncobj); >>> if (!fence) >>> return -EINVAL; >>> >>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h >>> index 5e8c5c027e09..2eda44def639 100644 >>> --- a/include/drm/drm_syncobj.h >>> +++ b/include/drm/drm_syncobj.h >>> @@ -30,15 +30,10 @@ >>> >>> struct drm_syncobj_cb; >>> >>> -enum drm_syncobj_type { >>> - DRM_SYNCOBJ_TYPE_BINARY, >>> - DRM_SYNCOBJ_TYPE_TIMELINE >>> -}; >>> - >>> /** >>> * struct drm_syncobj - sync object. >>> * >>> - * This structure defines a generic sync object which is timeline based. >>> + * This structure defines a generic sync object which wraps a &dma_fence. >>> */ >>> struct drm_syncobj { >>> /** >>> @@ -46,36 +41,19 @@ struct drm_syncobj { >>> */ >>> struct kref refcount; >>> /** >>> - * @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 >>> + * @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. >>> */ >>> - u64 timeline_context; >>> + struct dma_fence __rcu *fence; >>> /** >>> - * @timeline: syncobj timeline value, which indicates point is signaled. >>> + * @cb_list: List of callbacks to call when the &fence gets replaced. >>> */ >>> - 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 syncobj list and write-locks &fence. >>> + * @lock: Protects &cb_list and write-locks &fence. >>> */ >>> spinlock_t lock; >>> /** >>> @@ -90,7 +68,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 >>> @@ -128,6 +106,29 @@ 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, >>> @@ -141,7 +142,5 @@ 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_search_fence(struct drm_syncobj *syncobj, u64 point, u64 flags, >>> - struct dma_fence **fence); >>> >>> #endif >>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >>> index cebdb2541eb7..300f336633f2 100644 >>> --- a/include/uapi/drm/drm.h >>> +++ b/include/uapi/drm/drm.h >>> @@ -717,7 +717,6 @@ 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; >>> }; >>> >>> -- >>> 2.17.1 >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch
On Thu, Nov 8, 2018 at 9:12 AM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > Am 07.11.18 um 21:33 schrieb Daniel Vetter: > > On Fri, Oct 19, 2018 at 11:29:49AM +0200, Daniel Vetter wrote: > >> On Fri, Oct 19, 2018 at 11:27:11AM +0200, Christian König wrote: > >>> From: Christian König <easy2remember.chk@googlemail.com> > >>> > >>> Still contains some bugs. > >>> > >>> This reverts commit 48197bc564c7a1888c86024a1ba4f956e0ec2300. > >> Thanks for the super-quick handling, much appreciated. > >> > >>> Bugs: https://bugs.freedesktop.org/show_bug.cgi?id=108490 > >>> Signed-off-by: Christian König <Christian.Koenig@amd.com> > >> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Just quick question: Why did we not push the revert right away, but > > instead spend a few more days letting CI crash all over for a real fix? > > Seems a bit backwards to me ... > > Well to find more bugs. I mean that's what we have all this unit testing > for. The testing is so you can see what goes up in flames before you push, not afterwards. And imo when it has blown up in obviuos ways that test would have caught (the igts fully run on amdgpu too, or should at least), then it's better to restart properly. There's a lot more people using the upstream trees than just you&Chunming for developing timeline syncobj. -Daniel > It turned out to be the right decision because it not only pointed out > some more problems in the patch itself, but also not handled corner > cases in drivers as well as a problem in dma-fence-array. > > Except for one false positive warning from lockdep all of those should > be fixed by now and Chunming is already working on that last item. > > BTW: I've intentionally holding back most of the UAPI because we haven't > got any anv/radv reviews for that. The only UAPI currently exposed is > the DRM_SYNCOBJ_CREATE_TYPE_TIMELINE flag. > > Christian. > > > -Daniel > > > >> Cheers, Daniel > >> > >>> --- > >>> drivers/gpu/drm/drm_syncobj.c | 306 +++------------------ > >>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- > >>> include/drm/drm_syncobj.h | 67 +++-- > >>> include/uapi/drm/drm.h | 1 - > >>> 4 files changed, 75 insertions(+), 301 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > >>> index 57bf6006394d..e2c5b3ca4824 100644 > >>> --- a/drivers/gpu/drm/drm_syncobj.c > >>> +++ b/drivers/gpu/drm/drm_syncobj.c > >>> @@ -56,9 +56,6 @@ > >>> #include "drm_internal.h" > >>> #include <drm/drm_syncobj.h> > >>> > >>> -/* merge normal syncobj to timeline syncobj, the point interval is 1 */ > >>> -#define DRM_SYNCOBJ_BINARY_POINT 1 > >>> - > >>> struct drm_syncobj_stub_fence { > >>> struct dma_fence base; > >>> spinlock_t lock; > >>> @@ -74,11 +71,6 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops = { > >>> .get_timeline_name = drm_syncobj_stub_fence_get_name, > >>> }; > >>> > >>> -struct drm_syncobj_signal_pt { > >>> - struct dma_fence_array *fence_array; > >>> - u64 value; > >>> - struct list_head list; > >>> -}; > >>> > >>> /** > >>> * drm_syncobj_find - lookup and reference a sync object. > >>> @@ -121,8 +113,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, > >>> { > >>> int ret; > >>> > >>> - ret = drm_syncobj_search_fence(syncobj, 0, 0, fence); > >>> - if (!ret) > >>> + *fence = drm_syncobj_fence_get(syncobj); > >>> + if (*fence) > >>> return 1; > >>> > >>> spin_lock(&syncobj->lock); > >>> @@ -130,12 +122,10 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, > >>> * have the lock, try one more time just to be sure we don't add a > >>> * callback when a fence has already been set. > >>> */ > >>> - if (!list_empty(&syncobj->signal_pt_list)) { > >>> - spin_unlock(&syncobj->lock); > >>> - drm_syncobj_search_fence(syncobj, 0, 0, fence); > >>> - if (*fence) > >>> - return 1; > >>> - spin_lock(&syncobj->lock); > >>> + if (syncobj->fence) { > >>> + *fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, > >>> + lockdep_is_held(&syncobj->lock))); > >>> + ret = 1; > >>> } else { > >>> *fence = NULL; > >>> drm_syncobj_add_callback_locked(syncobj, cb, func); > >>> @@ -163,160 +153,6 @@ void drm_syncobj_remove_callback(struct drm_syncobj *syncobj, > >>> spin_unlock(&syncobj->lock); > >>> } > >>> > >>> -static void drm_syncobj_init(struct drm_syncobj *syncobj) > >>> -{ > >>> - spin_lock(&syncobj->lock); > >>> - syncobj->timeline_context = dma_fence_context_alloc(1); > >>> - syncobj->timeline = 0; > >>> - syncobj->signal_point = 0; > >>> - init_waitqueue_head(&syncobj->wq); > >>> - > >>> - INIT_LIST_HEAD(&syncobj->signal_pt_list); > >>> - spin_unlock(&syncobj->lock); > >>> -} > >>> - > >>> -static void drm_syncobj_fini(struct drm_syncobj *syncobj) > >>> -{ > >>> - struct drm_syncobj_signal_pt *signal_pt = NULL, *tmp; > >>> - > >>> - spin_lock(&syncobj->lock); > >>> - list_for_each_entry_safe(signal_pt, tmp, > >>> - &syncobj->signal_pt_list, list) { > >>> - list_del(&signal_pt->list); > >>> - dma_fence_put(&signal_pt->fence_array->base); > >>> - kfree(signal_pt); > >>> - } > >>> - spin_unlock(&syncobj->lock); > >>> -} > >>> - > >>> -static struct dma_fence > >>> -*drm_syncobj_find_signal_pt_for_point(struct drm_syncobj *syncobj, > >>> - uint64_t point) > >>> -{ > >>> - struct drm_syncobj_signal_pt *signal_pt; > >>> - > >>> - if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) && > >>> - (point <= syncobj->timeline)) { > >>> - struct drm_syncobj_stub_fence *fence = > >>> - kzalloc(sizeof(struct drm_syncobj_stub_fence), > >>> - GFP_KERNEL); > >>> - > >>> - if (!fence) > >>> - return NULL; > >>> - spin_lock_init(&fence->lock); > >>> - dma_fence_init(&fence->base, > >>> - &drm_syncobj_stub_fence_ops, > >>> - &fence->lock, > >>> - syncobj->timeline_context, > >>> - point); > >>> - > >>> - dma_fence_signal(&fence->base); > >>> - return &fence->base; > >>> - } > >>> - > >>> - list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) { > >>> - if (point > signal_pt->value) > >>> - continue; > >>> - if ((syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) && > >>> - (point != signal_pt->value)) > >>> - continue; > >>> - return dma_fence_get(&signal_pt->fence_array->base); > >>> - } > >>> - return NULL; > >>> -} > >>> - > >>> -static int drm_syncobj_create_signal_pt(struct drm_syncobj *syncobj, > >>> - struct dma_fence *fence, > >>> - u64 point) > >>> -{ > >>> - struct drm_syncobj_signal_pt *signal_pt = > >>> - kzalloc(sizeof(struct drm_syncobj_signal_pt), GFP_KERNEL); > >>> - struct drm_syncobj_signal_pt *tail_pt; > >>> - struct dma_fence **fences; > >>> - int num_fences = 0; > >>> - int ret = 0, i; > >>> - > >>> - if (!signal_pt) > >>> - return -ENOMEM; > >>> - if (!fence) > >>> - goto out; > >>> - > >>> - fences = kmalloc_array(sizeof(void *), 2, GFP_KERNEL); > >>> - if (!fences) { > >>> - ret = -ENOMEM; > >>> - goto out; > >>> - } > >>> - fences[num_fences++] = dma_fence_get(fence); > >>> - /* timeline syncobj must take this dependency */ > >>> - if (syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) { > >>> - spin_lock(&syncobj->lock); > >>> - if (!list_empty(&syncobj->signal_pt_list)) { > >>> - tail_pt = list_last_entry(&syncobj->signal_pt_list, > >>> - struct drm_syncobj_signal_pt, list); > >>> - fences[num_fences++] = > >>> - dma_fence_get(&tail_pt->fence_array->base); > >>> - } > >>> - spin_unlock(&syncobj->lock); > >>> - } > >>> - signal_pt->fence_array = dma_fence_array_create(num_fences, fences, > >>> - syncobj->timeline_context, > >>> - point, false); > >>> - if (!signal_pt->fence_array) { > >>> - ret = -ENOMEM; > >>> - goto fail; > >>> - } > >>> - > >>> - spin_lock(&syncobj->lock); > >>> - if (syncobj->signal_point >= point) { > >>> - DRM_WARN("A later signal is ready!"); > >>> - spin_unlock(&syncobj->lock); > >>> - goto exist; > >>> - } > >>> - signal_pt->value = point; > >>> - list_add_tail(&signal_pt->list, &syncobj->signal_pt_list); > >>> - syncobj->signal_point = point; > >>> - spin_unlock(&syncobj->lock); > >>> - wake_up_all(&syncobj->wq); > >>> - > >>> - return 0; > >>> -exist: > >>> - dma_fence_put(&signal_pt->fence_array->base); > >>> -fail: > >>> - for (i = 0; i < num_fences; i++) > >>> - dma_fence_put(fences[i]); > >>> - kfree(fences); > >>> -out: > >>> - kfree(signal_pt); > >>> - return ret; > >>> -} > >>> - > >>> -static void drm_syncobj_garbage_collection(struct drm_syncobj *syncobj) > >>> -{ > >>> - struct drm_syncobj_signal_pt *signal_pt, *tmp, *tail_pt; > >>> - > >>> - spin_lock(&syncobj->lock); > >>> - tail_pt = list_last_entry(&syncobj->signal_pt_list, > >>> - struct drm_syncobj_signal_pt, > >>> - list); > >>> - list_for_each_entry_safe(signal_pt, tmp, > >>> - &syncobj->signal_pt_list, list) { > >>> - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY && > >>> - signal_pt == tail_pt) > >>> - continue; > >>> - if (dma_fence_is_signaled(&signal_pt->fence_array->base)) { > >>> - syncobj->timeline = signal_pt->value; > >>> - list_del(&signal_pt->list); > >>> - dma_fence_put(&signal_pt->fence_array->base); > >>> - kfree(signal_pt); > >>> - } else { > >>> - /*signal_pt is in order in list, from small to big, so > >>> - * the later must not be signal either */ > >>> - break; > >>> - } > >>> - } > >>> - > >>> - spin_unlock(&syncobj->lock); > >>> -} > >>> /** > >>> * drm_syncobj_replace_fence - replace fence in a sync object. > >>> * @syncobj: Sync object to replace fence in > >>> @@ -329,29 +165,28 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, > >>> u64 point, > >>> struct dma_fence *fence) > >>> { > >>> - u64 pt_value = point; > >>> - > >>> - drm_syncobj_garbage_collection(syncobj); > >>> - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) { > >>> - if (!fence) { > >>> - drm_syncobj_fini(syncobj); > >>> - drm_syncobj_init(syncobj); > >>> - return; > >>> - } > >>> - pt_value = syncobj->signal_point + > >>> - DRM_SYNCOBJ_BINARY_POINT; > >>> - } > >>> - drm_syncobj_create_signal_pt(syncobj, fence, pt_value); > >>> - if (fence) { > >>> - struct drm_syncobj_cb *cur, *tmp; > >>> + struct dma_fence *old_fence; > >>> + struct drm_syncobj_cb *cur, *tmp; > >>> + > >>> + if (fence) > >>> + dma_fence_get(fence); > >>> > >>> - spin_lock(&syncobj->lock); > >>> + spin_lock(&syncobj->lock); > >>> + > >>> + old_fence = rcu_dereference_protected(syncobj->fence, > >>> + lockdep_is_held(&syncobj->lock)); > >>> + rcu_assign_pointer(syncobj->fence, fence); > >>> + > >>> + if (fence != old_fence) { > >>> list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) { > >>> list_del_init(&cur->node); > >>> cur->func(syncobj, cur); > >>> } > >>> - spin_unlock(&syncobj->lock); > >>> } > >>> + > >>> + spin_unlock(&syncobj->lock); > >>> + > >>> + dma_fence_put(old_fence); > >>> } > >>> EXPORT_SYMBOL(drm_syncobj_replace_fence); > >>> > >>> @@ -374,64 +209,6 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) > >>> return 0; > >>> } > >>> > >>> -static int > >>> -drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags, > >>> - struct dma_fence **fence) > >>> -{ > >>> - int ret = 0; > >>> - > >>> - if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { > >>> - ret = wait_event_interruptible(syncobj->wq, > >>> - point <= syncobj->signal_point); > >>> - if (ret < 0) > >>> - return ret; > >>> - } > >>> - spin_lock(&syncobj->lock); > >>> - *fence = drm_syncobj_find_signal_pt_for_point(syncobj, point); > >>> - if (!*fence) > >>> - ret = -EINVAL; > >>> - spin_unlock(&syncobj->lock); > >>> - return ret; > >>> -} > >>> - > >>> -/** > >>> - * drm_syncobj_search_fence - lookup and reference the fence in a sync object or > >>> - * in a timeline point > >>> - * @syncobj: sync object pointer > >>> - * @point: timeline point > >>> - * @flags: DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT or not > >>> - * @fence: out parameter for the fence > >>> - * > >>> - * if flags is DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT, the function will block > >>> - * here until specific timeline points is reached. > >>> - * if not, you need a submit thread and block in userspace until all future > >>> - * timeline points have materialized, only then you can submit to the kernel, > >>> - * otherwise, function will fail to return 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 > >>> - * dma_fence_put(). > >>> - */ > >>> -int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point, > >>> - u64 flags, struct dma_fence **fence) > >>> -{ > >>> - u64 pt_value = point; > >>> - > >>> - if (!syncobj) > >>> - return -ENOENT; > >>> - > >>> - drm_syncobj_garbage_collection(syncobj); > >>> - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) { > >>> - /*BINARY syncobj always wait on last pt */ > >>> - pt_value = syncobj->signal_point; > >>> - > >>> - if (pt_value == 0) > >>> - pt_value += DRM_SYNCOBJ_BINARY_POINT; > >>> - } > >>> - return drm_syncobj_point_get(syncobj, pt_value, flags, fence); > >>> -} > >>> -EXPORT_SYMBOL(drm_syncobj_search_fence); > >>> - > >>> /** > >>> * drm_syncobj_find_fence - lookup and reference the fence in a sync object > >>> * @file_private: drm file private pointer > >>> @@ -441,7 +218,7 @@ EXPORT_SYMBOL(drm_syncobj_search_fence); > >>> * @fence: out parameter for the fence > >>> * > >>> * This is just a convenience function that combines drm_syncobj_find() and > >>> - * drm_syncobj_lookup_fence(). > >>> + * drm_syncobj_fence_get(). > >>> * > >>> * 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 > >>> @@ -452,9 +229,15 @@ int drm_syncobj_find_fence(struct drm_file *file_private, > >>> struct dma_fence **fence) > >>> { > >>> struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle); > >>> - int ret; > >>> + int ret = 0; > >>> > >>> - ret = drm_syncobj_search_fence(syncobj, point, flags, fence); > >>> + if (!syncobj) > >>> + return -ENOENT; > >>> + > >>> + *fence = drm_syncobj_fence_get(syncobj); > >>> + if (!*fence) { > >>> + ret = -EINVAL; > >>> + } > >>> drm_syncobj_put(syncobj); > >>> return ret; > >>> } > >>> @@ -471,7 +254,7 @@ void drm_syncobj_free(struct kref *kref) > >>> struct drm_syncobj *syncobj = container_of(kref, > >>> struct drm_syncobj, > >>> refcount); > >>> - drm_syncobj_fini(syncobj); > >>> + drm_syncobj_replace_fence(syncobj, 0, NULL); > >>> kfree(syncobj); > >>> } > >>> EXPORT_SYMBOL(drm_syncobj_free); > >>> @@ -501,11 +284,6 @@ 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_BINARY; > >>> - drm_syncobj_init(syncobj); > >>> > >>> if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { > >>> ret = drm_syncobj_assign_null_handle(syncobj); > >>> @@ -788,8 +566,7 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data, > >>> return -EOPNOTSUPP; > >>> > >>> /* no valid flags yet */ > >>> - if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED | > >>> - DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)) > >>> + if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED) > >>> return -EINVAL; > >>> > >>> return drm_syncobj_create_as_handle(file_private, > >>> @@ -882,8 +659,9 @@ 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); > >>> - > >>> + /* This happens inside the syncobj lock */ > >>> + wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, > >>> + lockdep_is_held(&syncobj->lock))); > >>> wake_up_process(wait->task); > >>> } > >>> > >>> @@ -909,8 +687,7 @@ 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; > >>> - drm_syncobj_search_fence(syncobjs[i], 0, 0, > >>> - &entries[i].fence); > >>> + entries[i].fence = drm_syncobj_fence_get(syncobjs[i]); > >>> if (!entries[i].fence) { > >>> if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { > >>> continue; > >>> @@ -1172,13 +949,12 @@ 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_fini(syncobjs[i]); > >>> - drm_syncobj_init(syncobjs[i]); > >>> - } > >>> + for (i = 0; i < args->count_handles; i++) > >>> + drm_syncobj_replace_fence(syncobjs[i], 0, NULL); > >>> + > >>> drm_syncobj_array_free(syncobjs, args->count_handles); > >>> > >>> - return ret; > >>> + return 0; > >>> } > >>> > >>> int > >>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >>> index 6d99651c6c4b..22b4cb775576 100644 > >>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >>> @@ -2152,7 +2152,7 @@ await_fence_array(struct i915_execbuffer *eb, > >>> if (!(flags & I915_EXEC_FENCE_WAIT)) > >>> continue; > >>> > >>> - drm_syncobj_search_fence(syncobj, 0, 0, &fence); > >>> + fence = drm_syncobj_fence_get(syncobj); > >>> if (!fence) > >>> return -EINVAL; > >>> > >>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h > >>> index 5e8c5c027e09..2eda44def639 100644 > >>> --- a/include/drm/drm_syncobj.h > >>> +++ b/include/drm/drm_syncobj.h > >>> @@ -30,15 +30,10 @@ > >>> > >>> struct drm_syncobj_cb; > >>> > >>> -enum drm_syncobj_type { > >>> - DRM_SYNCOBJ_TYPE_BINARY, > >>> - DRM_SYNCOBJ_TYPE_TIMELINE > >>> -}; > >>> - > >>> /** > >>> * struct drm_syncobj - sync object. > >>> * > >>> - * This structure defines a generic sync object which is timeline based. > >>> + * This structure defines a generic sync object which wraps a &dma_fence. > >>> */ > >>> struct drm_syncobj { > >>> /** > >>> @@ -46,36 +41,19 @@ struct drm_syncobj { > >>> */ > >>> struct kref refcount; > >>> /** > >>> - * @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 > >>> + * @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. > >>> */ > >>> - u64 timeline_context; > >>> + struct dma_fence __rcu *fence; > >>> /** > >>> - * @timeline: syncobj timeline value, which indicates point is signaled. > >>> + * @cb_list: List of callbacks to call when the &fence gets replaced. > >>> */ > >>> - 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 syncobj list and write-locks &fence. > >>> + * @lock: Protects &cb_list and write-locks &fence. > >>> */ > >>> spinlock_t lock; > >>> /** > >>> @@ -90,7 +68,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 > >>> @@ -128,6 +106,29 @@ 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, > >>> @@ -141,7 +142,5 @@ 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_search_fence(struct drm_syncobj *syncobj, u64 point, u64 flags, > >>> - struct dma_fence **fence); > >>> > >>> #endif > >>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > >>> index cebdb2541eb7..300f336633f2 100644 > >>> --- a/include/uapi/drm/drm.h > >>> +++ b/include/uapi/drm/drm.h > >>> @@ -717,7 +717,6 @@ 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; > >>> }; > >>> > >>> -- > >>> 2.17.1 > >>> > >>> _______________________________________________ > >>> dri-devel mailing list > >>> dri-devel@lists.freedesktop.org > >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >> -- > >> Daniel Vetter > >> Software Engineer, Intel Corporation > >> http://blog.ffwll.ch >
Quoting Christian König (2018-10-19 10:27:11) > From: Christian König <easy2remember.chk@googlemail.com> > > Still contains some bugs. > > This reverts commit 48197bc564c7a1888c86024a1ba4f956e0ec2300. > > Bugs: https://bugs.freedesktop.org/show_bug.cgi?id=108490 > Signed-off-by: Christian König <Christian.Koenig@amd.com> The code for the old style of syncobj == dma_fence is very very slow, as now instead of using the syncobj as an idr to the fence, we must always search a linked list. A major regression for existing behaviour. -Chris
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 57bf6006394d..e2c5b3ca4824 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -56,9 +56,6 @@ #include "drm_internal.h" #include <drm/drm_syncobj.h> -/* merge normal syncobj to timeline syncobj, the point interval is 1 */ -#define DRM_SYNCOBJ_BINARY_POINT 1 - struct drm_syncobj_stub_fence { struct dma_fence base; spinlock_t lock; @@ -74,11 +71,6 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops = { .get_timeline_name = drm_syncobj_stub_fence_get_name, }; -struct drm_syncobj_signal_pt { - struct dma_fence_array *fence_array; - u64 value; - struct list_head list; -}; /** * drm_syncobj_find - lookup and reference a sync object. @@ -121,8 +113,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, { int ret; - ret = drm_syncobj_search_fence(syncobj, 0, 0, fence); - if (!ret) + *fence = drm_syncobj_fence_get(syncobj); + if (*fence) return 1; spin_lock(&syncobj->lock); @@ -130,12 +122,10 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj, * have the lock, try one more time just to be sure we don't add a * callback when a fence has already been set. */ - if (!list_empty(&syncobj->signal_pt_list)) { - spin_unlock(&syncobj->lock); - drm_syncobj_search_fence(syncobj, 0, 0, fence); - if (*fence) - return 1; - spin_lock(&syncobj->lock); + if (syncobj->fence) { + *fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, + lockdep_is_held(&syncobj->lock))); + ret = 1; } else { *fence = NULL; drm_syncobj_add_callback_locked(syncobj, cb, func); @@ -163,160 +153,6 @@ void drm_syncobj_remove_callback(struct drm_syncobj *syncobj, spin_unlock(&syncobj->lock); } -static void drm_syncobj_init(struct drm_syncobj *syncobj) -{ - spin_lock(&syncobj->lock); - syncobj->timeline_context = dma_fence_context_alloc(1); - syncobj->timeline = 0; - syncobj->signal_point = 0; - init_waitqueue_head(&syncobj->wq); - - INIT_LIST_HEAD(&syncobj->signal_pt_list); - spin_unlock(&syncobj->lock); -} - -static void drm_syncobj_fini(struct drm_syncobj *syncobj) -{ - struct drm_syncobj_signal_pt *signal_pt = NULL, *tmp; - - spin_lock(&syncobj->lock); - list_for_each_entry_safe(signal_pt, tmp, - &syncobj->signal_pt_list, list) { - list_del(&signal_pt->list); - dma_fence_put(&signal_pt->fence_array->base); - kfree(signal_pt); - } - spin_unlock(&syncobj->lock); -} - -static struct dma_fence -*drm_syncobj_find_signal_pt_for_point(struct drm_syncobj *syncobj, - uint64_t point) -{ - struct drm_syncobj_signal_pt *signal_pt; - - if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) && - (point <= syncobj->timeline)) { - struct drm_syncobj_stub_fence *fence = - kzalloc(sizeof(struct drm_syncobj_stub_fence), - GFP_KERNEL); - - if (!fence) - return NULL; - spin_lock_init(&fence->lock); - dma_fence_init(&fence->base, - &drm_syncobj_stub_fence_ops, - &fence->lock, - syncobj->timeline_context, - point); - - dma_fence_signal(&fence->base); - return &fence->base; - } - - list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) { - if (point > signal_pt->value) - continue; - if ((syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) && - (point != signal_pt->value)) - continue; - return dma_fence_get(&signal_pt->fence_array->base); - } - return NULL; -} - -static int drm_syncobj_create_signal_pt(struct drm_syncobj *syncobj, - struct dma_fence *fence, - u64 point) -{ - struct drm_syncobj_signal_pt *signal_pt = - kzalloc(sizeof(struct drm_syncobj_signal_pt), GFP_KERNEL); - struct drm_syncobj_signal_pt *tail_pt; - struct dma_fence **fences; - int num_fences = 0; - int ret = 0, i; - - if (!signal_pt) - return -ENOMEM; - if (!fence) - goto out; - - fences = kmalloc_array(sizeof(void *), 2, GFP_KERNEL); - if (!fences) { - ret = -ENOMEM; - goto out; - } - fences[num_fences++] = dma_fence_get(fence); - /* timeline syncobj must take this dependency */ - if (syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) { - spin_lock(&syncobj->lock); - if (!list_empty(&syncobj->signal_pt_list)) { - tail_pt = list_last_entry(&syncobj->signal_pt_list, - struct drm_syncobj_signal_pt, list); - fences[num_fences++] = - dma_fence_get(&tail_pt->fence_array->base); - } - spin_unlock(&syncobj->lock); - } - signal_pt->fence_array = dma_fence_array_create(num_fences, fences, - syncobj->timeline_context, - point, false); - if (!signal_pt->fence_array) { - ret = -ENOMEM; - goto fail; - } - - spin_lock(&syncobj->lock); - if (syncobj->signal_point >= point) { - DRM_WARN("A later signal is ready!"); - spin_unlock(&syncobj->lock); - goto exist; - } - signal_pt->value = point; - list_add_tail(&signal_pt->list, &syncobj->signal_pt_list); - syncobj->signal_point = point; - spin_unlock(&syncobj->lock); - wake_up_all(&syncobj->wq); - - return 0; -exist: - dma_fence_put(&signal_pt->fence_array->base); -fail: - for (i = 0; i < num_fences; i++) - dma_fence_put(fences[i]); - kfree(fences); -out: - kfree(signal_pt); - return ret; -} - -static void drm_syncobj_garbage_collection(struct drm_syncobj *syncobj) -{ - struct drm_syncobj_signal_pt *signal_pt, *tmp, *tail_pt; - - spin_lock(&syncobj->lock); - tail_pt = list_last_entry(&syncobj->signal_pt_list, - struct drm_syncobj_signal_pt, - list); - list_for_each_entry_safe(signal_pt, tmp, - &syncobj->signal_pt_list, list) { - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY && - signal_pt == tail_pt) - continue; - if (dma_fence_is_signaled(&signal_pt->fence_array->base)) { - syncobj->timeline = signal_pt->value; - list_del(&signal_pt->list); - dma_fence_put(&signal_pt->fence_array->base); - kfree(signal_pt); - } else { - /*signal_pt is in order in list, from small to big, so - * the later must not be signal either */ - break; - } - } - - spin_unlock(&syncobj->lock); -} /** * drm_syncobj_replace_fence - replace fence in a sync object. * @syncobj: Sync object to replace fence in @@ -329,29 +165,28 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 point, struct dma_fence *fence) { - u64 pt_value = point; - - drm_syncobj_garbage_collection(syncobj); - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) { - if (!fence) { - drm_syncobj_fini(syncobj); - drm_syncobj_init(syncobj); - return; - } - pt_value = syncobj->signal_point + - DRM_SYNCOBJ_BINARY_POINT; - } - drm_syncobj_create_signal_pt(syncobj, fence, pt_value); - if (fence) { - struct drm_syncobj_cb *cur, *tmp; + struct dma_fence *old_fence; + struct drm_syncobj_cb *cur, *tmp; + + if (fence) + dma_fence_get(fence); - spin_lock(&syncobj->lock); + spin_lock(&syncobj->lock); + + old_fence = rcu_dereference_protected(syncobj->fence, + lockdep_is_held(&syncobj->lock)); + rcu_assign_pointer(syncobj->fence, fence); + + if (fence != old_fence) { list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) { list_del_init(&cur->node); cur->func(syncobj, cur); } - spin_unlock(&syncobj->lock); } + + spin_unlock(&syncobj->lock); + + dma_fence_put(old_fence); } EXPORT_SYMBOL(drm_syncobj_replace_fence); @@ -374,64 +209,6 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) return 0; } -static int -drm_syncobj_point_get(struct drm_syncobj *syncobj, u64 point, u64 flags, - struct dma_fence **fence) -{ - int ret = 0; - - if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { - ret = wait_event_interruptible(syncobj->wq, - point <= syncobj->signal_point); - if (ret < 0) - return ret; - } - spin_lock(&syncobj->lock); - *fence = drm_syncobj_find_signal_pt_for_point(syncobj, point); - if (!*fence) - ret = -EINVAL; - spin_unlock(&syncobj->lock); - return ret; -} - -/** - * drm_syncobj_search_fence - lookup and reference the fence in a sync object or - * in a timeline point - * @syncobj: sync object pointer - * @point: timeline point - * @flags: DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT or not - * @fence: out parameter for the fence - * - * if flags is DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT, the function will block - * here until specific timeline points is reached. - * if not, you need a submit thread and block in userspace until all future - * timeline points have materialized, only then you can submit to the kernel, - * otherwise, function will fail to return 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 - * dma_fence_put(). - */ -int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point, - u64 flags, struct dma_fence **fence) -{ - u64 pt_value = point; - - if (!syncobj) - return -ENOENT; - - drm_syncobj_garbage_collection(syncobj); - if (syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) { - /*BINARY syncobj always wait on last pt */ - pt_value = syncobj->signal_point; - - if (pt_value == 0) - pt_value += DRM_SYNCOBJ_BINARY_POINT; - } - return drm_syncobj_point_get(syncobj, pt_value, flags, fence); -} -EXPORT_SYMBOL(drm_syncobj_search_fence); - /** * drm_syncobj_find_fence - lookup and reference the fence in a sync object * @file_private: drm file private pointer @@ -441,7 +218,7 @@ EXPORT_SYMBOL(drm_syncobj_search_fence); * @fence: out parameter for the fence * * This is just a convenience function that combines drm_syncobj_find() and - * drm_syncobj_lookup_fence(). + * drm_syncobj_fence_get(). * * 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 @@ -452,9 +229,15 @@ int drm_syncobj_find_fence(struct drm_file *file_private, struct dma_fence **fence) { struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle); - int ret; + int ret = 0; - ret = drm_syncobj_search_fence(syncobj, point, flags, fence); + if (!syncobj) + return -ENOENT; + + *fence = drm_syncobj_fence_get(syncobj); + if (!*fence) { + ret = -EINVAL; + } drm_syncobj_put(syncobj); return ret; } @@ -471,7 +254,7 @@ void drm_syncobj_free(struct kref *kref) struct drm_syncobj *syncobj = container_of(kref, struct drm_syncobj, refcount); - drm_syncobj_fini(syncobj); + drm_syncobj_replace_fence(syncobj, 0, NULL); kfree(syncobj); } EXPORT_SYMBOL(drm_syncobj_free); @@ -501,11 +284,6 @@ 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_BINARY; - drm_syncobj_init(syncobj); if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { ret = drm_syncobj_assign_null_handle(syncobj); @@ -788,8 +566,7 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data, return -EOPNOTSUPP; /* no valid flags yet */ - if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED | - DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)) + if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED) return -EINVAL; return drm_syncobj_create_as_handle(file_private, @@ -882,8 +659,9 @@ 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); - + /* This happens inside the syncobj lock */ + wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, + lockdep_is_held(&syncobj->lock))); wake_up_process(wait->task); } @@ -909,8 +687,7 @@ 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; - drm_syncobj_search_fence(syncobjs[i], 0, 0, - &entries[i].fence); + entries[i].fence = drm_syncobj_fence_get(syncobjs[i]); if (!entries[i].fence) { if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) { continue; @@ -1172,13 +949,12 @@ 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_fini(syncobjs[i]); - drm_syncobj_init(syncobjs[i]); - } + for (i = 0; i < args->count_handles; i++) + drm_syncobj_replace_fence(syncobjs[i], 0, NULL); + drm_syncobj_array_free(syncobjs, args->count_handles); - return ret; + return 0; } int diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 6d99651c6c4b..22b4cb775576 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -2152,7 +2152,7 @@ await_fence_array(struct i915_execbuffer *eb, if (!(flags & I915_EXEC_FENCE_WAIT)) continue; - drm_syncobj_search_fence(syncobj, 0, 0, &fence); + fence = drm_syncobj_fence_get(syncobj); if (!fence) return -EINVAL; diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index 5e8c5c027e09..2eda44def639 100644 --- a/include/drm/drm_syncobj.h +++ b/include/drm/drm_syncobj.h @@ -30,15 +30,10 @@ struct drm_syncobj_cb; -enum drm_syncobj_type { - DRM_SYNCOBJ_TYPE_BINARY, - DRM_SYNCOBJ_TYPE_TIMELINE -}; - /** * struct drm_syncobj - sync object. * - * This structure defines a generic sync object which is timeline based. + * This structure defines a generic sync object which wraps a &dma_fence. */ struct drm_syncobj { /** @@ -46,36 +41,19 @@ struct drm_syncobj { */ struct kref refcount; /** - * @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 + * @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. */ - u64 timeline_context; + struct dma_fence __rcu *fence; /** - * @timeline: syncobj timeline value, which indicates point is signaled. + * @cb_list: List of callbacks to call when the &fence gets replaced. */ - 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 syncobj list and write-locks &fence. + * @lock: Protects &cb_list and write-locks &fence. */ spinlock_t lock; /** @@ -90,7 +68,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 @@ -128,6 +106,29 @@ 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, @@ -141,7 +142,5 @@ 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_search_fence(struct drm_syncobj *syncobj, u64 point, u64 flags, - struct dma_fence **fence); #endif diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index cebdb2541eb7..300f336633f2 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -717,7 +717,6 @@ 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; };