[PATCH 1/7] dma-buf/dma-fence: Extract __dma_fence_is_later()

[PATCH 1/7] dma-buf/dma-fence: Extract __dma_fence_is_later()

[Chris Wilson]

Often we have the task of comparing two seqno known to be on the same
context, so provide a common __dma_fence_is_later().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
---
 include/linux/dma-fence.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index a5195a7d6f77..ac5987989e9a 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -336,6 +336,19 @@ dma_fence_is_signaled(struct dma_fence *fence)
 }
 
 /**
+ * __dma_fence_is_later - return if f1 is chronologically later than f2
+ * @f1:	[in]	the first fence's seqno
+ * @f2:	[in]	the second fence's seqno from the same context
+ *
+ * Returns true if f1 is chronologically later than f2. Both fences must be
+ * from the same context, since a seqno is not common across contexts.
+ */
+static inline bool __dma_fence_is_later(u32 f1, u32 f2)
+{
+	return (int)(f1 - f2) > 0;
+}
+
+/**
  * dma_fence_is_later - return if f1 is chronologically later than f2
  * @f1:	[in]	the first fence from the same context
  * @f2:	[in]	the second fence from the same context
@@ -349,7 +362,7 @@ static inline bool dma_fence_is_later(struct dma_fence *f1,
 	if (WARN_ON(f1->context != f2->context))
 		return false;
 
-	return (int)(f1->seqno - f2->seqno) > 0;
+	return __dma_fence_is_later(f1->seqno, f2->seqno);
 }
 
 /**
-- 
2.13.1

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

[PATCH 2/7] dma-buf/sw-sync: Fix the is-signaled test to handle u32 wraparound

[Chris Wilson]

Use the canonical __dma_fence_is_later() to compare the fence seqno
against the timeline seqno to check if the fence is signaled.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
---
 drivers/dma-buf/sw_sync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 69c5ff36e2f9..4d5d8c5e2534 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -219,7 +219,7 @@ static bool timeline_fence_signaled(struct dma_fence *fence)
 {
 	struct sync_timeline *parent = dma_fence_parent(fence);
 
-	return (fence->seqno > parent->value) ? false : true;
+	return !__dma_fence_is_later(fence->seqno, parent->value);
 }
 
 static bool timeline_fence_enable_signaling(struct dma_fence *fence)
-- 
2.13.1

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

[PATCH 3/7] dma-buf/sw-sync: Prevent user overflow on timeline advance

[Chris Wilson]

The timeline is u32, which limits any single advance to INT_MAX so that
we can detect all fences that need signaling.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
---
 drivers/dma-buf/sw_sync.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 4d5d8c5e2534..0e676d08aa70 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -345,6 +345,11 @@ static long sw_sync_ioctl_inc(struct sync_timeline *obj, unsigned long arg)
 	if (copy_from_user(&value, (void __user *)arg, sizeof(value)))
 		return -EFAULT;
 
+	while (value > INT_MAX)  {
+		sync_timeline_signal(obj, INT_MAX);
+		value -= INT_MAX;
+	}
+
 	sync_timeline_signal(obj, value);
 
 	return 0;
-- 
2.13.1

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

[PATCH 4/7] dma-buf/sw-sync: Reduce irqsave/irqrestore from known context

[Chris Wilson]

If we know the context under which we are called, then we can use the
simpler form of spin_lock_irq (saving the save/restore).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
---
 drivers/dma-buf/sw_sync.c    | 15 +++++++++------
 drivers/dma-buf/sync_debug.c | 14 ++++++--------
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 0e676d08aa70..fc733621987d 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -135,12 +135,11 @@ static void sync_timeline_put(struct sync_timeline *obj)
  */
 static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 {
-	unsigned long flags;
 	struct sync_pt *pt, *next;
 
 	trace_sync_timeline(obj);
 
-	spin_lock_irqsave(&obj->child_list_lock, flags);
+	spin_lock_irq(&obj->child_list_lock);
 
 	obj->value += inc;
 
@@ -150,7 +149,7 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 			list_del_init(&pt->active_list);
 	}
 
-	spin_unlock_irqrestore(&obj->child_list_lock, flags);
+	spin_unlock_irq(&obj->child_list_lock);
 }
 
 /**
@@ -167,7 +166,6 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 static struct sync_pt *sync_pt_create(struct sync_timeline *obj, int size,
 			     unsigned int value)
 {
-	unsigned long flags;
 	struct sync_pt *pt;
 
 	if (size < sizeof(*pt))
@@ -177,13 +175,16 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, int size,
 	if (!pt)
 		return NULL;
 
-	spin_lock_irqsave(&obj->child_list_lock, flags);
+	spin_lock_irq(&obj->child_list_lock);
+
 	sync_timeline_get(obj);
 	dma_fence_init(&pt->base, &timeline_fence_ops, &obj->child_list_lock,
 		       obj->context, value);
 	list_add_tail(&pt->child_list, &obj->child_list_head);
 	INIT_LIST_HEAD(&pt->active_list);
-	spin_unlock_irqrestore(&obj->child_list_lock, flags);
+
+	spin_unlock_irq(&obj->child_list_lock);
+
 	return pt;
 }
 
@@ -206,9 +207,11 @@ static void timeline_fence_release(struct dma_fence *fence)
 	unsigned long flags;
 
 	spin_lock_irqsave(fence->lock, flags);
+
 	list_del(&pt->child_list);
 	if (!list_empty(&pt->active_list))
 		list_del(&pt->active_list);
+
 	spin_unlock_irqrestore(fence->lock, flags);
 
 	sync_timeline_put(parent);
diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
index 82a6e7f6d37f..0e91632248ba 100644
--- a/drivers/dma-buf/sync_debug.c
+++ b/drivers/dma-buf/sync_debug.c
@@ -116,17 +116,16 @@ static void sync_print_fence(struct seq_file *s,
 static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj)
 {
 	struct list_head *pos;
-	unsigned long flags;
 
 	seq_printf(s, "%s: %d\n", obj->name, obj->value);
 
-	spin_lock_irqsave(&obj->child_list_lock, flags);
+	spin_lock_irq(&obj->child_list_lock);
 	list_for_each(pos, &obj->child_list_head) {
 		struct sync_pt *pt =
 			container_of(pos, struct sync_pt, child_list);
 		sync_print_fence(s, &pt->base, false);
 	}
-	spin_unlock_irqrestore(&obj->child_list_lock, flags);
+	spin_unlock_irq(&obj->child_list_lock);
 }
 
 static void sync_print_sync_file(struct seq_file *s,
@@ -151,12 +150,11 @@ static void sync_print_sync_file(struct seq_file *s,
 
 static int sync_debugfs_show(struct seq_file *s, void *unused)
 {
-	unsigned long flags;
 	struct list_head *pos;
 
 	seq_puts(s, "objs:\n--------------\n");
 
-	spin_lock_irqsave(&sync_timeline_list_lock, flags);
+	spin_lock_irq(&sync_timeline_list_lock);
 	list_for_each(pos, &sync_timeline_list_head) {
 		struct sync_timeline *obj =
 			container_of(pos, struct sync_timeline,
@@ -165,11 +163,11 @@ static int sync_debugfs_show(struct seq_file *s, void *unused)
 		sync_print_obj(s, obj);
 		seq_putc(s, '\n');
 	}
-	spin_unlock_irqrestore(&sync_timeline_list_lock, flags);
+	spin_unlock_irq(&sync_timeline_list_lock);
 
 	seq_puts(s, "fences:\n--------------\n");
 
-	spin_lock_irqsave(&sync_file_list_lock, flags);
+	spin_lock_irq(&sync_file_list_lock);
 	list_for_each(pos, &sync_file_list_head) {
 		struct sync_file *sync_file =
 			container_of(pos, struct sync_file, sync_file_list);
@@ -177,7 +175,7 @@ static int sync_debugfs_show(struct seq_file *s, void *unused)
 		sync_print_sync_file(s, sync_file);
 		seq_putc(s, '\n');
 	}
-	spin_unlock_irqrestore(&sync_file_list_lock, flags);
+	spin_unlock_irq(&sync_file_list_lock);
 	return 0;
 }
 
-- 
2.13.1

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

[PATCH 5/7] dma-buf/sw-sync: sync_pt is private and of fixed size

[Chris Wilson]

Since sync_pt is only allocated from a single location and is no longer
the base class for fences (that is struct dma_fence) it no longer needs
a generic unsized allocator.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
---
 drivers/dma-buf/sw_sync.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index fc733621987d..6effa1ce010e 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -155,7 +155,6 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 /**
  * sync_pt_create() - creates a sync pt
  * @parent:	fence's parent sync_timeline
- * @size:	size to allocate for this pt
  * @inc:	value of the fence
  *
  * Creates a new sync_pt as a child of @parent.  @size bytes will be
@@ -163,15 +162,12 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
  * the generic sync_timeline struct. Returns the sync_pt object or
  * NULL in case of error.
  */
