[PATCH 1/7] drm: add flags to drm_syncobj_find_fence

[PATCH 1/7] drm: add flags to drm_syncobj_find_fence

[Chunming Zhou]

flags can be used by driver to decide whether need to block wait submission.

Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
 drivers/gpu/drm/drm_syncobj.c          | 4 ++--
 drivers/gpu/drm/v3d/v3d_gem.c          | 4 ++--
 drivers/gpu/drm/vc4/vc4_gem.c          | 2 +-
 include/drm/drm_syncobj.h              | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index d9d2ede96490..412fac238575 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1102,7 +1102,7 @@ static int amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
 {
 	int r;
 	struct dma_fence *fence;
-	r = drm_syncobj_find_fence(p->filp, handle, 0, &fence);
+	r = drm_syncobj_find_fence(p->filp, handle, 0, 0, &fence);
 	if (r)
 		return r;
 
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index e9ce623d049e..f796c9fc3858 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -235,7 +235,7 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
  * dma_fence_put().
  */
 int drm_syncobj_find_fence(struct drm_file *file_private,
-			   u32 handle, u64 point,
+			   u32 handle, u64 point, u64 flags,
 			   struct dma_fence **fence)
 {
 	struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle);
@@ -506,7 +506,7 @@ static int drm_syncobj_export_sync_file(struct drm_file *file_private,
 	if (fd < 0)
 		return fd;
 
-	ret = drm_syncobj_find_fence(file_private, handle, 0, &fence);
+	ret = drm_syncobj_find_fence(file_private, handle, 0, 0, &fence);
 	if (ret)
 		goto err_put_fd;
 
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 70c54774400b..97477879d3d4 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -521,12 +521,12 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 	kref_init(&exec->refcount);
 
 	ret = drm_syncobj_find_fence(file_priv, args->in_sync_bcl,
-				     0, &exec->bin.in_fence);
+				     0, 0, &exec->bin.in_fence);
 	if (ret == -EINVAL)
 		goto fail;
 
 	ret = drm_syncobj_find_fence(file_priv, args->in_sync_rcl,
-				     0, &exec->render.in_fence);
+				     0, 0, &exec->render.in_fence);
 	if (ret == -EINVAL)
 		goto fail;
 
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index 5b22e996af6c..251198194c38 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -1173,7 +1173,7 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
 
 	if (args->in_sync) {
 		ret = drm_syncobj_find_fence(file_priv, args->in_sync,
-					     0, &in_fence);
+					     0, 0, &in_fence);
 		if (ret)
 			goto fail;
 
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
index 425432b85a87..2eda44def639 100644
--- a/include/drm/drm_syncobj.h
+++ b/include/drm/drm_syncobj.h
@@ -134,7 +134,7 @@ struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
 void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 point,
 			       struct dma_fence *fence);
 int drm_syncobj_find_fence(struct drm_file *file_private,
-			   u32 handle, u64 point,
+			   u32 handle, u64 point, u64 flags,
 			   struct dma_fence **fence);
 void drm_syncobj_free(struct kref *kref);
 int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/7] drm: add syncobj timeline support v8

[Chunming Zhou]

This patch is for VK_KHR_timeline_semaphore extension, semaphore is called syncobj in kernel side:
This extension introduces a new type of syncobj that has an integer payload
identifying a point in a timeline. Such timeline syncobjs support the
following operations:
   * CPU query - A host operation that allows querying the payload of the
     timeline syncobj.
   * CPU wait - A host operation that allows a blocking wait for a
     timeline syncobj to reach a specified value.
   * Device wait - A device operation that allows waiting for a
     timeline syncobj to reach a specified value.
   * Device signal - A device operation that allows advancing the
     timeline syncobj to a specified value.

v1:
Since it's a timeline, that means the front time point(PT) always is signaled before the late PT.
a. signal PT design:
Signal PT fence N depends on PT[N-1] fence and signal opertion fence, when PT[N] fence is signaled,
the timeline will increase to value of PT[N].
b. wait PT design:
Wait PT fence is signaled by reaching timeline point value, when timeline is increasing, will compare
wait PTs value with new timeline value, if PT value is lower than timeline value, then wait PT will be
signaled, otherwise keep in list. syncobj wait operation can wait on any point of timeline,
so need a RB tree to order them. And wait PT could ahead of signal PT, we need a sumission fence to
perform that.

v2:
1. remove unused DRM_SYNCOBJ_CREATE_TYPE_NORMAL. (Christian)
2. move unexposed denitions to .c file. (Daniel Vetter)
3. split up the change to drm_syncobj_find_fence() in a separate patch. (Christian)
4. split up the change to drm_syncobj_replace_fence() in a separate patch.
5. drop the submission_fence implementation and instead use wait_event() for that. (Christian)
6. WARN_ON(point != 0) for NORMAL type syncobj case. (Daniel Vetter)

v3:
1. replace normal syncobj with timeline implemenation. (Vetter and Christian)
    a. normal syncobj signal op will create a signal PT to tail of signal pt list.
    b. normal syncobj wait op will create a wait pt with last signal point, and this wait PT is only signaled by related signal point PT.
2. many bug fix and clean up
3. stub fence moving is moved to other patch.

v4：
1. fix RB tree loop with while(node=rb_first(...)). (Christian)
2. fix syncobj lifecycle. (Christian)
3. only enable_signaling when there is wait_pt. (Christian)
4. fix timeline path issues.
5. write a timeline test in libdrm

v5: (Christian)
1. semaphore is called syncobj in kernel side.
2. don't need 'timeline' characters in some function name.
3. keep syncobj cb.

v6: (Christian)
1. merge syncobj_timeline to syncobj structure.
2. simplify some check sentences.
3. some misc change.
4. fix CTS failed issue.

v7: (Christian)
1. error handling when creating signal pt.
2. remove timeline naming in func.
3. export flags in find_fence.
4. allow reset timeline.

v8:
1. use wait_event_interruptible without timeout
2. rename _TYPE_INDIVIDUAL to _TYPE_BINARY

individual syncobj is tested by ./deqp-vk -n dEQP-VK*semaphore*
timeline syncobj is tested by ./amdgpu_test -s 9

Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
Cc: Christian Konig <christian.koenig@amd.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Rakos <Daniel.Rakos@amd.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/drm_syncobj.c              | 287 ++++++++++++++++++---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   2 +-
 include/drm/drm_syncobj.h                  |  65 ++---
 include/uapi/drm/drm.h                     |   1 +
 4 files changed, 281 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index f796c9fc3858..67472bd77c83 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -56,6 +56,9 @@
 #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;
@@ -82,6 +85,11 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops = {
 	.release = drm_syncobj_stub_fence_release,
 };
 
+struct drm_syncobj_signal_pt {
+	struct dma_fence_array *base;
+	u64    value;
+	struct list_head list;
+};
 
 /**
  * drm_syncobj_find - lookup and reference a sync object.
@@ -124,8 +132,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
 {
 	int ret;
 
-	*fence = drm_syncobj_fence_get(syncobj);
-	if (*fence)
+	ret = drm_syncobj_search_fence(syncobj, 0, 0, fence);
+	if (!ret)
 		return 1;
 
 	spin_lock(&syncobj->lock);
@@ -133,10 +141,12 @@ 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 (syncobj->fence) {
-		*fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
-								 lockdep_is_held(&syncobj->lock)));
-		ret = 1;
+	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);
 	} else {
 		*fence = NULL;
 		drm_syncobj_add_callback_locked(syncobj, cb, func);
@@ -164,6 +174,159 @@ 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->base->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->base->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->base->base);
+		}
+		spin_unlock(&syncobj->lock);
+	}
+	signal_pt->base = dma_fence_array_create(num_fences, fences,
+						 syncobj->timeline_context,
+						 point, false);
+	if (!signal_pt->base) {
+		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->base->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->base->base)) {
+			syncobj->timeline = signal_pt->value;
+			list_del(&signal_pt->list);
+			dma_fence_put(&signal_pt->base->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
@@ -176,28 +339,29 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
 			       u64 point,
 			       struct dma_fence *fence)
 {
-	struct dma_fence *old_fence;
-	struct drm_syncobj_cb *cur, *tmp;
-
-	if (fence)
-		dma_fence_get(fence);
-
-	spin_lock(&syncobj->lock);
-
-	old_fence = rcu_dereference_protected(syncobj->fence,
-					      lockdep_is_held(&syncobj->lock));
-	rcu_assign_pointer(syncobj->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;
 
-	if (fence != old_fence) {
+		spin_lock(&syncobj->lock);
 		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);
 
@@ -220,6 +384,46 @@ 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;
+}
+
+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
@@ -228,7 +432,7 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
  * @fence: out parameter for the fence
  *
  * This is just a convenience function that combines drm_syncobj_find() and
- * drm_syncobj_fence_get().
+ * drm_syncobj_lookup_fence().
  *
  * Returns 0 on success or a negative error value on failure. On success @fence
  * contains a reference to the fence, which must be released by calling
@@ -239,15 +443,9 @@ 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 = 0;
-
-	if (!syncobj)
-		return -ENOENT;
+	int ret;
 
-	*fence = drm_syncobj_fence_get(syncobj);
-	if (!*fence) {
-		ret = -EINVAL;
-	}
+	ret = drm_syncobj_search_fence(syncobj, point, flags, fence);
 	drm_syncobj_put(syncobj);
 	return ret;
 }
@@ -264,7 +462,7 @@ void drm_syncobj_free(struct kref *kref)
 	struct drm_syncobj *syncobj = container_of(kref,
 						   struct drm_syncobj,
 						   refcount);
-	drm_syncobj_replace_fence(syncobj, 0, NULL);
+	drm_syncobj_fini(syncobj);
 	kfree(syncobj);
 }
 EXPORT_SYMBOL(drm_syncobj_free);
@@ -294,6 +492,11 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
 	kref_init(&syncobj->refcount);
 	INIT_LIST_HEAD(&syncobj->cb_list);
 	spin_lock_init(&syncobj->lock);
+	if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
+		syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
+	else
+		syncobj->type = DRM_SYNCOBJ_TYPE_BINARY;
+	drm_syncobj_init(syncobj);
 
 	if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
 		ret = drm_syncobj_assign_null_handle(syncobj);
@@ -576,7 +779,8 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
 		return -ENODEV;
 
 	/* no valid flags yet */
-	if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
+	if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
+			    DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
 		return -EINVAL;
 
 	return drm_syncobj_create_as_handle(file_private,
@@ -669,9 +873,8 @@ static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
 	struct syncobj_wait_entry *wait =
 		container_of(cb, struct syncobj_wait_entry, syncobj_cb);
 
-	/* This happens inside the syncobj lock */
-	wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
-							      lockdep_is_held(&syncobj->lock)));
+	drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
+
 	wake_up_process(wait->task);
 }
 
@@ -698,7 +901,8 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 	signaled_count = 0;
 	for (i = 0; i < count; ++i) {
 		entries[i].task = current;
-		entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
+		ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
+					       &entries[i].fence);
 		if (!entries[i].fence) {
 			if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
 				continue;
@@ -970,12 +1174,13 @@ drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
 	if (ret < 0)
 		return ret;
 
-	for (i = 0; i < args->count_handles; i++)
-		drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
-
+	for (i = 0; i < args->count_handles; i++) {
+		drm_syncobj_fini(syncobjs[i]);
+		drm_syncobj_init(syncobjs[i]);
+	}
 	drm_syncobj_array_free(syncobjs, args->count_handles);
 
-	return 0;
+	return ret;
 }
 
 int
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0a8d2d64f380..8a8d21b24119 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
 		if (!(flags & I915_EXEC_FENCE_WAIT))
 			continue;
 
-		fence = drm_syncobj_fence_get(syncobj);
+		drm_syncobj_search_fence(syncobj, 0, 0, &fence);
 		if (!fence)
 			return -EINVAL;
 
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
index 2eda44def639..85b36d4e53ee 100644
--- a/include/drm/drm_syncobj.h
+++ b/include/drm/drm_syncobj.h
@@ -30,6 +30,11 @@
 
 struct drm_syncobj_cb;
 
+enum drm_syncobj_type {
+	DRM_SYNCOBJ_TYPE_BINARY,
+	DRM_SYNCOBJ_TYPE_TIMELINE
+};
+
 /**
  * struct drm_syncobj - sync object.
  *
@@ -41,19 +46,36 @@ struct drm_syncobj {
 	 */
 	struct kref refcount;
 	/**
-	 * @fence:
-	 * NULL or a pointer to the fence bound to this object.
-	 *
-	 * This field should not be used directly. Use drm_syncobj_fence_get()
-	 * and drm_syncobj_replace_fence() instead.
+	 * @type: indicate syncobj type
+	 */
+	enum drm_syncobj_type type;
+	/**
+	 * @wq: wait signal operation work queue
+	 */
+	wait_queue_head_t	wq;
+	/**
+	 * @timeline_context: fence context used by timeline
 	 */
-	struct dma_fence __rcu *fence;
+	u64 timeline_context;
 	/**
-	 * @cb_list: List of callbacks to call when the &fence gets replaced.
+	 * @timeline: syncobj timeline value, which indicates point is signaled.
 	 */
+	u64 timeline;
+	/**
+	 * @signal_point: which indicates the latest signaler point.
+	 */
+	u64 signal_point;
+	/**
+	 * @signal_pt_list: signaler point list.
+	 */
+	struct list_head signal_pt_list;
+
+	/**
+         * @cb_list: List of callbacks to call when the &fence gets replaced.
+         */
 	struct list_head cb_list;
 	/**
-	 * @lock: Protects &cb_list and write-locks &fence.
+	 * @lock: Protects syncobj list and write-locks &fence.
 	 */
 	spinlock_t lock;
 	/**
@@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
 /**
  * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
  * @node: used by drm_syncob_add_callback to append this struct to
- *	  &drm_syncobj.cb_list
+ *       &drm_syncobj.cb_list
  * @func: drm_syncobj_func_t to call
  *
  * This struct will be initialized by drm_syncobj_add_callback, additional
@@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj *obj)
 	kref_put(&obj->refcount, drm_syncobj_free);
 }
 
-/**
- * drm_syncobj_fence_get - get a reference to a fence in a sync object
- * @syncobj: sync object.
- *
- * This acquires additional reference to &drm_syncobj.fence contained in @obj,
- * if not NULL. It is illegal to call this without already holding a reference.
- * No locks required.
- *
- * Returns:
- * Either the fence of @obj or NULL if there's none.
- */
-static inline struct dma_fence *
-drm_syncobj_fence_get(struct drm_syncobj *syncobj)
-{
-	struct dma_fence *fence;
-
-	rcu_read_lock();
-	fence = dma_fence_get_rcu_safe(&syncobj->fence);
-	rcu_read_unlock();
-
-	return fence;
-}
-
 struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
 				     u32 handle);
 void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 point,
@@ -142,5 +141,7 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
 int drm_syncobj_get_handle(struct drm_file *file_private,
 			   struct drm_syncobj *syncobj, u32 *handle);
 int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
