[repost] drm sync objects cleaned up

[repost] drm sync objects cleaned up

[Dave Airlie]

This set of sync object patches should address most of the issues
raised in review.

The major changes:
Race on destroy should be gone,
Documentation fixups
amdgpu internal use p instead of adev fixups

My plans are to write some igt tests this week, and try
to get some more review on what the API should allow (should
I lock it down to drm syncobj is just semaphores for now).

Dave.

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

[PATCH 1/8] sync_file: add type/flags to sync file object creation.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This allows us to create sync files with different semantics,
and clearly define the interoperation between them it also
provides flags to allow for tweaks on those semantics.

This provides a validation interface for drivers that accept
types from userspace so they can return EINVAL instead of ENOMEM.

This provides an ioctl for userspace to retrieve the type/flags
of an object it may recieve from somewhere else.

Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 Documentation/driver-api/dma-buf.rst |  6 ++++
 drivers/dma-buf/sw_sync.c            |  2 +-
 drivers/dma-buf/sync_file.c          | 64 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/drm_atomic.c         |  2 +-
 include/linux/sync_file.h            |  9 ++++-
 include/uapi/linux/sync_file.h       | 27 +++++++++++++++
 6 files changed, 103 insertions(+), 7 deletions(-)

diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst
index 31671b4..bf2f7d5 100644
--- a/Documentation/driver-api/dma-buf.rst
+++ b/Documentation/driver-api/dma-buf.rst
@@ -163,3 +163,9 @@ DMA Fence uABI/Sync File
 .. kernel-doc:: include/linux/sync_file.h
    :internal:
 
+
+Sync File IOCTL Definitions
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+.. kernel-doc:: include/uapi/linux/sync_file.h
+   :internal:
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 69c5ff3..1c47de6 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -315,7 +315,7 @@ static long sw_sync_ioctl_create_fence(struct sync_timeline *obj,
 		goto err;
 	}
 