-static struct sync_pt *sync_pt_create(struct sync_timeline *obj, int size,
-			     unsigned int value)
+static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
+				      unsigned int value)
 {
 	struct sync_pt *pt;
 
-	if (size < sizeof(*pt))
-		return NULL;
-
-	pt = kzalloc(size, GFP_KERNEL);
+	pt = kzalloc(sizeof(*pt), GFP_KERNEL);
 	if (!pt)
 		return NULL;
 
@@ -312,7 +308,7 @@ static long sw_sync_ioctl_create_fence(struct sync_timeline *obj,
 		goto err;
 	}
 
-	pt = sync_pt_create(obj, sizeof(*pt), data.value);
+	pt = sync_pt_create(obj, data.value);
 	if (!pt) {
 		err = -ENOMEM;
 		goto err;
-- 
2.13.1

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

[PATCH 6/7] dma-buf/sw-sync: Fix locking around sync_timeline lists

[Chris Wilson]

The sync_pt were not adding themselves atomically to the timeline lists,
corruption imminent.  Only a single list is required to track the
unsignaled sync_pt, so reduce it and rename the lock more appropriately
along with using idiomatic names to distinguish a list from links along
it.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
---
 drivers/dma-buf/sw_sync.c    | 39 ++++++++++++++-------------------------
 drivers/dma-buf/sync_debug.c |  9 ++++-----
 drivers/dma-buf/sync_debug.h | 21 ++++++++-------------
 3 files changed, 26 insertions(+), 43 deletions(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 6effa1ce010e..e51fe11bbbea 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -96,9 +96,8 @@ static struct sync_timeline *sync_timeline_create(const char *name)
 	obj->context = dma_fence_context_alloc(1);
 	strlcpy(obj->name, name, sizeof(obj->name));
 
-	INIT_LIST_HEAD(&obj->child_list_head);
-	INIT_LIST_HEAD(&obj->active_list_head);
-	spin_lock_init(&obj->child_list_lock);
+	INIT_LIST_HEAD(&obj->pt_list);
+	spin_lock_init(&obj->lock);
 
 	sync_timeline_debug_add(obj);
 
@@ -139,17 +138,15 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 
 	trace_sync_timeline(obj);
 
-	spin_lock_irq(&obj->child_list_lock);
+	spin_lock_irq(&obj->lock);
 
 	obj->value += inc;
 
-	list_for_each_entry_safe(pt, next, &obj->active_list_head,
-				 active_list) {
+	list_for_each_entry_safe(pt, next, &obj->pt_list, link)
 		if (dma_fence_is_signaled_locked(&pt->base))
-			list_del_init(&pt->active_list);
-	}
+			list_del_init(&pt->link);
 
-	spin_unlock_irq(&obj->child_list_lock);
+	spin_unlock_irq(&obj->lock);
 }
 
 /**
@@ -171,15 +168,15 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
 	if (!pt)
 		return NULL;
 
-	spin_lock_irq(&obj->child_list_lock);
-
 	sync_timeline_get(obj);
-	dma_fence_init(&pt->base, &timeline_fence_ops, &obj->child_list_lock,
+	dma_fence_init(&pt->base, &timeline_fence_ops, &obj->lock,
 		       obj->context, value);
-	list_add_tail(&pt->child_list, &obj->child_list_head);
-	INIT_LIST_HEAD(&pt->active_list);
+	INIT_LIST_HEAD(&pt->link);
 
-	spin_unlock_irq(&obj->child_list_lock);
+	spin_lock_irq(&obj->lock);
+	if (!dma_fence_is_signaled_locked(&pt->base))
+		list_add_tail(&pt->link, &obj->pt_list);
+	spin_unlock_irq(&obj->lock);
 
 	return pt;
 }
@@ -204,9 +201,8 @@ static void timeline_fence_release(struct dma_fence *fence)
 
 	spin_lock_irqsave(fence->lock, flags);
 
-	list_del(&pt->child_list);
-	if (!list_empty(&pt->active_list))
-		list_del(&pt->active_list);
+	if (!list_empty(&pt->link))
+		list_del(&pt->link);
 
 	spin_unlock_irqrestore(fence->lock, flags);
 
@@ -223,13 +219,6 @@ static bool timeline_fence_signaled(struct dma_fence *fence)
 
 static bool timeline_fence_enable_signaling(struct dma_fence *fence)
 {
-	struct sync_pt *pt = dma_fence_to_sync_pt(fence);
-	struct sync_timeline *parent = dma_fence_parent(fence);
-
-	if (timeline_fence_signaled(fence))
-		return false;
-
-	list_add_tail(&pt->active_list, &parent->active_list_head);
 	return true;
 }
 
diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
index 0e91632248ba..2264a075f6a9 100644
--- a/drivers/dma-buf/sync_debug.c
+++ b/drivers/dma-buf/sync_debug.c
@@ -119,13 +119,12 @@ static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj)
 
 	seq_printf(s, "%s: %d\n", obj->name, obj->value);
 
-	spin_lock_irq(&obj->child_list_lock);
-	list_for_each(pos, &obj->child_list_head) {
-		struct sync_pt *pt =
-			container_of(pos, struct sync_pt, child_list);
+	spin_lock_irq(&obj->lock);
+	list_for_each(pos, &obj->pt_list) {
+		struct sync_pt *pt = container_of(pos, struct sync_pt, link);
 		sync_print_fence(s, &pt->base, false);
 	}
-	spin_unlock_irq(&obj->child_list_lock);
+	spin_unlock_irq(&obj->lock);
 }
 
 static void sync_print_sync_file(struct seq_file *s,
diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
index 26fe8b9907b3..899ba0e19fd3 100644
--- a/drivers/dma-buf/sync_debug.h
+++ b/drivers/dma-buf/sync_debug.h
@@ -24,42 +24,37 @@
  * struct sync_timeline - sync object
  * @kref:		reference count on fence.
  * @name:		name of the sync_timeline. Useful for debugging
- * @child_list_head:	list of children sync_pts for this sync_timeline
- * @child_list_lock:	lock protecting @child_list_head and fence.status
- * @active_list_head:	list of active (unsignaled/errored) sync_pts
+ * @lock:		lock protecting @child_list_head and fence.status
+ * @pt_list:		list of active (unsignaled/errored) sync_pts
  * @sync_timeline_list:	membership in global sync_timeline_list
  */
 struct sync_timeline {
 	struct kref		kref;
 	char			name[32];
 
-	/* protected by child_list_lock */
+	/* protected by lock */
 	u64			context;
 	int			value;
 
-	struct list_head	child_list_head;
-	spinlock_t		child_list_lock;
-
-	struct list_head	active_list_head;
+	struct list_head	pt_list;
+	spinlock_t		lock;
 
 	struct list_head	sync_timeline_list;
 };
 
 static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
 {
-	return container_of(fence->lock, struct sync_timeline, child_list_lock);
+	return container_of(fence->lock, struct sync_timeline, lock);
 }
 
 /**
  * struct sync_pt - sync_pt object
  * @base: base fence object
- * @child_list: sync timeline child's list
- * @active_list: sync timeline active child's list
+ * @link: link on the sync timeline's list
  */
 struct sync_pt {
 	struct dma_fence base;
-	struct list_head child_list;
-	struct list_head active_list;
+	struct list_head link;
 };
 
 #ifdef CONFIG_SW_SYNC
-- 
2.13.1

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

[PATCH 7/7] dma-buf/sw-sync: Use an rbtree to sort fences in the timeline

[Chris Wilson]

Reduce the list iteration when incrementing the timeline by storing the
fences in increasing order.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
---
 drivers/dma-buf/sw_sync.c    | 49 ++++++++++++++++++++++++++++++++++++++------
 drivers/dma-buf/sync_debug.h |  5 +++++
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index e51fe11bbbea..8cef5d345316 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -96,6 +96,7 @@ static struct sync_timeline *sync_timeline_create(const char *name)
 	obj->context = dma_fence_context_alloc(1);
 	strlcpy(obj->name, name, sizeof(obj->name));
 
+	obj->pt_tree = RB_ROOT;
 	INIT_LIST_HEAD(&obj->pt_list);
 	spin_lock_init(&obj->lock);
 
@@ -142,9 +143,13 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 
 	obj->value += inc;
 
-	list_for_each_entry_safe(pt, next, &obj->pt_list, link)
-		if (dma_fence_is_signaled_locked(&pt->base))
-			list_del_init(&pt->link);
+	list_for_each_entry_safe(pt, next, &obj->pt_list, link) {
+		if (!dma_fence_is_signaled_locked(&pt->base))
+			break;
+
+		list_del_init(&pt->link);
+		rb_erase(&pt->node, &obj->pt_tree);
+	}
 
 	spin_unlock_irq(&obj->lock);
 }
@@ -174,8 +179,38 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
 	INIT_LIST_HEAD(&pt->link);
 
 	spin_lock_irq(&obj->lock);
-	if (!dma_fence_is_signaled_locked(&pt->base))
-		list_add_tail(&pt->link, &obj->pt_list);
+	if (!dma_fence_is_signaled_locked(&pt->base)) {
+		struct rb_node **p = &obj->pt_tree.rb_node;
+		struct rb_node *parent = NULL;
+
+		while (*p) {
+			struct sync_pt *other;
+			int cmp;
+
+			parent = *p;
+			other = rb_entry(parent, typeof(*pt), node);
+			cmp = value - other->base.seqno;
+			if (cmp > 0) {
+				p = &parent->rb_right;
+			} else if (cmp < 0) {
+				p = &parent->rb_left;
+			} else {
+				if (dma_fence_get_rcu(&other->base)) {
+					dma_fence_put(&pt->base);
+					pt = other;
+					goto unlock;
+				}
+				p = &parent->rb_left;
+			}
+		}
+		rb_link_node(&pt->node, parent, p);
+		rb_insert_color(&pt->node, &obj->pt_tree);
+
+		parent = rb_next(&pt->node);
+		list_add_tail(&pt->link,
+			      parent ? &rb_entry(parent, typeof(*pt), node)->link : &obj->pt_list);
+	}
+unlock:
 	spin_unlock_irq(&obj->lock);
 
 	return pt;
@@ -201,8 +236,10 @@ static void timeline_fence_release(struct dma_fence *fence)
 
 	spin_lock_irqsave(fence->lock, flags);
 
-	if (!list_empty(&pt->link))
+	if (!list_empty(&pt->link)) {
 		list_del(&pt->link);
+		rb_erase(&pt->node, &parent->pt_tree);
+	}
 
 	spin_unlock_irqrestore(fence->lock, flags);
 
diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
index 899ba0e19fd3..b7a5fab12179 100644
--- a/drivers/dma-buf/sync_debug.h
+++ b/drivers/dma-buf/sync_debug.h
@@ -14,6 +14,7 @@
 #define _LINUX_SYNC_H
 
 #include <linux/list.h>
+#include <linux/rbtree.h>
 #include <linux/spinlock.h>
 #include <linux/dma-fence.h>
 
@@ -25,6 +26,7 @@
  * @kref:		reference count on fence.
  * @name:		name of the sync_timeline. Useful for debugging
  * @lock:		lock protecting @child_list_head and fence.status
+ * @pt_tree:		rbtree of active (unsignaled/errored) sync_pts
  * @pt_list:		list of active (unsignaled/errored) sync_pts
  * @sync_timeline_list:	membership in global sync_timeline_list
  */
@@ -36,6 +38,7 @@ struct sync_timeline {
 	u64			context;
 	int			value;
 
+	struct rb_root		pt_tree;
 	struct list_head	pt_list;
 	spinlock_t		lock;
 
@@ -51,10 +54,12 @@ static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
  * struct sync_pt - sync_pt object
  * @base: base fence object
  * @link: link on the sync timeline's list
+ * @node: node in the sync timeline's tree
  */
 struct sync_pt {
 	struct dma_fence base;
 	struct list_head link;
+	struct rb_node node;
 };
 
 #ifdef CONFIG_SW_SYNC
-- 
2.13.1

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

✓ Fi.CI.BAT: success for series starting with [1/7] dma-buf/dma-fence: Extract __dma_fence_is_later()

[Patchwork]

== Series Details ==

Series: series starting with [1/7] dma-buf/dma-fence: Extract __dma_fence_is_later()
URL   : https://patchwork.freedesktop.org/series/26551/
State : success

== Summary ==

Series 26551v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/26551/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-pnv-d510) fdo#101597 +1

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597

fi-bdw-5557u     total:279  pass:264  dwarn:4   dfail:0   fail:0   skip:11  time:442s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:429s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:356s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:535s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:505s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:486s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:483s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:595s
fi-hsw-4770      total:279  pass:259  dwarn:4   dfail:0   fail:0   skip:16  time:439s
fi-hsw-4770r     total:279  pass:259  dwarn:4   dfail:0   fail:0   skip:16  time:415s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:418s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:492s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:473s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:573s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:581s
fi-pnv-d510      total:279  pass:222  dwarn:2   dfail:0   fail:0   skip:55  time:560s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:460s
fi-skl-6700hq    total:279  pass:223  dwarn:1   dfail:0   fail:30  skip:24  time:343s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:466s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:477s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:435s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:542s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:402s

ee2da0ac24fb8d50a03b263eb1fa2e82849eda95 drm-tip: 2017y-06m-29d-13h-14m-49s UTC integration manifest
e86e242 dma-buf/sw-sync: Use an rbtree to sort fences in the timeline
92386d5 dma-buf/sw-sync: Fix locking around sync_timeline lists
70feb63 dma-buf/sw-sync: sync_pt is private and of fixed size
7011ddb dma-buf/sw-sync: Reduce irqsave/irqrestore from known context
51a02cf dma-buf/sw-sync: Prevent user overflow on timeline advance
e1ffd68 dma-buf/sw-sync: Fix the is-signaled test to handle u32 wraparound
15fe0dd dma-buf/dma-fence: Extract __dma_fence_is_later()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5068/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] dma-buf/sw-sync: Fix locking around sync_timeline lists