+int drm_syncobj_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 300f336633f2..cebdb2541eb7 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -717,6 +717,7 @@ struct drm_prime_handle {
 struct drm_syncobj_create {
 	__u32 handle;
 #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
+#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
 	__u32 flags;
 };
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[Daniel Vetter]

On Mon, Oct 15, 2018 at 04:55:48PM +0800, Chunming Zhou wrote:
> This patch is for VK_KHR_timeline_semaphore extension, semaphore is called syncobj in kernel side:
> This extension introduces a new type of syncobj that has an integer payload
> identifying a point in a timeline. Such timeline syncobjs support the
> following operations:
>    * CPU query - A host operation that allows querying the payload of the
>      timeline syncobj.
>    * CPU wait - A host operation that allows a blocking wait for a
>      timeline syncobj to reach a specified value.
>    * Device wait - A device operation that allows waiting for a
>      timeline syncobj to reach a specified value.
>    * Device signal - A device operation that allows advancing the
>      timeline syncobj to a specified value.
> 
> v1:
> Since it's a timeline, that means the front time point(PT) always is signaled before the late PT.
> a. signal PT design:
> Signal PT fence N depends on PT[N-1] fence and signal opertion fence, when PT[N] fence is signaled,
> the timeline will increase to value of PT[N].
> b. wait PT design:
> Wait PT fence is signaled by reaching timeline point value, when timeline is increasing, will compare
> wait PTs value with new timeline value, if PT value is lower than timeline value, then wait PT will be
> signaled, otherwise keep in list. syncobj wait operation can wait on any point of timeline,
> so need a RB tree to order them. And wait PT could ahead of signal PT, we need a sumission fence to
> perform that.
> 
> v2:
> 1. remove unused DRM_SYNCOBJ_CREATE_TYPE_NORMAL. (Christian)
> 2. move unexposed denitions to .c file. (Daniel Vetter)
> 3. split up the change to drm_syncobj_find_fence() in a separate patch. (Christian)
> 4. split up the change to drm_syncobj_replace_fence() in a separate patch.
> 5. drop the submission_fence implementation and instead use wait_event() for that. (Christian)
> 6. WARN_ON(point != 0) for NORMAL type syncobj case. (Daniel Vetter)
> 
> v3:
> 1. replace normal syncobj with timeline implemenation. (Vetter and Christian)
>     a. normal syncobj signal op will create a signal PT to tail of signal pt list.
>     b. normal syncobj wait op will create a wait pt with last signal point, and this wait PT is only signaled by related signal point PT.
> 2. many bug fix and clean up
> 3. stub fence moving is moved to other patch.
> 
> v4：
> 1. fix RB tree loop with while(node=rb_first(...)). (Christian)
> 2. fix syncobj lifecycle. (Christian)
> 3. only enable_signaling when there is wait_pt. (Christian)
> 4. fix timeline path issues.
> 5. write a timeline test in libdrm
> 
> v5: (Christian)
> 1. semaphore is called syncobj in kernel side.
> 2. don't need 'timeline' characters in some function name.
> 3. keep syncobj cb.
> 
> v6: (Christian)
> 1. merge syncobj_timeline to syncobj structure.
> 2. simplify some check sentences.
> 3. some misc change.
> 4. fix CTS failed issue.
> 
> v7: (Christian)
> 1. error handling when creating signal pt.
> 2. remove timeline naming in func.
> 3. export flags in find_fence.
> 4. allow reset timeline.
> 
> v8:
> 1. use wait_event_interruptible without timeout
> 2. rename _TYPE_INDIVIDUAL to _TYPE_BINARY
> 
> individual syncobj is tested by ./deqp-vk -n dEQP-VK*semaphore*
> timeline syncobj is tested by ./amdgpu_test -s 9

Can we please have these low-level syncobj tests as part of igt, together
with all the other syncobj tests which are there already? Really doesn't
make much sense imo to splits things on the test suite front.
> 
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> Cc: Christian Konig <christian.koenig@amd.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Rakos <Daniel.Rakos@amd.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/drm_syncobj.c              | 287 ++++++++++++++++++---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   2 +-
>  include/drm/drm_syncobj.h                  |  65 ++---
>  include/uapi/drm/drm.h                     |   1 +
>  4 files changed, 281 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index f796c9fc3858..67472bd77c83 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -56,6 +56,9 @@
>  #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;
> @@ -82,6 +85,11 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops = {
>  	.release = drm_syncobj_stub_fence_release,
>  };
>  
> +struct drm_syncobj_signal_pt {
> +	struct dma_fence_array *base;

Out of curiosity, why the pointer and not embedding? base is kinda
misleading for a pointer.

> +	u64    value;
> +	struct list_head list;
> +};
>  
>  /**
>   * drm_syncobj_find - lookup and reference a sync object.
> @@ -124,8 +132,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
>  {
>  	int ret;
>  
> -	*fence = drm_syncobj_fence_get(syncobj);
> -	if (*fence)
> +	ret = drm_syncobj_search_fence(syncobj, 0, 0, fence);
> +	if (!ret)
>  		return 1;
>  
>  	spin_lock(&syncobj->lock);
> @@ -133,10 +141,12 @@ 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 (syncobj->fence) {
> -		*fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
> -								 lockdep_is_held(&syncobj->lock)));
> -		ret = 1;
> +	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);
>  	} else {
>  		*fence = NULL;
>  		drm_syncobj_add_callback_locked(syncobj, cb, func);
> @@ -164,6 +174,159 @@ 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->base->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->base->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->base->base);
> +		}
> +		spin_unlock(&syncobj->lock);
> +	}
> +	signal_pt->base = dma_fence_array_create(num_fences, fences,
> +						 syncobj->timeline_context,
> +						 point, false);
> +	if (!signal_pt->base) {
> +		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->base->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->base->base)) {
> +			syncobj->timeline = signal_pt->value;
> +			list_del(&signal_pt->list);
> +			dma_fence_put(&signal_pt->base->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
> @@ -176,28 +339,29 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>  			       u64 point,
>  			       struct dma_fence *fence)
>  {
> -	struct dma_fence *old_fence;
> -	struct drm_syncobj_cb *cur, *tmp;
> -
> -	if (fence)
> -		dma_fence_get(fence);
> -
> -	spin_lock(&syncobj->lock);
> -
> -	old_fence = rcu_dereference_protected(syncobj->fence,
> -					      lockdep_is_held(&syncobj->lock));
> -	rcu_assign_pointer(syncobj->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;
>  
> -	if (fence != old_fence) {
> +		spin_lock(&syncobj->lock);
>  		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);
>  
> @@ -220,6 +384,46 @@ 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;
> +}
> +
> +int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point,
> +			     u64 flags, struct dma_fence **fence)

Kerneldoc is missing for this one here.

I also think improving the overview comment to explain the timeline stuff,
e.g. link to the vulkan extension, and some words on how it's supposed to
be used (you need a submit thread and block in userspace until all future
timeline points have materialized, only then you can submit to the
kernel).

> +{
> +	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
> @@ -228,7 +432,7 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>   * @fence: out parameter for the fence
>   *
>   * This is just a convenience function that combines drm_syncobj_find() and
> - * drm_syncobj_fence_get().
> + * drm_syncobj_lookup_fence().
>   *
>   * Returns 0 on success or a negative error value on failure. On success @fence
>   * contains a reference to the fence, which must be released by calling
> @@ -239,15 +443,9 @@ 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 = 0;
> -
> -	if (!syncobj)
> -		return -ENOENT;
> +	int ret;
>  
> -	*fence = drm_syncobj_fence_get(syncobj);
> -	if (!*fence) {
> -		ret = -EINVAL;
> -	}
> +	ret = drm_syncobj_search_fence(syncobj, point, flags, fence);
>  	drm_syncobj_put(syncobj);
>  	return ret;
>  }
> @@ -264,7 +462,7 @@ void drm_syncobj_free(struct kref *kref)
>  	struct drm_syncobj *syncobj = container_of(kref,
>  						   struct drm_syncobj,
>  						   refcount);
> -	drm_syncobj_replace_fence(syncobj, 0, NULL);
> +	drm_syncobj_fini(syncobj);
>  	kfree(syncobj);
>  }
>  EXPORT_SYMBOL(drm_syncobj_free);
> @@ -294,6 +492,11 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
>  	kref_init(&syncobj->refcount);
>  	INIT_LIST_HEAD(&syncobj->cb_list);
>  	spin_lock_init(&syncobj->lock);
> +	if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
> +		syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
> +	else
> +		syncobj->type = DRM_SYNCOBJ_TYPE_BINARY;
> +	drm_syncobj_init(syncobj);
>  
>  	if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
>  		ret = drm_syncobj_assign_null_handle(syncobj);
> @@ -576,7 +779,8 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
>  		return -ENODEV;
>  
>  	/* no valid flags yet */
> -	if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
> +	if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
> +			    DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
>  		return -EINVAL;
>  
>  	return drm_syncobj_create_as_handle(file_private,
> @@ -669,9 +873,8 @@ static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
>  	struct syncobj_wait_entry *wait =
>  		container_of(cb, struct syncobj_wait_entry, syncobj_cb);
>  
> -	/* This happens inside the syncobj lock */
> -	wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
> -							      lockdep_is_held(&syncobj->lock)));
> +	drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
> +
>  	wake_up_process(wait->task);
>  }
>  
> @@ -698,7 +901,8 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>  	signaled_count = 0;
>  	for (i = 0; i < count; ++i) {
>  		entries[i].task = current;
> -		entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
> +		ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
> +					       &entries[i].fence);
>  		if (!entries[i].fence) {
>  			if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>  				continue;
> @@ -970,12 +1174,13 @@ drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
>  	if (ret < 0)
>  		return ret;
>  
> -	for (i = 0; i < args->count_handles; i++)
> -		drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
> -
> +	for (i = 0; i < args->count_handles; i++) {
> +		drm_syncobj_fini(syncobjs[i]);
> +		drm_syncobj_init(syncobjs[i]);
> +	}
>  	drm_syncobj_array_free(syncobjs, args->count_handles);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  int
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 0a8d2d64f380..8a8d21b24119 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
>  		if (!(flags & I915_EXEC_FENCE_WAIT))
>  			continue;
>  
> -		fence = drm_syncobj_fence_get(syncobj);
> +		drm_syncobj_search_fence(syncobj, 0, 0, &fence);
>  		if (!fence)
>  			return -EINVAL;
>  
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> index 2eda44def639..85b36d4e53ee 100644
> --- a/include/drm/drm_syncobj.h
> +++ b/include/drm/drm_syncobj.h
> @@ -30,6 +30,11 @@
>  
>  struct drm_syncobj_cb;
>  
> +enum drm_syncobj_type {
> +	DRM_SYNCOBJ_TYPE_BINARY,
> +	DRM_SYNCOBJ_TYPE_TIMELINE
> +};
> +
>  /**
>   * struct drm_syncobj - sync object.
>   *

The kerneldoc for this also isn't accurate anymore.

With the kernel-doc polished:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But I think we definitely want a full ack for the uapi from radv/anv guys
(plus the userspace for either of those).
-Daniel

> @@ -41,19 +46,36 @@ struct drm_syncobj {
>  	 */
>  	struct kref refcount;
>  	/**
> -	 * @fence:
> -	 * NULL or a pointer to the fence bound to this object.
> -	 *
> -	 * This field should not be used directly. Use drm_syncobj_fence_get()
> -	 * and drm_syncobj_replace_fence() instead.
> +	 * @type: indicate syncobj type
> +	 */
> +	enum drm_syncobj_type type;
> +	/**
> +	 * @wq: wait signal operation work queue
> +	 */
> +	wait_queue_head_t	wq;
> +	/**
> +	 * @timeline_context: fence context used by timeline
>  	 */
> -	struct dma_fence __rcu *fence;
> +	u64 timeline_context;
>  	/**
> -	 * @cb_list: List of callbacks to call when the &fence gets replaced.
> +	 * @timeline: syncobj timeline value, which indicates point is signaled.
>  	 */
> +	u64 timeline;
> +	/**
> +	 * @signal_point: which indicates the latest signaler point.
> +	 */
> +	u64 signal_point;
> +	/**
> +	 * @signal_pt_list: signaler point list.
> +	 */
> +	struct list_head signal_pt_list;
> +
> +	/**
> +         * @cb_list: List of callbacks to call when the &fence gets replaced.
> +         */
>  	struct list_head cb_list;
>  	/**
> -	 * @lock: Protects &cb_list and write-locks &fence.
> +	 * @lock: Protects syncobj list and write-locks &fence.
>  	 */
>  	spinlock_t lock;
>  	/**
> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
>  /**
>   * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
>   * @node: used by drm_syncob_add_callback to append this struct to
> - *	  &drm_syncobj.cb_list
> + *       &drm_syncobj.cb_list
>   * @func: drm_syncobj_func_t to call
>   *
>   * This struct will be initialized by drm_syncobj_add_callback, additional
> @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj *obj)
>  	kref_put(&obj->refcount, drm_syncobj_free);
>  }
>  
> -/**
> - * drm_syncobj_fence_get - get a reference to a fence in a sync object
> - * @syncobj: sync object.
> - *
> - * This acquires additional reference to &drm_syncobj.fence contained in @obj,
> - * if not NULL. It is illegal to call this without already holding a reference.
> - * No locks required.
> - *
> - * Returns:
> - * Either the fence of @obj or NULL if there's none.
> - */
> -static inline struct dma_fence *
> -drm_syncobj_fence_get(struct drm_syncobj *syncobj)
> -{
> -	struct dma_fence *fence;
> -
> -	rcu_read_lock();
> -	fence = dma_fence_get_rcu_safe(&syncobj->fence);
> -	rcu_read_unlock();
> -
> -	return fence;
> -}
> -
>  struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>  				     u32 handle);
>  void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 point,
> @@ -142,5 +141,7 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
>  int drm_syncobj_get_handle(struct drm_file *file_private,
>  			   struct drm_syncobj *syncobj, u32 *handle);
>  int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
> +int drm_syncobj_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 300f336633f2..cebdb2541eb7 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -717,6 +717,7 @@ struct drm_prime_handle {
>  struct drm_syncobj_create {
>  	__u32 handle;
>  #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
>  	__u32 flags;
>  };
>  
> -- 
> 2.17.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[zhoucm1]

On 2018年10月17日 16:09, Daniel Vetter wrote:
> On Mon, Oct 15, 2018 at 04:55:48PM +0800, Chunming Zhou wrote:
>> This patch is for VK_KHR_timeline_semaphore extension, semaphore is called syncobj in kernel side:
>> This extension introduces a new type of syncobj that has an integer payload
>> identifying a point in a timeline. Such timeline syncobjs support the
>> following operations:
>>     * CPU query - A host operation that allows querying the payload of the
>>       timeline syncobj.
>>     * CPU wait - A host operation that allows a blocking wait for a
>>       timeline syncobj to reach a specified value.
>>     * Device wait - A device operation that allows waiting for a
>>       timeline syncobj to reach a specified value.
>>     * Device signal - A device operation that allows advancing the
>>       timeline syncobj to a specified value.
>>
>> v1:
>> Since it's a timeline, that means the front time point(PT) always is signaled before the late PT.
>> a. signal PT design:
>> Signal PT fence N depends on PT[N-1] fence and signal opertion fence, when PT[N] fence is signaled,
>> the timeline will increase to value of PT[N].
>> b. wait PT design:
>> Wait PT fence is signaled by reaching timeline point value, when timeline is increasing, will compare
>> wait PTs value with new timeline value, if PT value is lower than timeline value, then wait PT will be
>> signaled, otherwise keep in list. syncobj wait operation can wait on any point of timeline,
>> so need a RB tree to order them. And wait PT could ahead of signal PT, we need a sumission fence to
>> perform that.
>>
>> v2:
>> 1. remove unused DRM_SYNCOBJ_CREATE_TYPE_NORMAL. (Christian)
>> 2. move unexposed denitions to .c file. (Daniel Vetter)
>> 3. split up the change to drm_syncobj_find_fence() in a separate patch. (Christian)
>> 4. split up the change to drm_syncobj_replace_fence() in a separate patch.
>> 5. drop the submission_fence implementation and instead use wait_event() for that. (Christian)
>> 6. WARN_ON(point != 0) for NORMAL type syncobj case. (Daniel Vetter)
>>
>> v3:
>> 1. replace normal syncobj with timeline implemenation. (Vetter and Christian)
>>      a. normal syncobj signal op will create a signal PT to tail of signal pt list.
>>      b. normal syncobj wait op will create a wait pt with last signal point, and this wait PT is only signaled by related signal point PT.
>> 2. many bug fix and clean up
>> 3. stub fence moving is moved to other patch.
>>
>> v4：
>> 1. fix RB tree loop with while(node=rb_first(...)). (Christian)
>> 2. fix syncobj lifecycle. (Christian)
>> 3. only enable_signaling when there is wait_pt. (Christian)
>> 4. fix timeline path issues.
>> 5. write a timeline test in libdrm
>>
>> v5: (Christian)
>> 1. semaphore is called syncobj in kernel side.
>> 2. don't need 'timeline' characters in some function name.
>> 3. keep syncobj cb.
>>
>> v6: (Christian)
>> 1. merge syncobj_timeline to syncobj structure.
>> 2. simplify some check sentences.
>> 3. some misc change.
>> 4. fix CTS failed issue.
>>
>> v7: (Christian)
>> 1. error handling when creating signal pt.
>> 2. remove timeline naming in func.
>> 3. export flags in find_fence.
>> 4. allow reset timeline.
>>
>> v8:
>> 1. use wait_event_interruptible without timeout
>> 2. rename _TYPE_INDIVIDUAL to _TYPE_BINARY
>>
>> individual syncobj is tested by ./deqp-vk -n dEQP-VK*semaphore*
>> timeline syncobj is tested by ./amdgpu_test -s 9
> Can we please have these low-level syncobj tests as part of igt, together
> with all the other syncobj tests which are there already?
Good suggestion first, I'm just not familiar with igt( build, run 
cmd...), maybe we can add it later.