-	sync_file = sync_file_create(&pt->base);
+	sync_file = sync_file_create(&pt->base, SYNC_FILE_TYPE_FENCE, 0);
 	dma_fence_put(&pt->base);
 	if (!sync_file) {
 		err = -ENOMEM;
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2321035..07392af 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -28,9 +28,32 @@
 
 static const struct file_operations sync_file_fops;
 
-static struct sync_file *sync_file_alloc(void)
+/**
+ * sync_file_validate_type_flags - validate type/flags for support
+ * @type: type of sync file object
+ * @flags: flags to sync object.
+ *
+ * Validates the flags are correct so userspace can get a more
+ * detailed error type.
+ */
+int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
+{
+	if (flags)
+		return -EINVAL;
+	if (type != SYNC_FILE_TYPE_FENCE)
+		return -EINVAL;
+	return 0;
+}
+EXPORT_SYMBOL(sync_file_validate_type_flags);
+
+static struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
 {
 	struct sync_file *sync_file;
+	int ret;
+
+	ret = sync_file_validate_type_flags(type, flags);
+	if (ret)
+		return NULL;
 
 	sync_file = kzalloc(sizeof(*sync_file), GFP_KERNEL);
 	if (!sync_file)
@@ -47,6 +70,8 @@ static struct sync_file *sync_file_alloc(void)
 
 	INIT_LIST_HEAD(&sync_file->cb.node);
 
+	sync_file->type = type;
+	sync_file->flags = flags;
 	return sync_file;
 
 err:
@@ -66,17 +91,21 @@ static void fence_check_cb_func(struct dma_fence *f, struct dma_fence_cb *cb)
 /**
  * sync_file_create() - creates a sync file
  * @fence:	fence to add to the sync_fence
+ * @type: type of sync file to create
+ * @flags: flags to create sync file with.
  *
  * 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 or NULL in case of error.
  */
-struct sync_file *sync_file_create(struct dma_fence *fence)
+struct sync_file *sync_file_create(struct dma_fence *fence,
+				   uint32_t type,
+				   uint32_t flags)
 {
 	struct sync_file *sync_file;
 
-	sync_file = sync_file_alloc();
+	sync_file = sync_file_alloc(type, flags);
 	if (!sync_file)
 		return NULL;
 
@@ -200,7 +229,10 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 	struct dma_fence **fences, **nfences, **a_fences, **b_fences;
 	int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
 
-	sync_file = sync_file_alloc();
+	if (a->type != b->type)
+		return NULL;
+
+	sync_file = sync_file_alloc(a->type, a->flags);
 	if (!sync_file)
 		return NULL;
 
@@ -437,6 +469,27 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	return ret;
 }
 
+static long sync_file_ioctl_type(struct sync_file *sync_file,
+				 unsigned long arg)
+{
+	struct sync_file_type type;
+	int ret;
+	if (copy_from_user(&type, (void __user *)arg, sizeof(type)))
+		return -EFAULT;
+
+	if (type.flags || type.type)
+		return -EINVAL;
+
+	type.type = sync_file->type;
+	type.flags = sync_file->flags;
+
+	if (copy_to_user((void __user *)arg, &type, sizeof(type)))
+		ret = -EFAULT;
+	else
+		ret = 0;
+	return ret;
+}
+
 static long sync_file_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg)
 {
@@ -449,6 +502,9 @@ static long sync_file_ioctl(struct file *file, unsigned int cmd,
 	case SYNC_IOC_FILE_INFO:
 		return sync_file_ioctl_fence_info(sync_file, arg);
 
+	case SYNC_IOC_TYPE:
+		return sync_file_ioctl_type(sync_file, arg);
+
 	default:
 		return -ENOTTY;
 	}
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a567310..bb5a740 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1917,7 +1917,7 @@ static int setup_out_fence(struct drm_out_fence_state *fence_state,
 	if (put_user(fence_state->fd, fence_state->out_fence_ptr))
 		return -EFAULT;
 
-	fence_state->sync_file = sync_file_create(fence);
+	fence_state->sync_file = sync_file_create(fence, SYNC_FILE_TYPE_FENCE, 0);
 	if (!fence_state->sync_file)
 		return -ENOMEM;
 
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 3e3ab84..ede4182 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -20,6 +20,8 @@
 #include <linux/spinlock.h>
 #include <linux/dma-fence.h>
 #include <linux/dma-fence-array.h>
+#include <uapi/linux/sync_file.h>
+
 
 /**
  * struct sync_file - sync file to export to the userspace
@@ -30,6 +32,8 @@
  * @wq:			wait queue for fence signaling
  * @fence:		fence with the fences in the sync_file
  * @cb:			fence callback information
+ * @type:               sync file type
+ * @flags:              flags used to create sync file
  */
 struct sync_file {
 	struct file		*file;
@@ -43,11 +47,14 @@ struct sync_file {
 
 	struct dma_fence	*fence;
 	struct dma_fence_cb cb;
+	uint32_t type;
+	uint32_t flags;
 };
 
 #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
 
-struct sync_file *sync_file_create(struct dma_fence *fence);
+int sync_file_validate_type_flags(uint32_t type, uint32_t flags);
+struct sync_file *sync_file_create(struct dma_fence *fence, uint32_t type, uint32_t flags);
 struct dma_fence *sync_file_get_fence(int fd);
 
 #endif /* _LINUX_SYNC_H */
diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
index 5b287d6..f439cda 100644
--- a/include/uapi/linux/sync_file.h
+++ b/include/uapi/linux/sync_file.h
@@ -69,6 +69,26 @@ struct sync_file_info {
 #define SYNC_IOC_MAGIC		'>'
 
 /**
+ * DOC: SYNC_FILE_TYPE_FENCE - fence sync file object
+ *
+ * This sync file is a wrapper around a dma fence or a dma fence array.
+ * It can be merged with another fence sync file object to create a new
+ * merged object.
+ * The fence backing this object cannot be replaced.
+ * This is useful for shared fences.
+ */
+#define SYNC_FILE_TYPE_FENCE 0
+
+/**
+ * struct sync_file_type - data returned from sync file type ioctl
+ * @type:	sync_file type
+ * @flags:	sync_file creation flags
+ */
+struct sync_file_type {
+	__u32	type;
+	__u32	flags;
+};
+/**
  * Opcodes  0, 1 and 2 were burned during a API change to avoid users of the
  * old API to get weird errors when trying to handling sync_files. The API
  * change happened during the de-stage of the Sync Framework when there was
@@ -94,4 +114,11 @@ struct sync_file_info {
  */
 #define SYNC_IOC_FILE_INFO	_IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
 
+/**
+ * DOC: SYNC_IOC_TYPE - get creation type and flags of sync_file.
+ *
+ * Takes a struct sync_file_type. Returns the created values of type and flags.
+ */
+#define SYNC_IOC_TYPE		_IOWR(SYNC_IOC_MAGIC, 5, struct sync_file_type)
+
 #endif /* _UAPI_LINUX_SYNC_H */
-- 
2.9.3

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

[PATCH 2/8] sync_file: export some interfaces needed by drm sync objects.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

These are just alloc and fdget interfaces needed by the drm sync
objects code.

Reviewed-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/dma-buf/sync_file.c | 21 +++++++++++++++++++--
 include/linux/sync_file.h   |  3 ++-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 07392af..50e635a 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -46,7 +46,16 @@ int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
 }
 EXPORT_SYMBOL(sync_file_validate_type_flags);
 
-static struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
+/**
+ * sync_file_alloc() - allocate an unfenced sync file
+ * @type: type of sync file object
+ * @flags: creation flags for sync file object
+ *
+ * Creates a sync_file.
+ * The sync_file can be released with fput(sync_file->file).
+ * Returns the sync_file or NULL in case of error.
+ */
+struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
 {
 	struct sync_file *sync_file;
 	int ret;
@@ -78,6 +87,7 @@ static struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
 	kfree(sync_file);
 	return NULL;
 }
+EXPORT_SYMBOL(sync_file_alloc);
 
 static void fence_check_cb_func(struct dma_fence *f, struct dma_fence_cb *cb)
 {
@@ -120,7 +130,13 @@ struct sync_file *sync_file_create(struct dma_fence *fence,
 }
 EXPORT_SYMBOL(sync_file_create);
 
-static struct sync_file *sync_file_fdget(int fd)
+/**
+ * sync_file_fdget - get a reference to the file and return sync_file.
+ * @fd: file descriptor pointing at a sync_file.
+ *
+ * Returns the corresponding sync_file object.
+ */
+struct sync_file *sync_file_fdget(int fd)
 {
 	struct file *file = fget(fd);
 
@@ -136,6 +152,7 @@ static struct sync_file *sync_file_fdget(int fd)
 	fput(file);
 	return NULL;
 }
+EXPORT_SYMBOL(sync_file_fdget);
 
 /**
  * sync_file_get_fence - get the fence related to the sync_file fd
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index ede4182..e683dd1 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -54,7 +54,8 @@ struct sync_file {
 #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
 
 int sync_file_validate_type_flags(uint32_t type, uint32_t flags);
+struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags);
 struct sync_file *sync_file_create(struct dma_fence *fence, uint32_t type, uint32_t flags);
 struct dma_fence *sync_file_get_fence(int fd);
-
+struct sync_file *sync_file_fdget(int fd);
 #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 4/8] sync_file: add a mutex to protect fence and callback members. (v4)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This patch allows the underlying fence in a sync_file to be changed
or set to NULL. This isn't currently required but for Vulkan
semaphores we need to be able to swap and reset the fence.

In order to faciliate this, it uses rcu to protect the fence,
along with a new mutex. The mutex also protects the callback.
It also checks for NULL when retrieving the rcu protected
fence in case it has been reset.

v1.1: fix the locking (Julia Lawall).
v2: use rcu try one
v3: fix poll to use proper rcu, fixup merge/fill ioctls
to not crash on NULL fence cases.
v4: use rcu in even more places, add missing fput

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/dma-buf/sync_file.c | 135 +++++++++++++++++++++++++++++++++++---------
 include/linux/sync_file.h   |   6 +-
 2 files changed, 112 insertions(+), 29 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 50e635a..d84de67 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -28,6 +28,10 @@
 
 static const struct file_operations sync_file_fops;
 
+#define sync_file_held(obj) lockdep_is_held(&(obj)->lock)
+#define sync_file_assert_held(obj) \
+	lockdep_assert_held(&(obj)->lock)
+
 /**
  * sync_file_validate_type_flags - validate type/flags for support
  * @type: type of sync file object
@@ -81,6 +85,10 @@ struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
 
 	sync_file->type = type;
 	sync_file->flags = flags;
+
+	RCU_INIT_POINTER(sync_file->fence, NULL);
+
+	mutex_init(&sync_file->lock);
 	return sync_file;
 
 err:
@@ -119,7 +127,9 @@ struct sync_file *sync_file_create(struct dma_fence *fence,
 	if (!sync_file)
 		return NULL;
 
-	sync_file->fence = dma_fence_get(fence);
+	dma_fence_get(fence);
+
+	RCU_INIT_POINTER(sync_file->fence, fence);
 
 	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
 		 fence->ops->get_driver_name(fence),
@@ -170,13 +180,28 @@ struct dma_fence *sync_file_get_fence(int fd)
 	if (!sync_file)
 		return NULL;
 
-	fence = dma_fence_get(sync_file->fence);
+	if (!rcu_access_pointer(sync_file->fence)) {
+		fput(sync_file->file);
+		return NULL;
+	}
+
+	rcu_read_lock();
+	fence = dma_fence_get_rcu_safe(&sync_file->fence);
+	rcu_read_unlock();
+
 	fput(sync_file->file);
 
 	return fence;
 }
 EXPORT_SYMBOL(sync_file_get_fence);
 
+static inline struct dma_fence *
+sync_file_get_fence_locked(struct sync_file *sync_file)
+{
+	return rcu_dereference_protected(sync_file->fence,
+					 sync_file_held(sync_file));
+}
+
 static int sync_file_set_fence(struct sync_file *sync_file,
 			       struct dma_fence **fences, int num_fences)
 {
@@ -189,7 +214,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,
@@ -198,24 +223,30 @@ 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;
 }
 
-static struct dma_fence **get_fences(struct sync_file *sync_file,
+/* must be called with rcu read lock taken */
+static struct dma_fence **get_fences(struct dma_fence **fence,
 				     int *num_fences)
 {
-	if (dma_fence_is_array(sync_file->fence)) {
-		struct dma_fence_array *array = to_dma_fence_array(sync_file->fence);
+	if (!*fence) {
+		*num_fences = 0;
+		return NULL;
+	}
+
+	if (dma_fence_is_array(*fence)) {
+		struct dma_fence_array *array = to_dma_fence_array(*fence);
 
 		*num_fences = array->num_fences;
 		return array->fences;
 	}
 
 	*num_fences = 1;
-	return &sync_file->fence;
+	return fence;
 }
 
 static void add_fence(struct dma_fence **fences,
@@ -245,18 +276,31 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 	struct sync_file *sync_file;
 	struct dma_fence **fences, **nfences, **a_fences, **b_fences;
 	int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
+	struct dma_fence *a_fence, *b_fence;
 
 	if (a->type != b->type)
 		return NULL;
 
-	sync_file = sync_file_alloc(a->type, a->flags);
-	if (!sync_file)
+	if (!rcu_access_pointer(a->fence) ||
+	    !rcu_access_pointer(b->fence))
 		return NULL;
 
-	a_fences = get_fences(a, &a_num_fences);
-	b_fences = get_fences(b, &b_num_fences);
+	rcu_read_lock();
+	a_fence = dma_fence_get_rcu_safe(&a->fence);
+	b_fence = dma_fence_get_rcu_safe(&b->fence);
+	rcu_read_unlock();
+
+	a_fences = get_fences(&a_fence, &a_num_fences);
+	b_fences = get_fences(&b_fence, &b_num_fences);
+	if (!a_num_fences || !b_num_fences)
+		goto put_src_fences;
+
 	if (a_num_fences > INT_MAX - b_num_fences)
-		return NULL;
+		goto put_src_fences;
+
+	sync_file = sync_file_alloc(a->type, a->flags);
+	if (!sync_file)
+		goto put_src_fences;
 
 	num_fences = a_num_fences + b_num_fences;
 
@@ -317,11 +361,16 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 		goto err;
 	}
 
+	dma_fence_put(a_fence);
+	dma_fence_put(b_fence);
 	strlcpy(sync_file->name, name, sizeof(sync_file->name));
 	return sync_file;
 
 err:
 	fput(sync_file->file);
+put_src_fences:
+	dma_fence_put(a_fence);
+	dma_fence_put(b_fence);
 	return NULL;
 
 }
@@ -330,10 +379,15 @@ static void sync_file_free(struct kref *kref)
 {
 	struct sync_file *sync_file = container_of(kref, struct sync_file,
 						     kref);
+	struct dma_fence *fence;
+
+	fence = rcu_dereference_protected(sync_file->fence, 1);
+	if (fence) {
+		if (test_bit(POLL_ENABLED, &fence->flags))
+			dma_fence_remove_callback(fence, &sync_file->cb);
+		dma_fence_put(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);
 	kfree(sync_file);
 }
 
@@ -348,16 +402,25 @@ 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;
+	unsigned int ret_val = 0;
+	struct dma_fence *fence;
 
 	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,
-					   fence_check_cb_func) < 0)
-			wake_up_all(&sync_file->wq);
+	mutex_lock(&sync_file->lock);
+
+	fence = sync_file_get_fence_locked(sync_file);
+	if (fence) {
+		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);
+		}
+		ret_val = dma_fence_is_signaled(fence) ? POLLIN : 0;
 	}
+	mutex_unlock(&sync_file->lock);
 
-	return dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
+	return ret_val;
 }
 
 static long sync_file_ioctl_merge(struct sync_file *sync_file,
@@ -433,6 +496,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	struct sync_file_info info;
 	struct sync_fence_info *fence_info = NULL;
 	struct dma_fence **fences;
+	struct dma_fence *fence;
 	__u32 size;
 	int num_fences, ret, i;
 
@@ -442,7 +506,15 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	if (info.flags || info.pad)
 		return -EINVAL;
 
-	fences = get_fences(sync_file, &num_fences);
+	rcu_read_lock();
+	fence = dma_fence_get_rcu_safe(&sync_file->fence);
+	rcu_read_unlock();
+
+	fences = get_fences(&fence, &num_fences);
+
+	/* if there are no fences in the sync_file just return */
+	if (!num_fences)
+		goto no_fences;
 
 	/*
 	 * Passing num_fences = 0 means that userspace doesn't want to
@@ -453,13 +525,17 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 	if (!info.num_fences)
 		goto no_fences;
 
-	if (info.num_fences < num_fences)
-		return -EINVAL;
+	if (info.num_fences < num_fences) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	size = num_fences * sizeof(*fence_info);
 	fence_info = kzalloc(size, GFP_KERNEL);
-	if (!fence_info)
-		return -ENOMEM;
+	if (!fence_info) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	for (i = 0; i < num_fences; i++)
 		sync_fill_fence_info(fences[i], &fence_info[i]);
@@ -472,7 +548,10 @@ 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);
+	if (num_fences)
+		info.status = dma_fence_is_signaled(sync_file->fence);
+	else
+		info.status = -ENOENT;
 	info.num_fences = num_fences;
 
 	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
@@ -482,7 +561,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
 
 out:
 	kfree(fence_info);
-
+	dma_fence_put(fence);
 	return ret;
 }
 
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index e683dd1..4bf661b 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -34,6 +34,7 @@
  * @cb:			fence callback information
  * @type:               sync file type
  * @flags:              flags used to create sync file
+ * @lock:               mutex to protect fence/cb - used for semaphores
  */
 struct sync_file {
 	struct file		*file;
@@ -45,10 +46,13 @@ struct sync_file {
 
 	wait_queue_head_t	wq;
 
-	struct dma_fence	*fence;
+	struct dma_fence __rcu	*fence;
 	struct dma_fence_cb cb;
 	uint32_t type;
 	uint32_t flags;
+
+	/* protects the fence pointer and cb */
+	struct mutex lock;
 };
 
 #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
-- 
2.9.3

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

[PATCH 6/8] drm/syncobj: add semaphore support helpers.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This just adds two helper interfaces to bridge the gap from
drivers to sync_file for the semaphore objects.

These will be used by the amdgpu driver.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_syncobj.c | 77 +++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_syncobj.h     | 37 +++++++++++++++++++++
 2 files changed, 114 insertions(+)
 create mode 100644 include/drm/drm_syncobj.h

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 9985893..6b8f544 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -39,6 +39,7 @@
 #include <drm/drmP.h>
 #include <linux/sync_file.h>
 #include "drm_internal.h"
+#include <drm/drm_syncobj.h>
 
 static struct sync_file *drm_syncobj_get(struct drm_file *file_private,
 					 u32 handle)
@@ -57,6 +58,82 @@ static struct sync_file *drm_syncobj_get(struct drm_file *file_private,
 	return sync_file;
 }
 
+static int drm_syncobj_swap_fences_nocheck(struct drm_file *file_private,
+					   uint32_t handle,
+					   struct dma_fence *fence,
+					   struct dma_fence **old_fence)
+{
+	struct sync_file *sync_file = drm_syncobj_get(file_private, handle);
+
+	if (!sync_file)
+		return -EINVAL;
+
+	*old_fence = sync_file_replace_fence(sync_file, fence);
+	fput(sync_file->file);
+	return 0;
+}
+
+/**
+ * drm_syncobj_swap_fences - lookup and replace fence in a sync object.
+ * @file_private - drm file private pointer.
+ * @handle - sync object handle
+ * @fence - new fence (or NULL) to back sync object.
+ * @old_fence - old fence backing sync object.
+ * Returns:
+ * 0 on success, or -EINVAL when the handle doesn't point at a valid
+ * sync_file with %SYNC_FILE_TYPE_SEMAPHORE.
+ *
+ * This function is used to swap the fence backing the sync object
+ * with a new one, it returns the old fence in old_fence;
+ */
+int drm_syncobj_swap_fences(struct drm_file *file_private,
+			    uint32_t handle,
+			    struct dma_fence *fence,
+			    struct dma_fence **old_fence)
+{
+	int r;
+
+	r = drm_syncobj_swap_fences_nocheck(file_private,
+					    handle,
+					    fence,
+					    old_fence);
+	if (r)
+		return r;
+
+
+	if (!*old_fence)
+		return -EINVAL;
+	return 0;
+}
+EXPORT_SYMBOL(drm_syncobj_swap_fences);
+
+/**
+ * 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
+ * sync_file with %SYNC_FILE_TYPE_SEMAPHORE.
+ *
+ * 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 dma_fence *old_fence;
+	int r;
+
+	r = drm_syncobj_swap_fences_nocheck(file_private, handle, fence, &old_fence);
+	if (r)
+		return r;
+	dma_fence_put(old_fence);
+	return 0;
+}
+EXPORT_SYMBOL(drm_syncobj_replace_fence);
+
 static int drm_syncobj_create(struct drm_file *file_private,
 				  unsigned type,
 				  unsigned flags, u32 *handle)
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
new file mode 100644
index 0000000..f614e06
--- /dev/null
+++ b/include/drm/drm_syncobj.h
@@ -0,0 +1,37 @@
+/*
+ * 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_swap_fences(struct drm_file *file_private,
+			    uint32_t handle,
+			    struct dma_fence *fence,
+			    struct dma_fence **old_fence);
+int drm_syncobj_replace_fence(struct drm_file *file_private,
+			      u32 handle,
+			      struct dma_fence *fence);
+
+#endif
-- 
2.9.3

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

[PATCH 7/8] 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

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

[PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

[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.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 82 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
 include/uapi/drm/amdgpu_drm.h           |  6 +++
 3 files changed, 88 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..77bfe80 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,41 @@ static int amdgpu_process_fence_dep(struct amdgpu_cs_parser *p,
 	return 0;
 }
 
+static int amdgpu_sem_lookup_and_sync(struct amdgpu_cs_parser *p,
+				      uint32_t handle)
+{
+	int r;
+	struct dma_fence *old_fence;
+
+	r = drm_syncobj_swap_fences(p->filp, handle, NULL, &old_fence);
+	if (r)
+		return r;
+
+	r = amdgpu_sync_fence(p->adev, &p->job->sync, old_fence);
+	dma_fence_put(old_fence);
+
+	return r;
+}
+
+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_sync(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 +1060,54 @@ 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_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 +1135,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

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

Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

[Chris Wilson]

On Tue, Apr 11, 2017 at 01:22:20PM +1000, Dave Airlie wrote:
> +static int amdgpu_sem_lookup_and_sync(struct amdgpu_cs_parser *p,
> +				      uint32_t handle)
> +{
> +	int r;
> +	struct dma_fence *old_fence;
> +
> +	r = drm_syncobj_swap_fences(p->filp, handle, NULL, &old_fence);
> +	if (r)
> +		return r;

I'm a bit puzzled over this interface as this means that all
in-semaphores are not reusable. That seems a bit odd as it means
userspace can't use the results from one engine on two+ parallel
engines. Searching kronos for VkSemaphore
https://www.khronos.org/registry/vulkan/specs/1.0/man/html/VkSemaphore.html
isn't enlightening...
-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 8/8] amdgpu: use sync file for shared semaphores (v2.1)

[Dave Airlie]

On 11 April 2017 at 22:42, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Apr 11, 2017 at 01:22:20PM +1000, Dave Airlie wrote:
>> +static int amdgpu_sem_lookup_and_sync(struct amdgpu_cs_parser *p,
>> +                                   uint32_t handle)
>> +{
>> +     int r;
>> +     struct dma_fence *old_fence;
>> +
>> +     r = drm_syncobj_swap_fences(p->filp, handle, NULL, &old_fence);
>> +     if (r)
>> +             return r;
>
> I'm a bit puzzled over this interface as this means that all
> in-semaphores are not reusable. That seems a bit odd as it means
> userspace can't use the results from one engine on two+ parallel
> engines. Searching kronos for VkSemaphore
> https://www.khronos.org/registry/vulkan/specs/1.0/man/html/VkSemaphore.html
> isn't enlightening...

You need to look in the vulkan spec itself.

But yes this is the semantics, vulkan semaphores are explicitly 1:1 objects.

One wait must have one signal. It's a semaphore not a fence, so you can't
have multiple engines getting the results from one semaphore.

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

RE: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

[Mao, David]

Does it means we have to submit command to trigger the semaphore wait/signal?

Best Regards,
David

-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Dave Airlie
Sent: Tuesday, April 11, 2017 11:22 AM
To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

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.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 82 ++++++++++++++++++++++++++++++++-  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
 include/uapi/drm/amdgpu_drm.h           |  6 +++
 3 files changed, 88 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..77bfe80 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,41 @@ static int amdgpu_process_fence_dep(struct amdgpu_cs_parser *p,
 	return 0;
 }
 
+static int amdgpu_sem_lookup_and_sync(struct amdgpu_cs_parser *p,
+				      uint32_t handle)
+{
+	int r;
+	struct dma_fence *old_fence;
+
+	r = drm_syncobj_swap_fences(p->filp, handle, NULL, &old_fence);
+	if (r)
+		return r;
+
+	r = amdgpu_sync_fence(p->adev, &p->job->sync, old_fence);
+	dma_fence_put(old_fence);
+
+	return r;
+}
+
+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_sync(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 +1060,54 @@ 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_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 +1135,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

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

Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

[Dave Airlie]

On 12 April 2017 at 12:36, Mao, David <David.Mao@amd.com> wrote:
> Does it means we have to submit command to trigger the semaphore wait/signal?

Yes, but I think that should be fine, we need to submit a job to the
scheduler to
get the waits to happen or to have a fence to fill into the signals.

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

RE: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

[Mao, David]

But how to handle the semaphore wait in the vkQueuePresentkHR?

Thanks. 
Best Regards,
David

-----Original Message-----
From: Dave Airlie [mailto:airlied@gmail.com] 
Sent: Wednesday, April 12, 2017 10:44 AM
To: Mao, David <David.Mao@amd.com>
Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

On 12 April 2017 at 12:36, Mao, David <David.Mao@amd.com> wrote:
> Does it means we have to submit command to trigger the semaphore wait/signal?

Yes, but I think that should be fine, we need to submit a job to the scheduler to get the waits to happen or to have a fence to fill into the signals.

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

Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

[Dave Airlie]

On 12 April 2017 at 12:49, Mao, David <David.Mao@amd.com> wrote:
> But how to handle the semaphore wait in the vkQueuePresentkHR?

The problem here is that really we'd want the presenting process to
do the signal once it submits the work for actual presentations (be
that the X server DDX or whatever).

However that is going to be a bit tricky, for radv I've just been
submitting an empty command stream submit, once the X server
lets us know we've presented.

I looked how the codebase before I started working on it worked,
and I can't see if it dealt with this properly either, the impression I get
is that it might submit the wait sems via the sem ioctl onto a ctx,
but the X server might be using a different ctx, so would never
execute the wait, and we'd execute the wait the next time we did
a command submission.

I suppose we could just queue up the vkQueuePresentKHR wait
sems in userspace instead of a NULL cs if this solution was acceptable.

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

RE: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

[Mao, David]

My point is it is reasonable to split the semaphore signal/wait with the command submission.
For the signal ioctl, we could just pick the last fence in the same schedule context, and we don't need to ask for a explicit flush or a dummy submission trick. 
The spec guarantee the signal always comes before the wait, which means, we could always get the valid fence. For the kernel sem object. 

Thanks. 
Best Regards,
David

-----Original Message-----
From: Dave Airlie [mailto:airlied@gmail.com] 
Sent: Wednesday, April 12, 2017 11:18 AM
To: Mao, David <David.Mao@amd.com>
Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

On 12 April 2017 at 12:49, Mao, David <David.Mao@amd.com> wrote:
> But how to handle the semaphore wait in the vkQueuePresentkHR?

The problem here is that really we'd want the presenting process to do the signal once it submits the work for actual presentations (be that the X server DDX or whatever).

However that is going to be a bit tricky, for radv I've just been submitting an empty command stream submit, once the X server lets us know we've presented.

I looked how the codebase before I started working on it worked, and I can't see if it dealt with this properly either, the impression I get is that it might submit the wait sems via the sem ioctl onto a ctx, but the X server might be using a different ctx, so would never execute the wait, and we'd execute the wait the next time we did a command submission.

I suppose we could just queue up the vkQueuePresentKHR wait sems in userspace instead of a NULL cs if this solution was acceptable.

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

Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

[Dave Airlie]

On 12 April 2017 at 13:34, Mao, David <David.Mao@amd.com> wrote:
> My point is it is reasonable to split the semaphore signal/wait with the command submission.
> For the signal ioctl, we could just pick the last fence in the same schedule context, and we don't need to ask for a explicit flush or a dummy submission trick.
> The spec guarantee the signal always comes before the wait, which means, we could always get the valid fence. For the kernel sem object.

I'm a bit vague on the schedule contexts stuff, but does anything
guarantee the X server present operation be in the same schedule
context?

This might be something for Christian to chime in on, we could I
suppose add ioctls to avoid the dummy CS submission, we could also
make dummy
CS submission simpler, if we submit no IBs then we could just have it
deal with the semaphores for those cases and avoid any explicit
flushes,
which saves reproducing the logic to wait and sync.

But at least for the wait case, we need to send something to the
scheduler to wait on, and that looks like the CS ioctl we have now
pretty much,
For the signal case there might be a better argument that an explicit
signal with last fence on this ctx could be used, however at least
with the way
radv works now, we definitely know the X server is finished with the
present buffer as it tells us via its own sync logic, at that point
radv submits
an empty CS with the signal semaphores, we'd really want to pass a
semaphore between the X server and client to do this perfectly.

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

RE: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

[Mao, David]

=> we'd really want to pass a semaphore between the X server and client to do this perfectly.
Do you means that you want X to signal the semaphore that waited by client, through special version of xsync?
We use pretty complex tricks to build synchronization logic upon the event and shm fence. 
But it would be better if we could use unified way to signal/wait the xsync fence and the semaphore object.
I can see the benefit for combining the semaphores' wait/signal into the submit routine, but how about extend the interface to allow null ib submission?
In this case, it will always return the last seq_no for null ib list, and the semaphore in signal list will be associated with last fence as well. 
IIRC, the semaphore wait is applied to schedule entity as the dependency, which means it don't need to be associated with schedule job as well. 

Thanks. 
Best Regards,
David
-----Original Message-----
From: Dave Airlie [mailto:airlied@gmail.com] 
Sent: Wednesday, April 12, 2017 11:58 AM
To: Mao, David <David.Mao@amd.com>
Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

On 12 April 2017 at 13:34, Mao, David <David.Mao@amd.com> wrote:
> My point is it is reasonable to split the semaphore signal/wait with the command submission.
> For the signal ioctl, we could just pick the last fence in the same schedule context, and we don't need to ask for a explicit flush or a dummy submission trick.
> The spec guarantee the signal always comes before the wait, which means, we could always get the valid fence. For the kernel sem object.

I'm a bit vague on the schedule contexts stuff, but does anything guarantee the X server present operation be in the same schedule context?

This might be something for Christian to chime in on, we could I suppose add ioctls to avoid the dummy CS submission, we could also make dummy CS submission simpler, if we submit no IBs then we could just have it deal with the semaphores for those cases and avoid any explicit flushes, which saves reproducing the logic to wait and sync.

But at least for the wait case, we need to send something to the scheduler to wait on, and that looks like the CS ioctl we have now pretty much, For the signal case there might be a better argument that an explicit signal with last fence on this ctx could be used, however at least with the way radv works now, we definitely know the X server is finished with the present buffer as it tells us via its own sync logic, at that point radv submits an empty CS with the signal semaphores, we'd really want to pass a semaphore between the X server and client to do this perfectly.

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

Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2.1)

[Christian König]

Am 12.04.2017 um 05:58 schrieb Dave Airlie:
> On 12 April 2017 at 13:34, Mao, David <David.Mao@amd.com> wrote:
>> My point is it is reasonable to split the semaphore signal/wait with the command submission.
>> For the signal ioctl, we could just pick the last fence in the same schedule context, and we don't need to ask for a explicit flush or a dummy submission trick.
>> The spec guarantee the signal always comes before the wait, which means, we could always get the valid fence. For the kernel sem object.
> I'm a bit vague on the schedule contexts stuff, but does anything
> guarantee the X server present operation be in the same schedule
> context?

Not even remotely. Since X and the application are separate processes 
they can't access each others schedule contexts.

> This might be something for Christian to chime in on, we could I
> suppose add ioctls to avoid the dummy CS submission, we could also
> make dummy
> CS submission simpler, if we submit no IBs then we could just have it
> deal with the semaphores for those cases and avoid any explicit
> flushes,
> which saves reproducing the logic to wait and sync.

Sorry, I'm not following the whole discussion. Why do we want a dummy 
submission in the first place? Just to have the semaphore in the 
signaled state?

> But at least for the wait case, we need to send something to the
> scheduler to wait on, and that looks like the CS ioctl we have now
> pretty much,
> For the signal case there might be a better argument that an explicit
> signal with last fence on this ctx could be used, however at least
> with the way
> radv works now, we definitely know the X server is finished with the
> present buffer as it tells us via its own sync logic, at that point
> radv submits
> an empty CS with the signal semaphores, we'd really want to pass a
> semaphore between the X server and client to do this perfectly.

Yes, agree. We should fix this on the user space level, not add any 
kernel workarounds.

Regards,
Christian.

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


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

Re: [repost] drm sync objects cleaned up

[Chris Wilson]

On Tue, Apr 11, 2017 at 01:22:12PM +1000, Dave Airlie wrote:
> This set of sync object patches should address most of the issues
> raised in review.
> 
> The major changes:
> Race on destroy should be gone,
> Documentation fixups
> amdgpu internal use p instead of adev fixups
> 
> My plans are to write some igt tests this week, and try
> to get some more review on what the API should allow (should
> I lock it down to drm syncobj is just semaphores for now).

Having an idr of handles is much, much nicer than fd and I want the same
for fences :)
-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: [repost] drm sync objects cleaned up

[Dave Airlie]

On 14 April 2017 at 19:45, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Apr 11, 2017 at 01:22:12PM +1000, Dave Airlie wrote:
>> This set of sync object patches should address most of the issues
>> raised in review.
>>
>> The major changes:
>> Race on destroy should be gone,
>> Documentation fixups
>> amdgpu internal use p instead of adev fixups
>>
>> My plans are to write some igt tests this week, and try
>> to get some more review on what the API should allow (should
>> I lock it down to drm syncobj is just semaphores for now).
>
> Having an idr of handles is much, much nicer than fd and I want the same
> for fences :)

Okay, but what form do you want the API to take for fences?

because I've just ported all this to using sem_file as the backend, instead
of sync_file which seems to oppose the goal of using it for fences.

For fences do you want upfront creation, then passing created fences object
into command submission that attach the internal fence?

Or do you want command submission to have out fence handles that it creates,
so we don't have any explicit syncobj create for fences?

Do you want those fences to be shareable across processes using sync_file
semantics?

I kinda feel like I'm going around in circles here and I'm going to just merge
something instead.

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

Re: [repost] drm sync objects cleaned up

[Chris Wilson]

On Wed, Apr 19, 2017 at 05:34:52AM +1000, Dave Airlie wrote:
> On 14 April 2017 at 19:45, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Tue, Apr 11, 2017 at 01:22:12PM +1000, Dave Airlie wrote:
> >> This set of sync object patches should address most of the issues
> >> raised in review.
> >>
> >> The major changes:
> >> Race on destroy should be gone,
> >> Documentation fixups
> >> amdgpu internal use p instead of adev fixups
> >>
> >> My plans are to write some igt tests this week, and try
> >> to get some more review on what the API should allow (should
> >> I lock it down to drm syncobj is just semaphores for now).
> >
> > Having an idr of handles is much, much nicer than fd and I want the same
> > for fences :)
> 
> Okay, but what form do you want the API to take for fences?
> 
> because I've just ported all this to using sem_file as the backend, instead
> of sync_file which seems to oppose the goal of using it for fences.
> 
> For fences do you want upfront creation, then passing created fences object
> into command submission that attach the internal fence?

Yes. My understanding is that fences differ from semaphores by allowing
use prior to execution. And that userspace (Vk) wants to be able to
create a fence, pass it to a listener (maybe different process) and then
attach it to an out-fence from a CS.

I looked at using a shared idr for fences/semaphores and the issue I hit
first was having to create a proxy dma-fence to initialise the syncobj
with, so that I could attach listeners before I was able to hook the
fence upto an execbuf.
 
> Or do you want command submission to have out fence handles that it creates,
> so we don't have any explicit syncobj create for fences?
> 
> Do you want those fences to be shareable across processes using sync_file
> semantics?

Yes. In the same vein as the semaphore syncobjs, an idr for cheap
creation/reuse within a process and export/import via sync_file fds.
Where these fd differ from sema is that they do support host CPU access
(these will work just like regular sync_files in that regard).
 
> I kinda feel like I'm going around in circles here and I'm going to just merge
> something instead.

Sure. It was just from the perspective of extending the i915 execbuffer2
that adding an array for semaphore in/out could be neatly generalised to
cover fences as well (and that the api for syncobj is a substantial
improvement over sync_merge and passing one fence in/out).
-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: [repost] drm sync objects cleaned up

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 4780 bytes --]

A few thoughts (from the anv perspective) that I put on IRC but may be
better in a mail.  In no particular order:

 1. Having the fd exported from a syncobj be a valid sync_file seems like a
fairly pointless feature to me.  If we can make things more sane by
throwing that out, I'm all for it.

 2. As far as sync_file interactions go, I think the far more useful thing
would be for you to be able to export a sync_file from a syncobj which
would create a sync_file that waits on the last submitted signal operation
on the syncobj and then have a way of creating a temporary syncobj that has
the fence from a sync_file.  Not sure how this would interact with future
fences.  If we can't figure it out, let's just forget it and not have any
defined interaction.

 3. I'd like to also be able to use syncobj for implementing VkFence
sharing.  Really, all this means is a drm_syncobj_wait ioctl.  Yes, with
the current sync_file stuff, you could turn it into a sync_file and poll
but I'd rather not burn the file descriptors.

 4. It would be a neat trick if drm_syncobj_wait could take a list of
syncobjs and wait on one or all of them as requested by the user.  That
said, this would be an optimization at best and I'm fine with waiting on
them one at a time.

 5. I'd like to better define what happens when someone tries to wait
twice.  I'm a big fan of the semantics of dma-buf dependencies: Each wait
operation waits on the most recently queued signal operation.  That seems
better to me than waiting causing an implicit reset and waiting twice being
invalid.  Among other things, it means that we don't have to worry bout the
semantics of exactly how execbuf fails if you ask it to wait on the same
sync file twice.  That said, it can be anything we want, I just want it to
be well-defined.

--Jason


On Tue, Apr 18, 2017 at 1:30 PM, Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
wrote:

> On Wed, Apr 19, 2017 at 05:34:52AM +1000, Dave Airlie wrote:
> > On 14 April 2017 at 19:45, Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
> wrote:
> > > On Tue, Apr 11, 2017 at 01:22:12PM +1000, Dave Airlie wrote:
> > >> This set of sync object patches should address most of the issues
> > >> raised in review.
> > >>
> > >> The major changes:
> > >> Race on destroy should be gone,
> > >> Documentation fixups
> > >> amdgpu internal use p instead of adev fixups
> > >>
> > >> My plans are to write some igt tests this week, and try
> > >> to get some more review on what the API should allow (should
> > >> I lock it down to drm syncobj is just semaphores for now).
> > >
> > > Having an idr of handles is much, much nicer than fd and I want the
> same
> > > for fences :)
> >
> > Okay, but what form do you want the API to take for fences?
> >
> > because I've just ported all this to using sem_file as the backend,
> instead
> > of sync_file which seems to oppose the goal of using it for fences.
> >
> > For fences do you want upfront creation, then passing created fences
> object
> > into command submission that attach the internal fence?
>
> Yes. My understanding is that fences differ from semaphores by allowing
> use prior to execution. And that userspace (Vk) wants to be able to
> create a fence, pass it to a listener (maybe different process) and then
> attach it to an out-fence from a CS.
>
> I looked at using a shared idr for fences/semaphores and the issue I hit
> first was having to create a proxy dma-fence to initialise the syncobj
> with, so that I could attach listeners before I was able to hook the
> fence upto an execbuf.
>
> > Or do you want command submission to have out fence handles that it
> creates,
> > so we don't have any explicit syncobj create for fences?
> >
> > Do you want those fences to be shareable across processes using sync_file
> > semantics?
>
> Yes. In the same vein as the semaphore syncobjs, an idr for cheap
> creation/reuse within a process and export/import via sync_file fds.
> Where these fd differ from sema is that they do support host CPU access
> (these will work just like regular sync_files in that regard).
>
> > I kinda feel like I'm going around in circles here and I'm going to just
> merge
> > something instead.
>
> Sure. It was just from the perspective of extending the i915 execbuffer2
> that adding an array for semaphore in/out could be neatly generalised to
> cover fences as well (and that the api for syncobj is a substantial
> improvement over sync_merge and passing one fence in/out).
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 6053 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [repost] drm sync objects cleaned up

[Dave Airlie]

On 19 April 2017 at 07:55, Jason Ekstrand <jason@jlekstrand.net> wrote:
> A few thoughts (from the anv perspective) that I put on IRC but may be
> better in a mail.  In no particular order:
>
>  1. Having the fd exported from a syncobj be a valid sync_file seems like a
> fairly pointless feature to me.  If we can make things more sane by throwing
> that out, I'm all for it.
>
>  2. As far as sync_file interactions go, I think the far more useful thing
> would be for you to be able to export a sync_file from a syncobj which would
> create a sync_file that waits on the last submitted signal operation on the
> syncobj and then have a way of creating a temporary syncobj that has the
> fence from a sync_file.  Not sure how this would interact with future
> fences.  If we can't figure it out, let's just forget it and not have any
> defined interaction.
>
>  3. I'd like to also be able to use syncobj for implementing VkFence
> sharing.  Really, all this means is a drm_syncobj_wait ioctl.  Yes, with the
> current sync_file stuff, you could turn it into a sync_file and poll but I'd
> rather not burn the file descriptors.

Okay this seems like a good goal.

>  4. It would be a neat trick if drm_syncobj_wait could take a list of
> syncobjs and wait on one or all of them as requested by the user.  That
> said, this would be an optimization at best and I'm fine with waiting on
> them one at a time.

And this.

>
>  5. I'd like to better define what happens when someone tries to wait twice.
> I'm a big fan of the semantics of dma-buf dependencies: Each wait operation
> waits on the most recently queued signal operation.  That seems better to me
> than waiting causing an implicit reset and waiting twice being invalid.
> Among other things, it means that we don't have to worry bout the semantics
> of exactly how execbuf fails if you ask it to wait on the same sync file
> twice.  That said, it can be anything we want, I just want it to be
> well-defined.

I've been thinking about this, and I think you are right, the fact that sems
are 1:1 signal:waiter is probably not necessary to enforce in kernel space,
if we don't replace the backing fence on a sem with NULL after waiting, I don't
think it should break a working userspace, and it will just make things simpler,
I can't see any real way a broken userspace can do much damage here either.

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

[PATCH 3/8] drm: introduce sync objects as sync file objects with no fd (v2)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

Sync objects are new toplevel drm object, that have the same
semantics as sync_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.

v2: do get/put on the sync file file to avoid race (Daniel)

Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 Documentation/gpu/drm-internals.rst |   5 +
 Documentation/gpu/drm-mm.rst        |   6 +
 drivers/gpu/drm/Makefile            |   2 +-
 drivers/gpu/drm/drm_fops.c          |   8 ++
 drivers/gpu/drm/drm_internal.h      |  15 ++
 drivers/gpu/drm/drm_ioctl.c         |  14 ++
 drivers/gpu/drm/drm_syncobj.c       | 266 ++++++++++++++++++++++++++++++++++++
 include/drm/drmP.h                  |   5 +
 include/drm/drm_drv.h               |   1 +
 include/uapi/drm/drm.h              |  27 ++++
 10 files changed, 348 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_syncobj.c

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/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..70c27a0 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -142,4 +142,19 @@ 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);
+int drm_syncobj_info_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..4a8ed66 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,17 @@ 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),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_INFO, drm_syncobj_info_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..9985893
--- /dev/null
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -0,0 +1,266 @@
+/*
+ * 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 sync_file objects,
+ * that don't use fd space, and can be converted to/from sync_file fds.
+ *
+ * Depending on the sync object type, they can expose different sync_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"
+
+static struct sync_file *drm_syncobj_get(struct drm_file *file_private,
+					 u32 handle)
+{
+	struct sync_file *sync_file;
+
+	spin_lock(&file_private->syncobj_table_lock);
+
+	/* Check if we currently have a reference on the object */
+	sync_file = idr_find(&file_private->syncobj_idr, handle);
+	if (sync_file)
+		get_file(sync_file->file);
+
+	spin_unlock(&file_private->syncobj_table_lock);
+
+	return sync_file;
+}
+
+static int drm_syncobj_create(struct drm_file *file_private,
+				  unsigned type,
+				  unsigned flags, u32 *handle)
+{
+	struct sync_file *sync_file;
+	int ret;
+
+	ret = sync_file_validate_type_flags(type, flags);
+	if (ret)
+		return ret;
+
+	sync_file = sync_file_alloc(type, flags);
+	if (!sync_file)
+		return -ENOMEM;
+
+	idr_preload(GFP_KERNEL);
+	spin_lock(&file_private->syncobj_table_lock);
+	ret = idr_alloc(&file_private->syncobj_idr, sync_file, 1, 0, GFP_NOWAIT);
+	spin_unlock(&file_private->syncobj_table_lock);
+
+	idr_preload_end();
+
+	if (ret < 0)
+		return ret;
+
+	*handle = ret;
+	return 0;
+}
+
+static void drm_syncobj_destroy(struct drm_file *file_private,
+				u32 handle)
+{
+	struct sync_file *sync_file = drm_syncobj_get(file_private, handle);
+	if (!sync_file)
+		return;
+
+	spin_lock(&file_private->syncobj_table_lock);
+	idr_remove(&file_private->syncobj_idr, handle);
+	spin_unlock(&file_private->syncobj_table_lock);
+
+	/* drop lookup reference */
+	fput(sync_file->file);
+	/* drop idr reference */
+	fput(sync_file->file);
+}
+
+static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
+				    u32 handle, int *fd)
+{
+	struct sync_file *sync_file = drm_syncobj_get(file_private, handle);
+	int ret;
+	if (!sync_file)
+		return -EINVAL;
+
+	ret = get_unused_fd_flags(O_CLOEXEC);
+	if (ret < 0) {
+		fput(sync_file->file);
+		return ret;
+	}
+	fd_install(ret, sync_file->file);
+	*fd = ret;
+	fput(sync_file->file);
+	return 0;
+}
+
+static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
+				    int fd, u32 *handle)
+{
+	struct sync_file *sync_file = sync_file_fdget(fd);
+	int ret;
+	if (!sync_file)
+		return -EINVAL;
+	idr_preload(GFP_KERNEL);
+	spin_lock(&file_private->syncobj_table_lock);
+	ret = idr_alloc(&file_private->syncobj_idr, sync_file, 1, 0, GFP_NOWAIT);
+	spin_unlock(&file_private->syncobj_table_lock);
+	idr_preload_end();
+
+	if (ret < 0) {
+		fput(sync_file->file);
+		return ret;
+	}
+	*handle = ret;
+	return 0;
+}
+
+static int drm_syncobj_info(struct drm_file *file_private,
+			    u32 handle, u32 *type, u32 *flags)
+{
+	struct sync_file *sync_file = drm_syncobj_get(file_private, handle);
+
+	if (!sync_file)
+		return -EINVAL;
+	*type = sync_file->type;
+	*flags = sync_file->flags;
+	fput(sync_file->file);
+	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 sync_file *sync_file = ptr;
+
+	fput(sync_file->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_info *args = data;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	return drm_syncobj_create(file_private, args->type,
+				 args->flags, &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;
+
+	drm_syncobj_destroy(file_private, args->handle);
+	return 0;
+}
+
+
+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);
+
+}
+
+int
+drm_syncobj_info_ioctl(struct drm_device *dev, void *data,
+		       struct drm_file *file_private)
+{
+	struct drm_syncobj_create_info *args = data;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	return drm_syncobj_info(file_private, args->handle,
+				&args->type, &args->flags);
+}
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/uapi/drm/drm.h b/include/uapi/drm/drm.h
index b2c5284..26b7e86 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		0x12
 
 /** DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
@@ -696,6 +697,26 @@ struct drm_prime_handle {
 	__s32 fd;
 };
 
+struct drm_syncobj_create_info {
+	__u32 handle;
+	__u32 type;
+	__u32 flags;
+	__u32 pad;
+};
+
+struct drm_syncobj_destroy {
+	__u32 handle;
+	__u32 pad;
+};
+
+struct drm_syncobj_handle {
+	__u32 handle;
+	/** Flags.. only applicable for handle->fd */
+	__u32 flags;
+
+	__s32 fd;
+};
+
 #if defined(__cplusplus)
 }
 #endif