[Sean Paul]

On Thu, Jun 29, 2017 at 01:59:29PM +0100, Chris Wilson wrote:
> The sync_pt were not adding themselves atomically to the timeline lists,
> corruption imminent.  Only a single list is required to track the
> unsignaled sync_pt, so reduce it and rename the lock more appropriately
> along with using idiomatic names to distinguish a list from links along
> it.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> ---
>  drivers/dma-buf/sw_sync.c    | 39 ++++++++++++++-------------------------
>  drivers/dma-buf/sync_debug.c |  9 ++++-----
>  drivers/dma-buf/sync_debug.h | 21 ++++++++-------------
>  3 files changed, 26 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 6effa1ce010e..e51fe11bbbea 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -96,9 +96,8 @@ static struct sync_timeline *sync_timeline_create(const char *name)
>  	obj->context = dma_fence_context_alloc(1);
>  	strlcpy(obj->name, name, sizeof(obj->name));
>  
> -	INIT_LIST_HEAD(&obj->child_list_head);
> -	INIT_LIST_HEAD(&obj->active_list_head);
> -	spin_lock_init(&obj->child_list_lock);
> +	INIT_LIST_HEAD(&obj->pt_list);
> +	spin_lock_init(&obj->lock);
>  
>  	sync_timeline_debug_add(obj);
>  
> @@ -139,17 +138,15 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>  
>  	trace_sync_timeline(obj);
>  
> -	spin_lock_irq(&obj->child_list_lock);
> +	spin_lock_irq(&obj->lock);
>  
>  	obj->value += inc;
>  
> -	list_for_each_entry_safe(pt, next, &obj->active_list_head,
> -				 active_list) {
> +	list_for_each_entry_safe(pt, next, &obj->pt_list, link)
>  		if (dma_fence_is_signaled_locked(&pt->base))
> -			list_del_init(&pt->active_list);
> -	}
> +			list_del_init(&pt->link);
>  
> -	spin_unlock_irq(&obj->child_list_lock);
> +	spin_unlock_irq(&obj->lock);
>  }
>  
>  /**
> @@ -171,15 +168,15 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
>  	if (!pt)
>  		return NULL;
>  
> -	spin_lock_irq(&obj->child_list_lock);
> -
>  	sync_timeline_get(obj);
> -	dma_fence_init(&pt->base, &timeline_fence_ops, &obj->child_list_lock,
> +	dma_fence_init(&pt->base, &timeline_fence_ops, &obj->lock,
>  		       obj->context, value);
> -	list_add_tail(&pt->child_list, &obj->child_list_head);
> -	INIT_LIST_HEAD(&pt->active_list);
> +	INIT_LIST_HEAD(&pt->link);
>  
> -	spin_unlock_irq(&obj->child_list_lock);
> +	spin_lock_irq(&obj->lock);
> +	if (!dma_fence_is_signaled_locked(&pt->base))
> +		list_add_tail(&pt->link, &obj->pt_list);
> +	spin_unlock_irq(&obj->lock);
>  
>  	return pt;
>  }
> @@ -204,9 +201,8 @@ static void timeline_fence_release(struct dma_fence *fence)
>  
>  	spin_lock_irqsave(fence->lock, flags);
>  
> -	list_del(&pt->child_list);
> -	if (!list_empty(&pt->active_list))
> -		list_del(&pt->active_list);
> +	if (!list_empty(&pt->link))
> +		list_del(&pt->link);
>  
>  	spin_unlock_irqrestore(fence->lock, flags);
>  
> @@ -223,13 +219,6 @@ static bool timeline_fence_signaled(struct dma_fence *fence)
>  
>  static bool timeline_fence_enable_signaling(struct dma_fence *fence)
>  {
> -	struct sync_pt *pt = dma_fence_to_sync_pt(fence);
> -	struct sync_timeline *parent = dma_fence_parent(fence);
> -
> -	if (timeline_fence_signaled(fence))
> -		return false;
> -
> -	list_add_tail(&pt->active_list, &parent->active_list_head);
>  	return true;

Shouldn't you still return false if the fence is already signaled?

>  }
>  
> diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
> index 0e91632248ba..2264a075f6a9 100644
> --- a/drivers/dma-buf/sync_debug.c
> +++ b/drivers/dma-buf/sync_debug.c
> @@ -119,13 +119,12 @@ static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj)
>  
>  	seq_printf(s, "%s: %d\n", obj->name, obj->value);
>  
> -	spin_lock_irq(&obj->child_list_lock);
> -	list_for_each(pos, &obj->child_list_head) {
> -		struct sync_pt *pt =
> -			container_of(pos, struct sync_pt, child_list);
> +	spin_lock_irq(&obj->lock);
> +	list_for_each(pos, &obj->pt_list) {
> +		struct sync_pt *pt = container_of(pos, struct sync_pt, link);
>  		sync_print_fence(s, &pt->base, false);
>  	}
> -	spin_unlock_irq(&obj->child_list_lock);
> +	spin_unlock_irq(&obj->lock);
>  }
>  
>  static void sync_print_sync_file(struct seq_file *s,
> diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
> index 26fe8b9907b3..899ba0e19fd3 100644
> --- a/drivers/dma-buf/sync_debug.h
> +++ b/drivers/dma-buf/sync_debug.h
> @@ -24,42 +24,37 @@
>   * struct sync_timeline - sync object
>   * @kref:		reference count on fence.
>   * @name:		name of the sync_timeline. Useful for debugging
> - * @child_list_head:	list of children sync_pts for this sync_timeline
> - * @child_list_lock:	lock protecting @child_list_head and fence.status
> - * @active_list_head:	list of active (unsignaled/errored) sync_pts
> + * @lock:		lock protecting @child_list_head and fence.status

s/child_list/pt_list/

> + * @pt_list:		list of active (unsignaled/errored) sync_pts
>   * @sync_timeline_list:	membership in global sync_timeline_list
>   */
>  struct sync_timeline {
>  	struct kref		kref;
>  	char			name[32];
>  
> -	/* protected by child_list_lock */
> +	/* protected by lock */
>  	u64			context;
>  	int			value;
>  
> -	struct list_head	child_list_head;
> -	spinlock_t		child_list_lock;
> -
> -	struct list_head	active_list_head;
> +	struct list_head	pt_list;
> +	spinlock_t		lock;
>  
>  	struct list_head	sync_timeline_list;
>  };
>  
>  static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
>  {
> -	return container_of(fence->lock, struct sync_timeline, child_list_lock);
> +	return container_of(fence->lock, struct sync_timeline, lock);
>  }
>  
>  /**
>   * struct sync_pt - sync_pt object
>   * @base: base fence object
> - * @child_list: sync timeline child's list
> - * @active_list: sync timeline active child's list
> + * @link: link on the sync timeline's list
>   */
>  struct sync_pt {
>  	struct dma_fence base;
> -	struct list_head child_list;
> -	struct list_head active_list;
> +	struct list_head link;
>  };
>  
>  #ifdef CONFIG_SW_SYNC
> -- 
> 2.13.1

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] dma-buf/sw-sync: Fix locking around sync_timeline lists