> Really doesn't
> make much sense imo to splits things on the test suite front.
>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>> Cc: Christian Konig <christian.koenig@amd.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: Daniel Rakos <Daniel.Rakos@amd.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/drm_syncobj.c              | 287 ++++++++++++++++++---
>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |   2 +-
>>   include/drm/drm_syncobj.h                  |  65 ++---
>>   include/uapi/drm/drm.h                     |   1 +
>>   4 files changed, 281 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>> index f796c9fc3858..67472bd77c83 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -56,6 +56,9 @@
>>   #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;
>> @@ -82,6 +85,11 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops = {
>>   	.release = drm_syncobj_stub_fence_release,
>>   };
>>   
>> +struct drm_syncobj_signal_pt {
>> +	struct dma_fence_array *base;
> Out of curiosity, why the pointer and not embedding? base is kinda
> misleading for a pointer.
Yeah, Christian doesn't like signal_pt lifecycle same as fence, so it's 
a pointer.
If you don't like 'base' name, I can change it.

>
>> +	u64    value;
>> +	struct list_head list;
>> +};
>>   
>>   /**
>>    * drm_syncobj_find - lookup and reference a sync object.
>> @@ -124,8 +132,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
>>   {
>>   	int ret;
>>   
>> -	*fence = drm_syncobj_fence_get(syncobj);
>> -	if (*fence)
>> +	ret = drm_syncobj_search_fence(syncobj, 0, 0, fence);
>> +	if (!ret)
>>   		return 1;
>>   
>>   	spin_lock(&syncobj->lock);
>> @@ -133,10 +141,12 @@ 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 (syncobj->fence) {
>> -		*fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
>> -								 lockdep_is_held(&syncobj->lock)));
>> -		ret = 1;
>> +	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);
>>   	} else {
>>   		*fence = NULL;
>>   		drm_syncobj_add_callback_locked(syncobj, cb, func);
>> @@ -164,6 +174,159 @@ 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->base->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->base->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->base->base);
>> +		}
>> +		spin_unlock(&syncobj->lock);
>> +	}
>> +	signal_pt->base = dma_fence_array_create(num_fences, fences,
>> +						 syncobj->timeline_context,
>> +						 point, false);
>> +	if (!signal_pt->base) {
>> +		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->base->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->base->base)) {
>> +			syncobj->timeline = signal_pt->value;
>> +			list_del(&signal_pt->list);
>> +			dma_fence_put(&signal_pt->base->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
>> @@ -176,28 +339,29 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>>   			       u64 point,
>>   			       struct dma_fence *fence)
>>   {
>> -	struct dma_fence *old_fence;
>> -	struct drm_syncobj_cb *cur, *tmp;
>> -
>> -	if (fence)
>> -		dma_fence_get(fence);
>> -
>> -	spin_lock(&syncobj->lock);
>> -
>> -	old_fence = rcu_dereference_protected(syncobj->fence,
>> -					      lockdep_is_held(&syncobj->lock));
>> -	rcu_assign_pointer(syncobj->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;
>>   
>> -	if (fence != old_fence) {
>> +		spin_lock(&syncobj->lock);
>>   		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);
>>   
>> @@ -220,6 +384,46 @@ 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;
>> +}
>> +
>> +int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point,
>> +			     u64 flags, struct dma_fence **fence)
> Kerneldoc is missing for this one here.
It's almost same as drm_syncobj_find_fence, is it still necessary?

>
> I also think improving the overview comment to explain the timeline stuff,
> e.g. link to the vulkan extension, and some words on how it's supposed to
> be used (you need a submit thread and block in userspace until all future
> timeline points have materialized, only then you can submit to the
> kernel).
>
>> +{
>> +	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
>> @@ -228,7 +432,7 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>>    * @fence: out parameter for the fence
>>    *
>>    * This is just a convenience function that combines drm_syncobj_find() and
>> - * drm_syncobj_fence_get().
>> + * drm_syncobj_lookup_fence().
>>    *
>>    * Returns 0 on success or a negative error value on failure. On success @fence
>>    * contains a reference to the fence, which must be released by calling
>> @@ -239,15 +443,9 @@ 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 = 0;
>> -
>> -	if (!syncobj)
>> -		return -ENOENT;
>> +	int ret;
>>   
>> -	*fence = drm_syncobj_fence_get(syncobj);
>> -	if (!*fence) {
>> -		ret = -EINVAL;
>> -	}
>> +	ret = drm_syncobj_search_fence(syncobj, point, flags, fence);
>>   	drm_syncobj_put(syncobj);
>>   	return ret;
>>   }
>> @@ -264,7 +462,7 @@ void drm_syncobj_free(struct kref *kref)
>>   	struct drm_syncobj *syncobj = container_of(kref,
>>   						   struct drm_syncobj,
>>   						   refcount);
>> -	drm_syncobj_replace_fence(syncobj, 0, NULL);
>> +	drm_syncobj_fini(syncobj);
>>   	kfree(syncobj);
>>   }
>>   EXPORT_SYMBOL(drm_syncobj_free);
>> @@ -294,6 +492,11 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
>>   	kref_init(&syncobj->refcount);
>>   	INIT_LIST_HEAD(&syncobj->cb_list);
>>   	spin_lock_init(&syncobj->lock);
>> +	if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
>> +		syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
>> +	else
>> +		syncobj->type = DRM_SYNCOBJ_TYPE_BINARY;
>> +	drm_syncobj_init(syncobj);
>>   
>>   	if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
>>   		ret = drm_syncobj_assign_null_handle(syncobj);
>> @@ -576,7 +779,8 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
>>   		return -ENODEV;
>>   
>>   	/* no valid flags yet */
>> -	if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
>> +	if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
>> +			    DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
>>   		return -EINVAL;
>>   
>>   	return drm_syncobj_create_as_handle(file_private,
>> @@ -669,9 +873,8 @@ static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
>>   	struct syncobj_wait_entry *wait =
>>   		container_of(cb, struct syncobj_wait_entry, syncobj_cb);
>>   
>> -	/* This happens inside the syncobj lock */
>> -	wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
>> -							      lockdep_is_held(&syncobj->lock)));
>> +	drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
>> +
>>   	wake_up_process(wait->task);
>>   }
>>   
>> @@ -698,7 +901,8 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>>   	signaled_count = 0;
>>   	for (i = 0; i < count; ++i) {
>>   		entries[i].task = current;
>> -		entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
>> +		ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
>> +					       &entries[i].fence);
>>   		if (!entries[i].fence) {
>>   			if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>>   				continue;
>> @@ -970,12 +1174,13 @@ drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	for (i = 0; i < args->count_handles; i++)
>> -		drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
>> -
>> +	for (i = 0; i < args->count_handles; i++) {
>> +		drm_syncobj_fini(syncobjs[i]);
>> +		drm_syncobj_init(syncobjs[i]);
>> +	}
>>   	drm_syncobj_array_free(syncobjs, args->count_handles);
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   int
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index 0a8d2d64f380..8a8d21b24119 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
>>   		if (!(flags & I915_EXEC_FENCE_WAIT))
>>   			continue;
>>   
>> -		fence = drm_syncobj_fence_get(syncobj);
>> +		drm_syncobj_search_fence(syncobj, 0, 0, &fence);
>>   		if (!fence)
>>   			return -EINVAL;
>>   
>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
>> index 2eda44def639..85b36d4e53ee 100644
>> --- a/include/drm/drm_syncobj.h
>> +++ b/include/drm/drm_syncobj.h
>> @@ -30,6 +30,11 @@
>>   
>>   struct drm_syncobj_cb;
>>   
>> +enum drm_syncobj_type {
>> +	DRM_SYNCOBJ_TYPE_BINARY,
>> +	DRM_SYNCOBJ_TYPE_TIMELINE
>> +};
>> +
>>   /**
>>    * struct drm_syncobj - sync object.
>>    *
> The kerneldoc for this also isn't accurate anymore.
Can kerneldoc be a separate patch?

>
> With the kernel-doc polished:
>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Thanks at least.

>
> But I think we definitely want a full ack for the uapi from radv/anv guys
> (plus the userspace for either of those).
Dave and Bas are right people, right?

Thanks,
David
> -Daniel
>
>> @@ -41,19 +46,36 @@ struct drm_syncobj {
>>   	 */
>>   	struct kref refcount;
>>   	/**
>> -	 * @fence:
>> -	 * NULL or a pointer to the fence bound to this object.
>> -	 *
>> -	 * This field should not be used directly. Use drm_syncobj_fence_get()
>> -	 * and drm_syncobj_replace_fence() instead.
>> +	 * @type: indicate syncobj type
>> +	 */
>> +	enum drm_syncobj_type type;
>> +	/**
>> +	 * @wq: wait signal operation work queue
>> +	 */
>> +	wait_queue_head_t	wq;
>> +	/**
>> +	 * @timeline_context: fence context used by timeline
>>   	 */
>> -	struct dma_fence __rcu *fence;
>> +	u64 timeline_context;
>>   	/**
>> -	 * @cb_list: List of callbacks to call when the &fence gets replaced.
>> +	 * @timeline: syncobj timeline value, which indicates point is signaled.
>>   	 */
>> +	u64 timeline;
>> +	/**
>> +	 * @signal_point: which indicates the latest signaler point.
>> +	 */
>> +	u64 signal_point;
>> +	/**
>> +	 * @signal_pt_list: signaler point list.
>> +	 */
>> +	struct list_head signal_pt_list;
>> +
>> +	/**
>> +         * @cb_list: List of callbacks to call when the &fence gets replaced.
>> +         */
>>   	struct list_head cb_list;
>>   	/**
>> -	 * @lock: Protects &cb_list and write-locks &fence.
>> +	 * @lock: Protects syncobj list and write-locks &fence.
>>   	 */
>>   	spinlock_t lock;
>>   	/**
>> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
>>   /**
>>    * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
>>    * @node: used by drm_syncob_add_callback to append this struct to
>> - *	  &drm_syncobj.cb_list
>> + *       &drm_syncobj.cb_list
>>    * @func: drm_syncobj_func_t to call
>>    *
>>    * This struct will be initialized by drm_syncobj_add_callback, additional
>> @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj *obj)
>>   	kref_put(&obj->refcount, drm_syncobj_free);
>>   }
>>   
>> -/**
>> - * drm_syncobj_fence_get - get a reference to a fence in a sync object
>> - * @syncobj: sync object.
>> - *
>> - * This acquires additional reference to &drm_syncobj.fence contained in @obj,
>> - * if not NULL. It is illegal to call this without already holding a reference.
>> - * No locks required.
>> - *
>> - * Returns:
>> - * Either the fence of @obj or NULL if there's none.
>> - */
>> -static inline struct dma_fence *
>> -drm_syncobj_fence_get(struct drm_syncobj *syncobj)
>> -{
>> -	struct dma_fence *fence;
>> -
>> -	rcu_read_lock();
>> -	fence = dma_fence_get_rcu_safe(&syncobj->fence);
>> -	rcu_read_unlock();
>> -
>> -	return fence;
>> -}
>> -
>>   struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>>   				     u32 handle);
>>   void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 point,
>> @@ -142,5 +141,7 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
>>   int drm_syncobj_get_handle(struct drm_file *file_private,
>>   			   struct drm_syncobj *syncobj, u32 *handle);
>>   int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
>> +int drm_syncobj_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 300f336633f2..cebdb2541eb7 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -717,6 +717,7 @@ struct drm_prime_handle {
>>   struct drm_syncobj_create {
>>   	__u32 handle;
>>   #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
>> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
>>   	__u32 flags;
>>   };
>>   
>> -- 
>> 2.17.1
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[Koenig, Christian]

Am 17.10.18 um 11:17 schrieb zhoucm1:
> [SNIP]
>>>   +struct drm_syncobj_signal_pt {
>>> +    struct dma_fence_array *base;
>> Out of curiosity, why the pointer and not embedding? base is kinda
>> misleading for a pointer.
> Yeah, Christian doesn't like signal_pt lifecycle same as fence, so 
> it's a pointer.
> If you don't like 'base' name, I can change it.

Well I never said that you can't embed the fence array into the signal_pt.

You just need to make sure that we don't affect the drm_syncobj 
lilecycle as well, e.g. that we don't also need to keep that around.

Regards,
Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[Daniel Vetter]

On Wed, Oct 17, 2018 at 11:29 AM Koenig, Christian
<Christian.Koenig@amd.com> wrote:
>
> Am 17.10.18 um 11:17 schrieb zhoucm1:
> > [SNIP]
> >>>   +struct drm_syncobj_signal_pt {
> >>> +    struct dma_fence_array *base;
> >> Out of curiosity, why the pointer and not embedding? base is kinda
> >> misleading for a pointer.
> > Yeah, Christian doesn't like signal_pt lifecycle same as fence, so
> > it's a pointer.
> > If you don't like 'base' name, I can change it.
>
> Well I never said that you can't embed the fence array into the signal_pt.
>
> You just need to make sure that we don't affect the drm_syncobj
> lilecycle as well, e.g. that we don't also need to keep that around.

I don't see a problem with that, as long as drm_syncobj keeps a
reference to the fence while it's on the timeline list. Which it
already does. And embedding would avoid that 2nd separate allocation,
aside from making base less confusing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[zhoucm1]

On 2018年10月17日 18:24, Daniel Vetter wrote:
> On Wed, Oct 17, 2018 at 11:29 AM Koenig, Christian
> <Christian.Koenig@amd.com> wrote:
>> Am 17.10.18 um 11:17 schrieb zhoucm1:
>>> [SNIP]
>>>>>    +struct drm_syncobj_signal_pt {
>>>>> +    struct dma_fence_array *base;
>>>> Out of curiosity, why the pointer and not embedding? base is kinda
>>>> misleading for a pointer.
>>> Yeah, Christian doesn't like signal_pt lifecycle same as fence, so
>>> it's a pointer.
>>> If you don't like 'base' name, I can change it.
>> Well I never said that you can't embed the fence array into the signal_pt.
>>
>> You just need to make sure that we don't affect the drm_syncobj
>> lilecycle as well, e.g. that we don't also need to keep that around.
> I don't see a problem with that, as long as drm_syncobj keeps a
> reference to the fence while it's on the timeline list. Which it
> already does. And embedding would avoid that 2nd separate allocation,
> aside from making base less confusing.
That's indeed my initial implementation for signal_pt/wait_pt with fence 
based, but after long and many discussions, we get current solution, as 
you see, the version is up to v8 :).

For here  why the pointer and not embedding?
Two reasons:
1. their lifecycles are not same.
2. It is a fence array usage, which always needs separate allocation, 
seems which is mandatory.
So it is a pointer.

But the name is historical from initial, and indeed be kinda misleading 
for a pointer, I will change it to fence_array instead in coming v9.

Thanks,
David Zhou

> -Daniel

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[Christian König]

Am 18.10.18 um 05:11 schrieb zhoucm1:
>
>
> On 2018年10月17日 18:24, Daniel Vetter wrote:
>> On Wed, Oct 17, 2018 at 11:29 AM Koenig, Christian
>> <Christian.Koenig@amd.com> wrote:
>>> Am 17.10.18 um 11:17 schrieb zhoucm1:
>>>> [SNIP]
>>>>>>    +struct drm_syncobj_signal_pt {
>>>>>> +    struct dma_fence_array *base;
>>>>> Out of curiosity, why the pointer and not embedding? base is kinda
>>>>> misleading for a pointer.
>>>> Yeah, Christian doesn't like signal_pt lifecycle same as fence, so
>>>> it's a pointer.
>>>> If you don't like 'base' name, I can change it.
>>> Well I never said that you can't embed the fence array into the 
>>> signal_pt.
>>>
>>> You just need to make sure that we don't affect the drm_syncobj
>>> lilecycle as well, e.g. that we don't also need to keep that around.
>> I don't see a problem with that, as long as drm_syncobj keeps a
>> reference to the fence while it's on the timeline list. Which it
>> already does. And embedding would avoid that 2nd separate allocation,
>> aside from making base less confusing.
> That's indeed my initial implementation for signal_pt/wait_pt with 
> fence based, but after long and many discussions, we get current 
> solution, as you see, the version is up to v8 :).
>
> For here  why the pointer and not embedding?
> Two reasons:
> 1. their lifecycles are not same.
> 2. It is a fence array usage, which always needs separate allocation, 
> seems which is mandatory.
> So it is a pointer.
>
> But the name is historical from initial, and indeed be kinda 
> misleading for a pointer, I will change it to fence_array instead in 
> coming v9.

To avoid running into a v10 I've just pushed this version upstream :)

The rest in the series looks good to me as well, but I certainly want 
the radv/anv developers to take a look as well as Daniel suggested.

Christian.

>
> Thanks,
> David Zhou
>
>> -Daniel
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[zhoucm1]

On 2018年10月18日 19:50, Christian König wrote:
> Am 18.10.18 um 05:11 schrieb zhoucm1:
>>
>>
>> On 2018年10月17日 18:24, Daniel Vetter wrote:
>>> On Wed, Oct 17, 2018 at 11:29 AM Koenig, Christian
>>> <Christian.Koenig@amd.com> wrote:
>>>> Am 17.10.18 um 11:17 schrieb zhoucm1:
>>>>> [SNIP]
>>>>>>>    +struct drm_syncobj_signal_pt {
>>>>>>> +    struct dma_fence_array *base;
>>>>>> Out of curiosity, why the pointer and not embedding? base is kinda
>>>>>> misleading for a pointer.
>>>>> Yeah, Christian doesn't like signal_pt lifecycle same as fence, so
>>>>> it's a pointer.
>>>>> If you don't like 'base' name, I can change it.
>>>> Well I never said that you can't embed the fence array into the 
>>>> signal_pt.
>>>>
>>>> You just need to make sure that we don't affect the drm_syncobj
>>>> lilecycle as well, e.g. that we don't also need to keep that around.
>>> I don't see a problem with that, as long as drm_syncobj keeps a
>>> reference to the fence while it's on the timeline list. Which it
>>> already does. And embedding would avoid that 2nd separate allocation,
>>> aside from making base less confusing.
>> That's indeed my initial implementation for signal_pt/wait_pt with 
>> fence based, but after long and many discussions, we get current 
>> solution, as you see, the version is up to v8 :).
>>
>> For here  why the pointer and not embedding?
>> Two reasons:
>> 1. their lifecycles are not same.
>> 2. It is a fence array usage, which always needs separate allocation, 
>> seems which is mandatory.
>> So it is a pointer.
>>
>> But the name is historical from initial, and indeed be kinda 
>> misleading for a pointer, I will change it to fence_array instead in 
>> coming v9.
>
> To avoid running into a v10 I've just pushed this version upstream :)
Thanks a lot.
>
> The rest in the series looks good to me as well,
Can I get your RB on them first?

> but I certainly want the radv/anv developers to take a look as well as 
> Daniel suggested.
Ping @Dave/Bas/Jason or other radv/anv developers, Could anyone of you 
take a look the rest of series for u/k interface? So that we can move to 
next step for libdrm patches?

Thanks,
David
>
> Christian.
>
>>
>> Thanks,
>> David Zhou
>>
>>> -Daniel
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[Daniel Vetter]

On Fri, Oct 19, 2018 at 10:29:55AM +0800, zhoucm1 wrote:
> 
> 
> On 2018年10月18日 19:50, Christian König wrote:
> > Am 18.10.18 um 05:11 schrieb zhoucm1:
> > > 
> > > 
> > > On 2018年10月17日 18:24, Daniel Vetter wrote:
> > > > On Wed, Oct 17, 2018 at 11:29 AM Koenig, Christian
> > > > <Christian.Koenig@amd.com> wrote:
> > > > > Am 17.10.18 um 11:17 schrieb zhoucm1:
> > > > > > [SNIP]
> > > > > > > >    +struct drm_syncobj_signal_pt {
> > > > > > > > +    struct dma_fence_array *base;
> > > > > > > Out of curiosity, why the pointer and not embedding? base is kinda
> > > > > > > misleading for a pointer.
> > > > > > Yeah, Christian doesn't like signal_pt lifecycle same as fence, so
> > > > > > it's a pointer.
> > > > > > If you don't like 'base' name, I can change it.
> > > > > Well I never said that you can't embed the fence array into
> > > > > the signal_pt.
> > > > > 
> > > > > You just need to make sure that we don't affect the drm_syncobj
> > > > > lilecycle as well, e.g. that we don't also need to keep that around.
> > > > I don't see a problem with that, as long as drm_syncobj keeps a
> > > > reference to the fence while it's on the timeline list. Which it
> > > > already does. And embedding would avoid that 2nd separate allocation,
> > > > aside from making base less confusing.
> > > That's indeed my initial implementation for signal_pt/wait_pt with
> > > fence based, but after long and many discussions, we get current
> > > solution, as you see, the version is up to v8 :).
> > > 
> > > For here  why the pointer and not embedding?
> > > Two reasons:
> > > 1. their lifecycles are not same.
> > > 2. It is a fence array usage, which always needs separate
> > > allocation, seems which is mandatory.
> > > So it is a pointer.
> > > 
> > > But the name is historical from initial, and indeed be kinda
> > > misleading for a pointer, I will change it to fence_array instead in
> > > coming v9.
> > 
> > To avoid running into a v10 I've just pushed this version upstream :)
> Thanks a lot.

(This time reply to the right patch, silly me)

Went boom:

https://bugs.freedesktop.org/show_bug.cgi?id=108490

Can we revert pls?

Also, can we please have igts for this stuff so that intel-gfx-ci could
test this properly before it's all fireworks?

Thanks, Daniel

> > 
> > The rest in the series looks good to me as well,
> Can I get your RB on them first?
> 
> > but I certainly want the radv/anv developers to take a look as well as
> > Daniel suggested.
> Ping @Dave/Bas/Jason or other radv/anv developers, Could anyone of you take
> a look the rest of series for u/k interface? So that we can move to next
> step for libdrm patches?
> 
> Thanks,
> David
> > 
> > Christian.
> > 
> > > 
> > > Thanks,
> > > David Zhou
> > > 
> > > > -Daniel
> > > 
> > > _______________________________________________
> > > amd-gfx mailing list
> > > amd-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[zhoucm1]

On 2018年10月19日 16:55, Daniel Vetter wrote:
> On Fri, Oct 19, 2018 at 10:29:55AM +0800, zhoucm1 wrote:
>>
>> On 2018年10月18日 19:50, Christian König wrote:
>>> Am 18.10.18 um 05:11 schrieb zhoucm1:
>>>>
>>>> On 2018年10月17日 18:24, Daniel Vetter wrote:
>>>>> On Wed, Oct 17, 2018 at 11:29 AM Koenig, Christian
>>>>> <Christian.Koenig@amd.com> wrote:
>>>>>> Am 17.10.18 um 11:17 schrieb zhoucm1:
>>>>>>> [SNIP]
>>>>>>>>>     +struct drm_syncobj_signal_pt {
>>>>>>>>> +    struct dma_fence_array *base;
>>>>>>>> Out of curiosity, why the pointer and not embedding? base is kinda
>>>>>>>> misleading for a pointer.
>>>>>>> Yeah, Christian doesn't like signal_pt lifecycle same as fence, so
>>>>>>> it's a pointer.
>>>>>>> If you don't like 'base' name, I can change it.
>>>>>> Well I never said that you can't embed the fence array into
>>>>>> the signal_pt.
>>>>>>
>>>>>> You just need to make sure that we don't affect the drm_syncobj
>>>>>> lilecycle as well, e.g. that we don't also need to keep that around.
>>>>> I don't see a problem with that, as long as drm_syncobj keeps a
>>>>> reference to the fence while it's on the timeline list. Which it
>>>>> already does. And embedding would avoid that 2nd separate allocation,
>>>>> aside from making base less confusing.
>>>> That's indeed my initial implementation for signal_pt/wait_pt with
>>>> fence based, but after long and many discussions, we get current
>>>> solution, as you see, the version is up to v8 :).
>>>>
>>>> For here  why the pointer and not embedding?
>>>> Two reasons:
>>>> 1. their lifecycles are not same.
>>>> 2. It is a fence array usage, which always needs separate
>>>> allocation, seems which is mandatory.
>>>> So it is a pointer.
>>>>
>>>> But the name is historical from initial, and indeed be kinda
>>>> misleading for a pointer, I will change it to fence_array instead in
>>>> coming v9.
>>> To avoid running into a v10 I've just pushed this version upstream :)
>> Thanks a lot.
> (This time reply to the right patch, silly me)
>
> Went boom:
>
> https://bugs.freedesktop.org/show_bug.cgi?id=108490
>
> Can we revert pls?
Sorry for bug, please.
>
> Also, can we please have igts for this stuff so that intel-gfx-ci could
> test this properly before it's all fireworks?
Seems we cannot avoid igt now and vulkan CTS isn't enough, I will find 
some time next week to lean IGT, looks v10 is need.

Regards,
David Zhou
>
> Thanks, Daniel
>
>>> The rest in the series looks good to me as well,
>> Can I get your RB on them first?
>>
>>> but I certainly want the radv/anv developers to take a look as well as
>>> Daniel suggested.
>> Ping @Dave/Bas/Jason or other radv/anv developers, Could anyone of you take
>> a look the rest of series for u/k interface? So that we can move to next
>> step for libdrm patches?
>>
>> Thanks,
>> David
>>> Christian.
>>>
>>>> Thanks,
>>>> David Zhou
>>>>
>>>>> -Daniel
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[zhoucm1]

On 2018年10月19日 17:20, zhoucm1 wrote:
>
>
> On 2018年10月19日 16:55, Daniel Vetter wrote:
>> On Fri, Oct 19, 2018 at 10:29:55AM +0800, zhoucm1 wrote:
>>>
>>> On 2018年10月18日 19:50, Christian König wrote:
>>>> Am 18.10.18 um 05:11 schrieb zhoucm1:
>>>>>
>>>>> On 2018年10月17日 18:24, Daniel Vetter wrote:
>>>>>> On Wed, Oct 17, 2018 at 11:29 AM Koenig, Christian
>>>>>> <Christian.Koenig@amd.com> wrote:
>>>>>>> Am 17.10.18 um 11:17 schrieb zhoucm1:
>>>>>>>> [SNIP]
>>>>>>>>>>     +struct drm_syncobj_signal_pt {
>>>>>>>>>> +    struct dma_fence_array *base;
>>>>>>>>> Out of curiosity, why the pointer and not embedding? base is 
>>>>>>>>> kinda
>>>>>>>>> misleading for a pointer.
>>>>>>>> Yeah, Christian doesn't like signal_pt lifecycle same as fence, so
>>>>>>>> it's a pointer.
>>>>>>>> If you don't like 'base' name, I can change it.
>>>>>>> Well I never said that you can't embed the fence array into
>>>>>>> the signal_pt.
>>>>>>>
>>>>>>> You just need to make sure that we don't affect the drm_syncobj
>>>>>>> lilecycle as well, e.g. that we don't also need to keep that 
>>>>>>> around.
>>>>>> I don't see a problem with that, as long as drm_syncobj keeps a
>>>>>> reference to the fence while it's on the timeline list. Which it
>>>>>> already does. And embedding would avoid that 2nd separate 
>>>>>> allocation,
>>>>>> aside from making base less confusing.
>>>>> That's indeed my initial implementation for signal_pt/wait_pt with
>>>>> fence based, but after long and many discussions, we get current
>>>>> solution, as you see, the version is up to v8 :).
>>>>>
>>>>> For here  why the pointer and not embedding?
>>>>> Two reasons:
>>>>> 1. their lifecycles are not same.
>>>>> 2. It is a fence array usage, which always needs separate
>>>>> allocation, seems which is mandatory.
>>>>> So it is a pointer.
>>>>>
>>>>> But the name is historical from initial, and indeed be kinda
>>>>> misleading for a pointer, I will change it to fence_array instead in
>>>>> coming v9.
>>>> To avoid running into a v10 I've just pushed this version upstream :)
>>> Thanks a lot.
>> (This time reply to the right patch, silly me)
>>
>> Went boom:
>>
>> https://bugs.freedesktop.org/show_bug.cgi?id=108490
>>
>> Can we revert pls?
> Sorry for bug, please.
In fact, the bug is already caught and fixed, just the fix part isn't in 
patch#1, but in patch#2:

Have you reverted? If not, I can send that fix in one minute.


Regards,
David Zhou
>>
>> Also, can we please have igts for this stuff so that intel-gfx-ci could
>> test this properly before it's all fireworks?
> Seems we cannot avoid igt now and vulkan CTS isn't enough, I will find 
> some time next week to lean IGT, looks v10 is need.
>
> Regards,
> David Zhou
>>
>> Thanks, Daniel
>>
>>>> The rest in the series looks good to me as well,
>>> Can I get your RB on them first?
>>>
>>>> but I certainly want the radv/anv developers to take a look as well as
>>>> Daniel suggested.
>>> Ping @Dave/Bas/Jason or other radv/anv developers, Could anyone of 
>>> you take
>>> a look the rest of series for u/k interface? So that we can move to 
>>> next
>>> step for libdrm patches?
>>>
>>> Thanks,
>>> David
>>>> Christian.
>>>>
>>>>> Thanks,
>>>>> David Zhou
>>>>>
>>>>>> -Daniel
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[zhoucm1]

[snip]
>>> Went boom:
>>>
>>> https://bugs.freedesktop.org/show_bug.cgi?id=108490
>>>
>>> Can we revert pls?
>> Sorry for bug, please.
> In fact, the bug is already caught and fixed, just the fix part isn't 
> in patch#1, but in patch#2:
>
> Have you reverted? If not, I can send that fix in one minute.
>
>
> Regards,
> David Zhou
>>>
>>> Also, can we please have igts for this stuff so that intel-gfx-ci could
>>> test this properly before it's all fireworks?
Hi Daniel V,

Could you point me which problem I encounter when I run syncobj_wait of igt?

jenkins@jenkins-MS-7984:~/freedesktop/igt-gpu-tools/tests$ ./syncobj_wait
IGT-Version: 1.23-g94ebd21 (x86_64) (Linux: 4.19.0-rc5-custom+ x86_64)
Test requirement not met in function igt_require_sw_sync, file 
sw_sync.c:240:
Test requirement: kernel_has_sw_sync()
Last errno: 2, No such file or directory

Thanks,
David Zhou
>> Seems we cannot avoid igt now and vulkan CTS isn't enough, I will 
>> find some time next week to lean IGT, looks v10 is need.
>>
>> Regards,
>> David Zhou
>>>
>>> Thanks, Daniel
>>>
>>>>> The rest in the series looks good to me as well,
>>>> Can I get your RB on them first?
>>>>
>>>>> but I certainly want the radv/anv developers to take a look as 
>>>>> well as
>>>>> Daniel suggested.
>>>> Ping @Dave/Bas/Jason or other radv/anv developers, Could anyone of 
>>>> you take
>>>> a look the rest of series for u/k interface? So that we can move to 
>>>> next
>>>> step for libdrm patches?
>>>>
>>>> Thanks,
>>>> David
>>>>> Christian.
>>>>>
>>>>>> Thanks,
>>>>>> David Zhou
>>>>>>
>>>>>>> -Daniel
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> amd-gfx@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[Daniel Vetter]

On Fri, Oct 19, 2018 at 12:45 PM zhoucm1 <zhoucm1@amd.com> wrote:
>
>
> [snip]
> >>> Went boom:
> >>>
> >>> https://bugs.freedesktop.org/show_bug.cgi?id=108490
> >>>
> >>> Can we revert pls?
> >> Sorry for bug, please.
> > In fact, the bug is already caught and fixed, just the fix part isn't
> > in patch#1, but in patch#2:
> >
> > Have you reverted? If not, I can send that fix in one minute.
> >
> >
> > Regards,
> > David Zhou
> >>>
> >>> Also, can we please have igts for this stuff so that intel-gfx-ci could
> >>> test this properly before it's all fireworks?
> Hi Daniel V,
>
> Could you point me which problem I encounter when I run syncobj_wait of igt?
>
> jenkins@jenkins-MS-7984:~/freedesktop/igt-gpu-tools/tests$ ./syncobj_wait
> IGT-Version: 1.23-g94ebd21 (x86_64) (Linux: 4.19.0-rc5-custom+ x86_64)
> Test requirement not met in function igt_require_sw_sync, file
> sw_sync.c:240:
> Test requirement: kernel_has_sw_sync()
> Last errno: 2, No such file or directory

The test isn't even run because one of it's requirements isn't met. In
this case here you don't have the SW_SYNC stuff enabled, which is used
to for testing. CONFIG_SW_SYNC in the kernel. You could do a quick igt
patch to change the igt_require() to an igt_require_f() with something
like "You need to enable CONFIG_SW_SYNC in your kernel build" as the
help text.
-Daniel

>
> Thanks,
> David Zhou
> >> Seems we cannot avoid igt now and vulkan CTS isn't enough, I will
> >> find some time next week to lean IGT, looks v10 is need.
> >>
> >> Regards,
> >> David Zhou
> >>>
> >>> Thanks, Daniel
> >>>
> >>>>> The rest in the series looks good to me as well,
> >>>> Can I get your RB on them first?
> >>>>
> >>>>> but I certainly want the radv/anv developers to take a look as
> >>>>> well as
> >>>>> Daniel suggested.
> >>>> Ping @Dave/Bas/Jason or other radv/anv developers, Could anyone of
> >>>> you take
> >>>> a look the rest of series for u/k interface? So that we can move to
> >>>> next
> >>>> step for libdrm patches?
> >>>>
> >>>> Thanks,
> >>>> David
> >>>>> Christian.
> >>>>>
> >>>>>> Thanks,
> >>>>>> David Zhou
> >>>>>>
> >>>>>>> -Daniel
> >>>>>> _______________________________________________
> >>>>>> amd-gfx mailing list
> >>>>>> amd-gfx@lists.freedesktop.org
> >>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >>
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[Daniel Vetter]

On Wed, Oct 17, 2018 at 11:17 AM zhoucm1 <zhoucm1@amd.com> wrote:
>
>
>
> On 2018年10月17日 16:09, Daniel Vetter wrote:
> > On Mon, Oct 15, 2018 at 04:55:48PM +0800, Chunming Zhou wrote:
> >> This patch is for VK_KHR_timeline_semaphore extension, semaphore is called syncobj in kernel side:
> >> This extension introduces a new type of syncobj that has an integer payload
> >> identifying a point in a timeline. Such timeline syncobjs support the
> >> following operations:
> >>     * CPU query - A host operation that allows querying the payload of the
> >>       timeline syncobj.
> >>     * CPU wait - A host operation that allows a blocking wait for a
> >>       timeline syncobj to reach a specified value.
> >>     * Device wait - A device operation that allows waiting for a
> >>       timeline syncobj to reach a specified value.
> >>     * Device signal - A device operation that allows advancing the
> >>       timeline syncobj to a specified value.
> >>
> >> v1:
> >> Since it's a timeline, that means the front time point(PT) always is signaled before the late PT.
> >> a. signal PT design:
> >> Signal PT fence N depends on PT[N-1] fence and signal opertion fence, when PT[N] fence is signaled,
> >> the timeline will increase to value of PT[N].
> >> b. wait PT design:
> >> Wait PT fence is signaled by reaching timeline point value, when timeline is increasing, will compare
> >> wait PTs value with new timeline value, if PT value is lower than timeline value, then wait PT will be
> >> signaled, otherwise keep in list. syncobj wait operation can wait on any point of timeline,
> >> so need a RB tree to order them. And wait PT could ahead of signal PT, we need a sumission fence to
> >> perform that.
> >>
> >> v2:
> >> 1. remove unused DRM_SYNCOBJ_CREATE_TYPE_NORMAL. (Christian)
> >> 2. move unexposed denitions to .c file. (Daniel Vetter)
> >> 3. split up the change to drm_syncobj_find_fence() in a separate patch. (Christian)
> >> 4. split up the change to drm_syncobj_replace_fence() in a separate patch.
> >> 5. drop the submission_fence implementation and instead use wait_event() for that. (Christian)
> >> 6. WARN_ON(point != 0) for NORMAL type syncobj case. (Daniel Vetter)
> >>
> >> v3:
> >> 1. replace normal syncobj with timeline implemenation. (Vetter and Christian)
> >>      a. normal syncobj signal op will create a signal PT to tail of signal pt list.
> >>      b. normal syncobj wait op will create a wait pt with last signal point, and this wait PT is only signaled by related signal point PT.
> >> 2. many bug fix and clean up
> >> 3. stub fence moving is moved to other patch.
> >>
> >> v4：
> >> 1. fix RB tree loop with while(node=rb_first(...)). (Christian)
> >> 2. fix syncobj lifecycle. (Christian)
> >> 3. only enable_signaling when there is wait_pt. (Christian)
> >> 4. fix timeline path issues.
> >> 5. write a timeline test in libdrm
> >>
> >> v5: (Christian)
> >> 1. semaphore is called syncobj in kernel side.
> >> 2. don't need 'timeline' characters in some function name.
> >> 3. keep syncobj cb.
> >>
> >> v6: (Christian)
> >> 1. merge syncobj_timeline to syncobj structure.
> >> 2. simplify some check sentences.
> >> 3. some misc change.
> >> 4. fix CTS failed issue.
> >>
> >> v7: (Christian)
> >> 1. error handling when creating signal pt.
> >> 2. remove timeline naming in func.
> >> 3. export flags in find_fence.
> >> 4. allow reset timeline.
> >>
> >> v8:
> >> 1. use wait_event_interruptible without timeout
> >> 2. rename _TYPE_INDIVIDUAL to _TYPE_BINARY
> >>
> >> individual syncobj is tested by ./deqp-vk -n dEQP-VK*semaphore*
> >> timeline syncobj is tested by ./amdgpu_test -s 9
> > Can we please have these low-level syncobj tests as part of igt, together
> > with all the other syncobj tests which are there already?
> Good suggestion first, I'm just not familiar with igt( build, run
> cmd...), maybe we can add it later.
>
> > Really doesn't
> > make much sense imo to splits things on the test suite front.
> >> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> >> Cc: Christian Konig <christian.koenig@amd.com>
> >> Cc: Dave Airlie <airlied@redhat.com>
> >> Cc: Daniel Rakos <Daniel.Rakos@amd.com>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> Reviewed-by: Christian König <christian.koenig@amd.com>
> >> ---
> >>   drivers/gpu/drm/drm_syncobj.c              | 287 ++++++++++++++++++---
> >>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |   2 +-
> >>   include/drm/drm_syncobj.h                  |  65 ++---
> >>   include/uapi/drm/drm.h                     |   1 +
> >>   4 files changed, 281 insertions(+), 74 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> >> index f796c9fc3858..67472bd77c83 100644
> >> --- a/drivers/gpu/drm/drm_syncobj.c
> >> +++ b/drivers/gpu/drm/drm_syncobj.c
> >> @@ -56,6 +56,9 @@
> >>   #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;
> >> @@ -82,6 +85,11 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops = {
> >>      .release = drm_syncobj_stub_fence_release,
> >>   };
> >>
> >> +struct drm_syncobj_signal_pt {
> >> +    struct dma_fence_array *base;
> > Out of curiosity, why the pointer and not embedding? base is kinda
> > misleading for a pointer.
> Yeah, Christian doesn't like signal_pt lifecycle same as fence, so it's
> a pointer.
> If you don't like 'base' name, I can change it.
>
> >
> >> +    u64    value;
> >> +    struct list_head list;
> >> +};
> >>
> >>   /**
> >>    * drm_syncobj_find - lookup and reference a sync object.
> >> @@ -124,8 +132,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
> >>   {
> >>      int ret;
> >>
> >> -    *fence = drm_syncobj_fence_get(syncobj);
> >> -    if (*fence)
> >> +    ret = drm_syncobj_search_fence(syncobj, 0, 0, fence);
> >> +    if (!ret)
> >>              return 1;
> >>
> >>      spin_lock(&syncobj->lock);
> >> @@ -133,10 +141,12 @@ 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 (syncobj->fence) {
> >> -            *fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
> >> -                                                             lockdep_is_held(&syncobj->lock)));
> >> -            ret = 1;
> >> +    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);
> >>      } else {
> >>              *fence = NULL;
> >>              drm_syncobj_add_callback_locked(syncobj, cb, func);
> >> @@ -164,6 +174,159 @@ 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->base->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->base->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->base->base);
> >> +            }
> >> +            spin_unlock(&syncobj->lock);
> >> +    }
> >> +    signal_pt->base = dma_fence_array_create(num_fences, fences,
> >> +                                             syncobj->timeline_context,
> >> +                                             point, false);
> >> +    if (!signal_pt->base) {
> >> +            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->base->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->base->base)) {
> >> +                    syncobj->timeline = signal_pt->value;
> >> +                    list_del(&signal_pt->list);
> >> +                    dma_fence_put(&signal_pt->base->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
> >> @@ -176,28 +339,29 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
> >>                             u64 point,
> >>                             struct dma_fence *fence)
> >>   {
> >> -    struct dma_fence *old_fence;
> >> -    struct drm_syncobj_cb *cur, *tmp;
> >> -
> >> -    if (fence)
> >> -            dma_fence_get(fence);
> >> -
> >> -    spin_lock(&syncobj->lock);
> >> -
> >> -    old_fence = rcu_dereference_protected(syncobj->fence,
> >> -                                          lockdep_is_held(&syncobj->lock));
> >> -    rcu_assign_pointer(syncobj->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;
> >>
> >> -    if (fence != old_fence) {
> >> +            spin_lock(&syncobj->lock);
> >>              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);
> >>
> >> @@ -220,6 +384,46 @@ 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;
> >> +}
> >> +
> >> +int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point,
> >> +                         u64 flags, struct dma_fence **fence)
> > Kerneldoc is missing for this one here.
> It's almost same as drm_syncobj_find_fence, is it still necessary?

Even more reasons, if you do a few links between closely related functions :-)

> > I also think improving the overview comment to explain the timeline stuff,
> > e.g. link to the vulkan extension, and some words on how it's supposed to
> > be used (you need a submit thread and block in userspace until all future
> > timeline points have materialized, only then you can submit to the
> > kernel).
> >
> >> +{
> >> +    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
> >> @@ -228,7 +432,7 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
> >>    * @fence: out parameter for the fence
> >>    *
> >>    * This is just a convenience function that combines drm_syncobj_find() and
> >> - * drm_syncobj_fence_get().
> >> + * drm_syncobj_lookup_fence().
> >>    *
> >>    * Returns 0 on success or a negative error value on failure. On success @fence
> >>    * contains a reference to the fence, which must be released by calling
> >> @@ -239,15 +443,9 @@ 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 = 0;
> >> -
> >> -    if (!syncobj)
> >> -            return -ENOENT;
> >> +    int ret;
> >>
> >> -    *fence = drm_syncobj_fence_get(syncobj);
> >> -    if (!*fence) {
> >> -            ret = -EINVAL;
> >> -    }
> >> +    ret = drm_syncobj_search_fence(syncobj, point, flags, fence);
> >>      drm_syncobj_put(syncobj);
> >>      return ret;
> >>   }
> >> @@ -264,7 +462,7 @@ void drm_syncobj_free(struct kref *kref)
> >>      struct drm_syncobj *syncobj = container_of(kref,
> >>                                                 struct drm_syncobj,
> >>                                                 refcount);
> >> -    drm_syncobj_replace_fence(syncobj, 0, NULL);
> >> +    drm_syncobj_fini(syncobj);
> >>      kfree(syncobj);
> >>   }
> >>   EXPORT_SYMBOL(drm_syncobj_free);
> >> @@ -294,6 +492,11 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
> >>      kref_init(&syncobj->refcount);
> >>      INIT_LIST_HEAD(&syncobj->cb_list);
> >>      spin_lock_init(&syncobj->lock);
> >> +    if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
> >> +            syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
> >> +    else
> >> +            syncobj->type = DRM_SYNCOBJ_TYPE_BINARY;
> >> +    drm_syncobj_init(syncobj);
> >>
> >>      if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
> >>              ret = drm_syncobj_assign_null_handle(syncobj);
> >> @@ -576,7 +779,8 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
> >>              return -ENODEV;
> >>
> >>      /* no valid flags yet */
> >> -    if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
> >> +    if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
> >> +                        DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
> >>              return -EINVAL;
> >>
> >>      return drm_syncobj_create_as_handle(file_private,
> >> @@ -669,9 +873,8 @@ static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
> >>      struct syncobj_wait_entry *wait =
> >>              container_of(cb, struct syncobj_wait_entry, syncobj_cb);
> >>
> >> -    /* This happens inside the syncobj lock */
> >> -    wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
> >> -                                                          lockdep_is_held(&syncobj->lock)));
> >> +    drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
> >> +
> >>      wake_up_process(wait->task);
> >>   }
> >>
> >> @@ -698,7 +901,8 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> >>      signaled_count = 0;
> >>      for (i = 0; i < count; ++i) {
> >>              entries[i].task = current;
> >> -            entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
> >> +            ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
> >> +                                           &entries[i].fence);
> >>              if (!entries[i].fence) {
> >>                      if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
> >>                              continue;
> >> @@ -970,12 +1174,13 @@ drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
> >>      if (ret < 0)
> >>              return ret;
> >>
> >> -    for (i = 0; i < args->count_handles; i++)
> >> -            drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
> >> -
> >> +    for (i = 0; i < args->count_handles; i++) {
> >> +            drm_syncobj_fini(syncobjs[i]);
> >> +            drm_syncobj_init(syncobjs[i]);
> >> +    }
> >>      drm_syncobj_array_free(syncobjs, args->count_handles);
> >>
> >> -    return 0;
> >> +    return ret;
> >>   }
> >>
> >>   int
> >> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >> index 0a8d2d64f380..8a8d21b24119 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> >> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
> >>              if (!(flags & I915_EXEC_FENCE_WAIT))
> >>                      continue;
> >>
> >> -            fence = drm_syncobj_fence_get(syncobj);
> >> +            drm_syncobj_search_fence(syncobj, 0, 0, &fence);
> >>              if (!fence)
> >>                      return -EINVAL;
> >>
> >> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> >> index 2eda44def639..85b36d4e53ee 100644
> >> --- a/include/drm/drm_syncobj.h
> >> +++ b/include/drm/drm_syncobj.h
> >> @@ -30,6 +30,11 @@
> >>
> >>   struct drm_syncobj_cb;
> >>
> >> +enum drm_syncobj_type {
> >> +    DRM_SYNCOBJ_TYPE_BINARY,
> >> +    DRM_SYNCOBJ_TYPE_TIMELINE
> >> +};
> >> +
> >>   /**
> >>    * struct drm_syncobj - sync object.
> >>    *
> > The kerneldoc for this also isn't accurate anymore.
> Can kerneldoc be a separate patch?

You already update the kerneldoc, but not completely in this patch.
That doesn't make sense to split up.

> > With the kernel-doc polished:
> >
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Thanks at least.
>
> >
> > But I think we definitely want a full ack for the uapi from radv/anv guys
> > (plus the userspace for either of those).
> Dave and Bas are right people, right?

For radv yes, for anv we want Jason Ekstrand I think.
-Daniel

>
> Thanks,
> David
> > -Daniel
> >
> >> @@ -41,19 +46,36 @@ struct drm_syncobj {
> >>       */
> >>      struct kref refcount;
> >>      /**
> >> -     * @fence:
> >> -     * NULL or a pointer to the fence bound to this object.
> >> -     *
> >> -     * This field should not be used directly. Use drm_syncobj_fence_get()
> >> -     * and drm_syncobj_replace_fence() instead.
> >> +     * @type: indicate syncobj type
> >> +     */
> >> +    enum drm_syncobj_type type;
> >> +    /**
> >> +     * @wq: wait signal operation work queue
> >> +     */
> >> +    wait_queue_head_t       wq;
> >> +    /**
> >> +     * @timeline_context: fence context used by timeline
> >>       */
> >> -    struct dma_fence __rcu *fence;
> >> +    u64 timeline_context;
> >>      /**
> >> -     * @cb_list: List of callbacks to call when the &fence gets replaced.
> >> +     * @timeline: syncobj timeline value, which indicates point is signaled.
> >>       */
> >> +    u64 timeline;
> >> +    /**
> >> +     * @signal_point: which indicates the latest signaler point.
> >> +     */
> >> +    u64 signal_point;
> >> +    /**
> >> +     * @signal_pt_list: signaler point list.
> >> +     */
> >> +    struct list_head signal_pt_list;
> >> +
> >> +    /**
> >> +         * @cb_list: List of callbacks to call when the &fence gets replaced.
> >> +         */
> >>      struct list_head cb_list;
> >>      /**
> >> -     * @lock: Protects &cb_list and write-locks &fence.
> >> +     * @lock: Protects syncobj list and write-locks &fence.
> >>       */
> >>      spinlock_t lock;
> >>      /**
> >> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
> >>   /**
> >>    * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
> >>    * @node: used by drm_syncob_add_callback to append this struct to
> >> - *    &drm_syncobj.cb_list
> >> + *       &drm_syncobj.cb_list
> >>    * @func: drm_syncobj_func_t to call
> >>    *
> >>    * This struct will be initialized by drm_syncobj_add_callback, additional
> >> @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj *obj)
> >>      kref_put(&obj->refcount, drm_syncobj_free);
> >>   }
> >>
> >> -/**
> >> - * drm_syncobj_fence_get - get a reference to a fence in a sync object
> >> - * @syncobj: sync object.
> >> - *
> >> - * This acquires additional reference to &drm_syncobj.fence contained in @obj,
> >> - * if not NULL. It is illegal to call this without already holding a reference.
> >> - * No locks required.
> >> - *
> >> - * Returns:
> >> - * Either the fence of @obj or NULL if there's none.
> >> - */
> >> -static inline struct dma_fence *
> >> -drm_syncobj_fence_get(struct drm_syncobj *syncobj)
> >> -{
> >> -    struct dma_fence *fence;
> >> -
> >> -    rcu_read_lock();
> >> -    fence = dma_fence_get_rcu_safe(&syncobj->fence);
> >> -    rcu_read_unlock();
> >> -
> >> -    return fence;
> >> -}
> >> -
> >>   struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
> >>                                   u32 handle);
> >>   void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 point,
> >> @@ -142,5 +141,7 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
> >>   int drm_syncobj_get_handle(struct drm_file *file_private,
> >>                         struct drm_syncobj *syncobj, u32 *handle);
> >>   int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
> >> +int drm_syncobj_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 300f336633f2..cebdb2541eb7 100644
> >> --- a/include/uapi/drm/drm.h
> >> +++ b/include/uapi/drm/drm.h
> >> @@ -717,6 +717,7 @@ struct drm_prime_handle {
> >>   struct drm_syncobj_create {
> >>      __u32 handle;
> >>   #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
> >> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
> >>      __u32 flags;
> >>   };
> >>
> >> --
> >> 2.17.1
> >>
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[zhoucm1]

+Jason as well.


On 2018年10月17日 18:22, Daniel Vetter wrote:
> On Wed, Oct 17, 2018 at 11:17 AM zhoucm1 <zhoucm1@amd.com> wrote:
>>
>>
>> On 2018年10月17日 16:09, Daniel Vetter wrote:
>>> On Mon, Oct 15, 2018 at 04:55:48PM +0800, Chunming Zhou wrote:
>>>> This patch is for VK_KHR_timeline_semaphore extension, semaphore is called syncobj in kernel side:
>>>> This extension introduces a new type of syncobj that has an integer payload
>>>> identifying a point in a timeline. Such timeline syncobjs support the
>>>> following operations:
>>>>      * CPU query - A host operation that allows querying the payload of the
>>>>        timeline syncobj.
>>>>      * CPU wait - A host operation that allows a blocking wait for a
>>>>        timeline syncobj to reach a specified value.
>>>>      * Device wait - A device operation that allows waiting for a
>>>>        timeline syncobj to reach a specified value.
>>>>      * Device signal - A device operation that allows advancing the
>>>>        timeline syncobj to a specified value.
>>>>
>>>> v1:
>>>> Since it's a timeline, that means the front time point(PT) always is signaled before the late PT.
>>>> a. signal PT design:
>>>> Signal PT fence N depends on PT[N-1] fence and signal opertion fence, when PT[N] fence is signaled,
>>>> the timeline will increase to value of PT[N].
>>>> b. wait PT design:
>>>> Wait PT fence is signaled by reaching timeline point value, when timeline is increasing, will compare
>>>> wait PTs value with new timeline value, if PT value is lower than timeline value, then wait PT will be
>>>> signaled, otherwise keep in list. syncobj wait operation can wait on any point of timeline,
>>>> so need a RB tree to order them. And wait PT could ahead of signal PT, we need a sumission fence to
>>>> perform that.
>>>>
>>>> v2:
>>>> 1. remove unused DRM_SYNCOBJ_CREATE_TYPE_NORMAL. (Christian)
>>>> 2. move unexposed denitions to .c file. (Daniel Vetter)
>>>> 3. split up the change to drm_syncobj_find_fence() in a separate patch. (Christian)
>>>> 4. split up the change to drm_syncobj_replace_fence() in a separate patch.
>>>> 5. drop the submission_fence implementation and instead use wait_event() for that. (Christian)
>>>> 6. WARN_ON(point != 0) for NORMAL type syncobj case. (Daniel Vetter)
>>>>
>>>> v3:
>>>> 1. replace normal syncobj with timeline implemenation. (Vetter and Christian)
>>>>       a. normal syncobj signal op will create a signal PT to tail of signal pt list.
>>>>       b. normal syncobj wait op will create a wait pt with last signal point, and this wait PT is only signaled by related signal point PT.
>>>> 2. many bug fix and clean up
>>>> 3. stub fence moving is moved to other patch.
>>>>
>>>> v4：
>>>> 1. fix RB tree loop with while(node=rb_first(...)). (Christian)
>>>> 2. fix syncobj lifecycle. (Christian)
>>>> 3. only enable_signaling when there is wait_pt. (Christian)
>>>> 4. fix timeline path issues.
>>>> 5. write a timeline test in libdrm
>>>>
>>>> v5: (Christian)
>>>> 1. semaphore is called syncobj in kernel side.
>>>> 2. don't need 'timeline' characters in some function name.
>>>> 3. keep syncobj cb.
>>>>
>>>> v6: (Christian)
>>>> 1. merge syncobj_timeline to syncobj structure.
>>>> 2. simplify some check sentences.
>>>> 3. some misc change.
>>>> 4. fix CTS failed issue.
>>>>
>>>> v7: (Christian)
>>>> 1. error handling when creating signal pt.
>>>> 2. remove timeline naming in func.
>>>> 3. export flags in find_fence.
>>>> 4. allow reset timeline.
>>>>
>>>> v8:
>>>> 1. use wait_event_interruptible without timeout
>>>> 2. rename _TYPE_INDIVIDUAL to _TYPE_BINARY
>>>>
>>>> individual syncobj is tested by ./deqp-vk -n dEQP-VK*semaphore*
>>>> timeline syncobj is tested by ./amdgpu_test -s 9
>>> Can we please have these low-level syncobj tests as part of igt, together
>>> with all the other syncobj tests which are there already?
>> Good suggestion first, I'm just not familiar with igt( build, run
>> cmd...), maybe we can add it later.
>>
>>> Really doesn't
>>> make much sense imo to splits things on the test suite front.
>>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>>> Cc: Christian Konig <christian.koenig@amd.com>
>>>> Cc: Dave Airlie <airlied@redhat.com>
>>>> Cc: Daniel Rakos <Daniel.Rakos@amd.com>
>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_syncobj.c              | 287 ++++++++++++++++++---
>>>>    drivers/gpu/drm/i915/i915_gem_execbuffer.c |   2 +-
>>>>    include/drm/drm_syncobj.h                  |  65 ++---
>>>>    include/uapi/drm/drm.h                     |   1 +
>>>>    4 files changed, 281 insertions(+), 74 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>>>> index f796c9fc3858..67472bd77c83 100644
>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>> @@ -56,6 +56,9 @@
>>>>    #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;
>>>> @@ -82,6 +85,11 @@ static const struct dma_fence_ops drm_syncobj_stub_fence_ops = {
>>>>       .release = drm_syncobj_stub_fence_release,
>>>>    };
>>>>
>>>> +struct drm_syncobj_signal_pt {
>>>> +    struct dma_fence_array *base;
>>> Out of curiosity, why the pointer and not embedding? base is kinda
>>> misleading for a pointer.
>> Yeah, Christian doesn't like signal_pt lifecycle same as fence, so it's
>> a pointer.
>> If you don't like 'base' name, I can change it.
>>
>>>> +    u64    value;
>>>> +    struct list_head list;
>>>> +};
>>>>
>>>>    /**
>>>>     * drm_syncobj_find - lookup and reference a sync object.
>>>> @@ -124,8 +132,8 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
>>>>    {
>>>>       int ret;
>>>>
>>>> -    *fence = drm_syncobj_fence_get(syncobj);
>>>> -    if (*fence)
>>>> +    ret = drm_syncobj_search_fence(syncobj, 0, 0, fence);
>>>> +    if (!ret)
>>>>               return 1;
>>>>
>>>>       spin_lock(&syncobj->lock);
>>>> @@ -133,10 +141,12 @@ 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 (syncobj->fence) {
>>>> -            *fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
>>>> -                                                             lockdep_is_held(&syncobj->lock)));
>>>> -            ret = 1;
>>>> +    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);
>>>>       } else {
>>>>               *fence = NULL;
>>>>               drm_syncobj_add_callback_locked(syncobj, cb, func);
>>>> @@ -164,6 +174,159 @@ 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->base->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->base->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->base->base);
>>>> +            }
>>>> +            spin_unlock(&syncobj->lock);
>>>> +    }
>>>> +    signal_pt->base = dma_fence_array_create(num_fences, fences,
>>>> +                                             syncobj->timeline_context,
>>>> +                                             point, false);
>>>> +    if (!signal_pt->base) {
>>>> +            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->base->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->base->base)) {
>>>> +                    syncobj->timeline = signal_pt->value;
>>>> +                    list_del(&signal_pt->list);
>>>> +                    dma_fence_put(&signal_pt->base->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
>>>> @@ -176,28 +339,29 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>>>>                              u64 point,
>>>>                              struct dma_fence *fence)
>>>>    {
>>>> -    struct dma_fence *old_fence;
>>>> -    struct drm_syncobj_cb *cur, *tmp;
>>>> -
>>>> -    if (fence)
>>>> -            dma_fence_get(fence);
>>>> -
>>>> -    spin_lock(&syncobj->lock);
>>>> -
>>>> -    old_fence = rcu_dereference_protected(syncobj->fence,
>>>> -                                          lockdep_is_held(&syncobj->lock));
>>>> -    rcu_assign_pointer(syncobj->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;
>>>>
>>>> -    if (fence != old_fence) {
>>>> +            spin_lock(&syncobj->lock);
>>>>               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);
>>>>
>>>> @@ -220,6 +384,46 @@ 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;
>>>> +}
>>>> +
>>>> +int drm_syncobj_search_fence(struct drm_syncobj *syncobj, u64 point,
>>>> +                         u64 flags, struct dma_fence **fence)
>>> Kerneldoc is missing for this one here.
>> It's almost same as drm_syncobj_find_fence, is it still necessary?
> Even more reasons, if you do a few links between closely related functions :-)
>
>>> I also think improving the overview comment to explain the timeline stuff,
>>> e.g. link to the vulkan extension, and some words on how it's supposed to
>>> be used (you need a submit thread and block in userspace until all future
>>> timeline points have materialized, only then you can submit to the
>>> kernel).
>>>
>>>> +{
>>>> +    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
>>>> @@ -228,7 +432,7 @@ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
>>>>     * @fence: out parameter for the fence
>>>>     *
>>>>     * This is just a convenience function that combines drm_syncobj_find() and
>>>> - * drm_syncobj_fence_get().
>>>> + * drm_syncobj_lookup_fence().
>>>>     *
>>>>     * Returns 0 on success or a negative error value on failure. On success @fence
>>>>     * contains a reference to the fence, which must be released by calling
>>>> @@ -239,15 +443,9 @@ 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 = 0;
>>>> -
>>>> -    if (!syncobj)
>>>> -            return -ENOENT;
>>>> +    int ret;
>>>>
>>>> -    *fence = drm_syncobj_fence_get(syncobj);
>>>> -    if (!*fence) {
>>>> -            ret = -EINVAL;
>>>> -    }
>>>> +    ret = drm_syncobj_search_fence(syncobj, point, flags, fence);
>>>>       drm_syncobj_put(syncobj);
>>>>       return ret;
>>>>    }
>>>> @@ -264,7 +462,7 @@ void drm_syncobj_free(struct kref *kref)
>>>>       struct drm_syncobj *syncobj = container_of(kref,
>>>>                                                  struct drm_syncobj,
>>>>                                                  refcount);
>>>> -    drm_syncobj_replace_fence(syncobj, 0, NULL);
>>>> +    drm_syncobj_fini(syncobj);
>>>>       kfree(syncobj);
>>>>    }
>>>>    EXPORT_SYMBOL(drm_syncobj_free);
>>>> @@ -294,6 +492,11 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
>>>>       kref_init(&syncobj->refcount);
>>>>       INIT_LIST_HEAD(&syncobj->cb_list);
>>>>       spin_lock_init(&syncobj->lock);
>>>> +    if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE)
>>>> +            syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE;
>>>> +    else
>>>> +            syncobj->type = DRM_SYNCOBJ_TYPE_BINARY;
>>>> +    drm_syncobj_init(syncobj);
>>>>
>>>>       if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
>>>>               ret = drm_syncobj_assign_null_handle(syncobj);
>>>> @@ -576,7 +779,8 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
>>>>               return -ENODEV;
>>>>
>>>>       /* no valid flags yet */
>>>> -    if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED)
>>>> +    if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED |
>>>> +                        DRM_SYNCOBJ_CREATE_TYPE_TIMELINE))
>>>>               return -EINVAL;
>>>>
>>>>       return drm_syncobj_create_as_handle(file_private,
>>>> @@ -669,9 +873,8 @@ static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
>>>>       struct syncobj_wait_entry *wait =
>>>>               container_of(cb, struct syncobj_wait_entry, syncobj_cb);
>>>>
>>>> -    /* This happens inside the syncobj lock */
>>>> -    wait->fence = dma_fence_get(rcu_dereference_protected(syncobj->fence,
>>>> -                                                          lockdep_is_held(&syncobj->lock)));
>>>> +    drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
>>>> +
>>>>       wake_up_process(wait->task);
>>>>    }
>>>>
>>>> @@ -698,7 +901,8 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>>>>       signaled_count = 0;
>>>>       for (i = 0; i < count; ++i) {
>>>>               entries[i].task = current;
>>>> -            entries[i].fence = drm_syncobj_fence_get(syncobjs[i]);
>>>> +            ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
>>>> +                                           &entries[i].fence);
>>>>               if (!entries[i].fence) {
>>>>                       if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>>>>                               continue;
>>>> @@ -970,12 +1174,13 @@ drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
>>>>       if (ret < 0)
>>>>               return ret;
>>>>
>>>> -    for (i = 0; i < args->count_handles; i++)
>>>> -            drm_syncobj_replace_fence(syncobjs[i], 0, NULL);
>>>> -
>>>> +    for (i = 0; i < args->count_handles; i++) {
>>>> +            drm_syncobj_fini(syncobjs[i]);
>>>> +            drm_syncobj_init(syncobjs[i]);
>>>> +    }
>>>>       drm_syncobj_array_free(syncobjs, args->count_handles);
>>>>
>>>> -    return 0;
>>>> +    return ret;
>>>>    }
>>>>
>>>>    int
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>> index 0a8d2d64f380..8a8d21b24119 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>> @@ -2137,7 +2137,7 @@ await_fence_array(struct i915_execbuffer *eb,
>>>>               if (!(flags & I915_EXEC_FENCE_WAIT))
>>>>                       continue;
>>>>
>>>> -            fence = drm_syncobj_fence_get(syncobj);
>>>> +            drm_syncobj_search_fence(syncobj, 0, 0, &fence);
>>>>               if (!fence)
>>>>                       return -EINVAL;
>>>>
>>>> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
>>>> index 2eda44def639..85b36d4e53ee 100644
>>>> --- a/include/drm/drm_syncobj.h
>>>> +++ b/include/drm/drm_syncobj.h
>>>> @@ -30,6 +30,11 @@
>>>>
>>>>    struct drm_syncobj_cb;
>>>>
>>>> +enum drm_syncobj_type {
>>>> +    DRM_SYNCOBJ_TYPE_BINARY,
>>>> +    DRM_SYNCOBJ_TYPE_TIMELINE
>>>> +};
>>>> +
>>>>    /**
>>>>     * struct drm_syncobj - sync object.
>>>>     *
>>> The kerneldoc for this also isn't accurate anymore.
>> Can kerneldoc be a separate patch?
> You already update the kerneldoc, but not completely in this patch.
> That doesn't make sense to split up.
>
>>> With the kernel-doc polished:
>>>
>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Thanks at least.
>>
>>> But I think we definitely want a full ack for the uapi from radv/anv guys
>>> (plus the userspace for either of those).
>> Dave and Bas are right people, right?
> For radv yes, for anv we want Jason Ekstrand I think.
> -Daniel
>
>> Thanks,
>> David
>>> -Daniel
>>>
>>>> @@ -41,19 +46,36 @@ struct drm_syncobj {
>>>>        */
>>>>       struct kref refcount;
>>>>       /**
>>>> -     * @fence:
>>>> -     * NULL or a pointer to the fence bound to this object.
>>>> -     *
>>>> -     * This field should not be used directly. Use drm_syncobj_fence_get()
>>>> -     * and drm_syncobj_replace_fence() instead.
>>>> +     * @type: indicate syncobj type
>>>> +     */
>>>> +    enum drm_syncobj_type type;
>>>> +    /**
>>>> +     * @wq: wait signal operation work queue
>>>> +     */
>>>> +    wait_queue_head_t       wq;
>>>> +    /**
>>>> +     * @timeline_context: fence context used by timeline
>>>>        */
>>>> -    struct dma_fence __rcu *fence;
>>>> +    u64 timeline_context;
>>>>       /**
>>>> -     * @cb_list: List of callbacks to call when the &fence gets replaced.
>>>> +     * @timeline: syncobj timeline value, which indicates point is signaled.
>>>>        */
>>>> +    u64 timeline;
>>>> +    /**
>>>> +     * @signal_point: which indicates the latest signaler point.
>>>> +     */
>>>> +    u64 signal_point;
>>>> +    /**
>>>> +     * @signal_pt_list: signaler point list.
>>>> +     */
>>>> +    struct list_head signal_pt_list;
>>>> +
>>>> +    /**
>>>> +         * @cb_list: List of callbacks to call when the &fence gets replaced.
>>>> +         */
>>>>       struct list_head cb_list;
>>>>       /**
>>>> -     * @lock: Protects &cb_list and write-locks &fence.
>>>> +     * @lock: Protects syncobj list and write-locks &fence.
>>>>        */
>>>>       spinlock_t lock;
>>>>       /**
>>>> @@ -68,7 +90,7 @@ typedef void (*drm_syncobj_func_t)(struct drm_syncobj *syncobj,
>>>>    /**
>>>>     * struct drm_syncobj_cb - callback for drm_syncobj_add_callback
>>>>     * @node: used by drm_syncob_add_callback to append this struct to
>>>> - *    &drm_syncobj.cb_list
>>>> + *       &drm_syncobj.cb_list
>>>>     * @func: drm_syncobj_func_t to call
>>>>     *
>>>>     * This struct will be initialized by drm_syncobj_add_callback, additional
>>>> @@ -106,29 +128,6 @@ drm_syncobj_put(struct drm_syncobj *obj)
>>>>       kref_put(&obj->refcount, drm_syncobj_free);
>>>>    }
>>>>
>>>> -/**
>>>> - * drm_syncobj_fence_get - get a reference to a fence in a sync object
>>>> - * @syncobj: sync object.
>>>> - *
>>>> - * This acquires additional reference to &drm_syncobj.fence contained in @obj,
>>>> - * if not NULL. It is illegal to call this without already holding a reference.
>>>> - * No locks required.
>>>> - *
>>>> - * Returns:
>>>> - * Either the fence of @obj or NULL if there's none.
>>>> - */
>>>> -static inline struct dma_fence *
>>>> -drm_syncobj_fence_get(struct drm_syncobj *syncobj)
>>>> -{
>>>> -    struct dma_fence *fence;
>>>> -
>>>> -    rcu_read_lock();
>>>> -    fence = dma_fence_get_rcu_safe(&syncobj->fence);
>>>> -    rcu_read_unlock();
>>>> -
>>>> -    return fence;
>>>> -}
>>>> -
>>>>    struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>>>>                                    u32 handle);
>>>>    void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, u64 point,
>>>> @@ -142,5 +141,7 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
>>>>    int drm_syncobj_get_handle(struct drm_file *file_private,
>>>>                          struct drm_syncobj *syncobj, u32 *handle);
>>>>    int drm_syncobj_get_fd(struct drm_syncobj *syncobj, int *p_fd);
>>>> +int drm_syncobj_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 300f336633f2..cebdb2541eb7 100644
>>>> --- a/include/uapi/drm/drm.h
>>>> +++ b/include/uapi/drm/drm.h
>>>> @@ -717,6 +717,7 @@ struct drm_prime_handle {
>>>>    struct drm_syncobj_create {
>>>>       __u32 handle;
>>>>    #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
>>>> +#define DRM_SYNCOBJ_CREATE_TYPE_TIMELINE (1 << 1)
>>>>       __u32 flags;
>>>>    };
>>>>
>>>> --
>>>> 2.17.1
>>>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/7] drm: add syncobj timeline support v8