@@ -814,6 +835,12 @@ 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_info)
+#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)
+#define DRM_IOCTL_SYNCOBJ_INFO		DRM_IOWR(0xC3, struct drm_syncobj_create_info)
+
 /**
  * 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 5/8] sync_file: add support for a semaphore object

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This object can be used to implement the Vulkan semaphores.

The object behaviour differs from fence, in that you can
replace the underlying fence, and you cannot merge semaphores.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/dma-buf/sync_file.c    | 36 +++++++++++++++++++++++++++++++++++-
 include/linux/sync_file.h      |  2 ++
 include/uapi/linux/sync_file.h | 14 ++++++++++++++
 3 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index d84de67..a5b17c0 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -44,7 +44,7 @@ int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
 {
 	if (flags)
 		return -EINVAL;
-	if (type != SYNC_FILE_TYPE_FENCE)
+	if (type != SYNC_FILE_TYPE_FENCE && type != SYNC_FILE_TYPE_SEMAPHORE)
 		return -EINVAL;
 	return 0;
 }
@@ -202,6 +202,38 @@ sync_file_get_fence_locked(struct sync_file *sync_file)
 					 sync_file_held(sync_file));
 }
 
+/**
+ * sync_file_replace_fence - replace the fence related to the sync_file
+ * @sync_file:	 sync file to replace fence in
+ * @fence: fence to replace with (or NULL for no fence).
+ * Returns previous fence.
+ */
+struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
+					  struct dma_fence *fence)
+{
+	struct dma_fence *ret_fence = NULL;
+
+	if (sync_file->type != SYNC_FILE_TYPE_SEMAPHORE)
+		return NULL;
+
+	if (fence)
+		dma_fence_get(fence);
+
+	mutex_lock(&sync_file->lock);
+
+	ret_fence = sync_file_get_fence_locked(sync_file);
+	if (ret_fence) {
+		if (test_bit(POLL_ENABLED, &ret_fence->flags))
+			dma_fence_remove_callback(ret_fence, &sync_file->cb);
+	}
+
+	RCU_INIT_POINTER(sync_file->fence, fence);
+
+	mutex_unlock(&sync_file->lock);
+	return ret_fence;
+}
+EXPORT_SYMBOL(sync_file_replace_fence);
+
 static int sync_file_set_fence(struct sync_file *sync_file,
 			       struct dma_fence **fences, int num_fences)
 {
@@ -280,6 +312,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
 
 	if (a->type != b->type)
 		return NULL;
+	if (a->type != SYNC_FILE_TYPE_FENCE)
+		return NULL;
 
 	if (!rcu_access_pointer(a->fence) ||
 	    !rcu_access_pointer(b->fence))
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 4bf661b..245c7da 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -62,4 +62,6 @@ struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags);
 struct sync_file *sync_file_create(struct dma_fence *fence, uint32_t type, uint32_t flags);
 struct dma_fence *sync_file_get_fence(int fd);
 struct sync_file *sync_file_fdget(int fd);
+struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
+					  struct dma_fence *fence);
 #endif /* _LINUX_SYNC_H */
diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
index f439cda..5f266e0 100644
--- a/include/uapi/linux/sync_file.h
+++ b/include/uapi/linux/sync_file.h
@@ -80,6 +80,20 @@ struct sync_file_info {
 #define SYNC_FILE_TYPE_FENCE 0
 
 /**
+ * DOC: SYNC_FILE_TYPE_SEMAPHORE - semaphore sync file object
+ *
+ * This is a sync file that operates like a Vulkan semaphore.
+ * The object should just be imported/exported but not use the
+ * sync file ioctls (except info).
+ * This object can have it's backing fence replaced multiple times.
+ * Each signal operation assigns a backing fence.
+ * Each wait operation waits on the current fence, and removes it.
+ * These operations should happen via driver command submission interfaces.
+ * This is useful for shared vulkan semaphores.
+ */
+#define SYNC_FILE_TYPE_SEMAPHORE 1
+
+/**
  * struct sync_file_type - data returned from sync file type ioctl
  * @type:	sync_file type
  * @flags:	sync_file creation flags
-- 
2.9.3

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

Re: [PATCH 5/8] sync_file: add support for a semaphore object

[Chris Wilson]

On Tue, Apr 11, 2017 at 01:22:17PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This object can be used to implement the Vulkan semaphores.
> 
> The object behaviour differs from fence, in that you can
> replace the underlying fence, and you cannot merge semaphores.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> +/**
> + * sync_file_replace_fence - replace the fence related to the sync_file
> + * @sync_file:	 sync file to replace fence in
> + * @fence: fence to replace with (or NULL for no fence).
> + * Returns previous fence.
> + */
> +struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
> +					  struct dma_fence *fence)
> +{
> +	struct dma_fence *ret_fence = NULL;
> +
> +	if (sync_file->type != SYNC_FILE_TYPE_SEMAPHORE)
> +		return NULL;
> +
> +	if (fence)
> +		dma_fence_get(fence);
> +
> +	mutex_lock(&sync_file->lock);
> +
> +	ret_fence = sync_file_get_fence_locked(sync_file);
> +	if (ret_fence) {
> +		if (test_bit(POLL_ENABLED, &ret_fence->flags))
> +			dma_fence_remove_callback(ret_fence, &sync_file->cb);
> +	}

Fails when sync_file_replace_fence is passed sync_file->fence.

if (test_and_clear_bit(POLL_ENABLED, &ret_fence->flags)) {
	dma_fence_remove_callback(ret_fence, &sync_file->cb);
	wake_up(&sync_file->wq); /* only needs the first to redo the add */
}

will get the waiter to reset the callback on a new fence, or the old
fence replacing itself. Otherwise the waiter will never be woken over
the change in fence, not even when the old or new fence is signaled.
-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 5/8] sync_file: add support for a semaphore object

[Dave Airlie]

On 11 April 2017 at 17:50, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Apr 11, 2017 at 01:22:17PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <airlied@redhat.com>
>>
>> This object can be used to implement the Vulkan semaphores.
>>
>> The object behaviour differs from fence, in that you can
>> replace the underlying fence, and you cannot merge semaphores.
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> ---
>> +/**
>> + * sync_file_replace_fence - replace the fence related to the sync_file
>> + * @sync_file:        sync file to replace fence in
>> + * @fence: fence to replace with (or NULL for no fence).
>> + * Returns previous fence.
>> + */
>> +struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
>> +                                       struct dma_fence *fence)
>> +{
>> +     struct dma_fence *ret_fence = NULL;
>> +
>> +     if (sync_file->type != SYNC_FILE_TYPE_SEMAPHORE)
>> +             return NULL;
>> +
>> +     if (fence)
>> +             dma_fence_get(fence);
>> +
>> +     mutex_lock(&sync_file->lock);
>> +
>> +     ret_fence = sync_file_get_fence_locked(sync_file);
>> +     if (ret_fence) {
>> +             if (test_bit(POLL_ENABLED, &ret_fence->flags))
>> +                     dma_fence_remove_callback(ret_fence, &sync_file->cb);
>> +     }
>
> Fails when sync_file_replace_fence is passed sync_file->fence.

Not sure what the best semantics are there, any opinions on barring
wakeups/polling on semaphore sync_files, and just punting this
until we need it.

It would definitely simplify things.

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

Re: [PATCH 5/8] sync_file: add support for a semaphore object

[Chris Wilson]

On Wed, Apr 12, 2017 at 12:36:37PM +1000, Dave Airlie wrote:
> On 11 April 2017 at 17:50, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Tue, Apr 11, 2017 at 01:22:17PM +1000, Dave Airlie wrote:
> >> From: Dave Airlie <airlied@redhat.com>
> >>
> >> This object can be used to implement the Vulkan semaphores.
> >>
> >> The object behaviour differs from fence, in that you can
> >> replace the underlying fence, and you cannot merge semaphores.
> >>
> >> Signed-off-by: Dave Airlie <airlied@redhat.com>
> >> ---
> >> +/**
> >> + * sync_file_replace_fence - replace the fence related to the sync_file
> >> + * @sync_file:        sync file to replace fence in
> >> + * @fence: fence to replace with (or NULL for no fence).
> >> + * Returns previous fence.
> >> + */
> >> +struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
> >> +                                       struct dma_fence *fence)
> >> +{
> >> +     struct dma_fence *ret_fence = NULL;
> >> +
> >> +     if (sync_file->type != SYNC_FILE_TYPE_SEMAPHORE)
> >> +             return NULL;
> >> +
> >> +     if (fence)
> >> +             dma_fence_get(fence);
> >> +
> >> +     mutex_lock(&sync_file->lock);
> >> +
> >> +     ret_fence = sync_file_get_fence_locked(sync_file);
> >> +     if (ret_fence) {
> >> +             if (test_bit(POLL_ENABLED, &ret_fence->flags))
> >> +                     dma_fence_remove_callback(ret_fence, &sync_file->cb);
> >> +     }
> >
> > Fails when sync_file_replace_fence is passed sync_file->fence.
> 
> Not sure what the best semantics are there, any opinions on barring
> wakeups/polling on semaphore sync_files, and just punting this
> until we need it.

I think getting it right now will make writing sw_sync-esque (i.e. cpu)
tests easier and more complete.
-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 5/8] sync_file: add support for a semaphore object

[Dave Airlie]

>>
>> Not sure what the best semantics are there, any opinions on barring
>> wakeups/polling on semaphore sync_files, and just punting this
>> until we need it.
>
> I think getting it right now will make writing sw_sync-esque (i.e. cpu)
> tests easier and more complete.

I just don't have any use case for it, so we would be writing code to
write tests for it.

That doesn't seem smart.

If there is a future non-testing use case, the API is expressive
enough for someone
to add a flag or new sync obj to allow polling and to add support in a
nice easily
digestible patch.

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

Re: [PATCH 5/8] sync_file: add support for a semaphore object

[Chris Wilson]

On Thu, Apr 13, 2017 at 05:05:27AM +1000, Dave Airlie wrote:
> >>
> >> Not sure what the best semantics are there, any opinions on barring
> >> wakeups/polling on semaphore sync_files, and just punting this
> >> until we need it.
> >
> > I think getting it right now will make writing sw_sync-esque (i.e. cpu)
> > tests easier and more complete.
> 
> I just don't have any use case for it, so we would be writing code to
> write tests for it.
> 
> That doesn't seem smart.
> 
> If there is a future non-testing use case, the API is expressive
> enough for someone
> to add a flag or new sync obj to allow polling and to add support in a
> nice easily
> digestible patch.

My first thought was to check the signaled status would be to use
poll(0), but that can be retrieved from the sync_file_status ioctl. But
to get that still needs for us to acquire an fd from the syncobj. And if
we were to want check the flag on a driver syncobj, we would need to be
able to export one. That doesn't look very promising...

The testing chain would look like

	create syncobj
	execbuf -> signal syncobj
	syncobj wait -> dummy/nop execbuf -> fence out

then use the fence as a proxy for testing the status of the syncobj.

For generic tests, we could add an interface for vgem to use syncobjs?

If there's no way to export an fd for the syncobj, then we don't need
to handle it in the fops. Just be sure to leave a breadcrumb behind so
that the first person who does try to pass back a syncobj fd is reminded
that they need to fill in the fops.
-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 5/8] sync_file: add support for a semaphore object

[Chris Wilson]

On Wed, Apr 12, 2017 at 09:01:32PM +0100, Chris Wilson wrote:
> On Thu, Apr 13, 2017 at 05:05:27AM +1000, Dave Airlie wrote:
> > >>
> > >> Not sure what the best semantics are there, any opinions on barring
> > >> wakeups/polling on semaphore sync_files, and just punting this
> > >> until we need it.
> > >
> > > I think getting it right now will make writing sw_sync-esque (i.e. cpu)
> > > tests easier and more complete.
> > 
> > I just don't have any use case for it, so we would be writing code to
> > write tests for it.
> > 
> > That doesn't seem smart.
> > 
> > If there is a future non-testing use case, the API is expressive
> > enough for someone
> > to add a flag or new sync obj to allow polling and to add support in a
> > nice easily
> > digestible patch.
> 
> My first thought was to check the signaled status would be to use
> poll(0), but that can be retrieved from the sync_file_status ioctl. But
> to get that still needs for us to acquire an fd from the syncobj. And if
> we were to want check the flag on a driver syncobj, we would need to be
> able to export one. That doesn't look very promising...

Hmm, you do export fd to pass syncobj between processes. Let's not start
with syncobj being a second class sync_file.
-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 5/8] sync_file: add support for a semaphore object

[Dave Airlie]

On 13 April 2017 at 06:39, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Apr 12, 2017 at 09:01:32PM +0100, Chris Wilson wrote:
>> On Thu, Apr 13, 2017 at 05:05:27AM +1000, Dave Airlie wrote:
>> > >>
>> > >> Not sure what the best semantics are there, any opinions on barring
>> > >> wakeups/polling on semaphore sync_files, and just punting this
>> > >> until we need it.
>> > >
>> > > I think getting it right now will make writing sw_sync-esque (i.e. cpu)
>> > > tests easier and more complete.
>> >
>> > I just don't have any use case for it, so we would be writing code to
>> > write tests for it.
>> >
>> > That doesn't seem smart.
>> >
>> > If there is a future non-testing use case, the API is expressive
>> > enough for someone
>> > to add a flag or new sync obj to allow polling and to add support in a
>> > nice easily
>> > digestible patch.
>>
>> My first thought was to check the signaled status would be to use
>> poll(0), but that can be retrieved from the sync_file_status ioctl. But
>> to get that still needs for us to acquire an fd from the syncobj. And if
>> we were to want check the flag on a driver syncobj, we would need to be
>> able to export one. That doesn't look very promising...
>
> Hmm, you do export fd to pass syncobj between processes. Let's not start
> with syncobj being a second class sync_file.

It's not the same semantics as a sync_file. Userspace should never be polling on
semaphores.

Semaphores are one process signals, one process waits, no semantics for
sniffing or use cases. Unless you have a practical use case for an new
or proposed
API I don't think we should start with adding functionality.

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

Re: [PATCH 5/8] sync_file: add support for a semaphore object

[Chris Wilson]

On Thu, Apr 13, 2017 at 06:51:17AM +1000, Dave Airlie wrote:
> On 13 April 2017 at 06:39, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Wed, Apr 12, 2017 at 09:01:32PM +0100, Chris Wilson wrote:
> >> On Thu, Apr 13, 2017 at 05:05:27AM +1000, Dave Airlie wrote:
> >> > >>
> >> > >> Not sure what the best semantics are there, any opinions on barring
> >> > >> wakeups/polling on semaphore sync_files, and just punting this
> >> > >> until we need it.
> >> > >
> >> > > I think getting it right now will make writing sw_sync-esque (i.e. cpu)
> >> > > tests easier and more complete.
> >> >
> >> > I just don't have any use case for it, so we would be writing code to
> >> > write tests for it.
> >> >
> >> > That doesn't seem smart.
> >> >
> >> > If there is a future non-testing use case, the API is expressive
> >> > enough for someone
> >> > to add a flag or new sync obj to allow polling and to add support in a
> >> > nice easily
> >> > digestible patch.
> >>
> >> My first thought was to check the signaled status would be to use
> >> poll(0), but that can be retrieved from the sync_file_status ioctl. But
> >> to get that still needs for us to acquire an fd from the syncobj. And if
> >> we were to want check the flag on a driver syncobj, we would need to be
> >> able to export one. That doesn't look very promising...
> >
> > Hmm, you do export fd to pass syncobj between processes. Let's not start
> > with syncobj being a second class sync_file.
> 
> It's not the same semantics as a sync_file. Userspace should never be polling on
> semaphores.
> 
> Semaphores are one process signals, one process waits, no semantics for
> sniffing or use cases. Unless you have a practical use case for an new
> or proposed
> API I don't think we should start with adding functionality.

The problem, as I see it, is that you are taking functionality away from
sync_file. If you are wrapping them up inside a sync_file, we have a
fair expectation that our code to handle sync_files will continue to
work.
-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 5/8] sync_file: add support for a semaphore object

[Dave Airlie]

>
> The problem, as I see it, is that you are taking functionality away from
> sync_file. If you are wrapping them up inside a sync_file, we have a
> fair expectation that our code to handle sync_files will continue to
> work.

What code? there is no code existing that should be sharing sync objects
with semaphores backing them, and trying to use them as sync_files.

This is parallel functionality.

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

Re: [PATCH 5/8] sync_file: add support for a semaphore object

[Chris Wilson]

On Thu, Apr 13, 2017 at 07:41:28AM +1000, Dave Airlie wrote:
> >
> > The problem, as I see it, is that you are taking functionality away from
> > sync_file. If you are wrapping them up inside a sync_file, we have a
> > fair expectation that our code to handle sync_files will continue to
> > work.
> 
> What code? there is no code existing that should be sharing sync objects
> with semaphores backing them, and trying to use them as sync_files.

By masquerading semaphores as sync_files, you are inviting them to be
used with the existing code to handle sync_file.
 
> This is parallel functionality.

And quite different from the existing CPU visible sync_files. Why try
stuffing them through the same interface? At the moment the only thing
they have in common is the single fence pointer, and really you want
them just for their handle/fd to a GPU channel.

After you strip out the fops from the semaphore code, doesn't it boil
down to:

static void semaphore_file_release(struct inode *inode, struct file *file)
{
	struct semaphore_file *s = file->private_data;

	dma_fence_put(s->fence);
	kfree(s);
}

static const struct file_operations semaphore_file_fops = {
	.release = semaphore_file_release,
};

struct semaphore_file *semaphore_file_create(void)
{
	struct semaphore_file *s;

	s = kzalloc(sizeof(*s), GFP_KERNEL);
	if (!s)
		return NULL;

	s->file = anon_inode_getfile("semaphore_"file",
				     &semaphore_file_fops, s, 0);

	return s;
}

Plus your new routines to manipulate the semaphore.

The commonality with the current sync_file {
		struct file *file;
		struct dma_fence *fence;
	};
could be extracted and sync_file become fence_file. Would it not help to
avoid any further confusion by treating them as two very distinct classes
of fd?

And for me to stop calling the user interface sync_file.
-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 5/8] sync_file: add support for a semaphore object

[Dave Airlie]

On 13 April 2017 at 08:34, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Apr 13, 2017 at 07:41:28AM +1000, Dave Airlie wrote:
>> >
>> > The problem, as I see it, is that you are taking functionality away from
>> > sync_file. If you are wrapping them up inside a sync_file, we have a
>> > fair expectation that our code to handle sync_files will continue to
>> > work.
>>
>> What code? there is no code existing that should be sharing sync objects
>> with semaphores backing them, and trying to use them as sync_files.
>
> By masquerading semaphores as sync_files, you are inviting them to be
> used with the existing code to handle sync_file.

But what existing code is going to get a sync object from another process
and blindly use it without knowing where it got it.

I'm not buying the argument that just because something is possible,
we have to support it.

>
> And quite different from the existing CPU visible sync_files. Why try
> stuffing them through the same interface? At the moment the only thing
> they have in common is the single fence pointer, and really you want
> them just for their handle/fd to a GPU channel.
>
> After you strip out the fops from the semaphore code, doesn't it boil
> down to:
>
> static void semaphore_file_release(struct inode *inode, struct file *file)
> {
>         struct semaphore_file *s = file->private_data;
>
>         dma_fence_put(s->fence);
>         kfree(s);
> }
>
> static const struct file_operations semaphore_file_fops = {
>         .release = semaphore_file_release,
> };
>
> struct semaphore_file *semaphore_file_create(void)
> {
>         struct semaphore_file *s;
>
>         s = kzalloc(sizeof(*s), GFP_KERNEL);
>         if (!s)
>                 return NULL;
>
>         s->file = anon_inode_getfile("semaphore_"file",
>                                      &semaphore_file_fops, s, 0);
>
>         return s;
> }
>
> Plus your new routines to manipulate the semaphore.
>
> The commonality with the current sync_file {
>                 struct file *file;
>                 struct dma_fence *fence;
>         };
> could be extracted and sync_file become fence_file. Would it not help to
> avoid any further confusion by treating them as two very distinct classes
> of fd?
>
> And for me to stop calling the user interface sync_file.

That's a better argument, but the consideration is that there are probably
use-cases for doing something with these in the future, and we'd be limiting
those by doing this. The current code adds the API we need without constraining
future use cases overly much, and avoids any pitfalls.

I'll consider whether we should just split sync_file, but you still have the
same problems you mention, you get an fd you do something you shouldn't with
it, it doesn't fix any of the problems you are raising.

If you have an opaque fd you don't know the providence of, and except some
operations to just work o nit, you are going to be disappointed.

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