[Chris Wilson]

Quoting Sean Paul (2017-06-29 18:22:10)
> On Thu, Jun 29, 2017 at 01:59:29PM +0100, Chris Wilson wrote:
> > The sync_pt were not adding themselves atomically to the timeline lists,
> > corruption imminent.  Only a single list is required to track the
> > unsignaled sync_pt, so reduce it and rename the lock more appropriately
> > along with using idiomatic names to distinguish a list from links along
> > it.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > ---
> >  drivers/dma-buf/sw_sync.c    | 39 ++++++++++++++-------------------------
> >  drivers/dma-buf/sync_debug.c |  9 ++++-----
> >  drivers/dma-buf/sync_debug.h | 21 ++++++++-------------
> >  3 files changed, 26 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> > index 6effa1ce010e..e51fe11bbbea 100644
> > --- a/drivers/dma-buf/sw_sync.c
> > +++ b/drivers/dma-buf/sw_sync.c
> > @@ -96,9 +96,8 @@ static struct sync_timeline *sync_timeline_create(const char *name)
> >       obj->context = dma_fence_context_alloc(1);
> >       strlcpy(obj->name, name, sizeof(obj->name));
> >  
> > -     INIT_LIST_HEAD(&obj->child_list_head);
> > -     INIT_LIST_HEAD(&obj->active_list_head);
> > -     spin_lock_init(&obj->child_list_lock);
> > +     INIT_LIST_HEAD(&obj->pt_list);
> > +     spin_lock_init(&obj->lock);
> >  
> >       sync_timeline_debug_add(obj);
> >  
> > @@ -139,17 +138,15 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
> >  
> >       trace_sync_timeline(obj);
> >  
> > -     spin_lock_irq(&obj->child_list_lock);
> > +     spin_lock_irq(&obj->lock);
> >  
> >       obj->value += inc;
> >  
> > -     list_for_each_entry_safe(pt, next, &obj->active_list_head,
> > -                              active_list) {
> > +     list_for_each_entry_safe(pt, next, &obj->pt_list, link)
> >               if (dma_fence_is_signaled_locked(&pt->base))
> > -                     list_del_init(&pt->active_list);
> > -     }
> > +                     list_del_init(&pt->link);
> >  
> > -     spin_unlock_irq(&obj->child_list_lock);
> > +     spin_unlock_irq(&obj->lock);
> >  }
> >  
> >  /**
> > @@ -171,15 +168,15 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
> >       if (!pt)
> >               return NULL;
> >  
> > -     spin_lock_irq(&obj->child_list_lock);
> > -
> >       sync_timeline_get(obj);
> > -     dma_fence_init(&pt->base, &timeline_fence_ops, &obj->child_list_lock,
> > +     dma_fence_init(&pt->base, &timeline_fence_ops, &obj->lock,
> >                      obj->context, value);
> > -     list_add_tail(&pt->child_list, &obj->child_list_head);
> > -     INIT_LIST_HEAD(&pt->active_list);
> > +     INIT_LIST_HEAD(&pt->link);
> >  
> > -     spin_unlock_irq(&obj->child_list_lock);
> > +     spin_lock_irq(&obj->lock);
> > +     if (!dma_fence_is_signaled_locked(&pt->base))
> > +             list_add_tail(&pt->link, &obj->pt_list);
> > +     spin_unlock_irq(&obj->lock);
> >  
> >       return pt;
> >  }
> > @@ -204,9 +201,8 @@ static void timeline_fence_release(struct dma_fence *fence)
> >  
> >       spin_lock_irqsave(fence->lock, flags);
> >  
> > -     list_del(&pt->child_list);
> > -     if (!list_empty(&pt->active_list))
> > -             list_del(&pt->active_list);
> > +     if (!list_empty(&pt->link))
> > +             list_del(&pt->link);
> >  
> >       spin_unlock_irqrestore(fence->lock, flags);
> >  
> > @@ -223,13 +219,6 @@ static bool timeline_fence_signaled(struct dma_fence *fence)
> >  
> >  static bool timeline_fence_enable_signaling(struct dma_fence *fence)
> >  {
> > -     struct sync_pt *pt = dma_fence_to_sync_pt(fence);
> > -     struct sync_timeline *parent = dma_fence_parent(fence);
> > -
> > -     if (timeline_fence_signaled(fence))
> > -             return false;
> > -
> > -     list_add_tail(&pt->active_list, &parent->active_list_head);
> >       return true;
> 
> Shouldn't you still return false if the fence is already signaled?

Yes/no :)

In this case, it is immaterial as the only way the timeline can advance
is underneath its big lock and by signaling all the fences. So by the
time dma_fence calls fence->ops->enable_signaling under that same lock
we already know that the fence isn't signaled and can't suddenly be
signaled in the middle of the function call.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] dma-buf/sw-sync: Use an rbtree to sort fences in the timeline

[Sean Paul]

On Thu, Jun 29, 2017 at 01:59:30PM +0100, Chris Wilson wrote:
> Reduce the list iteration when incrementing the timeline by storing the
> fences in increasing order.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> ---
>  drivers/dma-buf/sw_sync.c    | 49 ++++++++++++++++++++++++++++++++++++++------
>  drivers/dma-buf/sync_debug.h |  5 +++++
>  2 files changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index e51fe11bbbea..8cef5d345316 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -96,6 +96,7 @@ static struct sync_timeline *sync_timeline_create(const char *name)
>  	obj->context = dma_fence_context_alloc(1);
>  	strlcpy(obj->name, name, sizeof(obj->name));
>  
> +	obj->pt_tree = RB_ROOT;
>  	INIT_LIST_HEAD(&obj->pt_list);
>  	spin_lock_init(&obj->lock);
>  
> @@ -142,9 +143,13 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>  
>  	obj->value += inc;
>  
> -	list_for_each_entry_safe(pt, next, &obj->pt_list, link)
> -		if (dma_fence_is_signaled_locked(&pt->base))
> -			list_del_init(&pt->link);
> +	list_for_each_entry_safe(pt, next, &obj->pt_list, link) {
> +		if (!dma_fence_is_signaled_locked(&pt->base))
> +			break;
> +
> +		list_del_init(&pt->link);
> +		rb_erase(&pt->node, &obj->pt_tree);
> +	}
>  
>  	spin_unlock_irq(&obj->lock);
>  }
> @@ -174,8 +179,38 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
>  	INIT_LIST_HEAD(&pt->link);
>  
>  	spin_lock_irq(&obj->lock);
> -	if (!dma_fence_is_signaled_locked(&pt->base))
> -		list_add_tail(&pt->link, &obj->pt_list);
> +	if (!dma_fence_is_signaled_locked(&pt->base)) {
> +		struct rb_node **p = &obj->pt_tree.rb_node;
> +		struct rb_node *parent = NULL;
> +
> +		while (*p) {
> +			struct sync_pt *other;
> +			int cmp;
> +
> +			parent = *p;
> +			other = rb_entry(parent, typeof(*pt), node);
> +			cmp = value - other->base.seqno;

We're imposing an implicit limitation on userspace here that points cannot be
> INT_MAX apart (since they'll be in the wrong order in the tree). 

I wonder how much this patch will actually save, given that the number of active points
on a given timeline should be small. Do we have any evidence that this
optimization is warranted?

Sean

> +			if (cmp > 0) {
> +				p = &parent->rb_right;
> +			} else if (cmp < 0) {
> +				p = &parent->rb_left;
> +			} else {
> +				if (dma_fence_get_rcu(&other->base)) {
> +					dma_fence_put(&pt->base);
> +					pt = other;
> +					goto unlock;
> +				}
> +				p = &parent->rb_left;
> +			}
> +		}
> +		rb_link_node(&pt->node, parent, p);
> +		rb_insert_color(&pt->node, &obj->pt_tree);
> +
> +		parent = rb_next(&pt->node);
> +		list_add_tail(&pt->link,
> +			      parent ? &rb_entry(parent, typeof(*pt), node)->link : &obj->pt_list);
> +	}
> +unlock:
>  	spin_unlock_irq(&obj->lock);
>  
>  	return pt;
> @@ -201,8 +236,10 @@ static void timeline_fence_release(struct dma_fence *fence)
>  
>  	spin_lock_irqsave(fence->lock, flags);
>  
> -	if (!list_empty(&pt->link))
> +	if (!list_empty(&pt->link)) {
>  		list_del(&pt->link);
> +		rb_erase(&pt->node, &parent->pt_tree);
> +	}
>  
>  	spin_unlock_irqrestore(fence->lock, flags);
>  
> diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
> index 899ba0e19fd3..b7a5fab12179 100644
> --- a/drivers/dma-buf/sync_debug.h
> +++ b/drivers/dma-buf/sync_debug.h
> @@ -14,6 +14,7 @@
>  #define _LINUX_SYNC_H
>  
>  #include <linux/list.h>
> +#include <linux/rbtree.h>
>  #include <linux/spinlock.h>
>  #include <linux/dma-fence.h>
>  
> @@ -25,6 +26,7 @@
>   * @kref:		reference count on fence.
>   * @name:		name of the sync_timeline. Useful for debugging
>   * @lock:		lock protecting @child_list_head and fence.status
> + * @pt_tree:		rbtree of active (unsignaled/errored) sync_pts
>   * @pt_list:		list of active (unsignaled/errored) sync_pts
>   * @sync_timeline_list:	membership in global sync_timeline_list
>   */
> @@ -36,6 +38,7 @@ struct sync_timeline {
>  	u64			context;
>  	int			value;
>  
> +	struct rb_root		pt_tree;
>  	struct list_head	pt_list;
>  	spinlock_t		lock;
>  
> @@ -51,10 +54,12 @@ static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
>   * struct sync_pt - sync_pt object
>   * @base: base fence object
>   * @link: link on the sync timeline's list
> + * @node: node in the sync timeline's tree
>   */
>  struct sync_pt {
>  	struct dma_fence base;
>  	struct list_head link;
> +	struct rb_node node;
>  };
>  
>  #ifdef CONFIG_SW_SYNC
> -- 
> 2.13.1

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] dma-buf/sw-sync: Use an rbtree to sort fences in the timeline

[Chris Wilson]

Quoting Sean Paul (2017-06-29 19:10:11)
> On Thu, Jun 29, 2017 at 01:59:30PM +0100, Chris Wilson wrote:
> > Reduce the list iteration when incrementing the timeline by storing the
> > fences in increasing order.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > ---
> >  drivers/dma-buf/sw_sync.c    | 49 ++++++++++++++++++++++++++++++++++++++------
> >  drivers/dma-buf/sync_debug.h |  5 +++++
> >  2 files changed, 48 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> > index e51fe11bbbea..8cef5d345316 100644
> > --- a/drivers/dma-buf/sw_sync.c
> > +++ b/drivers/dma-buf/sw_sync.c
> > @@ -96,6 +96,7 @@ static struct sync_timeline *sync_timeline_create(const char *name)
> >       obj->context = dma_fence_context_alloc(1);
> >       strlcpy(obj->name, name, sizeof(obj->name));
> >  
> > +     obj->pt_tree = RB_ROOT;
> >       INIT_LIST_HEAD(&obj->pt_list);
> >       spin_lock_init(&obj->lock);
> >  
> > @@ -142,9 +143,13 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
> >  
> >       obj->value += inc;
> >  
> > -     list_for_each_entry_safe(pt, next, &obj->pt_list, link)
> > -             if (dma_fence_is_signaled_locked(&pt->base))
> > -                     list_del_init(&pt->link);
> > +     list_for_each_entry_safe(pt, next, &obj->pt_list, link) {
> > +             if (!dma_fence_is_signaled_locked(&pt->base))
> > +                     break;
> > +
> > +             list_del_init(&pt->link);
> > +             rb_erase(&pt->node, &obj->pt_tree);
> > +     }
> >  
> >       spin_unlock_irq(&obj->lock);
> >  }
> > @@ -174,8 +179,38 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
> >       INIT_LIST_HEAD(&pt->link);
> >  
> >       spin_lock_irq(&obj->lock);
> > -     if (!dma_fence_is_signaled_locked(&pt->base))
> > -             list_add_tail(&pt->link, &obj->pt_list);
> > +     if (!dma_fence_is_signaled_locked(&pt->base)) {
> > +             struct rb_node **p = &obj->pt_tree.rb_node;
> > +             struct rb_node *parent = NULL;
> > +
> > +             while (*p) {
> > +                     struct sync_pt *other;
> > +                     int cmp;
> > +
> > +                     parent = *p;
> > +                     other = rb_entry(parent, typeof(*pt), node);
> > +                     cmp = value - other->base.seqno;
> 
> We're imposing an implicit limitation on userspace here that points cannot be
> > INT_MAX apart (since they'll be in the wrong order in the tree). 

That's not a new limitation. It's an artifact of using the u32 timeline/seqno.

> I wonder how much this patch will actually save, given that the number of active points
> on a given timeline should be small. Do we have any evidence that this
> optimization is warranted?

The good news is that for empty/small trees, the overhead is tiny, less
than the cost of the syscall. I honestly don't know who uses sw_sync and
so would benefit. I figured I would throw it out here since it was
trivial.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] dma-buf/dma-fence: Extract __dma_fence_is_later()

[Sean Paul]

On Thu, Jun 29, 2017 at 01:59:24PM +0100, Chris Wilson wrote:
> Often we have the task of comparing two seqno known to be on the same
> context, so provide a common __dma_fence_is_later().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Sean Paul <seanpaul@chromium.org>

Hi Chris,
Thanks for writing the code and walking me through it.

Whole set is 
Reviewed-by: Sean Paul <seanpaul@chromium.org>

I'll leave Gustavo or Sumit to apply.

Sean


> Cc: Gustavo Padovan <gustavo@padovan.org>
> ---
>  include/linux/dma-fence.h | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index a5195a7d6f77..ac5987989e9a 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -336,6 +336,19 @@ dma_fence_is_signaled(struct dma_fence *fence)
>  }
>  
>  /**
> + * __dma_fence_is_later - return if f1 is chronologically later than f2
> + * @f1:	[in]	the first fence's seqno
> + * @f2:	[in]	the second fence's seqno from the same context
> + *
> + * Returns true if f1 is chronologically later than f2. Both fences must be
> + * from the same context, since a seqno is not common across contexts.
> + */
> +static inline bool __dma_fence_is_later(u32 f1, u32 f2)
> +{
> +	return (int)(f1 - f2) > 0;
> +}
> +
> +/**
>   * dma_fence_is_later - return if f1 is chronologically later than f2
>   * @f1:	[in]	the first fence from the same context
>   * @f2:	[in]	the second fence from the same context
> @@ -349,7 +362,7 @@ static inline bool dma_fence_is_later(struct dma_fence *f1,
>  	if (WARN_ON(f1->context != f2->context))
>  		return false;
>  
> -	return (int)(f1->seqno - f2->seqno) > 0;
> +	return __dma_fence_is_later(f1->seqno, f2->seqno);
>  }
>  
>  /**
> -- 
> 2.13.1

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] dma-buf/sw-sync: Fix locking around sync_timeline lists

[Chris Wilson]

The sync_pt were not adding themselves atomically to the timeline lists,
corruption imminent.  Only a single list is required to track the
unsignaled sync_pt, so reduce it and rename the lock more appropriately
along with using idiomatic names to distinguish a list from links along
it.

v2: Prevent spinlock recursion on free during create (next patch) and
fixup crossref in kerneldoc

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
---
 drivers/dma-buf/sw_sync.c    | 48 ++++++++++++++++++--------------------------
 drivers/dma-buf/sync_debug.c |  9 ++++-----
 drivers/dma-buf/sync_debug.h | 21 ++++++++-----------
 3 files changed, 31 insertions(+), 47 deletions(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 6effa1ce010e..f20d18c421a3 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -96,9 +96,8 @@ static struct sync_timeline *sync_timeline_create(const char *name)
 	obj->context = dma_fence_context_alloc(1);
 	strlcpy(obj->name, name, sizeof(obj->name));
 
-	INIT_LIST_HEAD(&obj->child_list_head);
-	INIT_LIST_HEAD(&obj->active_list_head);
-	spin_lock_init(&obj->child_list_lock);
+	INIT_LIST_HEAD(&obj->pt_list);
+	spin_lock_init(&obj->lock);
 
 	sync_timeline_debug_add(obj);
 
@@ -139,17 +138,15 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 
 	trace_sync_timeline(obj);
 
-	spin_lock_irq(&obj->child_list_lock);
+	spin_lock_irq(&obj->lock);
 
 	obj->value += inc;
 
-	list_for_each_entry_safe(pt, next, &obj->active_list_head,
-				 active_list) {
+	list_for_each_entry_safe(pt, next, &obj->pt_list, link)
 		if (dma_fence_is_signaled_locked(&pt->base))
-			list_del_init(&pt->active_list);
-	}
+			list_del_init(&pt->link);
 
-	spin_unlock_irq(&obj->child_list_lock);
+	spin_unlock_irq(&obj->lock);
 }
 
 /**
@@ -171,15 +168,15 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
 	if (!pt)
 		return NULL;
 
-	spin_lock_irq(&obj->child_list_lock);
-
 	sync_timeline_get(obj);
-	dma_fence_init(&pt->base, &timeline_fence_ops, &obj->child_list_lock,
+	dma_fence_init(&pt->base, &timeline_fence_ops, &obj->lock,
 		       obj->context, value);
-	list_add_tail(&pt->child_list, &obj->child_list_head);
-	INIT_LIST_HEAD(&pt->active_list);
+	INIT_LIST_HEAD(&pt->link);
 
-	spin_unlock_irq(&obj->child_list_lock);
+	spin_lock_irq(&obj->lock);
+	if (!dma_fence_is_signaled_locked(&pt->base))
+		list_add_tail(&pt->link, &obj->pt_list);
+	spin_unlock_irq(&obj->lock);
 
 	return pt;
 }
@@ -200,15 +197,15 @@ static void timeline_fence_release(struct dma_fence *fence)
 {
 	struct sync_pt *pt = dma_fence_to_sync_pt(fence);
 	struct sync_timeline *parent = dma_fence_parent(fence);
-	unsigned long flags;
 
-	spin_lock_irqsave(fence->lock, flags);
+	if (!list_empty(&pt->link)) {
+		unsigned long flags;
 
-	list_del(&pt->child_list);
-	if (!list_empty(&pt->active_list))
-		list_del(&pt->active_list);
-
-	spin_unlock_irqrestore(fence->lock, flags);
+		spin_lock_irqsave(fence->lock, flags);
+		if (!list_empty(&pt->link))
+			list_del(&pt->link);
+		spin_unlock_irqrestore(fence->lock, flags);
+	}
 
 	sync_timeline_put(parent);
 	dma_fence_free(fence);
@@ -223,13 +220,6 @@ static bool timeline_fence_signaled(struct dma_fence *fence)
 
 static bool timeline_fence_enable_signaling(struct dma_fence *fence)
 {
-	struct sync_pt *pt = dma_fence_to_sync_pt(fence);
-	struct sync_timeline *parent = dma_fence_parent(fence);
-
-	if (timeline_fence_signaled(fence))
-		return false;
-
-	list_add_tail(&pt->active_list, &parent->active_list_head);
 	return true;
 }
 
diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
index 0e91632248ba..2264a075f6a9 100644
--- a/drivers/dma-buf/sync_debug.c
+++ b/drivers/dma-buf/sync_debug.c
@@ -119,13 +119,12 @@ static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj)
 
 	seq_printf(s, "%s: %d\n", obj->name, obj->value);
 
-	spin_lock_irq(&obj->child_list_lock);
-	list_for_each(pos, &obj->child_list_head) {
-		struct sync_pt *pt =
-			container_of(pos, struct sync_pt, child_list);
+	spin_lock_irq(&obj->lock);
+	list_for_each(pos, &obj->pt_list) {
+		struct sync_pt *pt = container_of(pos, struct sync_pt, link);
 		sync_print_fence(s, &pt->base, false);
 	}
-	spin_unlock_irq(&obj->child_list_lock);
+	spin_unlock_irq(&obj->lock);
 }
 
 static void sync_print_sync_file(struct seq_file *s,
diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
index 26fe8b9907b3..6a2a8e69a7d0 100644
--- a/drivers/dma-buf/sync_debug.h
+++ b/drivers/dma-buf/sync_debug.h
@@ -24,42 +24,37 @@
  * struct sync_timeline - sync object
  * @kref:		reference count on fence.
  * @name:		name of the sync_timeline. Useful for debugging
- * @child_list_head:	list of children sync_pts for this sync_timeline
- * @child_list_lock:	lock protecting @child_list_head and fence.status
- * @active_list_head:	list of active (unsignaled/errored) sync_pts
+ * @lock:		lock protecting @pt_list and @value
+ * @pt_list:		list of active (unsignaled/errored) sync_pts
  * @sync_timeline_list:	membership in global sync_timeline_list
  */
 struct sync_timeline {
 	struct kref		kref;
 	char			name[32];
 
-	/* protected by child_list_lock */
+	/* protected by lock */
 	u64			context;
 	int			value;
 
-	struct list_head	child_list_head;
-	spinlock_t		child_list_lock;
-
-	struct list_head	active_list_head;
+	struct list_head	pt_list;
+	spinlock_t		lock;
 
 	struct list_head	sync_timeline_list;
 };
 
 static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
 {
-	return container_of(fence->lock, struct sync_timeline, child_list_lock);
+	return container_of(fence->lock, struct sync_timeline, lock);
 }
 
 /**
  * struct sync_pt - sync_pt object
  * @base: base fence object
- * @child_list: sync timeline child's list
- * @active_list: sync timeline active child's list
+ * @link: link on the sync timeline's list
  */
 struct sync_pt {
 	struct dma_fence base;
-	struct list_head child_list;
-	struct list_head active_list;
+	struct list_head link;
 };
 
 #ifdef CONFIG_SW_SYNC
-- 
2.13.2

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

[PATCH v2] dma-buf/sw-sync: Use an rbtree to sort fences in the timeline

[Chris Wilson]

Reduce the list iteration when incrementing the timeline by storing the
fences in increasing order.

v2: Prevent spinlock recursion on free during create

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
---
 drivers/dma-buf/sw_sync.c    | 49 ++++++++++++++++++++++++++++++++++++++------
 drivers/dma-buf/sync_debug.h |  9 ++++++++
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index f20d18c421a3..af1bc84802e5 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -96,6 +96,7 @@ static struct sync_timeline *sync_timeline_create(const char *name)
 	obj->context = dma_fence_context_alloc(1);
 	strlcpy(obj->name, name, sizeof(obj->name));
 
+	obj->pt_tree = RB_ROOT;
 	INIT_LIST_HEAD(&obj->pt_list);
 	spin_lock_init(&obj->lock);
 
@@ -142,9 +143,13 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 
 	obj->value += inc;
 
-	list_for_each_entry_safe(pt, next, &obj->pt_list, link)
-		if (dma_fence_is_signaled_locked(&pt->base))
-			list_del_init(&pt->link);
+	list_for_each_entry_safe(pt, next, &obj->pt_list, link) {
+		if (!dma_fence_is_signaled_locked(&pt->base))
+			break;
+
+		list_del_init(&pt->link);
+		rb_erase(&pt->node, &obj->pt_tree);
+	}
 
 	spin_unlock_irq(&obj->lock);
 }
@@ -174,8 +179,38 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
 	INIT_LIST_HEAD(&pt->link);
 
 	spin_lock_irq(&obj->lock);
-	if (!dma_fence_is_signaled_locked(&pt->base))
-		list_add_tail(&pt->link, &obj->pt_list);
+	if (!dma_fence_is_signaled_locked(&pt->base)) {
+		struct rb_node **p = &obj->pt_tree.rb_node;
+		struct rb_node *parent = NULL;
+
+		while (*p) {
+			struct sync_pt *other;
+			int cmp;
+
+			parent = *p;
+			other = rb_entry(parent, typeof(*pt), node);
+			cmp = value - other->base.seqno;
+			if (cmp > 0) {
+				p = &parent->rb_right;
+			} else if (cmp < 0) {
+				p = &parent->rb_left;
+			} else {
+				if (dma_fence_get_rcu(&other->base)) {
+					dma_fence_put(&pt->base);
+					pt = other;
+					goto unlock;
+				}
+				p = &parent->rb_left;
+			}
+		}
+		rb_link_node(&pt->node, parent, p);
+		rb_insert_color(&pt->node, &obj->pt_tree);
+
+		parent = rb_next(&pt->node);
+		list_add_tail(&pt->link,
+			      parent ? &rb_entry(parent, typeof(*pt), node)->link : &obj->pt_list);
+	}
+unlock:
 	spin_unlock_irq(&obj->lock);
 
 	return pt;
@@ -202,8 +237,10 @@ static void timeline_fence_release(struct dma_fence *fence)
 		unsigned long flags;
 
 		spin_lock_irqsave(fence->lock, flags);
-		if (!list_empty(&pt->link))
+		if (!list_empty(&pt->link)) {
 			list_del(&pt->link);
+			rb_erase(&pt->node, &parent->pt_tree);
+		}
 		spin_unlock_irqrestore(fence->lock, flags);
 	}
 
diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
index 6a2a8e69a7d0..363c0a717b77 100644
--- a/drivers/dma-buf/sync_debug.h
+++ b/drivers/dma-buf/sync_debug.h
@@ -14,6 +14,7 @@
 #define _LINUX_SYNC_H
 
 #include <linux/list.h>
+#include <linux/rbtree.h>
 #include <linux/spinlock.h>
 #include <linux/dma-fence.h>
 
@@ -24,7 +25,12 @@
  * struct sync_timeline - sync object
  * @kref:		reference count on fence.
  * @name:		name of the sync_timeline. Useful for debugging
+<<<<<<< HEAD
  * @lock:		lock protecting @pt_list and @value
+=======
+ * @lock:		lock protecting @child_list_head and fence.status
+ * @pt_tree:		rbtree of active (unsignaled/errored) sync_pts
+>>>>>>> 0f78df23b9ec... dma-buf/sw-sync: Use an rbtree to sort fences in the timeline
  * @pt_list:		list of active (unsignaled/errored) sync_pts
  * @sync_timeline_list:	membership in global sync_timeline_list
  */
@@ -36,6 +42,7 @@ struct sync_timeline {
 	u64			context;
 	int			value;
 
+	struct rb_root		pt_tree;
 	struct list_head	pt_list;
 	spinlock_t		lock;
 
@@ -51,10 +58,12 @@ static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
  * struct sync_pt - sync_pt object
  * @base: base fence object
  * @link: link on the sync timeline's list
+ * @node: node in the sync timeline's tree
  */
 struct sync_pt {
 	struct dma_fence base;
 	struct list_head link;
+	struct rb_node node;
 };
 
 #ifdef CONFIG_SW_SYNC
-- 
2.13.2

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

[PATCH v3] dma-buf/sw-sync: Use an rbtree to sort fences in the timeline

[Chris Wilson]

Reduce the list iteration when incrementing the timeline by storing the
fences in increasing order.

v2: Prevent spinlock recursion on free during create
v3: Fixup rebase conflict inside comments that escaped the compiler.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
---
 drivers/dma-buf/sw_sync.c    | 49 ++++++++++++++++++++++++++++++++++++++------
 drivers/dma-buf/sync_debug.h |  5 +++++
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index f20d18c421a3..af1bc84802e5 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -96,6 +96,7 @@ static struct sync_timeline *sync_timeline_create(const char *name)
 	obj->context = dma_fence_context_alloc(1);
 	strlcpy(obj->name, name, sizeof(obj->name));
 
+	obj->pt_tree = RB_ROOT;
 	INIT_LIST_HEAD(&obj->pt_list);
 	spin_lock_init(&obj->lock);
 
@@ -142,9 +143,13 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 
 	obj->value += inc;
 
-	list_for_each_entry_safe(pt, next, &obj->pt_list, link)
-		if (dma_fence_is_signaled_locked(&pt->base))
-			list_del_init(&pt->link);
+	list_for_each_entry_safe(pt, next, &obj->pt_list, link) {
+		if (!dma_fence_is_signaled_locked(&pt->base))
+			break;
+
+		list_del_init(&pt->link);
+		rb_erase(&pt->node, &obj->pt_tree);
+	}
 
 	spin_unlock_irq(&obj->lock);
 }
@@ -174,8 +179,38 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
 	INIT_LIST_HEAD(&pt->link);
 
 	spin_lock_irq(&obj->lock);
-	if (!dma_fence_is_signaled_locked(&pt->base))
-		list_add_tail(&pt->link, &obj->pt_list);
+	if (!dma_fence_is_signaled_locked(&pt->base)) {
+		struct rb_node **p = &obj->pt_tree.rb_node;
+		struct rb_node *parent = NULL;
+
+		while (*p) {
+			struct sync_pt *other;
+			int cmp;
+
+			parent = *p;
+			other = rb_entry(parent, typeof(*pt), node);
+			cmp = value - other->base.seqno;
+			if (cmp > 0) {
+				p = &parent->rb_right;
+			} else if (cmp < 0) {
+				p = &parent->rb_left;
+			} else {
+				if (dma_fence_get_rcu(&other->base)) {
+					dma_fence_put(&pt->base);
+					pt = other;
+					goto unlock;
+				}
+				p = &parent->rb_left;
+			}
+		}
+		rb_link_node(&pt->node, parent, p);
+		rb_insert_color(&pt->node, &obj->pt_tree);
+
+		parent = rb_next(&pt->node);
+		list_add_tail(&pt->link,
+			      parent ? &rb_entry(parent, typeof(*pt), node)->link : &obj->pt_list);
+	}
+unlock:
 	spin_unlock_irq(&obj->lock);
 
 	return pt;
@@ -202,8 +237,10 @@ static void timeline_fence_release(struct dma_fence *fence)
 		unsigned long flags;
 
 		spin_lock_irqsave(fence->lock, flags);
-		if (!list_empty(&pt->link))
+		if (!list_empty(&pt->link)) {
 			list_del(&pt->link);
+			rb_erase(&pt->node, &parent->pt_tree);
+		}
 		spin_unlock_irqrestore(fence->lock, flags);
 	}
 
diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
index 6a2a8e69a7d0..d615a89f774c 100644
--- a/drivers/dma-buf/sync_debug.h
+++ b/drivers/dma-buf/sync_debug.h
@@ -14,6 +14,7 @@
 #define _LINUX_SYNC_H
 
 #include <linux/list.h>
+#include <linux/rbtree.h>
 #include <linux/spinlock.h>
 #include <linux/dma-fence.h>
 
@@ -25,6 +26,7 @@
  * @kref:		reference count on fence.
  * @name:		name of the sync_timeline. Useful for debugging
  * @lock:		lock protecting @pt_list and @value
+ * @pt_tree:		rbtree of active (unsignaled/errored) sync_pts
  * @pt_list:		list of active (unsignaled/errored) sync_pts
  * @sync_timeline_list:	membership in global sync_timeline_list
  */
@@ -36,6 +38,7 @@ struct sync_timeline {
 	u64			context;
 	int			value;
 
+	struct rb_root		pt_tree;
 	struct list_head	pt_list;
 	spinlock_t		lock;
 
@@ -51,10 +54,12 @@ static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
  * struct sync_pt - sync_pt object
  * @base: base fence object
  * @link: link on the sync timeline's list
+ * @node: node in the sync timeline's tree
  */
 struct sync_pt {
 	struct dma_fence base;
 	struct list_head link;
+	struct rb_node node;
 };
 
 #ifdef CONFIG_SW_SYNC
-- 
2.13.2

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

✓ Fi.CI.BAT: success for series starting with [1/7] dma-buf/dma-fence: Extract __dma_fence_is_later() (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [1/7] dma-buf/dma-fence: Extract __dma_fence_is_later() (rev4)
URL   : https://patchwork.freedesktop.org/series/26551/
State : success

== Summary ==

Series 26551v4 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/26551/revisions/4/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-kbl-7560u) fdo#100125
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-pnv-d510) fdo#101597 +1

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597

fi-bdw-5557u     total:279  pass:264  dwarn:4   dfail:0   fail:0   skip:11  time:444s
fi-bdw-gvtdvm    total:279  pass:257  dwarn:8   dfail:0   fail:0   skip:14  time:427s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:355s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:522s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:509s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:486s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:481s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:593s
fi-hsw-4770      total:279  pass:259  dwarn:4   dfail:0   fail:0   skip:16  time:436s
fi-hsw-4770r     total:279  pass:259  dwarn:4   dfail:0   fail:0   skip:16  time:409s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:416s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:494s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:477s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:570s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:586s
fi-pnv-d510      total:279  pass:221  dwarn:3   dfail:0   fail:0   skip:55  time:557s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:463s
fi-skl-6700hq    total:279  pass:223  dwarn:1   dfail:0   fail:30  skip:24  time:342s
fi-skl-6700k     total:279  pass:257  dwarn:4   dfail:0   fail:0   skip:18  time:461s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:477s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:432s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:542s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:404s

1da9a6d8d2402de8798b36d75bdd7d8b49728a5e drm-tip: 2017y-06m-29d-20h-16m-45s UTC integration manifest
3a3dede dma-buf/sw-sync: sync_pt is private and of fixed size
3475165b dma-buf/sw-sync: Reduce irqsave/irqrestore from known context
43e6ab0 dma-buf/sw-sync: Prevent user overflow on timeline advance
a6dc7ca dma-buf/sw-sync: Fix the is-signaled test to handle u32 wraparound
8ba4a70 dma-buf/dma-fence: Extract __dma_fence_is_later()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5076/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] dma-buf/dma-fence: Extract __dma_fence_is_later()

[Gustavo Padovan]

Hi,

2017-06-29 Sean Paul <seanpaul@chromium.org>:

> On Thu, Jun 29, 2017 at 01:59:24PM +0100, Chris Wilson wrote:
> > Often we have the task of comparing two seqno known to be on the same
> > context, so provide a common __dma_fence_is_later().
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: Sean Paul <seanpaul@chromium.org>
> 
> Hi Chris,
> Thanks for writing the code and walking me through it.
> 
> Whole set is 
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> 
> I'll leave Gustavo or Sumit to apply.

Pushed all to drm-misc-next. Thanks!

Gustavo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx