[PATCH 0/7] de-stage SW_SYNC validation frawework

[PATCH 0/7] de-stage SW_SYNC validation frawework

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Hi Greg,

This is the last step in the Sync Framwork de-stage task. It de-stage
the SW_SYNC validation framework and the sync_debug info debugfs file.

The first 3 patches are clean up and improvements and the rest is preparation
to de-stage and then finally the actual de-stage.

Please review,

Gustavo

---
Gustavo Padovan (7):
  staging/android: move trace/sync.h to sync_trace.h
  staging/android: remove doc from sw_sync
  staging/android: display sync_pt name on debugfs
  staging/android: do not let userspace trigger WARN_ON
  staging/android: prepare sw_sync files for de-staging
  dma-buf/sw_sync: de-stage SW_SYNC
  staging/android: remove sync framework TODO

 drivers/dma-buf/Kconfig                            | 14 +++++++
 drivers/dma-buf/Makefile                           |  1 +
 drivers/{staging/android => dma-buf}/sw_sync.c     | 46 +++-------------------
 drivers/{staging/android => dma-buf}/sync_debug.c  |  7 ++--
 drivers/{staging/android => dma-buf}/sync_debug.h  | 26 +++++-------
 .../android/trace/sync.h => dma-buf/sync_trace.h}  |  6 +--
 drivers/staging/android/Kconfig                    | 13 ------
 drivers/staging/android/Makefile                   |  1 -
 drivers/staging/android/TODO                       |  8 ----
 9 files changed, 38 insertions(+), 84 deletions(-)
 rename drivers/{staging/android => dma-buf}/sw_sync.c (84%)
 rename drivers/{staging/android => dma-buf}/sync_debug.c (97%)
 rename drivers/{staging/android => dma-buf}/sync_debug.h (72%)
 rename drivers/{staging/android/trace/sync.h => dma-buf/sync_trace.h} (84%)

-- 
2.5.5

[PATCH 1/7] staging/android: remove doc from sw_sync

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

SW_SYNC should never be used by other pieces of the kernel apart from
sync_debug as it is only a Sync File Validation Framework, so hide any
info to avoid confuse this with a standard kernel internal API.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/staging/android/sw_sync.c    | 26 --------------------------
 drivers/staging/android/sync_debug.h | 15 ---------------
 2 files changed, 41 deletions(-)

diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
index 115c917..b4ae092 100644
--- a/drivers/staging/android/sw_sync.c
+++ b/drivers/staging/android/sw_sync.c
@@ -46,13 +46,6 @@ static inline struct sync_pt *fence_to_sync_pt(struct fence *fence)
 	return container_of(fence, struct sync_pt, base);
 }
 
-/**
- * sync_timeline_create() - creates a sync object
- * @name:	sync_timeline name
- *
- * Creates a new sync_timeline. Returns the sync_timeline object or NULL in
- * case of error.
- */
 struct sync_timeline *sync_timeline_create(const char *name)
 {
 	struct sync_timeline *obj;
@@ -94,14 +87,6 @@ static void sync_timeline_put(struct sync_timeline *obj)
 	kref_put(&obj->kref, sync_timeline_free);
 }
 
-/**
- * sync_timeline_signal() - signal a status change on a sync_timeline
- * @obj:	sync_timeline to signal
- * @inc:	num to increment on timeline->value
- *
- * A sync implementation should call this any time one of it's fences
- * has signaled or has an error condition.
- */
 static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 {
 	unsigned long flags;
@@ -122,17 +107,6 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 	spin_unlock_irqrestore(&obj->child_list_lock, flags);
 }
 
-/**
- * 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
- * allocated allowing for implementation specific data to be kept after
- * 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)
 {
diff --git a/drivers/staging/android/sync_debug.h b/drivers/staging/android/sync_debug.h
index 425ebc5..c44f447 100644
--- a/drivers/staging/android/sync_debug.h
+++ b/drivers/staging/android/sync_debug.h
@@ -20,15 +20,6 @@
 #include <linux/sync_file.h>
 #include <uapi/linux/sync_file.h>
 
-/**
- * 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
- * @sync_timeline_list:	membership in global sync_timeline_list
- */
 struct sync_timeline {
 	struct kref		kref;
 	char			name[32];
@@ -50,12 +41,6 @@ static inline struct sync_timeline *fence_parent(struct fence *fence)
 			    child_list_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
- */
 struct sync_pt {
 	struct fence base;
 	struct list_head child_list;
-- 
2.5.5

[PATCH 2/7] staging/android: display sync_pt name on debugfs

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

When creating a sync_pt the name received wasn't used anywhere.
Now we add it to the sync info debug output to make it easier to indetify
the userspace name of that sync pt.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/staging/android/sw_sync.c    | 16 ++++------------
 drivers/staging/android/sync_debug.c |  5 +++--
 drivers/staging/android/sync_debug.h |  9 +++++++++
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
index b4ae092..ea27512 100644
--- a/drivers/staging/android/sw_sync.c
+++ b/drivers/staging/android/sw_sync.c
@@ -37,15 +37,6 @@ struct sw_sync_create_fence_data {
 		struct sw_sync_create_fence_data)
 #define SW_SYNC_IOC_INC			_IOW(SW_SYNC_IOC_MAGIC, 1, __u32)
 
-static const struct fence_ops timeline_fence_ops;
-
-static inline struct sync_pt *fence_to_sync_pt(struct fence *fence)
-{
-	if (fence->ops != &timeline_fence_ops)
-		return NULL;
-	return container_of(fence, struct sync_pt, base);
-}
-
 struct sync_timeline *sync_timeline_create(const char *name)
 {
 	struct sync_timeline *obj;
@@ -108,7 +99,7 @@ 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 int value, char *name)
 {
 	unsigned long flags;
 	struct sync_pt *pt;
@@ -120,6 +111,7 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, int size,
 	if (!pt)
 		return NULL;
 
+	strlcpy(pt->name, name, sizeof(pt->name));
 	spin_lock_irqsave(&obj->child_list_lock, flags);
 	sync_timeline_get(obj);
 	fence_init(&pt->base, &timeline_fence_ops, &obj->child_list_lock,
@@ -191,7 +183,7 @@ static void timeline_fence_timeline_value_str(struct fence *fence,
 	snprintf(str, size, "%d", parent->value);
 }
 
-static const struct fence_ops timeline_fence_ops = {
+const struct fence_ops timeline_fence_ops = {
 	.get_driver_name = timeline_fence_get_driver_name,
 	.get_timeline_name = timeline_fence_get_timeline_name,
 	.enable_signaling = timeline_fence_enable_signaling,
@@ -252,7 +244,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, sizeof(*pt), data.value, data.name);
 	if (!pt) {
 		err = -ENOMEM;
 		goto err;
diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
index 4c5a855..b732ea3 100644
--- a/drivers/staging/android/sync_debug.c
+++ b/drivers/staging/android/sync_debug.c
@@ -75,13 +75,14 @@ static void sync_print_fence(struct seq_file *s, struct fence *fence, bool show)
 {
 	int status = 1;
 	struct sync_timeline *parent = fence_parent(fence);
+	struct sync_pt *pt = fence_to_sync_pt(fence);
 
 	if (fence_is_signaled_locked(fence))
 		status = fence->status;
 
-	seq_printf(s, "  %s%sfence %s",
+	seq_printf(s, "  %s%sfence %s %s",
 		   show ? parent->name : "",
-		   show ? "_" : "",
+		   show ? "_" : "", pt->name,
 		   sync_status_str(status));
 
 	if (status <= 0) {
diff --git a/drivers/staging/android/sync_debug.h b/drivers/staging/android/sync_debug.h
index c44f447..c14587c 100644
--- a/drivers/staging/android/sync_debug.h
+++ b/drivers/staging/android/sync_debug.h
@@ -43,10 +43,19 @@ static inline struct sync_timeline *fence_parent(struct fence *fence)
 
 struct sync_pt {
 	struct fence base;
+	char name[32];
 	struct list_head child_list;
 	struct list_head active_list;
 };
 
+extern const struct fence_ops timeline_fence_ops;
+static inline struct sync_pt *fence_to_sync_pt(struct fence *fence)
+{
+	if (fence->ops != &timeline_fence_ops)
+		return NULL;
+	return container_of(fence, struct sync_pt, base);
+}
+
 #ifdef CONFIG_SW_SYNC
 
 extern const struct file_operations sw_sync_debugfs_fops;
-- 
2.5.5

[PATCH 3/7] staging/android: do not let userspace trigger WARN_ON

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Closing the timeline without waiting all fences to signal is not
a critical failure, it is just bad usage from userspace so avoid
calling WARN_ON in this case.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/staging/android/sw_sync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
index ea27512..66837ca 100644
--- a/drivers/staging/android/sw_sync.c
+++ b/drivers/staging/android/sw_sync.c
@@ -142,7 +142,7 @@ static void timeline_fence_release(struct fence *fence)
 
 	spin_lock_irqsave(fence->lock, flags);
 	list_del(&pt->child_list);
-	if (WARN_ON_ONCE(!list_empty(&pt->active_list)))
+	if (!list_empty(&pt->active_list))
 		list_del(&pt->active_list);
 	spin_unlock_irqrestore(fence->lock, flags);
 
-- 
2.5.5

[PATCH 4/7] staging/android: move trace/sync.h to sync_trace.h

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

The common behaviour for trace headers is to have them in the same folder
they are used, instead of creating a special trace/ directory.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/staging/android/sw_sync.c                      | 2 +-
 drivers/staging/android/{trace/sync.h => sync_trace.h} | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)
 rename drivers/staging/android/{trace/sync.h => sync_trace.h} (84%)

diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
index 66837ca..25196f5 100644
--- a/drivers/staging/android/sw_sync.c
+++ b/drivers/staging/android/sw_sync.c
@@ -23,7 +23,7 @@
 #include "sync_debug.h"
 
 #define CREATE_TRACE_POINTS
-#include "trace/sync.h"
+#include "sync_trace.h"
 
 struct sw_sync_create_fence_data {
 	__u32	value;
diff --git a/drivers/staging/android/trace/sync.h b/drivers/staging/android/sync_trace.h
similarity index 84%
rename from drivers/staging/android/trace/sync.h
rename to drivers/staging/android/sync_trace.h
index 6b5ce96..ea485f7 100644
--- a/drivers/staging/android/trace/sync.h
+++ b/drivers/staging/android/sync_trace.h
@@ -1,11 +1,11 @@
 #undef TRACE_SYSTEM
-#define TRACE_INCLUDE_PATH ../../drivers/staging/android/trace
-#define TRACE_SYSTEM sync
+#define TRACE_INCLUDE_PATH ../../drivers/staging/android
+#define TRACE_SYSTEM sync_trace
 
 #if !defined(_TRACE_SYNC_H) || defined(TRACE_HEADER_MULTI_READ)
 #define _TRACE_SYNC_H
 
-#include "../sync_debug.h"
+#include "sync_debug.h"
 #include <linux/tracepoint.h>
 
 TRACE_EVENT(sync_timeline,
-- 
2.5.5

[PATCH 5/7] staging/android: prepare sw_sync files for de-staging

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Fix paths in the comments.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/staging/android/sync_debug.c | 2 +-
 drivers/staging/android/sync_debug.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
index b732ea3..5f0d41e 100644
--- a/drivers/staging/android/sync_debug.c
+++ b/drivers/staging/android/sync_debug.c
@@ -1,5 +1,5 @@
 /*
- * drivers/base/sync.c
+ * drivers/dma-buf/sync_debug.c
  *
  * Copyright (C) 2012 Google, Inc.
  *
diff --git a/drivers/staging/android/sync_debug.h b/drivers/staging/android/sync_debug.h
index c14587c..e359047 100644
--- a/drivers/staging/android/sync_debug.h
+++ b/drivers/staging/android/sync_debug.h
@@ -1,5 +1,5 @@
 /*
- * include/linux/sync.h
+ * drivers/dma-buf/sync_debug.h
  *
  * Copyright (C) 2012 Google, Inc.
  *
-- 
2.5.5

[PATCH 6/7] dma-buf/sw_sync: de-stage SW_SYNC

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

SW_SYNC allows to run tests on the sync_file framework via debugfs on

<debugfs>/sync/sw_sync

Opening and closing the file triggers creation and release of a sync
timeline. To create fences on this timeline the SW_SYNC_IOC_CREATE_FENCE
ioctl should be used. To increment the timeline value use SW_SYNC_IOC_INC.

Also it exports Sync information on

<debugfs>/sync/info

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/dma-buf/Kconfig                           | 14 ++++++++++++++
 drivers/dma-buf/Makefile                          |  1 +
 drivers/{staging/android => dma-buf}/sw_sync.c    |  0
 drivers/{staging/android => dma-buf}/sync_debug.c |  0
 drivers/{staging/android => dma-buf}/sync_debug.h |  0
 drivers/{staging/android => dma-buf}/sync_trace.h |  2 +-
 drivers/staging/android/Kconfig                   | 13 -------------
 drivers/staging/android/Makefile                  |  1 -
 8 files changed, 16 insertions(+), 15 deletions(-)
 rename drivers/{staging/android => dma-buf}/sw_sync.c (100%)
 rename drivers/{staging/android => dma-buf}/sync_debug.c (100%)
 rename drivers/{staging/android => dma-buf}/sync_debug.h (100%)
 rename drivers/{staging/android => dma-buf}/sync_trace.h (92%)

diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 9824bc4..7227022 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -8,4 +8,18 @@ config SYNC_FILE
 	---help---
 	  This option enables the fence framework synchronization to export
 	  sync_files to userspace that can represent one or more fences.
+
+config SW_SYNC
+	bool "Sync File Validation Framework"
+	default n
+	depends on SYNC_FILE
+	depends on DEBUG_FS
+	---help---
+	  A sync object driver that uses a 32bit counter to coordinate
+	  synchronization.  Useful when there is no hardware primitive backing
+	  the synchronization.
+
+	  WARNING: improper use of this can result in deadlocking kernel
+	  drivers from userspace. Intended for test and debug only.
+
 endmenu
diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
index 4a424ec..d2d02fc 100644
--- a/drivers/dma-buf/Makefile
+++ b/drivers/dma-buf/Makefile
@@ -1,2 +1,3 @@
 obj-y := dma-buf.o fence.o reservation.o seqno-fence.o
 obj-$(CONFIG_SYNC_FILE)		+= sync_file.o
+obj-$(CONFIG_SW_SYNC)		+= sw_sync.o sync_debug.o
diff --git a/drivers/staging/android/sw_sync.c b/drivers/dma-buf/sw_sync.c
similarity index 100%
rename from drivers/staging/android/sw_sync.c
rename to drivers/dma-buf/sw_sync.c
diff --git a/drivers/staging/android/sync_debug.c b/drivers/dma-buf/sync_debug.c
similarity index 100%
rename from drivers/staging/android/sync_debug.c
rename to drivers/dma-buf/sync_debug.c
diff --git a/drivers/staging/android/sync_debug.h b/drivers/dma-buf/sync_debug.h
similarity index 100%
rename from drivers/staging/android/sync_debug.h
rename to drivers/dma-buf/sync_debug.h
diff --git a/drivers/staging/android/sync_trace.h b/drivers/dma-buf/sync_trace.h
similarity index 92%
rename from drivers/staging/android/sync_trace.h
rename to drivers/dma-buf/sync_trace.h
index ea485f7..d13d59f 100644
--- a/drivers/staging/android/sync_trace.h
+++ b/drivers/dma-buf/sync_trace.h
@@ -1,5 +1,5 @@
 #undef TRACE_SYSTEM
-#define TRACE_INCLUDE_PATH ../../drivers/staging/android
+#define TRACE_INCLUDE_PATH ../../drivers/dma-buf
 #define TRACE_SYSTEM sync_trace
 
 #if !defined(_TRACE_SYNC_H) || defined(TRACE_HEADER_MULTI_READ)
diff --git a/drivers/staging/android/Kconfig b/drivers/staging/android/Kconfig
index 06e41d2..6c00d6f 100644
--- a/drivers/staging/android/Kconfig
+++ b/drivers/staging/android/Kconfig
@@ -24,19 +24,6 @@ config ANDROID_LOW_MEMORY_KILLER
 	  scripts (/init.rc), and it defines priority values with minimum free memory size
 	  for each priority.
 
-config SW_SYNC
-	bool "Software synchronization framework"
-	default n
-	depends on SYNC_FILE
-	depends on DEBUG_FS
-	---help---
-	  A sync object driver that uses a 32bit counter to coordinate
-	  synchronization.  Useful when there is no hardware primitive backing
-	  the synchronization.
-
-	  WARNING: improper use of this can result in deadlocking kernel
-	  drivers from userspace. Intended for test and debug only.
-
 source "drivers/staging/android/ion/Kconfig"
 
 endif # if ANDROID
diff --git a/drivers/staging/android/Makefile b/drivers/staging/android/Makefile
index 7ca61b7..7ed1be7 100644
--- a/drivers/staging/android/Makefile
+++ b/drivers/staging/android/Makefile
@@ -4,4 +4,3 @@ obj-y					+= ion/
 
 obj-$(CONFIG_ASHMEM)			+= ashmem.o
 obj-$(CONFIG_ANDROID_LOW_MEMORY_KILLER)	+= lowmemorykiller.o
-obj-$(CONFIG_SW_SYNC)			+= sw_sync.o sync_debug.o
-- 
2.5.5

[PATCH 7/7] staging/android: remove sync framework TODO

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Sync Framework was de-staged to drivers/dma-buf/, so remove it entries
in the TODO file.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/staging/android/TODO | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
index 64d8c87..8f3ac37 100644
--- a/drivers/staging/android/TODO
+++ b/drivers/staging/android/TODO
@@ -25,13 +25,5 @@ ion/
    exposes existing cma regions and doesn't reserve unecessarily memory when
    booting a system which doesn't use ion.
 
-sync framework:
- - remove CONFIG_SW_SYNC_USER, it is used only for testing/debugging and
- should not be upstreamed.
- - port CONFIG_SW_SYNC_USER tests interfaces to use debugfs somehow
- - port libsync tests to kselftest
- - clean up and ABI check for security issues
- - move it to drivers/base/dma-buf
-
 Please send patches to Greg Kroah-Hartman <greg@kroah.com> and Cc:
 Arve Hjønnevåg <arve@android.com> and Riley Andrews <riandrews@android.com>
-- 
2.5.5

Re: [PATCH 5/7] staging/android: prepare sw_sync files for de-staging

[Joe Perches]

On Mon, 2016-06-20 at 12:53 -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Fix paths in the comments.

Why is it useful to have the path or filename embedded
in the file at
all?

Re: [PATCH 5/7] staging/android: prepare sw_sync files for de-staging

[Gustavo Padovan]

2016-06-20 Joe Perches <joe@perches.com>:

> On Mon, 2016-06-20 at 12:53 -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Fix paths in the comments.
> 
> Why is it useful to have the path or filename embedded
> in the file at
> all?

I just kept it as is. Thinking about this now I don't see
this as useful, I'll send v2 removing it.

	Gustavo

[PATCH v2] staging/android: prepare sw_sync files for de-staging

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

remove file paths in the comments and add short description about each
file.

v2: remove file paths instead of just change them.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/staging/android/sw_sync.c    | 2 +-
 drivers/staging/android/sync_debug.c | 2 +-
 drivers/staging/android/sync_debug.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
index 25196f5..a355e9b 100644
--- a/drivers/staging/android/sw_sync.c
+++ b/drivers/staging/android/sw_sync.c
@@ -1,5 +1,5 @@
 /*
- * drivers/dma-buf/sw_sync.c
+ * Sync File validation framework
  *
  * Copyright (C) 2012 Google, Inc.
  *
diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
index b732ea3..03bcc6f 100644
--- a/drivers/staging/android/sync_debug.c
+++ b/drivers/staging/android/sync_debug.c
@@ -1,5 +1,5 @@
 /*
- * drivers/base/sync.c
+ * Sync File validation framework and debug information
  *
  * Copyright (C) 2012 Google, Inc.
  *
diff --git a/drivers/staging/android/sync_debug.h b/drivers/staging/android/sync_debug.h
index c14587c..63cfcff 100644
--- a/drivers/staging/android/sync_debug.h
+++ b/drivers/staging/android/sync_debug.h
@@ -1,5 +1,5 @@
 /*
- * include/linux/sync.h
+ * Sync File validation framework
  *
  * Copyright (C) 2012 Google, Inc.
  *
-- 
2.5.5

Re: [PATCH 7/7] staging/android: remove sync framework TODO

[Emil Velikov]

Hi Gustavo,

On 20 June 2016 at 16:53, Gustavo Padovan <gustavo@padovan.org> wrote:
> - - port libsync tests to kselftest

I believe the tests haven't landed yet right, so this should stay right ?

-Emil

Re: [PATCH 7/7] staging/android: remove sync framework TODO

[Gustavo Padovan]

2016-06-23 Emil Velikov <emil.l.velikov@gmail.com>:

> Hi Gustavo,
> 
> On 20 June 2016 at 16:53, Gustavo Padovan <gustavo@padovan.org> wrote:
> > - - port libsync tests to kselftest
> 
> I believe the tests haven't landed yet right, so this should stay right ?

Yes, you are right. That part is still missing in upstream.

	Gustavo

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Pavel Machek]

Hi!

> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Hi Greg,
> 
> This is the last step in the Sync Framwork de-stage task. It

Typo: "fram_e_work"

> de-stage
> the SW_SYNC validation framework and the sync_debug info debugfs file.
> 
> The first 3 patches are clean up and improvements and the rest is preparation
> to de-stage and then finally the actual de-stage.

Could we get some kind of description what the sync framework does?
There are no useful comments in /sw_sync.c. There's no Documentation/
files. I don't know what this is supposed to do...

								Pavel

> Please review,
> 
> Gustavo
> 
> ---
> Gustavo Padovan (7):
>   staging/android: move trace/sync.h to sync_trace.h
>   staging/android: remove doc from sw_sync
>   staging/android: display sync_pt name on debugfs
>   staging/android: do not let userspace trigger WARN_ON
>   staging/android: prepare sw_sync files for de-staging
>   dma-buf/sw_sync: de-stage SW_SYNC
>   staging/android: remove sync framework TODO
> 
>  drivers/dma-buf/Kconfig                            | 14 +++++++
>  drivers/dma-buf/Makefile                           |  1 +
>  drivers/{staging/android => dma-buf}/sw_sync.c     | 46 +++-------------------
>  drivers/{staging/android => dma-buf}/sync_debug.c  |  7 ++--
>  drivers/{staging/android => dma-buf}/sync_debug.h  | 26 +++++-------
>  .../android/trace/sync.h => dma-buf/sync_trace.h}  |  6 +--
>  drivers/staging/android/Kconfig                    | 13 ------
>  drivers/staging/android/Makefile                   |  1 -
>  drivers/staging/android/TODO                       |  8 ----
>  9 files changed, 38 insertions(+), 84 deletions(-)
>  rename drivers/{staging/android => dma-buf}/sw_sync.c (84%)
>  rename drivers/{staging/android => dma-buf}/sync_debug.c (97%)
>  rename drivers/{staging/android => dma-buf}/sync_debug.h (72%)
>  rename drivers/{staging/android/trace/sync.h => dma-buf/sync_trace.h} (84%)
> 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Gustavo Padovan]

Hi Greg,

Any comment on this?

Thanks,

Gustavo

2016-06-20 Gustavo Padovan <gustavo@padovan.org>:

> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Hi Greg,
> 
> This is the last step in the Sync Framwork de-stage task. It de-stage
> the SW_SYNC validation framework and the sync_debug info debugfs file.
> 
> The first 3 patches are clean up and improvements and the rest is preparation
> to de-stage and then finally the actual de-stage.
> 
> Please review,
> 
> Gustavo
> 
> ---
> Gustavo Padovan (7):
>   staging/android: move trace/sync.h to sync_trace.h
>   staging/android: remove doc from sw_sync
>   staging/android: display sync_pt name on debugfs
>   staging/android: do not let userspace trigger WARN_ON
>   staging/android: prepare sw_sync files for de-staging
>   dma-buf/sw_sync: de-stage SW_SYNC
>   staging/android: remove sync framework TODO
> 
>  drivers/dma-buf/Kconfig                            | 14 +++++++
>  drivers/dma-buf/Makefile                           |  1 +
>  drivers/{staging/android => dma-buf}/sw_sync.c     | 46 +++-------------------
>  drivers/{staging/android => dma-buf}/sync_debug.c  |  7 ++--
>  drivers/{staging/android => dma-buf}/sync_debug.h  | 26 +++++-------
>  .../android/trace/sync.h => dma-buf/sync_trace.h}  |  6 +--
>  drivers/staging/android/Kconfig                    | 13 ------
>  drivers/staging/android/Makefile                   |  1 -
>  drivers/staging/android/TODO                       |  8 ----
>  9 files changed, 38 insertions(+), 84 deletions(-)
>  rename drivers/{staging/android => dma-buf}/sw_sync.c (84%)
>  rename drivers/{staging/android => dma-buf}/sync_debug.c (97%)
>  rename drivers/{staging/android => dma-buf}/sync_debug.h (72%)
>  rename drivers/{staging/android/trace/sync.h => dma-buf/sync_trace.h} (84%)
> 
> -- 
> 2.5.5
> 

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Gustavo Padovan]

Hi,

Do you think there is time to get this in for 4.8?

Thanks.

2016-06-20 Gustavo Padovan <gustavo@padovan.org>:

> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Hi Greg,
> 
> This is the last step in the Sync Framwork de-stage task. It de-stage
> the SW_SYNC validation framework and the sync_debug info debugfs file.
> 
> The first 3 patches are clean up and improvements and the rest is preparation
> to de-stage and then finally the actual de-stage.
> 
> Please review,
> 
> Gustavo
> 
> ---
> Gustavo Padovan (7):
>   staging/android: move trace/sync.h to sync_trace.h
>   staging/android: remove doc from sw_sync
>   staging/android: display sync_pt name on debugfs
>   staging/android: do not let userspace trigger WARN_ON
>   staging/android: prepare sw_sync files for de-staging
>   dma-buf/sw_sync: de-stage SW_SYNC
>   staging/android: remove sync framework TODO
> 
>  drivers/dma-buf/Kconfig                            | 14 +++++++
>  drivers/dma-buf/Makefile                           |  1 +
>  drivers/{staging/android => dma-buf}/sw_sync.c     | 46 +++-------------------
>  drivers/{staging/android => dma-buf}/sync_debug.c  |  7 ++--
>  drivers/{staging/android => dma-buf}/sync_debug.h  | 26 +++++-------
>  .../android/trace/sync.h => dma-buf/sync_trace.h}  |  6 +--
>  drivers/staging/android/Kconfig                    | 13 ------
>  drivers/staging/android/Makefile                   |  1 -
>  drivers/staging/android/TODO                       |  8 ----
>  9 files changed, 38 insertions(+), 84 deletions(-)
>  rename drivers/{staging/android => dma-buf}/sw_sync.c (84%)
>  rename drivers/{staging/android => dma-buf}/sync_debug.c (97%)
>  rename drivers/{staging/android => dma-buf}/sync_debug.h (72%)
>  rename drivers/{staging/android/trace/sync.h => dma-buf/sync_trace.h} (84%)
> 
> -- 
> 2.5.5
> 

	Gustavo

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Pavel Machek]

On Mon 2016-08-08 16:08:12, Gustavo Padovan wrote:
> 2016-08-07 Pavel Machek <pavel@ucw.cz>:
> 
> > On Sun 2016-07-24 15:21:11, Greg Kroah-Hartman wrote:
> > > On Mon, Jul 18, 2016 at 04:12:45PM -0300, Gustavo Padovan wrote:
> > > > Hi,
> > > > 
> > > > Do you think there is time to get this in for 4.8?
> > > 
> > > No, it was too late on my end, due to travel and vacation, sorry.  I'll
> > > queue it up for 4.9-rc1.
> > 
> > Could we get some documentation what this does? Is it visilble to
> > userspace?
> 
> This interface is only intended for testing and validation, there are
> ioctls on the debugfs file that can be accessed by userspace but there
> isn't any exported kernel header with this info. The tester should know
> and add a internal header to be able to access it. We want to prevent
> people from misusing this feature by not advertising it nor providing
> documentation.

You are playing dangerous game here. debugfs is not normally considered stable,
but otoh... ioctls on debugfs?

Anyway, please provide some documentation. Kernel hackers need to know what this does.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Greg Kroah-Hartman]

On Mon, Jul 18, 2016 at 04:12:45PM -0300, Gustavo Padovan wrote:
> Hi,
> 
> Do you think there is time to get this in for 4.8?

No, it was too late on my end, due to travel and vacation, sorry.  I'll
queue it up for 4.9-rc1.

thanks,

greg k-h

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Pavel Machek]

On Sun 2016-07-24 15:21:11, Greg Kroah-Hartman wrote:
> On Mon, Jul 18, 2016 at 04:12:45PM -0300, Gustavo Padovan wrote:
> > Hi,
> > 
> > Do you think there is time to get this in for 4.8?
> 
> No, it was too late on my end, due to travel and vacation, sorry.  I'll
> queue it up for 4.9-rc1.

Could we get some documentation what this does? Is it visilble to
userspace?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/7] staging/android: display sync_pt name on debugfs

[Maarten Lankhorst]

Op 20-06-16 om 17:53 schreef Gustavo Padovan:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> When creating a sync_pt the name received wasn't used anywhere.
> Now we add it to the sync info debug output to make it easier to indetify
> the userspace name of that sync pt.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/staging/android/sw_sync.c    | 16 ++++------------
>  drivers/staging/android/sync_debug.c |  5 +++--
>  drivers/staging/android/sync_debug.h |  9 +++++++++
>  3 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
> index b4ae092..ea27512 100644
> --- a/drivers/staging/android/sw_sync.c
> +++ b/drivers/staging/android/sw_sync.c
> @@ -37,15 +37,6 @@ struct sw_sync_create_fence_data {
>  		struct sw_sync_create_fence_data)
>  #define SW_SYNC_IOC_INC			_IOW(SW_SYNC_IOC_MAGIC, 1, __u32)
>  
> -static const struct fence_ops timeline_fence_ops;
> -
> -static inline struct sync_pt *fence_to_sync_pt(struct fence *fence)
> -{
> -	if (fence->ops != &timeline_fence_ops)
> -		return NULL;
> -	return container_of(fence, struct sync_pt, base);
> -}
> -
>  struct sync_timeline *sync_timeline_create(const char *name)
>  {
>  	struct sync_timeline *obj;
> @@ -108,7 +99,7 @@ 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 int value, char *name)
>  {
>  	unsigned long flags;
>  	struct sync_pt *pt;
> @@ -120,6 +111,7 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, int size,
>  	if (!pt)
>  		return NULL;
>  
> +	strlcpy(pt->name, name, sizeof(pt->name));
>  	spin_lock_irqsave(&obj->child_list_lock, flags);
>  	sync_timeline_get(obj);
>  	fence_init(&pt->base, &timeline_fence_ops, &obj->child_list_lock,
> @@ -191,7 +183,7 @@ static void timeline_fence_timeline_value_str(struct fence *fence,
>  	snprintf(str, size, "%d", parent->value);
>  }
>  
> -static const struct fence_ops timeline_fence_ops = {
> +const struct fence_ops timeline_fence_ops = {
>  	.get_driver_name = timeline_fence_get_driver_name,
>  	.get_timeline_name = timeline_fence_get_timeline_name,
>  	.enable_signaling = timeline_fence_enable_signaling,
> @@ -252,7 +244,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, sizeof(*pt), data.value, data.name);
>  	if (!pt) {
>  		err = -ENOMEM;
>  		goto err;
> diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
> index 4c5a855..b732ea3 100644
> --- a/drivers/staging/android/sync_debug.c
> +++ b/drivers/staging/android/sync_debug.c
> @@ -75,13 +75,14 @@ static void sync_print_fence(struct seq_file *s, struct fence *fence, bool show)
>  {
>  	int status = 1;
>  	struct sync_timeline *parent = fence_parent(fence);
> +	struct sync_pt *pt = fence_to_sync_pt(fence);
>  
>  	if (fence_is_signaled_locked(fence))
>  		status = fence->status;
>  
> -	seq_printf(s, "  %s%sfence %s",
> +	seq_printf(s, "  %s%sfence %s %s",
>  		   show ? parent->name : "",
> -		   show ? "_" : "",
> +		   show ? "_" : "", pt->name,
>  		   sync_status_str(status));
>  
NAK,
A fence in sync_print_fence can be of any type. If you want to print the name, use the fence_value_str callback.

~Maarten

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Gustavo Padovan]

2016-08-07 Pavel Machek <pavel@ucw.cz>:

> On Sun 2016-07-24 15:21:11, Greg Kroah-Hartman wrote:
> > On Mon, Jul 18, 2016 at 04:12:45PM -0300, Gustavo Padovan wrote:
> > > Hi,
> > > 
> > > Do you think there is time to get this in for 4.8?
> > 
> > No, it was too late on my end, due to travel and vacation, sorry.  I'll
> > queue it up for 4.9-rc1.
> 
> Could we get some documentation what this does? Is it visilble to
> userspace?

This interface is only intended for testing and validation, there are
ioctls on the debugfs file that can be accessed by userspace but there
isn't any exported kernel header with this info. The tester should know
and add a internal header to be able to access it. We want to prevent
people from misusing this feature by not advertising it nor providing
documentation.

There will be, though, kselftest for this interface, that I send out
once SW_SYNC is de-staged.

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Gustavo Padovan]

2016-07-24 Pavel Machek <pavel@ucw.cz>:

> On Mon 2016-08-08 16:08:12, Gustavo Padovan wrote:
> > 2016-08-07 Pavel Machek <pavel@ucw.cz>:
> > 
> > > On Sun 2016-07-24 15:21:11, Greg Kroah-Hartman wrote:
> > > > On Mon, Jul 18, 2016 at 04:12:45PM -0300, Gustavo Padovan wrote:
> > > > > Hi,
> > > > > 
> > > > > Do you think there is time to get this in for 4.8?
> > > > 
> > > > No, it was too late on my end, due to travel and vacation, sorry.  I'll
> > > > queue it up for 4.9-rc1.
> > > 
> > > Could we get some documentation what this does? Is it visilble to
> > > userspace?
> > 
> > This interface is only intended for testing and validation, there are
> > ioctls on the debugfs file that can be accessed by userspace but there
> > isn't any exported kernel header with this info. The tester should know
> > and add a internal header to be able to access it. We want to prevent
> > people from misusing this feature by not advertising it nor providing
> > documentation.
> 
> You are playing dangerous game here. debugfs is not normally considered stable,
> but otoh... ioctls on debugfs?
> 
> Anyway, please provide some documentation. Kernel hackers need to know what this does.

Okay, where do you think is the best place? Would documentation inside
the .c file suffice for you?

Re: [PATCH 2/7] staging/android: display sync_pt name on debugfs

[Gustavo Padovan]

2016-08-08 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>:

> Op 20-06-16 om 17:53 schreef Gustavo Padovan:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > When creating a sync_pt the name received wasn't used anywhere.
> > Now we add it to the sync info debug output to make it easier to indetify
> > the userspace name of that sync pt.
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/staging/android/sw_sync.c    | 16 ++++------------
> >  drivers/staging/android/sync_debug.c |  5 +++--
> >  drivers/staging/android/sync_debug.h |  9 +++++++++
> >  3 files changed, 16 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
> > index b4ae092..ea27512 100644
> > --- a/drivers/staging/android/sw_sync.c
> > +++ b/drivers/staging/android/sw_sync.c
> > @@ -37,15 +37,6 @@ struct sw_sync_create_fence_data {
> >  		struct sw_sync_create_fence_data)
> >  #define SW_SYNC_IOC_INC			_IOW(SW_SYNC_IOC_MAGIC, 1, __u32)
> >  
> > -static const struct fence_ops timeline_fence_ops;
> > -
> > -static inline struct sync_pt *fence_to_sync_pt(struct fence *fence)
> > -{
> > -	if (fence->ops != &timeline_fence_ops)
> > -		return NULL;
> > -	return container_of(fence, struct sync_pt, base);
> > -}
> > -
> >  struct sync_timeline *sync_timeline_create(const char *name)
> >  {
> >  	struct sync_timeline *obj;
> > @@ -108,7 +99,7 @@ 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 int value, char *name)
> >  {
> >  	unsigned long flags;
> >  	struct sync_pt *pt;
> > @@ -120,6 +111,7 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, int size,
> >  	if (!pt)
> >  		return NULL;
> >  
> > +	strlcpy(pt->name, name, sizeof(pt->name));
> >  	spin_lock_irqsave(&obj->child_list_lock, flags);
> >  	sync_timeline_get(obj);
> >  	fence_init(&pt->base, &timeline_fence_ops, &obj->child_list_lock,
> > @@ -191,7 +183,7 @@ static void timeline_fence_timeline_value_str(struct fence *fence,
> >  	snprintf(str, size, "%d", parent->value);
> >  }
> >  
> > -static const struct fence_ops timeline_fence_ops = {
> > +const struct fence_ops timeline_fence_ops = {
> >  	.get_driver_name = timeline_fence_get_driver_name,
> >  	.get_timeline_name = timeline_fence_get_timeline_name,
> >  	.enable_signaling = timeline_fence_enable_signaling,
> > @@ -252,7 +244,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, sizeof(*pt), data.value, data.name);
> >  	if (!pt) {
> >  		err = -ENOMEM;
> >  		goto err;
> > diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
> > index 4c5a855..b732ea3 100644
> > --- a/drivers/staging/android/sync_debug.c
> > +++ b/drivers/staging/android/sync_debug.c
> > @@ -75,13 +75,14 @@ static void sync_print_fence(struct seq_file *s, struct fence *fence, bool show)
> >  {
> >  	int status = 1;
> >  	struct sync_timeline *parent = fence_parent(fence);
> > +	struct sync_pt *pt = fence_to_sync_pt(fence);
> >  
> >  	if (fence_is_signaled_locked(fence))
> >  		status = fence->status;
> >  
> > -	seq_printf(s, "  %s%sfence %s",
> > +	seq_printf(s, "  %s%sfence %s %s",
> >  		   show ? parent->name : "",
> > -		   show ? "_" : "",
> > +		   show ? "_" : "", pt->name,
> >  		   sync_status_str(status));
> >  
> NAK,
> A fence in sync_print_fence can be of any type. If you want to print the name, use the fence_value_str callback.

Indeed. But fence_value_str doesn't return the sync_pt name, but the
seqno of that fence. I'll keep this change out for the de-staging and
then try to come with something that works better.

	Gustavo

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Daniel Vetter]

On Sun, Jul 24, 2016 at 05:00:31PM +0200, Pavel Machek wrote:
> On Mon 2016-08-08 16:08:12, Gustavo Padovan wrote:
> > 2016-08-07 Pavel Machek <pavel@ucw.cz>:
> > 
> > > On Sun 2016-07-24 15:21:11, Greg Kroah-Hartman wrote:
> > > > On Mon, Jul 18, 2016 at 04:12:45PM -0300, Gustavo Padovan wrote:
> > > > > Hi,
> > > > > 
> > > > > Do you think there is time to get this in for 4.8?
> > > > 
> > > > No, it was too late on my end, due to travel and vacation, sorry.  I'll
> > > > queue it up for 4.9-rc1.
> > > 
> > > Could we get some documentation what this does? Is it visilble to
> > > userspace?
> > 
> > This interface is only intended for testing and validation, there are
> > ioctls on the debugfs file that can be accessed by userspace but there
> > isn't any exported kernel header with this info. The tester should know
> > and add a internal header to be able to access it. We want to prevent
> > people from misusing this feature by not advertising it nor providing
> > documentation.
> 
> You are playing dangerous game here. debugfs is not normally considered stable,
> but otoh... ioctls on debugfs?

It's not considered stable. The idea is that we also add the existing
testcases to kselftest. It's purely a bit of interface to be able to drive
run the test logic for real fences. What it really tests is the fence
interface (which is public in the uapi headers and all that), but to be
able to do that we need some (hw-independent way) to expose fences, which
this provides.

Long term we might even do this as a proper interface (with some
restrictions to make it safe and avoid userspace pulling the kernel over
the table). And then rip out sw_sync entirely.

Imo there's no need at all for docs for this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/7] staging/android: display sync_pt name on debugfs

[Maarten Lankhorst]

Op 08-08-16 om 21:59 schreef Gustavo Padovan:
> 2016-08-08 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>:
>
>> Op 20-06-16 om 17:53 schreef Gustavo Padovan:
>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>
>>> When creating a sync_pt the name received wasn't used anywhere.
>>> Now we add it to the sync info debug output to make it easier to indetify
>>> the userspace name of that sync pt.
>>>
>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>> ---
>>>  drivers/staging/android/sw_sync.c    | 16 ++++------------
>>>  drivers/staging/android/sync_debug.c |  5 +++--
>>>  drivers/staging/android/sync_debug.h |  9 +++++++++
>>>  3 files changed, 16 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
>>> index b4ae092..ea27512 100644
>>> --- a/drivers/staging/android/sw_sync.c
>>> +++ b/drivers/staging/android/sw_sync.c
>>> @@ -37,15 +37,6 @@ struct sw_sync_create_fence_data {
>>>  		struct sw_sync_create_fence_data)
>>>  #define SW_SYNC_IOC_INC			_IOW(SW_SYNC_IOC_MAGIC, 1, __u32)
>>>  
>>> -static const struct fence_ops timeline_fence_ops;
>>> -
>>> -static inline struct sync_pt *fence_to_sync_pt(struct fence *fence)
>>> -{
>>> -	if (fence->ops != &timeline_fence_ops)
>>> -		return NULL;
>>> -	return container_of(fence, struct sync_pt, base);
>>> -}
>>> -
>>>  struct sync_timeline *sync_timeline_create(const char *name)
>>>  {
>>>  	struct sync_timeline *obj;
>>> @@ -108,7 +99,7 @@ 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 int value, char *name)
>>>  {
>>>  	unsigned long flags;
>>>  	struct sync_pt *pt;
>>> @@ -120,6 +111,7 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, int size,
>>>  	if (!pt)
>>>  		return NULL;
>>>  
>>> +	strlcpy(pt->name, name, sizeof(pt->name));
>>>  	spin_lock_irqsave(&obj->child_list_lock, flags);
>>>  	sync_timeline_get(obj);
>>>  	fence_init(&pt->base, &timeline_fence_ops, &obj->child_list_lock,
>>> @@ -191,7 +183,7 @@ static void timeline_fence_timeline_value_str(struct fence *fence,
>>>  	snprintf(str, size, "%d", parent->value);
>>>  }
>>>  
>>> -static const struct fence_ops timeline_fence_ops = {
>>> +const struct fence_ops timeline_fence_ops = {
>>>  	.get_driver_name = timeline_fence_get_driver_name,
>>>  	.get_timeline_name = timeline_fence_get_timeline_name,
>>>  	.enable_signaling = timeline_fence_enable_signaling,
>>> @@ -252,7 +244,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, sizeof(*pt), data.value, data.name);
>>>  	if (!pt) {
>>>  		err = -ENOMEM;
>>>  		goto err;
>>> diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
>>> index 4c5a855..b732ea3 100644
>>> --- a/drivers/staging/android/sync_debug.c
>>> +++ b/drivers/staging/android/sync_debug.c
>>> @@ -75,13 +75,14 @@ static void sync_print_fence(struct seq_file *s, struct fence *fence, bool show)
>>>  {
>>>  	int status = 1;
>>>  	struct sync_timeline *parent = fence_parent(fence);
>>> +	struct sync_pt *pt = fence_to_sync_pt(fence);
>>>  
>>>  	if (fence_is_signaled_locked(fence))
>>>  		status = fence->status;
>>>  
>>> -	seq_printf(s, "  %s%sfence %s",
>>> +	seq_printf(s, "  %s%sfence %s %s",
>>>  		   show ? parent->name : "",
>>> -		   show ? "_" : "",
>>> +		   show ? "_" : "", pt->name,
>>>  		   sync_status_str(status));
>>>  
>> NAK,
>> A fence in sync_print_fence can be of any type. If you want to print the name, use the fence_value_str callback.
> Indeed. But fence_value_str doesn't return the sync_pt name, but the
> seqno of that fence. I'll keep this change out for the de-staging and
> then try to come with something that works better.
>
> 	Gustavo

This will probably cause a kernel panic if you keep it like it is and you look at
debugfs for non sync_pt fences, it should definitely be fixed before destaging.
Ignoring sync_pt name would be better than crashing.

~Maarten

Re: [PATCH 2/7] staging/android: display sync_pt name on debugfs

[Gustavo Padovan]

2016-08-09 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>:

> Op 08-08-16 om 21:59 schreef Gustavo Padovan:
> > 2016-08-08 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>:
> >
> >> Op 20-06-16 om 17:53 schreef Gustavo Padovan:
> >>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>>
> >>> When creating a sync_pt the name received wasn't used anywhere.
> >>> Now we add it to the sync info debug output to make it easier to indetify
> >>> the userspace name of that sync pt.
> >>>
> >>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>> ---
> >>>  drivers/staging/android/sw_sync.c    | 16 ++++------------
> >>>  drivers/staging/android/sync_debug.c |  5 +++--
> >>>  drivers/staging/android/sync_debug.h |  9 +++++++++
> >>>  3 files changed, 16 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/android/sw_sync.c b/drivers/staging/android/sw_sync.c
> >>> index b4ae092..ea27512 100644
> >>> --- a/drivers/staging/android/sw_sync.c
> >>> +++ b/drivers/staging/android/sw_sync.c
> >>> @@ -37,15 +37,6 @@ struct sw_sync_create_fence_data {
> >>>  		struct sw_sync_create_fence_data)
> >>>  #define SW_SYNC_IOC_INC			_IOW(SW_SYNC_IOC_MAGIC, 1, __u32)
> >>>  
> >>> -static const struct fence_ops timeline_fence_ops;
> >>> -
> >>> -static inline struct sync_pt *fence_to_sync_pt(struct fence *fence)
> >>> -{
> >>> -	if (fence->ops != &timeline_fence_ops)
> >>> -		return NULL;
> >>> -	return container_of(fence, struct sync_pt, base);
> >>> -}
> >>> -
> >>>  struct sync_timeline *sync_timeline_create(const char *name)
> >>>  {
> >>>  	struct sync_timeline *obj;
> >>> @@ -108,7 +99,7 @@ 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 int value, char *name)
> >>>  {
> >>>  	unsigned long flags;
> >>>  	struct sync_pt *pt;
> >>> @@ -120,6 +111,7 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, int size,
> >>>  	if (!pt)
> >>>  		return NULL;
> >>>  
> >>> +	strlcpy(pt->name, name, sizeof(pt->name));
> >>>  	spin_lock_irqsave(&obj->child_list_lock, flags);
> >>>  	sync_timeline_get(obj);
> >>>  	fence_init(&pt->base, &timeline_fence_ops, &obj->child_list_lock,
> >>> @@ -191,7 +183,7 @@ static void timeline_fence_timeline_value_str(struct fence *fence,
> >>>  	snprintf(str, size, "%d", parent->value);
> >>>  }
> >>>  
> >>> -static const struct fence_ops timeline_fence_ops = {
> >>> +const struct fence_ops timeline_fence_ops = {
> >>>  	.get_driver_name = timeline_fence_get_driver_name,
> >>>  	.get_timeline_name = timeline_fence_get_timeline_name,
> >>>  	.enable_signaling = timeline_fence_enable_signaling,
> >>> @@ -252,7 +244,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, sizeof(*pt), data.value, data.name);
> >>>  	if (!pt) {
> >>>  		err = -ENOMEM;
> >>>  		goto err;
> >>> diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
> >>> index 4c5a855..b732ea3 100644
> >>> --- a/drivers/staging/android/sync_debug.c
> >>> +++ b/drivers/staging/android/sync_debug.c
> >>> @@ -75,13 +75,14 @@ static void sync_print_fence(struct seq_file *s, struct fence *fence, bool show)
> >>>  {
> >>>  	int status = 1;
> >>>  	struct sync_timeline *parent = fence_parent(fence);
> >>> +	struct sync_pt *pt = fence_to_sync_pt(fence);
> >>>  
> >>>  	if (fence_is_signaled_locked(fence))
> >>>  		status = fence->status;
> >>>  
> >>> -	seq_printf(s, "  %s%sfence %s",
> >>> +	seq_printf(s, "  %s%sfence %s %s",
> >>>  		   show ? parent->name : "",
> >>> -		   show ? "_" : "",
> >>> +		   show ? "_" : "", pt->name,
> >>>  		   sync_status_str(status));
> >>>  
> >> NAK,
> >> A fence in sync_print_fence can be of any type. If you want to print the name, use the fence_value_str callback.
> > Indeed. But fence_value_str doesn't return the sync_pt name, but the
> > seqno of that fence. I'll keep this change out for the de-staging and
> > then try to come with something that works better.
> >
> > 	Gustavo
> 
> This will probably cause a kernel panic if you keep it like it is and you look at
> debugfs for non sync_pt fences, it should definitely be fixed before destaging.
> Ignoring sync_pt name would be better than crashing.

No, I meant ignoring sync_pt name for now. I've already sent v2 without
it.

Gustavo

Re: [PATCH 0/7] de-stage SW_SYNC validation frawework

[Pavel Machek]

On Tue 2016-08-09 08:04:54, Daniel Vetter wrote:
> On Sun, Jul 24, 2016 at 05:00:31PM +0200, Pavel Machek wrote:
> > On Mon 2016-08-08 16:08:12, Gustavo Padovan wrote:
> > > 2016-08-07 Pavel Machek <pavel@ucw.cz>:
> > > 
> > > > On Sun 2016-07-24 15:21:11, Greg Kroah-Hartman wrote:
> > > > > On Mon, Jul 18, 2016 at 04:12:45PM -0300, Gustavo Padovan wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > Do you think there is time to get this in for 4.8?
> > > > > 
> > > > > No, it was too late on my end, due to travel and vacation, sorry.  I'll
> > > > > queue it up for 4.9-rc1.
> > > > 
> > > > Could we get some documentation what this does? Is it visilble to
> > > > userspace?
> > > 
> > > This interface is only intended for testing and validation, there are
> > > ioctls on the debugfs file that can be accessed by userspace but there
> > > isn't any exported kernel header with this info. The tester should know
> > > and add a internal header to be able to access it. We want to prevent
> > > people from misusing this feature by not advertising it nor providing
> > > documentation.
> > 
> > You are playing dangerous game here. debugfs is not normally considered stable,
> > but otoh... ioctls on debugfs?
> 
> It's not considered stable. The idea is that we also add the existing
> testcases to kselftest. It's purely a bit of interface to be able to drive
> run the test logic for real fences. What it really tests is the fence
> interface (which is public in the uapi headers and all that), but to be
> able to do that we need some (hw-independent way) to expose fences, which
> this provides.
> 
> Long term we might even do this as a proper interface (with some
> restrictions to make it safe and avoid userspace pulling the kernel over
> the table). And then rip out sw_sync entirely.
> 
> Imo there's no need at all for docs for this.

There's full directory of files, with absolutely zero comments/documentation. They
are quite hard to understand. Plus it has userland interface.

IMO comment should be added, explaining what it is testing, that interface is not considered
stable, and where the test lives.

I know what "fence" is in the cpu sense (mfence and friends), but I'm not sure 
what "real fence" is in this context.

Best regards,
										Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html