[rfc repost] drm sync objects - a new beginning (make ickle happier?)

[rfc repost] drm sync objects - a new beginning (make ickle happier?)

[Dave Airlie]

Okay I've taken Chris's suggestions to heart and reworked things
around a sem_file to see how they might look.

This means the drm_syncobj are currently only useful for semaphores,
the flags field could be used in future to use it for other things,
and we can reintroduce some of the API then if needed.

This refactors sync_file first to add some basic rcu wrappers
about the fence pointer, as this point never updates this should
all be fine unlocked.

It then creates the sem_file with a mutex, and uses that to
track the semaphores with reduced fops and the replace and
get APIs.

Then it reworks the drm stuff on top, and fixes amdgpu bug
with old_fence.

Let's see if anyone prefers one approach over the other.

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

[PATCH 1/7] sync_file: get rid of internal reference count.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

sync_file uses the reference count of the file, the internal
kref was never getting moved past 1.

We can reintroduce this if we decide we need it later.

[airlied: fix buildbot warnings]

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/dma-buf/sync_file.c | 13 ++-----------
 include/linux/sync_file.h   |  3 ---
 2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2321035..dc89b1d 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -41,8 +41,6 @@ static struct sync_file *sync_file_alloc(void)
 	if (IS_ERR(sync_file->file))
 		goto err;
 
-	kref_init(&sync_file->kref);
-
 	init_waitqueue_head(&sync_file->wq);
 
 	INIT_LIST_HEAD(&sync_file->cb.node);
@@ -277,22 +275,15 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 
 }
 
-static void sync_file_free(struct kref *kref)
+static int sync_file_release(struct inode *inode, struct file *file)
 {
-	struct sync_file *sync_file = container_of(kref, struct sync_file,
-						     kref);
+	struct sync_file *sync_file = file->private_data;
 
 	if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
 		dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
 	dma_fence_put(sync_file->fence);
 	kfree(sync_file);
-}
-
-static int sync_file_release(struct inode *inode, struct file *file)
-{
-	struct sync_file *sync_file = file->private_data;
 
-	kref_put(&sync_file->kref, sync_file_free);
 	return 0;
 }
 
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 3e3ab84..d37beef 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -14,7 +14,6 @@
 #define _LINUX_SYNC_FILE_H
 
 #include <linux/types.h>
-#include <linux/kref.h>
 #include <linux/ktime.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>
@@ -24,7 +23,6 @@
 /**
  * struct sync_file - sync file to export to the userspace
  * @file:		file representing this fence
- * @kref:		reference count on fence.
  * @name:		name of sync_file.  Useful for debugging
  * @sync_file_list:	membership in global file list
  * @wq:			wait queue for fence signaling
@@ -33,7 +31,6 @@
  */
 struct sync_file {
 	struct file		*file;
-	struct kref		kref;
 	char			name[32];
 #ifdef CONFIG_DEBUG_FS
 	struct list_head	sync_file_list;
-- 
2.9.3

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

[PATCH 2/7] sync_file: mark the fence pointer as rcu.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

The current code never updates this pointer after init,
and it should always be initialised with a valid value.

However in the future we want to share some code with
semaphore files, and those will want to use rcu to update
this pointer. However none of the places sync_file access
the pointer from should correspond with semaphore files,
so this just wraps things with init and dereference wrappers.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/dma-buf/sync_file.c | 32 +++++++++++++++++++-------------
 include/linux/sync_file.h   |  2 +-
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index dc89b1d..9b7ad7e 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -78,7 +78,7 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
 	if (!sync_file)
 		return NULL;
 
-	sync_file->fence = dma_fence_get(fence);
+	RCU_INIT_POINTER(sync_file->fence, dma_fence_get(fence));
 
 	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
 		 fence->ops->get_driver_name(fence),
@@ -122,7 +122,7 @@ struct dma_fence *sync_file_get_fence(int fd)
 	if (!sync_file)
 		return NULL;
 
-	fence = dma_fence_get(sync_file->fence);
+	fence = dma_fence_get(rcu_dereference_protected(sync_file->fence, 1));
 	fput(sync_file->file);
 
 	return fence;
@@ -141,7 +141,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
 	 * we own the reference of the dma_fence_array creation.
 	 */
 	if (num_fences == 1) {
-		sync_file->fence = fences[0];
+		RCU_INIT_POINTER(sync_file->fence, fences[0]);
 		kfree(fences);
 	} else {
 		array = dma_fence_array_create(num_fences, fences,
@@ -150,7 +150,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
 		if (!array)
 			return -ENOMEM;
 
-		sync_file->fence = &array->base;
+		RCU_INIT_POINTER(sync_file->fence, &array->base);
 	}
 
 	return 0;
@@ -159,8 +159,9 @@ static int sync_file_set_fence(struct sync_file *sync_file,
 static struct dma_fence **get_fences(struct sync_file *sync_file,
 				     int *num_fences)
 {
-	if (dma_fence_is_array(sync_file->fence)) {
-		struct dma_fence_array *array = to_dma_fence_array(sync_file->fence);
+	struct dma_fence *fence = rcu_dereference_protected(sync_file->fence, 1);
+	if (dma_fence_is_array(fence)) {
+		struct dma_fence_array *array = to_dma_fence_array(fence);
 
 		*num_fences = array->num_fences;
 		return array->fences;
@@ -278,10 +279,12 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 static int sync_file_release(struct inode *inode, struct file *file)
 {
 	struct sync_file *sync_file = file->private_data;
+	struct dma_fence *fence;
 
-	if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
-		dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
-	dma_fence_put(sync_file->fence);
+	fence = rcu_dereference_protected(sync_file->fence, 1);
+	if (test_bit(POLL_ENABLED, &fence->flags))
+		dma_fence_remove_callback(fence, &sync_file->cb);
+	dma_fence_put(fence);
 	kfree(sync_file);
 
 	return 0;
@@ -290,16 +293,19 @@ static int sync_file_release(struct inode *inode, struct file *file)
 static unsigned int sync_file_poll(struct file *file, poll_table *wait)
 {
 	struct sync_file *sync_file = file->private_data;
+	struct dma_fence *fence;
+
+	fence = rcu_dereference_protected(sync_file->fence, 1);
 
 	poll_wait(file, &sync_file->wq, wait);
 
-	if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) {
-		if (dma_fence_add_callback(sync_file->fence, &sync_file->cb,
+	if (!test_and_set_bit(POLL_ENABLED, &fence->flags)) {
+		if (dma_fence_add_callback(fence, &sync_file->cb,
 					   fence_check_cb_func) < 0)
 			wake_up_all(&sync_file->wq);
 	}
 
-	return dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
+	return dma_fence_is_signaled(fence) ? POLLIN : 0;
 }
 
 static long sync_file_ioctl_merge(struct sync_file *sync_file,
@@ -414,7 +420,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 
 no_fences:
 	strlcpy(info.name, sync_file->name, sizeof(info.name));
-	info.status = dma_fence_is_signaled(sync_file->fence);
+	info.status = dma_fence_is_signaled(rcu_dereference_protected(sync_file->fence, 1));
 	info.num_fences = num_fences;
 
 	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index d37beef..adbc0b9 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -38,7 +38,7 @@ struct sync_file {
 
 	wait_queue_head_t	wq;
 
-	struct dma_fence	*fence;
+	struct dma_fence	__rcu *fence;
 	struct dma_fence_cb cb;
 };
 
-- 
2.9.3

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

[PATCH 3/7] sync_file: split out fence_file base class from sync_file.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This just splits out a common base object to be shared
between sync_file and sem_files.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/dma-buf/sync_file.c  | 49 +++++++++++++++++++++++++++-----------------
 drivers/gpu/drm/drm_atomic.c |  4 ++--
 include/linux/sync_file.h    | 17 ++++++++++-----
 3 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 9b7ad7e..2342d8b 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -28,17 +28,28 @@
 
 static const struct file_operations sync_file_fops;
 
+static int fence_file_init(struct fence_file *fence_file,
+			   const struct file_operations *fops)
+{
+	fence_file->file = anon_inode_getfile("fence_file", fops,
+					     fence_file, 0);
+	if (IS_ERR(fence_file->file))
+		return PTR_ERR(fence_file->file);
+	return 0;
+}
+
 static struct sync_file *sync_file_alloc(void)
 {
 	struct sync_file *sync_file;
+	int ret;
 
 	sync_file = kzalloc(sizeof(*sync_file), GFP_KERNEL);
 	if (!sync_file)
 		return NULL;
 
-	sync_file->file = anon_inode_getfile("sync_file", &sync_file_fops,
-					     sync_file, 0);
-	if (IS_ERR(sync_file->file))
+	ret = fence_file_init(&sync_file->base,
+			      &sync_file_fops);
+	if (ret)
 		goto err;
 
 	init_waitqueue_head(&sync_file->wq);
@@ -67,7 +78,7 @@ static void fence_check_cb_func(struct dma_fence *f, struct dma_fence_cb *cb)
  *
  * Creates a sync_file containg @fence. This function acquires and additional
  * reference of @fence for the newly-created &sync_file, if it succeeds. The
- * sync_file can be released with fput(sync_file->file). Returns the
+ * sync_file can be released with fput(sync_file->base.file). Returns the
  * sync_file or NULL in case of error.
  */
 struct sync_file *sync_file_create(struct dma_fence *fence)
@@ -78,7 +89,7 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
 	if (!sync_file)
 		return NULL;
 
-	RCU_INIT_POINTER(sync_file->fence, dma_fence_get(fence));
+	RCU_INIT_POINTER(sync_file->base.fence, dma_fence_get(fence));
 
 	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
 		 fence->ops->get_driver_name(fence),
@@ -122,8 +133,8 @@ struct dma_fence *sync_file_get_fence(int fd)
 	if (!sync_file)
 		return NULL;
 
-	fence = dma_fence_get(rcu_dereference_protected(sync_file->fence, 1));
-	fput(sync_file->file);
+	fence = dma_fence_get(rcu_dereference_protected(sync_file->base.fence, 1));
+	fput(sync_file->base.file);
 
 	return fence;
 }
@@ -141,7 +152,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
 	 * we own the reference of the dma_fence_array creation.
 	 */
 	if (num_fences == 1) {
-		RCU_INIT_POINTER(sync_file->fence, fences[0]);
+		RCU_INIT_POINTER(sync_file->base.fence, fences[0]);
 		kfree(fences);
 	} else {
 		array = dma_fence_array_create(num_fences, fences,
@@ -150,7 +161,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
 		if (!array)
 			return -ENOMEM;
 
-		RCU_INIT_POINTER(sync_file->fence, &array->base);
+		RCU_INIT_POINTER(sync_file->base.fence, &array->base);
 	}
 
 	return 0;
@@ -159,7 +170,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
 static struct dma_fence **get_fences(struct sync_file *sync_file,
 				     int *num_fences)
 {
-	struct dma_fence *fence = rcu_dereference_protected(sync_file->fence, 1);
+	struct dma_fence *fence = rcu_dereference_protected(sync_file->base.fence, 1);
 	if (dma_fence_is_array(fence)) {
 		struct dma_fence_array *array = to_dma_fence_array(fence);
 
@@ -168,7 +179,7 @@ static struct dma_fence **get_fences(struct sync_file *sync_file,
 	}
 
 	*num_fences = 1;
-	return &sync_file->fence;
+	return &sync_file->base.fence;
 }
 
 static void add_fence(struct dma_fence **fences,
@@ -271,7 +282,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 	return sync_file;
 
 err:
-	fput(sync_file->file);
+	fput(sync_file->base.file);
 	return NULL;
 
 }
@@ -281,7 +292,7 @@ static int sync_file_release(struct inode *inode, struct file *file)
 	struct sync_file *sync_file = file->private_data;
 	struct dma_fence *fence;
 
-	fence = rcu_dereference_protected(sync_file->fence, 1);
+	fence = rcu_dereference_protected(sync_file->base.fence, 1);
 	if (test_bit(POLL_ENABLED, &fence->flags))
 		dma_fence_remove_callback(fence, &sync_file->cb);
 	dma_fence_put(fence);
@@ -295,7 +306,7 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait)
 	struct sync_file *sync_file = file->private_data;
 	struct dma_fence *fence;
 
-	fence = rcu_dereference_protected(sync_file->fence, 1);
+	fence = rcu_dereference_protected(sync_file->base.fence, 1);
 
 	poll_wait(file, &sync_file->wq, wait);
 
@@ -348,15 +359,15 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file,
 		goto err_put_fence3;
 	}
 
-	fd_install(fd, fence3->file);
-	fput(fence2->file);
+	fd_install(fd, fence3->base.file);
+	fput(fence2->base.file);
 	return 0;
 
 err_put_fence3:
-	fput(fence3->file);
+	fput(fence3->base.file);
 
 err_put_fence2:
-	fput(fence2->file);
+	fput(fence2->base.file);
 
 err_put_fd:
 	put_unused_fd(fd);
@@ -420,7 +431,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 
 no_fences:
 	strlcpy(info.name, sync_file->name, sizeof(info.name));
-	info.status = dma_fence_is_signaled(rcu_dereference_protected(sync_file->fence, 1));
+	info.status = dma_fence_is_signaled(rcu_dereference_protected(sync_file->base.fence, 1));
 	info.num_fences = num_fences;
 
 	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a567310..16053a0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2012,7 +2012,7 @@ static void complete_crtc_signaling(struct drm_device *dev,
 	if (install_fds) {
 		for (i = 0; i < num_fences; i++)
 			fd_install(fence_state[i].fd,
-				   fence_state[i].sync_file->file);
+				   fence_state[i].sync_file->base.file);
 
 		kfree(fence_state);
 		return;
@@ -2036,7 +2036,7 @@ static void complete_crtc_signaling(struct drm_device *dev,
 
 	for (i = 0; i < num_fences; i++) {
 		if (fence_state[i].sync_file)
-			fput(fence_state[i].sync_file->file);
+			fput(fence_state[i].sync_file->base.file);
 		if (fence_state[i].fd >= 0)
 			put_unused_fd(fence_state[i].fd);
 
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index adbc0b9..b0ae1cf 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -21,24 +21,31 @@
 #include <linux/dma-fence-array.h>
 
 /**
- * struct sync_file - sync file to export to the userspace
+ * struct fence_file - basic file to fence mapping
  * @file:		file representing this fence
+ * @fence:		fence with the fences in the fence_file
+ */
+struct fence_file {
+	struct file		*file;
+	struct dma_fence	__rcu *fence;
+};
+
+/**
+ * struct sync_file - sync file to export to the userspace
+ * @base:               base fence file
  * @name:		name of sync_file.  Useful for debugging
  * @sync_file_list:	membership in global file list
  * @wq:			wait queue for fence signaling
- * @fence:		fence with the fences in the sync_file
  * @cb:			fence callback information
  */
 struct sync_file {
-	struct file		*file;
+	struct fence_file       base;
 	char			name[32];
 #ifdef CONFIG_DEBUG_FS
 	struct list_head	sync_file_list;
 #endif
 
 	wait_queue_head_t	wq;
-
-	struct dma_fence	__rcu *fence;
 	struct dma_fence_cb cb;
 };
 
-- 
2.9.3

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

[PATCH 4/7] sync_file: add support for sem_file

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This adds support for a file that has semaphore semantics
(for Vulkan shared semaphores).

These objects are persistent objects that can have a
fence that changes. When the object is signaled, a fence
is attached to it, and when an object is waited on, the
fence is removed. All interactions with these objects
should be via command submission routines in the drm
drivers. The sem_file is just for passing the sems between
processes.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/dma-buf/sync_file.c | 101 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sync_file.h   |  16 +++++++
 2 files changed, 117 insertions(+)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2342d8b..a88d786 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -468,3 +468,104 @@ static const struct file_operations sync_file_fops = {
 	.unlocked_ioctl = sync_file_ioctl,
 	.compat_ioctl = sync_file_ioctl,
 };
+
+static int sem_file_release(struct inode *inode, struct file *file)
+{
+	struct sem_file *sem_file = file->private_data;
+	struct dma_fence *fence;
+
+	fence = rcu_dereference_protected(sem_file->base.fence, 1);
+	dma_fence_put(fence);
+	kfree(sem_file);
+
+	return 0;
+}
+
+static const struct file_operations sem_file_fops = {
+	.release = sem_file_release,
+};
+
+struct sem_file *sem_file_alloc(void)
+{
+	struct sem_file *sem_file;
+	int ret;
+
+	sem_file = kzalloc(sizeof(*sem_file), GFP_KERNEL);
+	if (!sem_file)
+		return NULL;
+
+	ret = fence_file_init(&sem_file->base,
+			      &sem_file_fops);
+	if (ret)
+		goto err;
+
+	RCU_INIT_POINTER(sem_file->base.fence, NULL);
+	mutex_init(&sem_file->lock);
+
+	return sem_file;
+
+err:
+	kfree(sem_file);
+	return NULL;
+}
+EXPORT_SYMBOL(sem_file_alloc);
+
+struct sem_file *sem_file_fdget(int fd)
+{
+	struct file *file = fget(fd);
+
+	if (!file)
+		return NULL;
+
+	if (file->f_op != &sem_file_fops)
+		goto err;
+
+	return file->private_data;
+
+err:
+	fput(file);
+	return NULL;
+}
+EXPORT_SYMBOL(sem_file_fdget);
+
+#define sem_file_held(obj) lockdep_is_held(&(obj)->lock)
+
+struct dma_fence *sem_file_get_fence(struct sem_file *sem_file)
+{
+	struct dma_fence *fence;
+
+	if (!rcu_access_pointer(sem_file->base.fence)) {
+		return NULL;
+	}
+
+	rcu_read_lock();
+	fence = dma_fence_get_rcu_safe(&sem_file->base.fence);
+	rcu_read_unlock();
+	return fence;
+}
+EXPORT_SYMBOL(sem_file_get_fence);
+
+static inline struct dma_fence *
+sem_file_get_fence_locked(struct sem_file *sem_file)
+{
+	return rcu_dereference_protected(sem_file->base.fence,
+					 sem_file_held(sem_file));
+}
+
+int sem_file_replace_fence(struct sem_file *sem_file,
+			   struct dma_fence *fence,
+			   struct dma_fence **old_fence)
+{
+	struct dma_fence *ret_fence = NULL;
+
+	if (fence)
+		dma_fence_get(fence);
+
+	mutex_lock(&sem_file->lock);
+	ret_fence = sem_file_get_fence_locked(sem_file);
+	RCU_INIT_POINTER(sem_file->base.fence, fence);
+	mutex_unlock(&sem_file->lock);
+	*old_fence = ret_fence;
+	return 0;
+}
+EXPORT_SYMBOL(sem_file_replace_fence);
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index b0ae1cf..49735c8 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -54,4 +54,20 @@ struct sync_file {
 struct sync_file *sync_file_create(struct dma_fence *fence);
 struct dma_fence *sync_file_get_fence(int fd);
 
+
+/**
+ * struct sem_file - shared semaphore file for userspace.
+ * @base: base fence file.
+ * @lock: mutex to lock the fence_file fence ptr.
+ */
+struct sem_file {
+	struct fence_file base;
+	struct mutex lock;
+};
+struct sem_file *sem_file_create(void);
+struct sem_file *sem_file_fdget(int fd);
+struct dma_fence *sem_file_get_fence(struct sem_file *sem_file);
+int sem_file_replace_fence(struct sem_file *sem_file,
+			   struct dma_fence *fence,
+			   struct dma_fence **old_fence);
 #endif /* _LINUX_SYNC_H */
-- 
2.9.3

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

[PATCH 5/7] drm: introduce sync objects as wrappers for sem files

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

Sync objects are new toplevel drm object, that have the same
semantics as sem_file objects, but without requiring an fd
to be consumed to support them.

This patch just enables the DRM interface to create these
objects, it doesn't actually provide any semaphore objects
or new features.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 Documentation/gpu/drm-internals.rst |   5 +
 Documentation/gpu/drm-mm.rst        |   6 +
 drivers/dma-buf/sync_file.c         |   4 +-
 drivers/gpu/drm/Makefile            |   2 +-
 drivers/gpu/drm/drm_fops.c          |   8 +
 drivers/gpu/drm/drm_internal.h      |  13 ++
 drivers/gpu/drm/drm_ioctl.c         |  12 ++
 drivers/gpu/drm/drm_syncobj.c       | 285 ++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h                  |   5 +
 include/drm/drm_drv.h               |   1 +
 include/drm/drm_syncobj.h           |  36 +++++
 include/uapi/drm/drm.h              |  25 ++++
 12 files changed, 399 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_syncobj.c
 create mode 100644 include/drm/drm_syncobj.h

diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
index e35920d..743b751 100644
--- a/Documentation/gpu/drm-internals.rst
+++ b/Documentation/gpu/drm-internals.rst
@@ -98,6 +98,11 @@ DRIVER_ATOMIC
     implement appropriate obj->atomic_get_property() vfuncs for any
     modeset objects with driver specific properties.
 
+DRIVER_SYNCOBJ
+    Driver support sync objects. These are just sync files that don't
+    use a file descriptor. They can be used to implement Vulkan shared
+    semaphores.
+
 Major, Minor and Patchlevel
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
index f5760b1..28aebe8 100644
--- a/Documentation/gpu/drm-mm.rst
+++ b/Documentation/gpu/drm-mm.rst
@@ -483,3 +483,9 @@ DRM Cache Handling
 
 .. kernel-doc:: drivers/gpu/drm/drm_cache.c
    :export:
+
+DRM Sync Objects
+===========================
+
+.. kernel-doc:: drivers/gpu/drm/drm_syncobj.c
+   :export:
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index a88d786..734d61f 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -485,7 +485,7 @@ static const struct file_operations sem_file_fops = {
 	.release = sem_file_release,
 };
 
-struct sem_file *sem_file_alloc(void)
+struct sem_file *sem_file_create(void)
 {
 	struct sem_file *sem_file;
 	int ret;
@@ -508,7 +508,7 @@ struct sem_file *sem_file_alloc(void)
 	kfree(sem_file);
 	return NULL;
 }
-EXPORT_SYMBOL(sem_file_alloc);
+EXPORT_SYMBOL(sem_file_create);
 
 struct sem_file *sem_file_fdget(int fd)
 {
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 3ee9579..b5e565c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -16,7 +16,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_framebuffer.o drm_connector.o drm_blend.o \
 		drm_encoder.o drm_mode_object.o drm_property.o \
 		drm_plane.o drm_color_mgmt.o drm_print.o \
-		drm_dumb_buffers.o drm_mode_config.o
+		drm_dumb_buffers.o drm_mode_config.o drm_syncobj.o
 
 drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
 drm-$(CONFIG_DRM_VM) += drm_vm.o
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index afdf5b1..9a61df2 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -219,6 +219,9 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_open(dev, priv);
 
+	if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		drm_syncobj_open(priv);
+
 	if (drm_core_check_feature(dev, DRIVER_PRIME))
 		drm_prime_init_file_private(&priv->prime);
 
@@ -266,6 +269,8 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 out_prime_destroy:
 	if (drm_core_check_feature(dev, DRIVER_PRIME))
 		drm_prime_destroy_file_private(&priv->prime);
+	if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		drm_syncobj_release(priv);
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_release(dev, priv);
 	put_pid(priv->pid);
@@ -400,6 +405,9 @@ int drm_release(struct inode *inode, struct file *filp)
 		drm_property_destroy_user_blobs(dev, file_priv);
 	}
 
+	if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		drm_syncobj_release(file_priv);
+
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_release(dev, file_priv);
 
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index f37388c..44ef903 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -142,4 +142,17 @@ static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
 {
 	return 0;
 }
+
 #endif
+
+/* drm_syncobj.c */
+void drm_syncobj_open(struct drm_file *file_private);
+void drm_syncobj_release(struct drm_file *file_private);
+int drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
+			     struct drm_file *file_private);
+int drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
+			      struct drm_file *file_private);
+int drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file_private);
+int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file_private);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index a7c61c2..6da7adc 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -240,6 +240,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 		req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0;
 		req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0;
 		return 0;
+	case DRM_CAP_SYNCOBJ:
+		req->value = drm_core_check_feature(dev, DRIVER_SYNCOBJ);
+		return 0;
 	}
 
 	/* Other caps only work with KMS drivers */
@@ -641,6 +644,15 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_CREATE, drm_syncobj_create_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_DESTROY, drm_syncobj_destroy_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD, drm_syncobj_handle_to_fd_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
new file mode 100644
index 0000000..9cd768a
--- /dev/null
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -0,0 +1,285 @@
+/*
+ * Copyright © 2017 Red Hat
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *
+ */
+
+/**
+ * DOC: Overview
+ *
+ * DRM synchronisation objects (syncobj) are wrappers for sem_file objects,
+ * that don't use fd space, and can be converted to/from sem_file fds.
+ *
+ * Depending on the sync object type, they can expose different sem_file
+ * semantics and restrictions.
+ *
+ * Their primary use-case is to implement Vulkan shared semaphores.
+ */
+
+#include <drm/drmP.h>
+#include <linux/sync_file.h>
+#include "drm_internal.h"
+#include <drm/drm_syncobj.h>
+
+static struct sem_file *drm_syncobj_get(struct drm_file *file_private,
+					 u32 handle)
+{
+	struct sem_file *sem_file;
+
+	spin_lock(&file_private->syncobj_table_lock);
+
+	/* Check if we currently have a reference on the object */
+	sem_file = idr_find(&file_private->syncobj_idr, handle);
+	if (sem_file)
+		get_file(sem_file->base.file);
+
+	spin_unlock(&file_private->syncobj_table_lock);
+
+	return sem_file;
+}
+
+/**
+ * drm_syncobj_replace_fence - lookup and replace fence in a sync object.
+ * @file_private - drm file private pointer.
+ * @handle - syncobj handle to lookup
+ * @fence - fence to install in sync file.
+ * Returns:
+ * 0 on success, or -EINVAL when the handle doesn't point at a valid sem file.
+ *
+ * This looks up a sync object and replaces the fence on it, freeing
+ * the old one.
+ */
+int drm_syncobj_replace_fence(struct drm_file *file_private,
+			      u32 handle,
+			      struct dma_fence *fence)
+{
+	struct sem_file *sem_file = drm_syncobj_get(file_private, handle);
+	struct dma_fence *old_fence = NULL;
+	int r;
+
+	if (!sem_file)
+		return -EINVAL;
+
+	r = sem_file_replace_fence(sem_file, fence, &old_fence);
+	dma_fence_put(old_fence);
+	return r;
+}
+EXPORT_SYMBOL(drm_syncobj_replace_fence);
+
+int drm_syncobj_fence_get(struct drm_file *file_private,
+			  u32 handle,
+			  struct dma_fence **fence)
+{
+	struct sem_file *sem_file = drm_syncobj_get(file_private, handle);
+
+	if (!sem_file)
+		return -EINVAL;
+
+	*fence = sem_file_get_fence(sem_file);
+	if (!*fence)
+		return -EINVAL;
+	return 0;
+}
+EXPORT_SYMBOL(drm_syncobj_fence_get);
+
+static int drm_syncobj_create(struct drm_file *file_private,
+			      u32 *handle)
+{
+	struct sem_file *sem_file;
+	int ret;
+
+	sem_file = sem_file_create();
+	if (!sem_file)
+		return -ENOMEM;
+
+	idr_preload(GFP_KERNEL);
+	spin_lock(&file_private->syncobj_table_lock);
+	ret = idr_alloc(&file_private->syncobj_idr, sem_file, 1, 0, GFP_NOWAIT);
+	spin_unlock(&file_private->syncobj_table_lock);
+
+	idr_preload_end();
+
+	if (ret < 0) {
+		fput(sem_file->base.file);
+		return ret;
+	}
+
+	*handle = ret;
+	return 0;
+}
+
+static int drm_syncobj_destroy(struct drm_file *file_private,
+			       u32 handle)
+{
+	struct sem_file *sem_file;
+
+	spin_lock(&file_private->syncobj_table_lock);
+	sem_file = idr_remove(&file_private->syncobj_idr, handle);
+	spin_unlock(&file_private->syncobj_table_lock);
+
+	if (!sem_file)
+		return -EINVAL;
+
+	fput(sem_file->base.file);
+	return 0;
+}
+
+static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
+				    u32 handle, int *fd)
+{
+	struct sem_file *sem_file = drm_syncobj_get(file_private, handle);
+	int ret;
+	if (!sem_file)
+		return -EINVAL;
+
+	ret = get_unused_fd_flags(O_CLOEXEC);
+	if (ret < 0) {
+		fput(sem_file->base.file);
+		return ret;
+	}
+	fd_install(ret, sem_file->base.file);
+	*fd = ret;
+	/* leave reference with fd */
+	return 0;
+}
+
+static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
+				    int fd, u32 *handle)
+{
+	struct sem_file *sem_file = sem_file_fdget(fd);
+	int ret;
+	if (!sem_file)
+		return -EINVAL;
+
+	idr_preload(GFP_KERNEL);
+	spin_lock(&file_private->syncobj_table_lock);
+	ret = idr_alloc(&file_private->syncobj_idr, sem_file, 1, 0, GFP_NOWAIT);
+	spin_unlock(&file_private->syncobj_table_lock);
+	idr_preload_end();
+
+	if (ret < 0) {
+		fput(sem_file->base.file);
+		return ret;
+	}
+	*handle = ret;
+	return 0;
+}
+
+/**
+ * drm_syncobj_open - initalizes syncobj file-private structures at devnode open time
+ * @dev: drm_device which is being opened by userspace
+ * @file_private: drm file-private structure to set up
+ *
+ * Called at device open time, sets up the structure for handling refcounting
+ * of sync objects.
+ */
+void
+drm_syncobj_open(struct drm_file *file_private)
+{
+	idr_init(&file_private->syncobj_idr);
+	spin_lock_init(&file_private->syncobj_table_lock);
+}
+
+static int
+drm_syncobj_release_handle(int id, void *ptr, void *data)
+{
+	struct sem_file *sem_file = ptr;
+
+	fput(sem_file->base.file);
+	return 0;
+}
+
+/**
+ * drm_syncobj_release - release file-private sync object resources
+ * @dev: drm_device which is being closed by userspace
+ * @file_private: drm file-private structure to clean up
+ *
+ * Called at close time when the filp is going away.
+ *
+ * Releases any remaining references on objects by this filp.
+ */
+void
+drm_syncobj_release(struct drm_file *file_private)
+{
+	idr_for_each(&file_private->syncobj_idr,
+		     &drm_syncobj_release_handle, file_private);
+	idr_destroy(&file_private->syncobj_idr);
+}
+
+int
+drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
+			 struct drm_file *file_private)
+{
+	struct drm_syncobj_create *args = data;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	/* no valid flags yet */
+	if (args->flags)
+		return -EINVAL;
+
+	return drm_syncobj_create(file_private,
+				  &args->handle);
+}
+
+int
+drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
+			  struct drm_file *file_private)
+{
+	struct drm_syncobj_destroy *args = data;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	/* make sure padding is empty */
+	if (args->pad)
+		return -EINVAL;
+	return drm_syncobj_destroy(file_private, args->handle);
+}
+
+int
+drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file_private)
+{
+	struct drm_syncobj_handle *args = data;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	return drm_syncobj_handle_to_fd(file_private, args->handle,
+					&args->fd);
+}
+
+int
+drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file_private)
+{
+	struct drm_syncobj_handle *args = data;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	return drm_syncobj_fd_to_handle(file_private, args->fd,
+					&args->handle);
+}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 6105c05..1d48f6f 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -405,6 +405,11 @@ struct drm_file {
 	/** Lock for synchronization of access to object_idr. */
 	spinlock_t table_lock;
 
+	/** Mapping of sync object handles to object pointers. */
+	struct idr syncobj_idr;
+	/** Lock for synchronization of access to syncobj_idr. */
+	spinlock_t syncobj_table_lock;
+
 	struct file *filp;
 	void *driver_priv;
 
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 5699f42..48ff06b 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -53,6 +53,7 @@ struct drm_mode_create_dumb;
 #define DRIVER_RENDER			0x8000
 #define DRIVER_ATOMIC			0x10000
 #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
+#define DRIVER_SYNCOBJ                  0x40000
 
 /**
  * struct drm_driver - DRM driver structure
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
new file mode 100644
index 0000000..714b658
--- /dev/null
+++ b/include/drm/drm_syncobj.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright © 2017 Red Hat
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *
+ */
+#ifndef __DRM_SYNCOBJ_H__
+#define __DRM_SYNCOBJ_H__
+
+int drm_syncobj_fence_get(struct drm_file *file_private,
+			  u32 handle,
+			  struct dma_fence **fence);
+int drm_syncobj_replace_fence(struct drm_file *file_private,
+			      u32 handle,
+			      struct dma_fence *fence);
+
+#endif
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index b2c5284..dd0b99c 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -647,6 +647,7 @@ struct drm_gem_open {
 #define DRM_CAP_CURSOR_HEIGHT		0x9
 #define DRM_CAP_ADDFB2_MODIFIERS	0x10
 #define DRM_CAP_PAGE_FLIP_TARGET	0x11
+#define DRM_CAP_SYNCOBJ		0x13
 
 /** DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
@@ -696,6 +697,25 @@ struct drm_prime_handle {
 	__s32 fd;
 };
 
+struct drm_syncobj_create {
+	__u32 handle;
+	__u32 flags;
+};
+
+struct drm_syncobj_destroy {
+	__u32 handle;
+	__u32 pad;
+};
+
+struct drm_syncobj_handle {
+	__u32 handle;
+	/** Flags.. only applicable for handle->fd */
+	__u32 flags;
+
+	__s32 fd;
+	__u32 pad;
+};
+
 #if defined(__cplusplus)
 }
 #endif
@@ -814,6 +834,11 @@ extern "C" {
 #define DRM_IOCTL_MODE_CREATEPROPBLOB	DRM_IOWR(0xBD, struct drm_mode_create_blob)
 #define DRM_IOCTL_MODE_DESTROYPROPBLOB	DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
 
+#define DRM_IOCTL_SYNCOBJ_CREATE	DRM_IOWR(0xBF, struct drm_syncobj_create)
+#define DRM_IOCTL_SYNCOBJ_DESTROY	DRM_IOWR(0xC0, struct drm_syncobj_destroy)
+#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD	DRM_IOWR(0xC1, struct drm_syncobj_handle)
+#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE	DRM_IOWR(0xC2, struct drm_syncobj_handle)
+
 /**
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
-- 
2.9.3

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

[PATCH 6/7] amdgpu/cs: split out fence dependency checking

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This just splits out the fence depenency checking into it's
own function to make it easier to add semaphore dependencies.

Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 85 +++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 99424cb..df25b32 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -963,56 +963,65 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev,
 	return 0;
 }
 
-static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
-				  struct amdgpu_cs_parser *p)
+static int amdgpu_process_fence_dep(struct amdgpu_cs_parser *p,
+				    struct amdgpu_cs_chunk *chunk)
 {
 	struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
-	int i, j, r;
-
-	for (i = 0; i < p->nchunks; ++i) {
-		struct drm_amdgpu_cs_chunk_dep *deps;
-		struct amdgpu_cs_chunk *chunk;
-		unsigned num_deps;
+	unsigned num_deps;
+	int i, r;
+	struct drm_amdgpu_cs_chunk_dep *deps;
 
-		chunk = &p->chunks[i];
+	deps = (struct drm_amdgpu_cs_chunk_dep *)chunk->kdata;
+	num_deps = chunk->length_dw * 4 /
+		sizeof(struct drm_amdgpu_cs_chunk_dep);
 
-		if (chunk->chunk_id != AMDGPU_CHUNK_ID_DEPENDENCIES)
-			continue;
+	for (i = 0; i < num_deps; ++i) {
+		struct amdgpu_ring *ring;
+		struct amdgpu_ctx *ctx;
+		struct dma_fence *fence;
 
-		deps = (struct drm_amdgpu_cs_chunk_dep *)chunk->kdata;
-		num_deps = chunk->length_dw * 4 /
-			sizeof(struct drm_amdgpu_cs_chunk_dep);
+		r = amdgpu_cs_get_ring(p->adev, deps[i].ip_type,
+				       deps[i].ip_instance,
+				       deps[i].ring, &ring);
+		if (r)
+			return r;
 
-		for (j = 0; j < num_deps; ++j) {
-			struct amdgpu_ring *ring;
-			struct amdgpu_ctx *ctx;
-			struct dma_fence *fence;
+		ctx = amdgpu_ctx_get(fpriv, deps[i].ctx_id);
+		if (ctx == NULL)
+			return -EINVAL;
 
-			r = amdgpu_cs_get_ring(adev, deps[j].ip_type,
-					       deps[j].ip_instance,
-					       deps[j].ring, &ring);
+		fence = amdgpu_ctx_get_fence(ctx, ring,
+					     deps[i].handle);
+		if (IS_ERR(fence)) {
+			r = PTR_ERR(fence);
+			amdgpu_ctx_put(ctx);
+			return r;
+		} else if (fence) {
+			r = amdgpu_sync_fence(p->adev, &p->job->sync,
+					      fence);
+			dma_fence_put(fence);
+			amdgpu_ctx_put(ctx);
 			if (r)
 				return r;
+		}
+	}
+	return 0;
+}
 
-			ctx = amdgpu_ctx_get(fpriv, deps[j].ctx_id);
-			if (ctx == NULL)
-				return -EINVAL;
+static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
+				  struct amdgpu_cs_parser *p)
+{
+	int i, r;
 
-			fence = amdgpu_ctx_get_fence(ctx, ring,
-						     deps[j].handle);
-			if (IS_ERR(fence)) {
-				r = PTR_ERR(fence);
-				amdgpu_ctx_put(ctx);
-				return r;
+	for (i = 0; i < p->nchunks; ++i) {
+		struct amdgpu_cs_chunk *chunk;
 
-			} else if (fence) {
-				r = amdgpu_sync_fence(adev, &p->job->sync,
-						      fence);
-				dma_fence_put(fence);
-				amdgpu_ctx_put(ctx);
-				if (r)
-					return r;
-			}
+		chunk = &p->chunks[i];
+
+		if (chunk->chunk_id == AMDGPU_CHUNK_ID_DEPENDENCIES) {
+			r = amdgpu_process_fence_dep(p, chunk);
+			if (r)
+				return r;
 		}
 	}
 
-- 
2.9.3

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

[PATCH 7/7] amdgpu: use sync file for shared semaphores (v3)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This creates a new command submission chunk for amdgpu
to add wait and signal sync objects around the submission.

Sync objects are managed via the drm syncobj ioctls.

The command submission interface is enhanced with two new
chunks, one for semaphore waiting, one for semaphore signalling
and just takes a list of handles for each.

This is based on work originally done by David Zhou at AMD,
with input from Christian Konig on what things should look like.

NOTE: this interface addition needs a version bump to expose
it to userspace.

v1.1: keep file reference on import.
v2: move to using syncobjs
v2.1: change some APIs to just use p pointer.
v3: make more robust against CS failures, we now add the
wait sems but only remove them once the CS job has been
submitted.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 112 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |   2 +-
 include/uapi/drm/amdgpu_drm.h           |   6 ++
 3 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index df25b32..ed114c6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -27,6 +27,7 @@
 #include <linux/pagemap.h>
 #include <drm/drmP.h>
 #include <drm/amdgpu_drm.h>
+#include <drm/drm_syncobj.h>
 #include "amdgpu.h"
 #include "amdgpu_trace.h"
 
@@ -217,6 +218,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
 			break;
 
 		case AMDGPU_CHUNK_ID_DEPENDENCIES:
+		case AMDGPU_CHUNK_ID_SEM_WAIT:
+		case AMDGPU_CHUNK_ID_SEM_SIGNAL:
 			break;
 
 		default:
@@ -1008,6 +1011,65 @@ static int amdgpu_process_fence_dep(struct amdgpu_cs_parser *p,
 	return 0;
 }
 
+static int amdgpu_sem_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
+					     uint32_t handle)
+{
+	int r;
+	struct dma_fence *fence;
+	r = drm_syncobj_fence_get(p->filp, handle, &fence);
+	if (r)
+		return r;
+
+	r = amdgpu_sync_fence(p->adev, &p->job->sync, fence);
+	dma_fence_put(fence);
+
+	return r;
+}
+
+static int amdgpu_sem_lookup_and_remove(struct amdgpu_cs_parser *p,
+				      uint32_t handle)
+{
+	return drm_syncobj_replace_fence(p->filp, handle, NULL);
+}
+
+static int amdgpu_process_sem_wait_dep(struct amdgpu_cs_parser *p,
+				       struct amdgpu_cs_chunk *chunk)
+{
+	unsigned num_deps;
+	int i, r;
+	struct drm_amdgpu_cs_chunk_sem *deps;
+
+	deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
+	num_deps = chunk->length_dw * 4 /
+		sizeof(struct drm_amdgpu_cs_chunk_sem);
+
+	for (i = 0; i < num_deps; ++i) {
+		r = amdgpu_sem_lookup_and_add_to_sync(p, deps[i].handle);
+		if (r)
+			return r;
+	}
+	return 0;
+}
+
+static int amdgpu_process_sem_wait_post(struct amdgpu_cs_parser *p,
+					struct amdgpu_cs_chunk *chunk)
+{
+	unsigned num_deps;
+	int i, r;
+	struct drm_amdgpu_cs_chunk_sem *deps;
+
+	deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
+	num_deps = chunk->length_dw * 4 /
+		sizeof(struct drm_amdgpu_cs_chunk_sem);
+
+	for (i = 0; i < num_deps; ++i) {
+		r = amdgpu_sem_lookup_and_remove(p, deps[i].handle);
+		if (r)
+			return r;
+	}
+	return 0;
+}
+
 static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
 				  struct amdgpu_cs_parser *p)
 {
@@ -1022,12 +1084,60 @@ static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
 			r = amdgpu_process_fence_dep(p, chunk);
 			if (r)
 				return r;
+		} else if (chunk->chunk_id == AMDGPU_CHUNK_ID_SEM_WAIT) {
+			r = amdgpu_process_sem_wait_dep(p, chunk);
+			if (r)
+				return r;
 		}
 	}
 
 	return 0;
 }
 
+static int amdgpu_process_sem_signal_dep(struct amdgpu_cs_parser *p,
+					 struct amdgpu_cs_chunk *chunk)
+{
+	unsigned num_deps;
+	int i, r;
+	struct drm_amdgpu_cs_chunk_sem *deps;
+
+	deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
+	num_deps = chunk->length_dw * 4 /
+		sizeof(struct drm_amdgpu_cs_chunk_sem);
+
+	for (i = 0; i < num_deps; ++i) {
+		r = drm_syncobj_replace_fence(p->filp, deps[i].handle,
+					      p->fence);
+		if (r)
+			return r;
+	}
+	return 0;
+}
+
+static int amdgpu_cs_post_dependencies(struct amdgpu_cs_parser *p)
+{
+	int i, r;
+
+	for (i = 0; i < p->nchunks; ++i) {
+		struct amdgpu_cs_chunk *chunk;
+
+		chunk = &p->chunks[i];
+
+		if (chunk->chunk_id == AMDGPU_CHUNK_ID_SEM_WAIT) {
+			r = amdgpu_process_sem_wait_post(p, chunk);
+			if (r)
+				return r;
+		}
+
+		if (chunk->chunk_id == AMDGPU_CHUNK_ID_SEM_SIGNAL) {
+			r = amdgpu_process_sem_signal_dep(p, chunk);
+			if (r)
+				return r;
+		}
+	}
+	return 0;
+}
+
 static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 			    union drm_amdgpu_cs *cs)
 {
@@ -1055,7 +1165,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 	trace_amdgpu_cs_ioctl(job);
 	amd_sched_entity_push_job(&job->base);
 
-	return 0;
+	return amdgpu_cs_post_dependencies(p);
 }
 
 int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b76cd69..e95951e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -683,7 +683,7 @@ static struct drm_driver kms_driver = {
 	.driver_features =
 	    DRIVER_USE_AGP |
 	    DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
-	    DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET,
+	    DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
 	.load = amdgpu_driver_load_kms,
 	.open = amdgpu_driver_open_kms,
 	.preclose = amdgpu_driver_preclose_kms,
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 5797283..647c520 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -390,6 +390,8 @@ struct drm_amdgpu_gem_va {
 #define AMDGPU_CHUNK_ID_IB		0x01
 #define AMDGPU_CHUNK_ID_FENCE		0x02
 #define AMDGPU_CHUNK_ID_DEPENDENCIES	0x03
+#define AMDGPU_CHUNK_ID_SEM_WAIT        0x04
+#define AMDGPU_CHUNK_ID_SEM_SIGNAL      0x05
 
 struct drm_amdgpu_cs_chunk {
 	__u32		chunk_id;
@@ -454,6 +456,10 @@ struct drm_amdgpu_cs_chunk_fence {
 	__u32 offset;
 };
 
+struct drm_amdgpu_cs_chunk_sem {
+	__u32 handle;
+};
+
 struct drm_amdgpu_cs_chunk_data {
 	union {
 		struct drm_amdgpu_cs_chunk_ib		ib_data;
-- 
2.9.3

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

Re: [PATCH 4/7] sync_file: add support for sem_file

[Chris Wilson]

On Thu, Apr 13, 2017 at 11:41:41AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This adds support for a file that has semaphore semantics
> (for Vulkan shared semaphores).
> 
> These objects are persistent objects that can have a
> fence that changes. When the object is signaled, a fence
> is attached to it, and when an object is waited on, the
> fence is removed. All interactions with these objects
> should be via command submission routines in the drm
> drivers. The sem_file is just for passing the sems between
> processes.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/dma-buf/sync_file.c | 101 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sync_file.h   |  16 +++++++
>  2 files changed, 117 insertions(+)
> 
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 2342d8b..a88d786 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -468,3 +468,104 @@ static const struct file_operations sync_file_fops = {
>  	.unlocked_ioctl = sync_file_ioctl,
>  	.compat_ioctl = sync_file_ioctl,
>  };
> +
> +static int sem_file_release(struct inode *inode, struct file *file)
> +{
> +	struct sem_file *sem_file = file->private_data;
> +	struct dma_fence *fence;
> +
> +	fence = rcu_dereference_protected(sem_file->base.fence, 1);
> +	dma_fence_put(fence);
> +	kfree(sem_file);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations sem_file_fops = {
> +	.release = sem_file_release,
> +};
> +
> +struct sem_file *sem_file_alloc(void)
> +{
> +	struct sem_file *sem_file;
> +	int ret;
> +
> +	sem_file = kzalloc(sizeof(*sem_file), GFP_KERNEL);
> +	if (!sem_file)
> +		return NULL;
> +
> +	ret = fence_file_init(&sem_file->base,
> +			      &sem_file_fops);
> +	if (ret)
> +		goto err;
> +
> +	RCU_INIT_POINTER(sem_file->base.fence, NULL);
> +	mutex_init(&sem_file->lock);
> +
> +	return sem_file;
> +
> +err:
> +	kfree(sem_file);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(sem_file_alloc);
> +
> +struct sem_file *sem_file_fdget(int fd)
> +{
> +	struct file *file = fget(fd);
> +
> +	if (!file)
> +		return NULL;
> +
> +	if (file->f_op != &sem_file_fops)
> +		goto err;
> +
> +	return file->private_data;
> +
> +err:
> +	fput(file);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(sem_file_fdget);
> +
> +#define sem_file_held(obj) lockdep_is_held(&(obj)->lock)
> +
> +struct dma_fence *sem_file_get_fence(struct sem_file *sem_file)
> +{
> +	struct dma_fence *fence;
> +
> +	if (!rcu_access_pointer(sem_file->base.fence)) {
> +		return NULL;
> +	}
> +
> +	rcu_read_lock();
> +	fence = dma_fence_get_rcu_safe(&sem_file->base.fence);
> +	rcu_read_unlock();
> +	return fence;
> +}
> +EXPORT_SYMBOL(sem_file_get_fence);
> +
> +static inline struct dma_fence *
> +sem_file_get_fence_locked(struct sem_file *sem_file)
> +{
> +	return rcu_dereference_protected(sem_file->base.fence,
> +					 sem_file_held(sem_file));
> +}
> +
> +int sem_file_replace_fence(struct sem_file *sem_file,
> +			   struct dma_fence *fence,
> +			   struct dma_fence **old_fence)
> +{
> +	struct dma_fence *ret_fence = NULL;
> +
> +	if (fence)
> +		dma_fence_get(fence);
> +
> +	mutex_lock(&sem_file->lock);
> +	ret_fence = sem_file_get_fence_locked(sem_file);
> +	RCU_INIT_POINTER(sem_file->base.fence, fence);
> +	mutex_unlock(&sem_file->lock);

Is xchg() universal?

struct dma_fence *sem_file_replace_fence(struct sem_file *sem_file,
					 struct dma_fence *fence)
{
	return xchg(&sem_file->base.fence, dma_fence_get(fence));
}

safe against the rcu read and kills off the mutex.

I think this is the cleaner approach, precisely because it stops me
having the delusion that the semaphores and sync_file are
interchangeable, and I won't ask if I can merge semaphores together, or
if I can inspect the state with the CPU.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/7] sync_file: get rid of internal reference count.

[Gustavo Padovan]

2017-04-13 Dave Airlie <airlied@gmail.com>:

> From: Dave Airlie <airlied@redhat.com>
> 
> sync_file uses the reference count of the file, the internal
> kref was never getting moved past 1.
> 
> We can reintroduce this if we decide we need it later.
> 
> [airlied: fix buildbot warnings]
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/dma-buf/sync_file.c | 13 ++-----------
>  include/linux/sync_file.h   |  3 ---
>  2 files changed, 2 insertions(+), 14 deletions(-)

Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>

Or should I just push this one?

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

Re: [PATCH 1/7] sync_file: get rid of internal reference count.

[Sumit Semwal]

On 17 April 2017 at 18:43, Gustavo Padovan <gustavo@padovan.org> wrote:
> 2017-04-13 Dave Airlie <airlied@gmail.com>:
>
>> From: Dave Airlie <airlied@redhat.com>
>>
>> sync_file uses the reference count of the file, the internal
>> kref was never getting moved past 1.
>>
>> We can reintroduce this if we decide we need it later.
>>
>> [airlied: fix buildbot warnings]
>>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> ---
>>  drivers/dma-buf/sync_file.c | 13 ++-----------
>>  include/linux/sync_file.h   |  3 ---
>>  2 files changed, 2 insertions(+), 14 deletions(-)
>
> Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
>
> Or should I just push this one?
Of course, go right ahead, Gustavo :)
>
> Gustavo
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] sync_file: get rid of internal reference count.

[Gustavo Padovan]

2017-04-17 Sumit Semwal <sumit.semwal@linaro.org>:

> On 17 April 2017 at 18:43, Gustavo Padovan <gustavo@padovan.org> wrote:
> > 2017-04-13 Dave Airlie <airlied@gmail.com>:
> >
> >> From: Dave Airlie <airlied@redhat.com>
> >>
> >> sync_file uses the reference count of the file, the internal
> >> kref was never getting moved past 1.
> >>
> >> We can reintroduce this if we decide we need it later.
> >>
> >> [airlied: fix buildbot warnings]
> >>
> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Signed-off-by: Dave Airlie <airlied@redhat.com>
> >> ---
> >>  drivers/dma-buf/sync_file.c | 13 ++-----------
> >>  include/linux/sync_file.h   |  3 ---
> >>  2 files changed, 2 insertions(+), 14 deletions(-)
> >
> > Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> Acked-by: Sumit Semwal <sumit.semwal@linaro.org>
> >
> > Or should I just push this one?
> Of course, go right ahead, Gustavo :)

pushed to drm-misc-next.

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

Re: [PATCH 3/7] sync_file: split out fence_file base class from sync_file.

[Christian König]

Am 13.04.2017 um 03:41 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
>
> This just splits out a common base object to be shared
> between sync_file and sem_files.

I don't think I like that approach.

A good argument of using sync_file instead of self coding it was to be 
able to be able to use the resulting fd with the atomic mode setting IOCTLs.

That advantage will completely vanish if we introduce a new fd type.

Regards,
Christian.

> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>   drivers/dma-buf/sync_file.c  | 49 +++++++++++++++++++++++++++-----------------
>   drivers/gpu/drm/drm_atomic.c |  4 ++--
>   include/linux/sync_file.h    | 17 ++++++++++-----
>   3 files changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 9b7ad7e..2342d8b 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -28,17 +28,28 @@
>   
>   static const struct file_operations sync_file_fops;
>   
> +static int fence_file_init(struct fence_file *fence_file,
> +			   const struct file_operations *fops)
> +{
> +	fence_file->file = anon_inode_getfile("fence_file", fops,
> +					     fence_file, 0);
> +	if (IS_ERR(fence_file->file))
> +		return PTR_ERR(fence_file->file);
> +	return 0;
> +}
> +
>   static struct sync_file *sync_file_alloc(void)
>   {
>   	struct sync_file *sync_file;
> +	int ret;
>   
>   	sync_file = kzalloc(sizeof(*sync_file), GFP_KERNEL);
>   	if (!sync_file)
>   		return NULL;
>   
> -	sync_file->file = anon_inode_getfile("sync_file", &sync_file_fops,
> -					     sync_file, 0);
> -	if (IS_ERR(sync_file->file))
> +	ret = fence_file_init(&sync_file->base,
> +			      &sync_file_fops);
> +	if (ret)
>   		goto err;
>   
>   	init_waitqueue_head(&sync_file->wq);
> @@ -67,7 +78,7 @@ static void fence_check_cb_func(struct dma_fence *f, struct dma_fence_cb *cb)
>    *
>    * Creates a sync_file containg @fence. This function acquires and additional
>    * reference of @fence for the newly-created &sync_file, if it succeeds. The
> - * sync_file can be released with fput(sync_file->file). Returns the
> + * sync_file can be released with fput(sync_file->base.file). Returns the
>    * sync_file or NULL in case of error.
>    */
>   struct sync_file *sync_file_create(struct dma_fence *fence)
> @@ -78,7 +89,7 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
>   	if (!sync_file)
>   		return NULL;
>   
> -	RCU_INIT_POINTER(sync_file->fence, dma_fence_get(fence));
> +	RCU_INIT_POINTER(sync_file->base.fence, dma_fence_get(fence));
>   
>   	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
>   		 fence->ops->get_driver_name(fence),
> @@ -122,8 +133,8 @@ struct dma_fence *sync_file_get_fence(int fd)
>   	if (!sync_file)
>   		return NULL;
>   
> -	fence = dma_fence_get(rcu_dereference_protected(sync_file->fence, 1));
> -	fput(sync_file->file);
> +	fence = dma_fence_get(rcu_dereference_protected(sync_file->base.fence, 1));
> +	fput(sync_file->base.file);
>   
>   	return fence;
>   }
> @@ -141,7 +152,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
>   	 * we own the reference of the dma_fence_array creation.
>   	 */
>   	if (num_fences == 1) {
> -		RCU_INIT_POINTER(sync_file->fence, fences[0]);
> +		RCU_INIT_POINTER(sync_file->base.fence, fences[0]);
>   		kfree(fences);
>   	} else {
>   		array = dma_fence_array_create(num_fences, fences,
> @@ -150,7 +161,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
>   		if (!array)
>   			return -ENOMEM;
>   
> -		RCU_INIT_POINTER(sync_file->fence, &array->base);
> +		RCU_INIT_POINTER(sync_file->base.fence, &array->base);
>   	}
>   
>   	return 0;
> @@ -159,7 +170,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
>   static struct dma_fence **get_fences(struct sync_file *sync_file,
>   				     int *num_fences)
>   {
> -	struct dma_fence *fence = rcu_dereference_protected(sync_file->fence, 1);
> +	struct dma_fence *fence = rcu_dereference_protected(sync_file->base.fence, 1);
>   	if (dma_fence_is_array(fence)) {
>   		struct dma_fence_array *array = to_dma_fence_array(fence);
>   
> @@ -168,7 +179,7 @@ static struct dma_fence **get_fences(struct sync_file *sync_file,
>   	}
>   
>   	*num_fences = 1;
> -	return &sync_file->fence;
> +	return &sync_file->base.fence;
>   }
>   
>   static void add_fence(struct dma_fence **fences,
> @@ -271,7 +282,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
>   	return sync_file;
>   
>   err:
> -	fput(sync_file->file);
> +	fput(sync_file->base.file);
>   	return NULL;
>   
>   }
> @@ -281,7 +292,7 @@ static int sync_file_release(struct inode *inode, struct file *file)
>   	struct sync_file *sync_file = file->private_data;
>   	struct dma_fence *fence;
>   
> -	fence = rcu_dereference_protected(sync_file->fence, 1);
> +	fence = rcu_dereference_protected(sync_file->base.fence, 1);
>   	if (test_bit(POLL_ENABLED, &fence->flags))
>   		dma_fence_remove_callback(fence, &sync_file->cb);
>   	dma_fence_put(fence);
> @@ -295,7 +306,7 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait)
>   	struct sync_file *sync_file = file->private_data;
>   	struct dma_fence *fence;
>   
> -	fence = rcu_dereference_protected(sync_file->fence, 1);
> +	fence = rcu_dereference_protected(sync_file->base.fence, 1);
>   
>   	poll_wait(file, &sync_file->wq, wait);
>   
> @@ -348,15 +359,15 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file,
>   		goto err_put_fence3;
>   	}
>   
> -	fd_install(fd, fence3->file);
> -	fput(fence2->file);
> +	fd_install(fd, fence3->base.file);
> +	fput(fence2->base.file);
>   	return 0;
>   
>   err_put_fence3:
> -	fput(fence3->file);
> +	fput(fence3->base.file);
>   
>   err_put_fence2:
> -	fput(fence2->file);
> +	fput(fence2->base.file);
>   
>   err_put_fd:
>   	put_unused_fd(fd);
> @@ -420,7 +431,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>   
>   no_fences:
>   	strlcpy(info.name, sync_file->name, sizeof(info.name));
> -	info.status = dma_fence_is_signaled(rcu_dereference_protected(sync_file->fence, 1));
> +	info.status = dma_fence_is_signaled(rcu_dereference_protected(sync_file->base.fence, 1));
>   	info.num_fences = num_fences;
>   
>   	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..16053a0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2012,7 +2012,7 @@ static void complete_crtc_signaling(struct drm_device *dev,
>   	if (install_fds) {
>   		for (i = 0; i < num_fences; i++)
>   			fd_install(fence_state[i].fd,
> -				   fence_state[i].sync_file->file);
> +				   fence_state[i].sync_file->base.file);
>   
>   		kfree(fence_state);
>   		return;
> @@ -2036,7 +2036,7 @@ static void complete_crtc_signaling(struct drm_device *dev,
>   
>   	for (i = 0; i < num_fences; i++) {
>   		if (fence_state[i].sync_file)
> -			fput(fence_state[i].sync_file->file);
> +			fput(fence_state[i].sync_file->base.file);
>   		if (fence_state[i].fd >= 0)
>   			put_unused_fd(fence_state[i].fd);
>   
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index adbc0b9..b0ae1cf 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -21,24 +21,31 @@
>   #include <linux/dma-fence-array.h>
>   
>   /**
> - * struct sync_file - sync file to export to the userspace
> + * struct fence_file - basic file to fence mapping
>    * @file:		file representing this fence
> + * @fence:		fence with the fences in the fence_file
> + */
> +struct fence_file {
> +	struct file		*file;
> +	struct dma_fence	__rcu *fence;
> +};
> +
> +/**
> + * struct sync_file - sync file to export to the userspace
> + * @base:               base fence file
>    * @name:		name of sync_file.  Useful for debugging
>    * @sync_file_list:	membership in global file list
>    * @wq:			wait queue for fence signaling
> - * @fence:		fence with the fences in the sync_file
>    * @cb:			fence callback information
>    */
>   struct sync_file {
> -	struct file		*file;
> +	struct fence_file       base;
>   	char			name[32];
>   #ifdef CONFIG_DEBUG_FS
>   	struct list_head	sync_file_list;
>   #endif
>   
>   	wait_queue_head_t	wq;
> -
> -	struct dma_fence	__rcu *fence;
>   	struct dma_fence_cb cb;
>   };
>   


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

Re: [rfc repost] drm sync objects - a new beginning (make ickle happier?)

[Christian König]

Am 13.04.2017 um 03:41 schrieb Dave Airlie:
> Okay I've taken Chris's suggestions to heart and reworked things
> around a sem_file to see how they might look.
>
> This means the drm_syncobj are currently only useful for semaphores,
> the flags field could be used in future to use it for other things,
> and we can reintroduce some of the API then if needed.
>
> This refactors sync_file first to add some basic rcu wrappers
> about the fence pointer, as this point never updates this should
> all be fine unlocked.
>
> It then creates the sem_file with a mutex, and uses that to
> track the semaphores with reduced fops and the replace and
> get APIs.
>
> Then it reworks the drm stuff on top, and fixes amdgpu bug
> with old_fence.
>
> Let's see if anyone prefers one approach over the other.

Yeah, I clearly prefer keeping only one object type for synchronization 
in the kernel.

As I wrote in the other mail the argument of using the sync file for 
semaphores was to be able to use it as in fence with the atomic mode 
setting as well.

That a wait consumes a previous signal should be a specific behavior of 
the operation and not the property of the object.

In other words I'm fine with using the sync_file in a 1:1 fashion with 
Vulkan, but for the atomic API we probably want 1:N to be able to flip a 
rendering result on multiple CRTCs at the same time.

Regards,
Christian.

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


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

Re: [rfc repost] drm sync objects - a new beginning (make ickle happier?)

[James Jones]

On 04/19/2017 05:07 AM, Christian König wrote:
> Am 13.04.2017 um 03:41 schrieb Dave Airlie:
>> Okay I've taken Chris's suggestions to heart and reworked things
>> around a sem_file to see how they might look.
>>
>> This means the drm_syncobj are currently only useful for semaphores,
>> the flags field could be used in future to use it for other things,
>> and we can reintroduce some of the API then if needed.
>>
>> This refactors sync_file first to add some basic rcu wrappers
>> about the fence pointer, as this point never updates this should
>> all be fine unlocked.
>>
>> It then creates the sem_file with a mutex, and uses that to
>> track the semaphores with reduced fops and the replace and
>> get APIs.
>>
>> Then it reworks the drm stuff on top, and fixes amdgpu bug
>> with old_fence.
>>
>> Let's see if anyone prefers one approach over the other.
>
> Yeah, I clearly prefer keeping only one object type for synchronization
> in the kernel.
>
> As I wrote in the other mail the argument of using the sync file for
> semaphores was to be able to use it as in fence with the atomic mode
> setting as well.

This may introduce incompatibilities in userspace though, as the 
response to Dave's original series' pointed out.  For example, the 
Vulkan extensions that allow importing sync files expect them to behave 
as sync files currently do, not as these new objects do.  Introducing 
the new behavior would invalidate language in those specifications, 
causing problems with the very use case I suspect these changes are 
trying to address.  Those specs are not finalized, so it could be fixed, 
but I think that highlights the general concern.

> That a wait consumes a previous signal should be a specific behavior of
> the operation and not the property of the object.
>
> In other words I'm fine with using the sync_file in a 1:1 fashion with
> Vulkan, but for the atomic API we probably want 1:N to be able to flip a
> rendering result on multiple CRTCs at the same time.

Agreed, this usage seems valuable too.  Sem files still have a fence in 
them, and that doesn't seem like an implementation detail that needs to 
be hidden from userspace.  Vulkan solved this very issue by letting 
applications directly extract the sync_file fd from a Vulkan semaphore 
so they could use it with native operations that specifically require a 
sync file, via the experimental external semaphore extensions.  Perhaps 
there could be a sem file -> sync file conversion operation with 
semantics similar to a Vulkan semaphore -> sync file export operation? 
Note the Vulkan semantics for this are in churn, so it might be worth 
holding off a bit on adding that interface if this is the path you use, 
but it shouldn't need to block this series from my high-level read.

Thanks,
-James

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

Re: [rfc repost] drm sync objects - a new beginning (make ickle happier?)

[Dave Airlie]

On 19 April 2017 at 22:07, Christian König <deathsimple@vodafone.de> wrote:
> Am 13.04.2017 um 03:41 schrieb Dave Airlie:
>>
>> Okay I've taken Chris's suggestions to heart and reworked things
>> around a sem_file to see how they might look.
>>
>> This means the drm_syncobj are currently only useful for semaphores,
>> the flags field could be used in future to use it for other things,
>> and we can reintroduce some of the API then if needed.
>>
>> This refactors sync_file first to add some basic rcu wrappers
>> about the fence pointer, as this point never updates this should
>> all be fine unlocked.
>>
>> It then creates the sem_file with a mutex, and uses that to
>> track the semaphores with reduced fops and the replace and
>> get APIs.
>>
>> Then it reworks the drm stuff on top, and fixes amdgpu bug
>> with old_fence.
>>
>> Let's see if anyone prefers one approach over the other.
>
>
> Yeah, I clearly prefer keeping only one object type for synchronization in
> the kernel.
>
> As I wrote in the other mail the argument of using the sync file for
> semaphores was to be able to use it as in fence with the atomic mode setting
> as well.
>
> That a wait consumes a previous signal should be a specific behavior of the
> operation and not the property of the object.
>
> In other words I'm fine with using the sync_file in a 1:1 fashion with
> Vulkan, but for the atomic API we probably want 1:N to be able to flip a
> rendering result on multiple CRTCs at the same time.

Well ideally atomic modesetting should be moved to using syncobjects
as an option.

I'd rather sync_files were limited in scope to interaction with non-drm drivers,
and possibly interprocess operations, consuming fd's is bad and merging doesn't
really fix that.

I'm starting to narrow down to what I think the sync_obj needs to do, and I'm
contemplating a bit more something like the following:

a) no file backing, a simple kref object that gets tracked in an idr
(like a gem object).
This object can have an optional fence attached to it. If there is no
fence, it's unsignalled,
if there is a fence it's signalled. We should provide an interface to
wait on multiple of
these objects that can support Vulkan fence waiting api. We should use
these objects
in command submission interfaces to provide Vulkan fence and semaphore APIs.
These objects will never be grouped or have multiple fences in them.
From a kernel
API we can have multiple waiters and shouldn't enforce the Vulkan semaphore 1:1
at this level, userspace can just create it's own semantics on top.
Open:
These objects are created in advance and then filled in by command
submission ioctls,
or do we want the option for a command submission ioctl to return a
newly created one
of these?

b) sharing of sync objects.
By default we provide a sync_obj->fd conversion, this fd is opaque and
can be passed
between processes to implement things like Vulkan semaphore passing.

c) interoperation with sync_file.
We should provide a mechanism to move a group of sync objects into a sync_file,
so it can be passed into sync_file APIs. We should provide a mechanism
to map a sync_file
into a group of sync objects. This will be a possible 1:n conversion.
Open:
Does the sync_file->syncobj operation create sync objects? or do we
need to pass in a preallocated group?
Does the sync_file->syncobj or syncobj->sync_file operations have any
side effects? I'm tending towards not,
(i.e. no signalling or anything else).

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

Re: [rfc repost] drm sync objects - a new beginning (make ickle happier?)

[Dave Airlie]

On 20 April 2017 at 04:42, Dave Airlie <airlied@gmail.com> wrote:
> On 19 April 2017 at 22:07, Christian König <deathsimple@vodafone.de> wrote:
>> Am 13.04.2017 um 03:41 schrieb Dave Airlie:
>>>
>>> Okay I've taken Chris's suggestions to heart and reworked things
>>> around a sem_file to see how they might look.
>>>
>>> This means the drm_syncobj are currently only useful for semaphores,
>>> the flags field could be used in future to use it for other things,
>>> and we can reintroduce some of the API then if needed.
>>>
>>> This refactors sync_file first to add some basic rcu wrappers
>>> about the fence pointer, as this point never updates this should
>>> all be fine unlocked.
>>>
>>> It then creates the sem_file with a mutex, and uses that to
>>> track the semaphores with reduced fops and the replace and
>>> get APIs.
>>>
>>> Then it reworks the drm stuff on top, and fixes amdgpu bug
>>> with old_fence.
>>>
>>> Let's see if anyone prefers one approach over the other.
>>
>>
>> Yeah, I clearly prefer keeping only one object type for synchronization in
>> the kernel.
>>
>> As I wrote in the other mail the argument of using the sync file for
>> semaphores was to be able to use it as in fence with the atomic mode setting
>> as well.
>>
>> That a wait consumes a previous signal should be a specific behavior of the
>> operation and not the property of the object.
>>
>> In other words I'm fine with using the sync_file in a 1:1 fashion with
>> Vulkan, but for the atomic API we probably want 1:N to be able to flip a
>> rendering result on multiple CRTCs at the same time.
>
> Well ideally atomic modesetting should be moved to using syncobjects
> as an option.
>
> I'd rather sync_files were limited in scope to interaction with non-drm drivers,
> and possibly interprocess operations, consuming fd's is bad and merging doesn't
> really fix that.
>
> I'm starting to narrow down to what I think the sync_obj needs to do, and I'm
> contemplating a bit more something like the following:
>
> a) no file backing, a simple kref object that gets tracked in an idr
> (like a gem object).
> This object can have an optional fence attached to it. If there is no
> fence, it's unsignalled,
> if there is a fence it's signalled.

This should say, if there's a fence, the status of the fence.
Thanks to Andres for pointing it out, it is 5am.

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

Re: [rfc repost] drm sync objects - a new beginning (make ickle happier?)

[Christian König]

Am 19.04.2017 um 21:14 schrieb Dave Airlie:
> On 20 April 2017 at 04:42, Dave Airlie <airlied@gmail.com> wrote:
>> On 19 April 2017 at 22:07, Christian König <deathsimple@vodafone.de> wrote:
>>> Am 13.04.2017 um 03:41 schrieb Dave Airlie:
>>>> Okay I've taken Chris's suggestions to heart and reworked things
>>>> around a sem_file to see how they might look.
>>>>
>>>> This means the drm_syncobj are currently only useful for semaphores,
>>>> the flags field could be used in future to use it for other things,
>>>> and we can reintroduce some of the API then if needed.
>>>>
>>>> This refactors sync_file first to add some basic rcu wrappers
>>>> about the fence pointer, as this point never updates this should
>>>> all be fine unlocked.
>>>>
>>>> It then creates the sem_file with a mutex, and uses that to
>>>> track the semaphores with reduced fops and the replace and
>>>> get APIs.
>>>>
>>>> Then it reworks the drm stuff on top, and fixes amdgpu bug
>>>> with old_fence.
>>>>
>>>> Let's see if anyone prefers one approach over the other.
>>>
>>> Yeah, I clearly prefer keeping only one object type for synchronization in
>>> the kernel.
>>>
>>> As I wrote in the other mail the argument of using the sync file for
>>> semaphores was to be able to use it as in fence with the atomic mode setting
>>> as well.
>>>
>>> That a wait consumes a previous signal should be a specific behavior of the
>>> operation and not the property of the object.
>>>
>>> In other words I'm fine with using the sync_file in a 1:1 fashion with
>>> Vulkan, but for the atomic API we probably want 1:N to be able to flip a
>>> rendering result on multiple CRTCs at the same time.
>> Well ideally atomic modesetting should be moved to using syncobjects
>> as an option.
>>
>> I'd rather sync_files were limited in scope to interaction with non-drm drivers,
>> and possibly interprocess operations, consuming fd's is bad and merging doesn't
>> really fix that.

I agree that sync_files are a bit inflexible, but we can probably all 
agree that lesson learned from flink names is that we should stick with 
fd's for interprocess operations.

So at least that is a good starting point and the additional semantics 
of replacing the fence instead of merging it doesn't sound like so much 
of a difference here.

Another really good design decision of the sync_files is that you can't 
build deadlocks with it.

>> I'm starting to narrow down to what I think the sync_obj needs to do, and I'm
>> contemplating a bit more something like the following:
>>
>> a) no file backing, a simple kref object that gets tracked in an idr
>> (like a gem object).

I'm not even sure if an idr with a new object type is the right 
approach. See for amdgpu we have the fence numbers which are returned to 
userspace with every command submission.

Those fence numbers can be translated back into a fence structure (as 
long as they aren't ancient and already signaled).

So what we could do is just add a functionality to export those fence 
numbers into a semaphore fd with optionally replacing the fence inside 
an existing semaphore fd.

This way you only need to keep fds from foreign processes.

>> This object can have an optional fence attached to it. If there is no
>> fence, it's unsignalled,
>> if there is a fence it's signalled.

The price question is what happens when you try to wait for an object 
which doesn't (yet) have a fence attached to it?

Waiting is not really an option, since then you can created deadlocks 
inside the kernel. E.g. CS A waits for CS B and B waits for A.

So I think that creating an unsignaled semaphore fd should be forbidden 
in the first place.

> This should say, if there's a fence, the status of the fence.
> Thanks to Andres for pointing it out, it is 5am.

Yeah, I know what you mean. How cute little ones are the lack of sleep 
makes thinking straight rather complicated.

Christian.

>
> Dave.


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