[Chris Wilson]

Quoting Chunming Zhou (2018-10-15 09:55:48)
> This patch is for VK_KHR_timeline_semaphore extension, semaphore is called syncobj in kernel side:
> This extension introduces a new type of syncobj that has an integer payload
> identifying a point in a timeline. Such timeline syncobjs support the
> following operations:
>    * CPU query - A host operation that allows querying the payload of the
>      timeline syncobj.
>    * CPU wait - A host operation that allows a blocking wait for a
>      timeline syncobj to reach a specified value.
>    * Device wait - A device operation that allows waiting for a
>      timeline syncobj to reach a specified value.
>    * Device signal - A device operation that allows advancing the
>      timeline syncobj to a specified value.


Spinlock recursion:

https://bugs.freedesktop.org/show_bug.cgi?id=108490
<6> [363.585915] [IGT] syncobj_wait: executing
<6> [363.646916] [IGT] syncobj_wait: starting subtest wait-for-submit-delayed-submit
<4> [363.752517]
<4> [363.752542] ============================================
<4> [363.752561] WARNING: possible recursive locking detected
<4> [363.752582] 4.19.0-rc8-CI-CI_DRM_5006+ #1 Tainted: G U
<4> [363.752602] --------------------------------------------
<4> [363.752621] syncobj_wait/2297 is trying to acquire lock:
<4> [363.752641] 0000000029228054 (&(&syncobj->lock)->rlock){+.+.}, at: drm_syncobj_garbage_collection+0x25/0x140
<4> [363.752699] \x0abut task is already holding lock:
<4> [363.752721] 0000000029228054 (&(&syncobj->lock)->rlock){+.+.}, at: drm_syncobj_replace_fence+0x1b6/0x2c0
<4> [363.752768] \x0aother info that might help us debug this:
<4> [363.752791] Possible unsafe locking scenario:\x0a
<4> [363.752812] CPU0
<4> [363.752824] ----
<4> [363.752835] lock(&(&syncobj->lock)->rlock);
<4> [363.752856] lock(&(&syncobj->lock)->rlock);
<4> [363.752877] \x0a *** DEADLOCK ***\x0a
<4> [363.752905] May be due to missing lock nesting notation\x0a
<4> [363.752932] 1 lock held by syncobj_wait/2297:
<4> [363.752949] #0: 0000000029228054 (&(&syncobj->lock)->rlock){+.+.}, at: drm_syncobj_replace_fence+0x1b6/0x2c0
<4> [363.753001] \x0astack backtrace:
<4> [363.753030] CPU: 2 PID: 2297 Comm: syncobj_wait Tainted: G U 4.19.0-rc8-CI-CI_DRM_5006+ #1
<4> [363.753060] Hardware name: Google Caroline/Caroline, BIOS MrChromebox 08/27/2018
<4> [363.753083] Call Trace:
<4> [363.753109] dump_stack+0x67/0x9b
<4> [363.753136] __lock_acquire+0xc67/0x1b50
<4> [363.753164] ? deactivate_slab.isra.26+0x7a4/0x7e0
<4> [363.753203] ? __lock_acquire+0x3c8/0x1b50
<4> [363.753242] ? __wake_up_common_lock+0x5e/0xb0
<4> [363.753285] ? lock_acquire+0xa6/0x1c0
<4> [363.753306] lock_acquire+0xa6/0x1c0
<4> [363.753334] ? drm_syncobj_garbage_collection+0x25/0x140
<4> [363.753365] _raw_spin_lock+0x2a/0x40
<4> [363.753392] ? drm_syncobj_garbage_collection+0x25/0x140
<4> [363.753419] drm_syncobj_garbage_collection+0x25/0x140
<4> [363.753450] drm_syncobj_search_fence+0x38/0x200
<4> [363.753478] ? drm_syncobj_replace_fence+0x1b6/0x2c0
<4> [363.753509] syncobj_wait_syncobj_func+0x11/0x20
<4> [363.753536] drm_syncobj_replace_fence+0x1f8/0x2c0
<4> [363.753561] ? drm_syncobj_handle_to_fd_ioctl+0x170/0x170
<4> [363.753585] drm_syncobj_fd_to_handle_ioctl+0x120/0x1d0
<4> [363.753609] ? drm_syncobj_handle_to_fd_ioctl+0x170/0x170
<4> [363.753638] drm_ioctl_kernel+0x81/0xf0
<4> [363.753665] drm_ioctl+0x2e6/0x3a0
<4> [363.753686] ? drm_syncobj_handle_to_fd_ioctl+0x170/0x170
<4> [363.753717] ? lock_acquire+0xa6/0x1c0
<4> [363.753743] do_vfs_ioctl+0xa0/0x6d0
<4> [363.753769] ? __fget+0xfc/0x1e0
<4> [363.753792] ksys_ioctl+0x35/0x60
<4> [363.753816] __x64_sys_ioctl+0x11/0x20
<4> [363.753840] do_syscall_64+0x55/0x190
<4> [363.753866] entry_SYSCALL_64_after_hwframe+0x49/0xbe
<4> [363.753890] RIP: 0033:0x7f0d84f9a5d7
<4> [363.753911] Code: b3 66 90 48 8b 05 b1 48 2d 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 81 48 2d 00 f7 d8 64 89 01 48
<4> [363.753959] RSP: 002b:00007f0d79804ac8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
<4> [363.753990] RAX: ffffffffffffffda RBX: 00007f0d74000b20 RCX: 00007f0d84f9a5d7
<4> [363.754016] RDX: 00007f0d79804b50 RSI: 00000000c01064c2 RDI: 0000000000000005
<4> [363.754040] RBP: 00007f0d79804b50 R08: 0000000000000000 R09: 000000000000001e
<4> [363.754064] R10: 0000000000000054 R11: 0000000000000246 R12: 00000000c01064c2
<4> [363.754088] R13: 0000000000000005 R14: 00007f0d74000b20 R15: 000055ab44e2c3f8
-Chris
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/7] drm: add support of syncobj timeline point wait v2

[Chunming Zhou]

points array is one-to-one match with syncobjs array.
v2:
add seperate ioctl for timeline point wait, otherwise break uapi.
v3:
userspace can specify two kinds waits::
a. Wait for time point to be completed.
b. and wait for time point to become available

Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
 drivers/gpu/drm/drm_internal.h |   2 +
 drivers/gpu/drm/drm_ioctl.c    |   2 +
 drivers/gpu/drm/drm_syncobj.c  | 118 ++++++++++++++++++++++++++++-----
 include/uapi/drm/drm.h         |  18 +++++
 4 files changed, 124 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 0c4eb4a9ab31..566d44e3c782 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -183,6 +183,8 @@ int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file_private);
 int drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
 			   struct drm_file *file_private);
+int drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
+				    struct drm_file *file_private);
 int drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
 			    struct drm_file *file_private);
 int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 6b4a633b4240..c0891614f516 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -669,6 +669,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_syncobj_wait_ioctl,
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT, drm_syncobj_timeline_wait_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_RESET, drm_syncobj_reset_ioctl,
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, drm_syncobj_signal_ioctl,
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 67472bd77c83..f3f11ac2ef28 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -126,13 +126,14 @@ static void drm_syncobj_add_callback_locked(struct drm_syncobj *syncobj,
 }
 
 static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
+						 u64 point,
 						 struct dma_fence **fence,
 						 struct drm_syncobj_cb *cb,
 						 drm_syncobj_func_t func)
 {
 	int ret;
 
-	ret = drm_syncobj_search_fence(syncobj, 0, 0, fence);
+	ret = drm_syncobj_search_fence(syncobj, point, 0, fence);
 	if (!ret)
 		return 1;
 
@@ -143,7 +144,7 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
 	 */
 	if (!list_empty(&syncobj->signal_pt_list)) {
 		spin_unlock(&syncobj->lock);
-		drm_syncobj_search_fence(syncobj, 0, 0, fence);
+		drm_syncobj_search_fence(syncobj, point, 0, fence);
 		if (*fence)
 			return 1;
 		spin_lock(&syncobj->lock);
@@ -354,13 +355,17 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
 	drm_syncobj_create_signal_pt(syncobj, fence, pt_value);
 	if (fence) {
 		struct drm_syncobj_cb *cur, *tmp;
+		struct list_head cb_list;
+
+		INIT_LIST_HEAD(&cb_list);
 
 		spin_lock(&syncobj->lock);
-		list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
+		list_splice_init(&syncobj->cb_list, &cb_list);
+		spin_unlock(&syncobj->lock);
+		list_for_each_entry_safe(cur, tmp, &cb_list, node) {
 			list_del_init(&cur->node);
 			cur->func(syncobj, cur);
 		}
-		spin_unlock(&syncobj->lock);
 	}
 }
 EXPORT_SYMBOL(drm_syncobj_replace_fence);
@@ -856,6 +861,7 @@ struct syncobj_wait_entry {
 	struct dma_fence *fence;
 	struct dma_fence_cb fence_cb;
 	struct drm_syncobj_cb syncobj_cb;
+	u64    point;
 };
 
 static void syncobj_wait_fence_func(struct dma_fence *fence,
@@ -873,12 +879,13 @@ static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
 	struct syncobj_wait_entry *wait =
 		container_of(cb, struct syncobj_wait_entry, syncobj_cb);
 
-	drm_syncobj_search_fence(syncobj, 0, 0, &wait->fence);
+	drm_syncobj_search_fence(syncobj, wait->point, 0, &wait->fence);
 
 	wake_up_process(wait->task);
 }
 
 static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
+						  void __user *user_points,
 						  uint32_t count,
 						  uint32_t flags,
 						  signed long timeout,
@@ -886,13 +893,38 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 {
 	struct syncobj_wait_entry *entries;
 	struct dma_fence *fence;
+	uint64_t *points;
 	signed long ret;
 	uint32_t signaled_count, i;
 
-	entries = kcalloc(count, sizeof(*entries), GFP_KERNEL);
-	if (!entries)
+	points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
+	if (points == NULL)
 		return -ENOMEM;
 
+	if (!user_points) {
+		memset(points, 0, count * sizeof(uint64_t));
+	} else if (copy_from_user(points, user_points, sizeof(uint64_t) * count)) {
+		ret = -EFAULT;
+		goto err_free_points;
+	}
+
+
+	if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE) {
+		struct dma_fence *fence;
+		for (i = 0; i < count; ++i) {
+			ret = drm_syncobj_search_fence(syncobjs[i], points[i],
+						       DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
+						       &fence);
+			if (ret)
+				goto err_free_points;
+		}
+		goto err_free_points;
+	}
+	entries = kcalloc(count, sizeof(*entries), GFP_KERNEL);
+	if (!entries) {
+		ret = -ENOMEM;
+		goto err_free_points;
+	}
 	/* Walk the list of sync objects and initialize entries.  We do
 	 * this up-front so that we can properly return -EINVAL if there is
 	 * a syncobj with a missing fence and then never have the chance of
@@ -901,7 +933,8 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 	signaled_count = 0;
 	for (i = 0; i < count; ++i) {
 		entries[i].task = current;
-		ret = drm_syncobj_search_fence(syncobjs[i], 0, 0,
+		entries[i].point = points[i];
+		ret = drm_syncobj_search_fence(syncobjs[i], points[i], 0,
 					       &entries[i].fence);
 		if (!entries[i].fence) {
 			if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
@@ -940,6 +973,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 	if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
 		for (i = 0; i < count; ++i) {
 			drm_syncobj_fence_get_or_add_callback(syncobjs[i],
+							      entries[i].point,
 							      &entries[i].fence,
 							      &entries[i].syncobj_cb,
 							      syncobj_wait_syncobj_func);
@@ -1003,6 +1037,9 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 	}
 	kfree(entries);
 
+err_free_points:
+	kfree(points);
+
 	return ret;
 }
 
@@ -1041,20 +1078,33 @@ static signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec)
 static int drm_syncobj_array_wait(struct drm_device *dev,
 				  struct drm_file *file_private,
 				  struct drm_syncobj_wait *wait,
-				  struct drm_syncobj **syncobjs)
+				  struct drm_syncobj_timeline_wait *timeline_wait,
+				  struct drm_syncobj **syncobjs, bool timeline)
 {
-	signed long timeout = drm_timeout_abs_to_jiffies(wait->timeout_nsec);
+	signed long timeout = 0;
 	signed long ret = 0;
 	uint32_t first = ~0;
 
-	ret = drm_syncobj_array_wait_timeout(syncobjs,
-					     wait->count_handles,
-					     wait->flags,
-					     timeout, &first);
+	if (!timeline) {
+		timeout = drm_timeout_abs_to_jiffies(wait->timeout_nsec);
+		ret = drm_syncobj_array_wait_timeout(syncobjs,
+						     NULL,
+						     wait->count_handles,
+						     wait->flags,
+						     timeout, &first);
+		wait->first_signaled = first;
+	} else {
+		timeout = drm_timeout_abs_to_jiffies(timeline_wait->timeout_nsec);
+		ret = drm_syncobj_array_wait_timeout(syncobjs,
+						     u64_to_user_ptr(timeline_wait->points),
+						     timeline_wait->count_handles,
+						     timeline_wait->flags,
+						     timeout, &first);
+		timeline_wait->first_signaled = first;
+	}
 	if (ret < 0)
 		return ret;
 
-	wait->first_signaled = first;
 	if (ret == 0)
 		return -ETIME;
 	return 0;
@@ -1142,13 +1192,49 @@ drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
 		return ret;
 
 	ret = drm_syncobj_array_wait(dev, file_private,
-				     args, syncobjs);
+				     args, NULL, syncobjs, false);
+
+	drm_syncobj_array_free(syncobjs, args->count_handles);
+
+	return ret;
+}
+
+int
+drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
+				struct drm_file *file_private)
+{
+	struct drm_syncobj_timeline_wait *args = data;
+	struct drm_syncobj **syncobjs;
+	int ret = 0;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	if (args->flags & ~(DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL |
+			    DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
+			    DRM_SYNCOBJ_WAIT_FLAGS_WAIT_COMPLETED |
+			    DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE))
+		return -EINVAL;
+
+	if (args->count_handles == 0)
+		return -EINVAL;
+
+	ret = drm_syncobj_array_find(file_private,
+				     u64_to_user_ptr(args->handles),
+				     args->count_handles,
+				     &syncobjs);
+	if (ret < 0)
+		return ret;
+
+	ret = drm_syncobj_array_wait(dev, file_private,
+				     NULL, args, syncobjs, true);
 
 	drm_syncobj_array_free(syncobjs, args->count_handles);
 
 	return ret;
 }
 
+
 int
 drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file_private)
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index cebdb2541eb7..c8bc1414753d 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -738,6 +738,10 @@ struct drm_syncobj_handle {
 
 #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
 #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1)
+/* wait for time point to be completed */
+#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_COMPLETED (1 << 2)
+/* wait for time point to become available */
+#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE (1 << 3)
 struct drm_syncobj_wait {
 	__u64 handles;
 	/* absolute timeout */
@@ -748,6 +752,19 @@ struct drm_syncobj_wait {
 	__u32 pad;
 };
 
+struct drm_syncobj_timeline_wait {
+	__u64 handles;
+	/* wait on specific timeline point for every handles*/
+	__u64 points;
+	/* absolute timeout */
+	__s64 timeout_nsec;
+	__u32 count_handles;
+	__u32 flags;
+	__u32 first_signaled; /* only valid when not waiting all */
+	__u32 pad;
+};
+
+
 struct drm_syncobj_array {
 	__u64 handles;
 	__u32 count_handles;
@@ -910,6 +927,7 @@ extern "C" {
 #define DRM_IOCTL_MODE_GET_LEASE	DRM_IOWR(0xC8, struct drm_mode_get_lease)
 #define DRM_IOCTL_MODE_REVOKE_LEASE	DRM_IOWR(0xC9, struct drm_mode_revoke_lease)
 
+#define DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT	DRM_IOWR(0xCA, struct drm_syncobj_timeline_wait)
 /**
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/7] drm: add timeline syncobj payload query ioctl v2

[Chunming Zhou]

user mode can query timeline payload.
v2: check return value of copy_to_user

Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
 drivers/gpu/drm/drm_internal.h |  2 ++
 drivers/gpu/drm/drm_ioctl.c    |  2 ++
 drivers/gpu/drm/drm_syncobj.c  | 52 ++++++++++++++++++++++++++++++++++
 include/uapi/drm/drm.h         | 11 +++++++
 4 files changed, 67 insertions(+)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 566d44e3c782..9c4826411a3c 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -189,6 +189,8 @@ int drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,
 			    struct drm_file *file_private);
 int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
 			     struct drm_file *file_private);
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+			    struct drm_file *file_private);
 
 /* drm_framebuffer.c */
 void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index c0891614f516..c3c0617e6372 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -675,6 +675,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, drm_syncobj_signal_ioctl,
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index f3f11ac2ef28..17124d40532f 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1304,3 +1304,55 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
 
 	return ret;
 }
+
+static void drm_syncobj_timeline_query_payload(struct drm_syncobj *syncobj,
+					       uint64_t *point)
+{
+	if (syncobj->type != DRM_SYNCOBJ_TYPE_TIMELINE) {
+		DRM_ERROR("Normal syncobj cann't be queried!");
+		*point = 0;
+		return;
+	}
+	*point = syncobj->signal_point;
+}
+
+int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
+			    struct drm_file *file_private)
+{
+	struct drm_syncobj_timeline_query *args = data;
+	struct drm_syncobj **syncobjs;
+	uint64_t *points;
+	uint32_t i;
+	int ret;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	if (args->count_handles == 0)
+		return -EINVAL;
+
+	ret = drm_syncobj_array_find(file_private,
+				     u64_to_user_ptr(args->handles),
+				     args->count_handles,
+				     &syncobjs);
+	if (ret < 0)
+		return ret;
+
+
+	points = kmalloc_array(args->count_handles, sizeof(*points),
+			       GFP_KERNEL);
+	if (points == NULL) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	for (i = 0; i < args->count_handles; i++)
+		drm_syncobj_timeline_query_payload(syncobjs[i], &points[i]);
+	ret = copy_to_user(u64_to_user_ptr(args->points), points,
+			   sizeof(uint64_t) * args->count_handles) ? -EFAULT : 0;
+
+	kfree(points);
+out:
+	drm_syncobj_array_free(syncobjs, args->count_handles);
+
+	return ret;
+}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index c8bc1414753d..23c4979d8a1c 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -771,6 +771,15 @@ struct drm_syncobj_array {
 	__u32 pad;
 };
 
+struct drm_syncobj_timeline_query {
+	__u64 handles;
+	/* points are timeline syncobjs payloads returned by query ioctl */
+	__u64 points;
+	__u32 count_handles;
+	__u32 pad;
+};
+
+
 /* Query current scanout sequence number */
 struct drm_crtc_get_sequence {
 	__u32 crtc_id;		/* requested crtc_id */
@@ -928,6 +937,8 @@ extern "C" {
 #define DRM_IOCTL_MODE_REVOKE_LEASE	DRM_IOWR(0xC9, struct drm_mode_revoke_lease)
 
 #define DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT	DRM_IOWR(0xCA, struct drm_syncobj_timeline_wait)
+#define DRM_IOCTL_SYNCOBJ_QUERY		DRM_IOWR(0xCB, struct drm_syncobj_timeline_query)
+
 /**
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/7] drm: add timeline support for syncobj export/import

[Chunming Zhou]

user space can specify timeline point fence to export/import.

Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
 drivers/gpu/drm/drm_internal.h |  4 ++
 drivers/gpu/drm/drm_ioctl.c    |  4 ++
 drivers/gpu/drm/drm_syncobj.c  | 76 ++++++++++++++++++++++++++++++----
 include/uapi/drm/drm.h         | 11 +++++
 4 files changed, 88 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 9c4826411a3c..5ad6cbdb68ab 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -181,6 +181,10 @@ int drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file_private);
 int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file_private);
+int drm_syncobj_handle_to_fd_ioctl2(struct drm_device *dev, void *data,
+				    struct drm_file *file_private);
+int drm_syncobj_fd_to_handle_ioctl2(struct drm_device *dev, void *data,
+				    struct drm_file *file_private);
 int drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
 			   struct drm_file *file_private);
 int drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index c3c0617e6372..b80c2c28aee0 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -667,6 +667,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD2, drm_syncobj_handle_to_fd_ioctl2,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE2, drm_syncobj_fd_to_handle_ioctl2,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_syncobj_wait_ioctl,
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT, drm_syncobj_timeline_wait_ioctl,
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 17124d40532f..aed492570d85 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -683,7 +683,7 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
 }
 
 static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
-					      int fd, int handle)
+					      int fd, int handle, uint64_t point)
 {
 	struct dma_fence *fence = sync_file_get_fence(fd);
 	struct drm_syncobj *syncobj;
@@ -697,14 +697,14 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
 		return -ENOENT;
 	}
 
-	drm_syncobj_replace_fence(syncobj, 0, fence);
+	drm_syncobj_replace_fence(syncobj, point, fence);
 	dma_fence_put(fence);
 	drm_syncobj_put(syncobj);
 	return 0;
 }
 
 static int drm_syncobj_export_sync_file(struct drm_file *file_private,
-					int handle, int *p_fd)
+					int handle, uint64_t point, int *p_fd)
 {
 	int ret;
 	struct dma_fence *fence;
@@ -714,7 +714,7 @@ static int drm_syncobj_export_sync_file(struct drm_file *file_private,
 	if (fd < 0)
 		return fd;
 
-	ret = drm_syncobj_find_fence(file_private, handle, 0, 0, &fence);
+	ret = drm_syncobj_find_fence(file_private, handle, point, 0, &fence);
 	if (ret)
 		goto err_put_fd;
 
@@ -823,9 +823,14 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 	    args->flags != DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
 		return -EINVAL;
 
-	if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
+	if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE) {
+		struct drm_syncobj *syncobj = drm_syncobj_find(file_private,
+							       args->handle);
+		if (!syncobj || syncobj->type != DRM_SYNCOBJ_TYPE_BINARY)
+			return -EINVAL;
 		return drm_syncobj_export_sync_file(file_private, args->handle,
-						    &args->fd);
+						    0, &args->fd);
+	}
 
 	return drm_syncobj_handle_to_fd(file_private, args->handle,
 					&args->fd);
@@ -837,6 +842,61 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 {
 	struct drm_syncobj_handle *args = data;
 
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	if (args->pad)
+		return -EINVAL;
+
+	if (args->flags != 0 &&
+	    args->flags != DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE)
+		return -EINVAL;
+
+	if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE) {
+		struct drm_syncobj *syncobj = drm_syncobj_find(file_private,
+							       args->handle);
+		if (!syncobj || syncobj->type != DRM_SYNCOBJ_TYPE_BINARY)
+			return -EINVAL;
+		return drm_syncobj_import_sync_file_fence(file_private,
+							  args->fd,
+							  args->handle,
+							  0);
+	}
+
+	return drm_syncobj_fd_to_handle(file_private, args->fd,
+					&args->handle);
+}
+
+int
+drm_syncobj_handle_to_fd_ioctl2(struct drm_device *dev, void *data,
+				   struct drm_file *file_private)
+{
+	struct drm_syncobj_handle2 *args = data;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	if (args->pad)
+		return -EINVAL;
+
+	if (args->flags != 0 &&
+	    args->flags != DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
+		return -EINVAL;
+
+	if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
+		return drm_syncobj_export_sync_file(file_private, args->handle,
+						    args->point, &args->fd);
+
+	return drm_syncobj_handle_to_fd(file_private, args->handle,
+					&args->fd);
+}
+
+int
+drm_syncobj_fd_to_handle_ioctl2(struct drm_device *dev, void *data,
+				   struct drm_file *file_private)
+{
+	struct drm_syncobj_handle2 *args = data;
+
 	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
 		return -ENODEV;
 
@@ -850,12 +910,14 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 	if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE)
 		return drm_syncobj_import_sync_file_fence(file_private,
 							  args->fd,
-							  args->handle);
+							  args->handle,
+							  args->point);
 
 	return drm_syncobj_fd_to_handle(file_private, args->fd,
 					&args->handle);
 }
 
+
 struct syncobj_wait_entry {
 	struct task_struct *task;
 	struct dma_fence *fence;
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 23c4979d8a1c..da5df2a42fc3 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -735,6 +735,15 @@ struct drm_syncobj_handle {
 	__s32 fd;
 	__u32 pad;
 };
+struct drm_syncobj_handle2 {
+	__u32 handle;
+	__u32 flags;
+	__u64 point;
+
+	__s32 fd;
+	__u32 pad;
+};
+
 
 #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
 #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1)
@@ -938,6 +947,8 @@ extern "C" {
 
 #define DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT	DRM_IOWR(0xCA, struct drm_syncobj_timeline_wait)
 #define DRM_IOCTL_SYNCOBJ_QUERY		DRM_IOWR(0xCB, struct drm_syncobj_timeline_query)
+#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD2	DRM_IOWR(0xCC, struct drm_syncobj_handle2)
+#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE2	DRM_IOWR(0xCD, struct drm_syncobj_handle2)
 
 /**
  * Device specific ioctls should only be in their respective headers
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 6/7] drm/amdgpu: add timeline support in amdgpu CS v2

[Chunming Zhou]

syncobj wait/signal operation is appending in command submission.
v2: separate to two kinds in/out_deps functions

Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h    |   8 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 111 +++++++++++++++++++++----
 include/uapi/drm/amdgpu_drm.h          |   9 ++
 3 files changed, 112 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 447c4c7a36d6..6e4a3db56833 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -975,6 +975,11 @@ struct amdgpu_cs_chunk {
 	void			*kdata;
 };
 
+struct amdgpu_cs_syncobj_post_dep {
+	struct drm_syncobj *post_dep_syncobj;
+	u64 point;
+};
+
 struct amdgpu_cs_parser {
 	struct amdgpu_device	*adev;
 	struct drm_file		*filp;
@@ -1003,9 +1008,8 @@ struct amdgpu_cs_parser {
 
 	/* user fence */
 	struct amdgpu_bo_list_entry	uf_entry;
-
+	struct amdgpu_cs_syncobj_post_dep *post_dep_syncobjs;
 	unsigned num_post_dep_syncobjs;
-	struct drm_syncobj **post_dep_syncobjs;
 };
 
 static inline u32 amdgpu_get_ib_value(struct amdgpu_cs_parser *p,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 412fac238575..7429e7941f4c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -204,6 +204,8 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, union drm_amdgpu_cs
 		case AMDGPU_CHUNK_ID_DEPENDENCIES:
 		case AMDGPU_CHUNK_ID_SYNCOBJ_IN:
 		case AMDGPU_CHUNK_ID_SYNCOBJ_OUT:
+		case AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT:
+		case AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL:
 			break;
 
 		default:
@@ -783,7 +785,7 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error,
 					   &parser->validated);
 
 	for (i = 0; i < parser->num_post_dep_syncobjs; i++)
-		drm_syncobj_put(parser->post_dep_syncobjs[i]);
+		drm_syncobj_put(parser->post_dep_syncobjs[i].post_dep_syncobj);
 	kfree(parser->post_dep_syncobjs);
 
 	dma_fence_put(parser->fence);
@@ -1098,13 +1100,17 @@ static int amdgpu_cs_process_fence_dep(struct amdgpu_cs_parser *p,
 }
 
 static int amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
-						 uint32_t handle)
+						 uint32_t handle, u64 point,
+						 u64 flags)
 {
 	int r;
 	struct dma_fence *fence;
-	r = drm_syncobj_find_fence(p->filp, handle, 0, 0, &fence);
-	if (r)
+
+	r = drm_syncobj_find_fence(p->filp, handle, point, flags, &fence);
+	if (r) {
+		DRM_ERROR("syncobj %u failed to find fence!\n", handle);
 		return r;
+	}
 
 	r = amdgpu_sync_fence(p->adev, &p->job->sync, fence, true);
 	dma_fence_put(fence);
@@ -1115,46 +1121,108 @@ static int amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
 static int amdgpu_cs_process_syncobj_in_dep(struct amdgpu_cs_parser *p,
 					    struct amdgpu_cs_chunk *chunk)
 {
+	struct drm_amdgpu_cs_chunk_sem *deps;
 	unsigned num_deps;
 	int i, r;
-	struct drm_amdgpu_cs_chunk_sem *deps;
 
 	deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
 	num_deps = chunk->length_dw * 4 /
 		sizeof(struct drm_amdgpu_cs_chunk_sem);
+	for (i = 0; i < num_deps; ++i) {
+		r = amdgpu_syncobj_lookup_and_add_to_sync(p, deps[i].handle,
+							  0, 0);
+		if (r)
+			return r;
+	}
+
+	return 0;
+}
+
+
+static int amdgpu_cs_process_syncobj_timeline_in_dep(struct amdgpu_cs_parser *p,
+						     struct amdgpu_cs_chunk *chunk)
+{
+	struct drm_amdgpu_cs_chunk_syncobj *syncobj_deps;
+	unsigned num_deps;
+	int i, r;
 
+	syncobj_deps = (struct drm_amdgpu_cs_chunk_syncobj *)chunk->kdata;
+	num_deps = chunk->length_dw * 4 /
+		sizeof(struct drm_amdgpu_cs_chunk_syncobj);
 	for (i = 0; i < num_deps; ++i) {
-		r = amdgpu_syncobj_lookup_and_add_to_sync(p, deps[i].handle);
+		r = amdgpu_syncobj_lookup_and_add_to_sync(p,
+							  syncobj_deps[i].handle,
+							  syncobj_deps[i].point,
+							  syncobj_deps[i].flags);
 		if (r)
 			return r;
 	}
+
 	return 0;
 }
 
 static int amdgpu_cs_process_syncobj_out_dep(struct amdgpu_cs_parser *p,
 					     struct amdgpu_cs_chunk *chunk)
 {
+	struct drm_amdgpu_cs_chunk_sem *deps;
 	unsigned num_deps;
 	int i;
-	struct drm_amdgpu_cs_chunk_sem *deps;
+
 	deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
 	num_deps = chunk->length_dw * 4 /
 		sizeof(struct drm_amdgpu_cs_chunk_sem);
 
 	p->post_dep_syncobjs = kmalloc_array(num_deps,
-					     sizeof(struct drm_syncobj *),
+					     sizeof(struct amdgpu_cs_syncobj_post_dep),
 					     GFP_KERNEL);
 	p->num_post_dep_syncobjs = 0;
 
 	if (!p->post_dep_syncobjs)
 		return -ENOMEM;
 
+
 	for (i = 0; i < num_deps; ++i) {
-		p->post_dep_syncobjs[i] = drm_syncobj_find(p->filp, deps[i].handle);
-		if (!p->post_dep_syncobjs[i])
+		p->post_dep_syncobjs[i].post_dep_syncobj =
+			drm_syncobj_find(p->filp, deps[i].handle);
+		if (!p->post_dep_syncobjs[i].post_dep_syncobj)
 			return -EINVAL;
+		p->post_dep_syncobjs[i].point = 0;
 		p->num_post_dep_syncobjs++;
 	}
+
+	return 0;
+}
+
+
+static int amdgpu_cs_process_syncobj_timeline_out_dep(struct amdgpu_cs_parser *p,
+						      struct amdgpu_cs_chunk
+						      *chunk)
+{
+	struct drm_amdgpu_cs_chunk_syncobj *syncobj_deps;
+	unsigned num_deps;
+	int i;
+
+	syncobj_deps = (struct drm_amdgpu_cs_chunk_syncobj *)chunk->kdata;
+	num_deps = chunk->length_dw * 4 /
+		sizeof(struct drm_amdgpu_cs_chunk_syncobj);
+
+	p->post_dep_syncobjs = kmalloc_array(num_deps,
+					     sizeof(struct amdgpu_cs_syncobj_post_dep),
+					     GFP_KERNEL);
+	p->num_post_dep_syncobjs = 0;
+
+	if (!p->post_dep_syncobjs)
+		return -ENOMEM;
+
+	for (i = 0; i < num_deps; ++i) {
+		p->post_dep_syncobjs[i].post_dep_syncobj =
+			drm_syncobj_find(p->filp, syncobj_deps[i].handle);
+		if (!p->post_dep_syncobjs[i].post_dep_syncobj)
+			return -EINVAL;
+		p->post_dep_syncobjs[i].point = syncobj_deps[i].point;
+		p->num_post_dep_syncobjs++;
+	}
+
 	return 0;
 }
 
@@ -1168,18 +1236,32 @@ static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
 
 		chunk = &p->chunks[i];
 
-		if (chunk->chunk_id == AMDGPU_CHUNK_ID_DEPENDENCIES) {
+		switch (chunk->chunk_id) {
+		case AMDGPU_CHUNK_ID_DEPENDENCIES:
 			r = amdgpu_cs_process_fence_dep(p, chunk);
 			if (r)
 				return r;
-		} else if (chunk->chunk_id == AMDGPU_CHUNK_ID_SYNCOBJ_IN) {
+			break;
+		case AMDGPU_CHUNK_ID_SYNCOBJ_IN:
 			r = amdgpu_cs_process_syncobj_in_dep(p, chunk);
 			if (r)
 				return r;
-		} else if (chunk->chunk_id == AMDGPU_CHUNK_ID_SYNCOBJ_OUT) {
+			break;
+		case AMDGPU_CHUNK_ID_SYNCOBJ_OUT:
 			r = amdgpu_cs_process_syncobj_out_dep(p, chunk);
 			if (r)
 				return r;
+			break;
+		case AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT:
+			r = amdgpu_cs_process_syncobj_timeline_in_dep(p, chunk);
+			if (r)
+				return r;
+			break;
+		case AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL:
+			r = amdgpu_cs_process_syncobj_timeline_out_dep(p, chunk);
+			if (r)
+				return r;
+			break;
 		}
 	}
 
@@ -1191,7 +1273,8 @@ static void amdgpu_cs_post_dependencies(struct amdgpu_cs_parser *p)
 	int i;
 
 	for (i = 0; i < p->num_post_dep_syncobjs; ++i)
-		drm_syncobj_replace_fence(p->post_dep_syncobjs[i], 0, p->fence);
+		drm_syncobj_replace_fence(p->post_dep_syncobjs[i].post_dep_syncobj,
+					  p->post_dep_syncobjs[i].point, p->fence);
 }
 
 static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 1ceec56de015..44a34a77f986 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -517,6 +517,8 @@ struct drm_amdgpu_gem_va {
 #define AMDGPU_CHUNK_ID_SYNCOBJ_IN      0x04
 #define AMDGPU_CHUNK_ID_SYNCOBJ_OUT     0x05
 #define AMDGPU_CHUNK_ID_BO_HANDLES      0x06
+#define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_WAIT    0x07
+#define AMDGPU_CHUNK_ID_SYNCOBJ_TIMELINE_SIGNAL  0x08
 
 struct drm_amdgpu_cs_chunk {
 	__u32		chunk_id;
@@ -592,6 +594,13 @@ struct drm_amdgpu_cs_chunk_sem {
 	__u32 handle;
 };
 
+struct drm_amdgpu_cs_chunk_syncobj {
+       __u32 handle;
+       __u32 flags;
+       __u64 point;
+};
+
+
 #define AMDGPU_FENCE_TO_HANDLE_GET_SYNCOBJ	0
 #define AMDGPU_FENCE_TO_HANDLE_GET_SYNCOBJ_FD	1
 #define AMDGPU_FENCE_TO_HANDLE_GET_SYNC_FILE_FD	2
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 7/7] drm/amdgpu: update version for timeline syncobj support in amdgpu

[Chunming Zhou]

Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 6870909da926..58cba492ba55 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -70,9 +70,10 @@
  * - 3.25.0 - Add support for sensor query info (stable pstate sclk/mclk).
  * - 3.26.0 - GFX9: Process AMDGPU_IB_FLAG_TC_WB_NOT_INVALIDATE.
  * - 3.27.0 - Add new chunk to to AMDGPU_CS to enable BO_LIST creation.
+ * - 3.28.0 - Add syncobj timeline support to AMDGPU_CS.
  */
 #define KMS_DRIVER_MAJOR	3
-#define KMS_DRIVER_MINOR	27
+#define KMS_DRIVER_MINOR	28
 #define KMS_DRIVER_PATCHLEVEL	0
 
 int amdgpu_vram_limit = 0;
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 7/7] drm/amdgpu: update version for timeline syncobj support in amdgpu

[Zhou, David(ChunMing)]

Ping...
Christian, Could I get your RB on the series? And help me to push to drm-misc?
After that I can rebase libdrm header file based on drm-next.

Thanks,
David Zhou

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Chunming Zhou
> Sent: Monday, October 15, 2018 4:56 PM
> To: dri-devel@lists.freedesktop.org
> Cc: Zhou, David(ChunMing) <David1.Zhou@amd.com>; amd-
> gfx@lists.freedesktop.org
> Subject: [PATCH 7/7] drm/amdgpu: update version for timeline syncobj
> support in amdgpu
> 
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 6870909da926..58cba492ba55 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -70,9 +70,10 @@
>   * - 3.25.0 - Add support for sensor query info (stable pstate sclk/mclk).
>   * - 3.26.0 - GFX9: Process AMDGPU_IB_FLAG_TC_WB_NOT_INVALIDATE.
>   * - 3.27.0 - Add new chunk to to AMDGPU_CS to enable BO_LIST creation.
> + * - 3.28.0 - Add syncobj timeline support to AMDGPU_CS.
>   */
>  #define KMS_DRIVER_MAJOR	3
> -#define KMS_DRIVER_MINOR	27
> +#define KMS_DRIVER_MINOR	28
>  #define KMS_DRIVER_PATCHLEVEL	0
> 
>  int amdgpu_vram_limit = 0;
> --
> 2.17.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] drm/amdgpu: update version for timeline syncobj support in amdgpu

[Koenig, Christian]

I'm on sick leave today.

But I will see what I can do later in the afternoon,
Christian.

Am 15.10.2018 um 11:01 schrieb Zhou, David(ChunMing):
> Ping...
> Christian, Could I get your RB on the series? And help me to push to drm-misc?
> After that I can rebase libdrm header file based on drm-next.
>
> Thanks,
> David Zhou
>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Chunming Zhou
>> Sent: Monday, October 15, 2018 4:56 PM
>> To: dri-devel@lists.freedesktop.org
>> Cc: Zhou, David(ChunMing) <David1.Zhou@amd.com>; amd-
>> gfx@lists.freedesktop.org
>> Subject: [PATCH 7/7] drm/amdgpu: update version for timeline syncobj
>> support in amdgpu
>>
>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 6870909da926..58cba492ba55 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -70,9 +70,10 @@
>>    * - 3.25.0 - Add support for sensor query info (stable pstate sclk/mclk).
>>    * - 3.26.0 - GFX9: Process AMDGPU_IB_FLAG_TC_WB_NOT_INVALIDATE.
>>    * - 3.27.0 - Add new chunk to to AMDGPU_CS to enable BO_LIST creation.
>> + * - 3.28.0 - Add syncobj timeline support to AMDGPU_CS.
>>    */
>>   #define KMS_DRIVER_MAJOR	3
>> -#define KMS_DRIVER_MINOR	27
>> +#define KMS_DRIVER_MINOR	28
>>   #define KMS_DRIVER_PATCHLEVEL	0
>>
>>   int amdgpu_vram_limit = 0;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 7/7] drm/amdgpu: update version for timeline syncobj support in amdgpu

[Christian König]

I've added my rb to patch #1 and pushed it to drm-misc-next.

I would really like to get an rb from other people on patch #2 before 
proceeding.

Daniel, Dave and all the other usual suspects on the list what is your 
opinion on this implementation?

Christian.

Am 15.10.2018 um 11:04 schrieb Koenig, Christian:
> I'm on sick leave today.
>
> But I will see what I can do later in the afternoon,
> Christian.
>
> Am 15.10.2018 um 11:01 schrieb Zhou, David(ChunMing):
>> Ping...
>> Christian, Could I get your RB on the series? And help me to push to drm-misc?
>> After that I can rebase libdrm header file based on drm-next.
>>
>> Thanks,
>> David Zhou
>>
>>> -----Original Message-----
>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>> Chunming Zhou
>>> Sent: Monday, October 15, 2018 4:56 PM
>>> To: dri-devel@lists.freedesktop.org
>>> Cc: Zhou, David(ChunMing) <David1.Zhou@amd.com>; amd-
>>> gfx@lists.freedesktop.org
>>> Subject: [PATCH 7/7] drm/amdgpu: update version for timeline syncobj
>>> support in amdgpu
>>>
>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index 6870909da926..58cba492ba55 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -70,9 +70,10 @@
>>>     * - 3.25.0 - Add support for sensor query info (stable pstate sclk/mclk).
>>>     * - 3.26.0 - GFX9: Process AMDGPU_IB_FLAG_TC_WB_NOT_INVALIDATE.
>>>     * - 3.27.0 - Add new chunk to to AMDGPU_CS to enable BO_LIST creation.
>>> + * - 3.28.0 - Add syncobj timeline support to AMDGPU_CS.
>>>     */
>>>    #define KMS_DRIVER_MAJOR	3
>>> -#define KMS_DRIVER_MINOR	27
>>> +#define KMS_DRIVER_MINOR	28
>>>    #define KMS_DRIVER_PATCHLEVEL	0
>>>
>>>    int amdgpu_vram_limit = 0;
>>> --
>>> 2.17.1
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 7/7] drm/amdgpu: update version for timeline syncobj support in amdgpu

[zhoucm1]

On 2018年10月16日 20:54, Christian König wrote:
> I've added my rb to patch #1 and pushed it to drm-misc-next.
>
> I would really like to get an rb from other people on patch #2 before 
> proceeding.
>
> Daniel, Dave and all the other usual suspects on the list what is your 
> opinion on this implementation?
Thanks for head up, @Daniel, @Dave, or the others, Could you take a look 
for the series?

Thanks,
David
>
> Christian.
>
> Am 15.10.2018 um 11:04 schrieb Koenig, Christian:
>> I'm on sick leave today.
>>
>> But I will see what I can do later in the afternoon,
>> Christian.
>>
>> Am 15.10.2018 um 11:01 schrieb Zhou, David(ChunMing):
>>> Ping...
>>> Christian, Could I get your RB on the series? And help me to push to 
>>> drm-misc?
>>> After that I can rebase libdrm header file based on drm-next.
>>>
>>> Thanks,
>>> David Zhou
>>>
>>>> -----Original Message-----
>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>> Chunming Zhou
>>>> Sent: Monday, October 15, 2018 4:56 PM
>>>> To: dri-devel@lists.freedesktop.org
>>>> Cc: Zhou, David(ChunMing) <David1.Zhou@amd.com>; amd-
>>>> gfx@lists.freedesktop.org
>>>> Subject: [PATCH 7/7] drm/amdgpu: update version for timeline syncobj
>>>> support in amdgpu
>>>>
>>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> index 6870909da926..58cba492ba55 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> @@ -70,9 +70,10 @@
>>>>     * - 3.25.0 - Add support for sensor query info (stable pstate 
>>>> sclk/mclk).
>>>>     * - 3.26.0 - GFX9: Process AMDGPU_IB_FLAG_TC_WB_NOT_INVALIDATE.
>>>>     * - 3.27.0 - Add new chunk to to AMDGPU_CS to enable BO_LIST 
>>>> creation.
>>>> + * - 3.28.0 - Add syncobj timeline support to AMDGPU_CS.
>>>>     */
>>>>    #define KMS_DRIVER_MAJOR    3
>>>> -#define KMS_DRIVER_MINOR    27
>>>> +#define KMS_DRIVER_MINOR    28
>>>>    #define KMS_DRIVER_PATCHLEVEL    0
>>>>
>>>>    int amdgpu_vram_limit = 0;
>>>> -- 
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx