[rfc] drm sync objects (search for spock)

[rfc] drm sync objects (search for spock)

[Dave Airlie]

Okay I've gone around the sun with these a few times, and
pretty much implemented what I said last week.

This is pretty much a complete revamp.

1. sync objects are self contained drm objects, they
have a file reference so can be passed between processes.

2. Added a sync object wait interface modelled on the vulkan
fence waiting API.

3. sync_file interaction is explicitly different than
opaque fd passing, you import a sync file state into an
existing syncobj, or create a new sync_file from an
existing syncobj. This means no touching the sync file code
at all. \o/

I haven't used rcu anywhere here, I've used xchg to swap
fence pointers in the hope that's safe. If this does need
rcu'ing I suggest we do it in a follow on patch to minimise
the review pain.

Dave.

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

[PATCH 1/5] drm: introduce sync objects

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

Sync objects are new toplevel drm object, that contain a
pointer to a fence. This fence can be updated via command
submission ioctls via drivers.

There is also a generic wait obj API modelled on the vulkan
wait API (with code modelled on some amdgpu code).

These objects can be converted to an opaque fd that can be
passes between processes.

TODO: define sync_file interaction.

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

diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
index e35920d..2ea3bce 100644
--- a/Documentation/gpu/drm-internals.rst
+++ b/Documentation/gpu/drm-internals.rst
@@ -98,6 +98,9 @@ DRIVER_ATOMIC
     implement appropriate obj->atomic_get_property() vfuncs for any
     modeset objects with driver specific properties.
 
+DRIVER_SYNCOBJ
+    Driver support drm sync objects.
+
 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..44ef903 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -142,4 +142,17 @@ static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
 {
 	return 0;
 }
+
 #endif
+
+/* drm_syncobj.c */
+void drm_syncobj_open(struct drm_file *file_private);
+void drm_syncobj_release(struct drm_file *file_private);
+int drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
+			     struct drm_file *file_private);
+int drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
+			      struct drm_file *file_private);
+int drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file_private);
+int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file_private);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index a7c61c2..6da7adc 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -240,6 +240,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 		req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0;
 		req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0;
 		return 0;
+	case DRM_CAP_SYNCOBJ:
+		req->value = drm_core_check_feature(dev, DRIVER_SYNCOBJ);
+		return 0;
 	}
 
 	/* Other caps only work with KMS drivers */
@@ -641,6 +644,15 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_CREATE, drm_syncobj_create_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_DESTROY, drm_syncobj_destroy_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD, drm_syncobj_handle_to_fd_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
+		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
new file mode 100644
index 0000000..e6a99d4
--- /dev/null
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -0,0 +1,374 @@
+/*
+ * 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 a persistent objects,
+ * that contain an optional fence. The fence can be updated with a new
+ * fence, or be NULL.
+ *
+ * syncobj's can be export to fd's and back, these fd's are opaque and
+ * have no other use case, except passing the syncobj between processes.
+ *
+ * TODO: sync_file interactions, waiting
+ *
+ * Their primary use-case is to implement Vulkan fences and semaphores.
+ *
+ * syncobj have a kref reference count, but also have an optional file.
+ * The file is only created once the syncobj is exported.
+ * The file takes a reference on the kref.
+ */
+
+#include <drm/drmP.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/anon_inodes.h>
+
+#include "drm_internal.h"
+#include <drm/drm_syncobj.h>
+
+static struct drm_syncobj *drm_syncobj_get(struct drm_file *file_private,
+					   u32 handle)
+{
+	struct drm_syncobj *syncobj;
+
+	spin_lock(&file_private->syncobj_table_lock);
+
+	/* Check if we currently have a reference on the object */
+	syncobj = idr_find(&file_private->syncobj_idr, handle);
+	if (syncobj)
+		drm_syncobj_reference(syncobj);
+
+	spin_unlock(&file_private->syncobj_table_lock);
+
+	return syncobj;
+}
+
+/**
+ * drm_syncobj_replace_fence - lookup and replace fence in a sync object.
+ * @file_private - drm file private pointer.
+ * @handle - syncobj handle to lookup
+ * @fence - fence to install in sync file.
+ * Returns:
+ * 0 on success, or -EINVAL when the handle doesn't point at a valid sem file.
+ *
+ * This looks up a sync object and replaces the fence on it, freeing
+ * the old one.
+ */
+int drm_syncobj_replace_fence(struct drm_file *file_private,
+			      u32 handle,
+			      struct dma_fence *fence)
+{
+	struct drm_syncobj *syncobj = drm_syncobj_get(file_private, handle);
+	struct dma_fence *old_fence = NULL;
+
+	if (!syncobj)
+		return -EINVAL;
+
+	if (fence)
+		dma_fence_get(fence);
+	old_fence = xchg(&syncobj->fence, fence);
+
+	dma_fence_put(old_fence);
+	drm_syncobj_unreference(syncobj);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_syncobj_replace_fence);
+
+int drm_syncobj_fence_get(struct drm_file *file_private,
+			  u32 handle,
+			  struct dma_fence **fence)
+{
+	struct drm_syncobj *syncobj = drm_syncobj_get(file_private, handle);
+	int ret = 0;
+
+	if (!syncobj)
+		return -EINVAL;
+
+	*fence = dma_fence_get(syncobj->fence);
+	if (!*fence) {
+		ret = -EINVAL;
+	}
+	drm_syncobj_unreference(syncobj);
+	return ret;
+}
+EXPORT_SYMBOL(drm_syncobj_fence_get);
+
+void drm_syncobj_free(struct kref *kref)
+{
+	struct drm_syncobj *syncobj = container_of(kref,
+						   struct drm_syncobj,
+						   refcount);
+	kfree(syncobj);
+}
+
+static int drm_syncobj_create(struct drm_file *file_private,
+			      u32 *handle)
+{
+	int ret;
+	struct drm_syncobj *syncobj;
+
+	syncobj = kzalloc(sizeof(struct drm_syncobj), GFP_KERNEL);
+	if (!syncobj)
+		return -ENOMEM;
+
+	mutex_init(&syncobj->file_lock);
+	kref_init(&syncobj->refcount);
+
+	idr_preload(GFP_KERNEL);
+	spin_lock(&file_private->syncobj_table_lock);
+	ret = idr_alloc(&file_private->syncobj_idr, syncobj, 1, 0, GFP_NOWAIT);
+	spin_unlock(&file_private->syncobj_table_lock);
+
+	idr_preload_end();
+
+	if (ret < 0) {
+		drm_syncobj_unreference(syncobj);
+		return ret;
+	}
+
+	*handle = ret;
+	return 0;
+}
+
+static int drm_syncobj_destroy(struct drm_file *file_private,
+			       u32 handle)
+{
+	struct drm_syncobj *syncobj;
+
+	spin_lock(&file_private->syncobj_table_lock);
+	syncobj = idr_remove(&file_private->syncobj_idr, handle);
+	spin_unlock(&file_private->syncobj_table_lock);
+
+	if (!syncobj)
+		return -EINVAL;
+
+	drm_syncobj_unreference(syncobj);
+	return 0;
+}
+
+static int drm_syncobj_file_release(struct inode *inode, struct file *file)
+{
+	struct drm_syncobj *syncobj = file->private_data;
+
+	drm_syncobj_unreference(syncobj);
+	return 0;
+}
+
+static const struct file_operations drm_syncobj_file_fops = {
+	.release = drm_syncobj_file_release,
+};
+
+static int drm_syncobj_alloc_file_locked(struct drm_syncobj *syncobj)
+{
+	struct file *file = anon_inode_getfile("syncobj_file",
+					       &drm_syncobj_file_fops,
+					       syncobj, 0);
+	if (IS_ERR(file))
+		return PTR_ERR(file);
+
+	/* the file holds a reference to the object */
+	drm_syncobj_reference(syncobj);
+	syncobj->file = file;
+
+	return 0;
+}
+
+static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
+				    u32 handle, int *p_fd)
+{
+	struct drm_syncobj *syncobj = drm_syncobj_get(file_private, handle);
+	int ret;
+	int fd;
+
+	if (!syncobj)
+		return -EINVAL;
+
+	fd = get_unused_fd_flags(O_CLOEXEC);
+	if (fd < 0) {
+		drm_syncobj_unreference(syncobj);
+		return fd;
+	}
+
+	mutex_lock(&syncobj->file_lock);
+	if (!syncobj->file) {
+		ret = drm_syncobj_alloc_file_locked(syncobj);
+		if (ret)
+			goto out_unlock;
+	}
+	fd_install(fd, syncobj->file);
+	mutex_unlock(&syncobj->file_lock);
+	drm_syncobj_unreference(syncobj);
+	*p_fd = fd;
+	return 0;
+out_unlock:
+	put_unused_fd(fd);
+	mutex_unlock(&syncobj->file_lock);
+	drm_syncobj_unreference(syncobj);
+	return ret;
+}
+
+static struct drm_syncobj *drm_syncobj_fdget(int fd)
+{
+	struct file *file = fget(fd);
+
+	if (!file)
+		return NULL;
+	if (file->f_op != &drm_syncobj_file_fops)
+		goto err;
+
+	return file->private_data;
+err:
+	fput(file);
+	return NULL;
+};
+
+static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
+				    int fd, u32 *handle)
+{
+	struct drm_syncobj *syncobj = drm_syncobj_fdget(fd);
+	int ret;
+
+	if (!syncobj)
+		return -EINVAL;
+
+	/* take a reference to put in the idr */
+	drm_syncobj_reference(syncobj);
+
+	idr_preload(GFP_KERNEL);
+	spin_lock(&file_private->syncobj_table_lock);
+	ret = idr_alloc(&file_private->syncobj_idr, syncobj, 1, 0, GFP_NOWAIT);
+	spin_unlock(&file_private->syncobj_table_lock);
+	idr_preload_end();
+
+	if (ret < 0) {
+		fput(syncobj->file);
+		return ret;
+	}
+	*handle = ret;
+	return 0;
+}
+
+/**
+ * drm_syncobj_open - initalizes syncobj file-private structures at devnode open time
+ * @dev: drm_device which is being opened by userspace
+ * @file_private: drm file-private structure to set up
+ *
+ * Called at device open time, sets up the structure for handling refcounting
+ * of sync objects.
+ */
+void
+drm_syncobj_open(struct drm_file *file_private)
+{
+	idr_init(&file_private->syncobj_idr);
+	spin_lock_init(&file_private->syncobj_table_lock);
+}
+
+static int
+drm_syncobj_release_handle(int id, void *ptr, void *data)
+{
+	struct drm_syncobj *syncobj = ptr;
+
+	drm_syncobj_unreference(syncobj);
+	return 0;
+}
+
+/**
+ * drm_syncobj_release - release file-private sync object resources
+ * @dev: drm_device which is being closed by userspace
+ * @file_private: drm file-private structure to clean up
+ *
+ * Called at close time when the filp is going away.
+ *
+ * Releases any remaining references on objects by this filp.
+ */
+void
+drm_syncobj_release(struct drm_file *file_private)
+{
+	idr_for_each(&file_private->syncobj_idr,
+		     &drm_syncobj_release_handle, file_private);
+	idr_destroy(&file_private->syncobj_idr);
+}
+
+int
+drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
+			 struct drm_file *file_private)
+{
+	struct drm_syncobj_create *args = data;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	/* no valid flags yet */
+	if (args->flags)
+		return -EINVAL;
+
+	return drm_syncobj_create(file_private,
+				  &args->handle);
+}
+
+int
+drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
+			  struct drm_file *file_private)
+{
+	struct drm_syncobj_destroy *args = data;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	/* make sure padding is empty */
+	if (args->pad)
+		return -EINVAL;
+	return drm_syncobj_destroy(file_private, args->handle);
+}
+
+int
+drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file_private)
+{
+	struct drm_syncobj_handle *args = data;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	return drm_syncobj_handle_to_fd(file_private, args->handle,
+					&args->fd);
+}
+
+int
+drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file_private)
+{
+	struct drm_syncobj_handle *args = data;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	return drm_syncobj_fd_to_handle(file_private, args->fd,
+					&args->handle);
+}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 6105c05..1d48f6f 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -405,6 +405,11 @@ struct drm_file {
 	/** Lock for synchronization of access to object_idr. */
 	spinlock_t table_lock;
 
+	/** Mapping of sync object handles to object pointers. */
+	struct idr syncobj_idr;
+	/** Lock for synchronization of access to syncobj_idr. */
+	spinlock_t syncobj_table_lock;
+
 	struct file *filp;
 	void *driver_priv;
 
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 5699f42..48ff06b 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -53,6 +53,7 @@ struct drm_mode_create_dumb;
 #define DRIVER_RENDER			0x8000
 #define DRIVER_ATOMIC			0x10000
 #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
+#define DRIVER_SYNCOBJ                  0x40000
 
 /**
  * struct drm_driver - DRM driver structure
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
new file mode 100644
index 0000000..3cc42b7
--- /dev/null
+++ b/include/drm/drm_syncobj.h
@@ -0,0 +1,104 @@
+/*
+ * 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__
+
+#include "linux/dma-fence.h"
+
+/**
+ * struct drm_syncobj - sync object.
+ *
+ * This structure defines a generic sync object which wraps a dma fence.
+ */
+struct drm_syncobj {
+	/**
+	 * @refcount:
+	 *
+	 * Reference count of this object.
+	 */
+	struct kref refcount;
+	/**
+	 * @fence:
+	 * NULL or a pointer to the fence bound to this object.
+	 */
+	struct dma_fence *fence;
+	/**
+	 * @file_lock
+	 * mutex protecting the struct file pointer.
+	 */
+	struct mutex file_lock;
+	/**
+	 * @file
+	 * a file backing for this syncobj.
+	 */
+	struct file *file;
+};
+
+void drm_syncobj_free(struct kref *kref);
+
+/**
+ * drm_gem_object_reference - acquire a GEM BO reference
+ * @obj: GEM buffer object
+ *
+ * This acquires additional reference to @obj. It is illegal to call this
+ * without already holding a reference. No locks required.
+ */
+static inline void
+drm_syncobj_reference(struct drm_syncobj *obj)
+{
+	kref_get(&obj->refcount);
+}
+
+/**
+ * __drm_gem_object_unreference - raw function to release a GEM BO reference
+ * @obj: GEM buffer object
+ *
+ * This function is meant to be used by drivers which are not encumbered with
+ * &drm_device.struct_mutex legacy locking and which are using the
+ * gem_free_object_unlocked callback. It avoids all the locking checks and
+ * locking overhead of drm_gem_object_unreference() and
+ * drm_gem_object_unreference_unlocked().
+ *
+ * Drivers should never call this directly in their code. Instead they should
+ * wrap it up into a ``driver_gem_object_unreference(struct driver_gem_object
+ * *obj)`` wrapper function, and use that. Shared code should never call this, to
+ * avoid breaking drivers by accident which still depend upon
+ * &drm_device.struct_mutex locking.
+ */
+static inline void
+drm_syncobj_unreference(struct drm_syncobj *obj)
+{
+	kref_put(&obj->refcount, drm_syncobj_free);
+}
+
+int drm_syncobj_fence_get(struct drm_file *file_private,
+			  u32 handle,
+			  struct dma_fence **fence);
+int drm_syncobj_replace_fence(struct drm_file *file_private,
+			      u32 handle,
+			      struct dma_fence *fence);
+
+#endif
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index b2c5284..dd0b99c 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -647,6 +647,7 @@ struct drm_gem_open {
 #define DRM_CAP_CURSOR_HEIGHT		0x9
 #define DRM_CAP_ADDFB2_MODIFIERS	0x10
 #define DRM_CAP_PAGE_FLIP_TARGET	0x11
+#define DRM_CAP_SYNCOBJ		0x13
 
 /** DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
@@ -696,6 +697,25 @@ struct drm_prime_handle {
 	__s32 fd;
 };
 
+struct drm_syncobj_create {
+	__u32 handle;
+	__u32 flags;
+};
+
+struct drm_syncobj_destroy {
+	__u32 handle;
+	__u32 pad;
+};
+
+struct drm_syncobj_handle {
+	__u32 handle;
+	/** Flags.. only applicable for handle->fd */
+	__u32 flags;
+
+	__s32 fd;
+	__u32 pad;
+};
+
 #if defined(__cplusplus)
 }
 #endif
@@ -814,6 +834,11 @@ extern "C" {
 #define DRM_IOCTL_MODE_CREATEPROPBLOB	DRM_IOWR(0xBD, struct drm_mode_create_blob)
 #define DRM_IOCTL_MODE_DESTROYPROPBLOB	DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
 
+#define DRM_IOCTL_SYNCOBJ_CREATE	DRM_IOWR(0xBF, struct drm_syncobj_create)
+#define DRM_IOCTL_SYNCOBJ_DESTROY	DRM_IOWR(0xC0, struct drm_syncobj_destroy)
+#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD	DRM_IOWR(0xC1, struct drm_syncobj_handle)
+#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE	DRM_IOWR(0xC2, struct drm_syncobj_handle)
+
 /**
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
-- 
2.9.3

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

Re: [PATCH 1/5] drm: introduce sync objects

[zhoucm1]

On 2017年04月26日 11:28, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> Sync objects are new toplevel drm object, that contain a
> pointer to a fence. This fence can be updated via command
> submission ioctls via drivers.
>
> There is also a generic wait obj API modelled on the vulkan
> wait API (with code modelled on some amdgpu code).
>
> These objects can be converted to an opaque fd that can be
> passes between processes.
>
> TODO: define sync_file interaction.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>   Documentation/gpu/drm-internals.rst |   3 +
>   Documentation/gpu/drm-mm.rst        |   6 +
>   drivers/gpu/drm/Makefile            |   2 +-
>   drivers/gpu/drm/drm_fops.c          |   8 +
>   drivers/gpu/drm/drm_internal.h      |  13 ++
>   drivers/gpu/drm/drm_ioctl.c         |  12 ++
>   drivers/gpu/drm/drm_syncobj.c       | 374 ++++++++++++++++++++++++++++++++++++
>   include/drm/drmP.h                  |   5 +
>   include/drm/drm_drv.h               |   1 +
>   include/drm/drm_syncobj.h           | 104 ++++++++++
>   include/uapi/drm/drm.h              |  25 +++
>   11 files changed, 552 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/drm_syncobj.c
>   create mode 100644 include/drm/drm_syncobj.h
>
> diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
> index e35920d..2ea3bce 100644
> --- a/Documentation/gpu/drm-internals.rst
> +++ b/Documentation/gpu/drm-internals.rst
> @@ -98,6 +98,9 @@ DRIVER_ATOMIC
>       implement appropriate obj->atomic_get_property() vfuncs for any
>       modeset objects with driver specific properties.
>   
> +DRIVER_SYNCOBJ
> +    Driver support drm sync objects.
> +
>   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..44ef903 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -142,4 +142,17 @@ static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
>   {
>   	return 0;
>   }
> +
>   #endif
> +
> +/* drm_syncobj.c */
> +void drm_syncobj_open(struct drm_file *file_private);
> +void drm_syncobj_release(struct drm_file *file_private);
> +int drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
> +			     struct drm_file *file_private);
> +int drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
> +			      struct drm_file *file_private);
> +int drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> +				   struct drm_file *file_private);
> +int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> +				   struct drm_file *file_private);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index a7c61c2..6da7adc 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -240,6 +240,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>   		req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0;
>   		req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0;
>   		return 0;
> +	case DRM_CAP_SYNCOBJ:
> +		req->value = drm_core_check_feature(dev, DRIVER_SYNCOBJ);
> +		return 0;
>   	}
>   
>   	/* Other caps only work with KMS drivers */
> @@ -641,6 +644,15 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +
> +	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_CREATE, drm_syncobj_create_ioctl,
> +		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_DESTROY, drm_syncobj_destroy_ioctl,
> +		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD, drm_syncobj_handle_to_fd_ioctl,
> +		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
> +		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
>   };
>   
>   #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> new file mode 100644
> index 0000000..e6a99d4
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -0,0 +1,374 @@
> +/*
> + * 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 a persistent objects,
> + * that contain an optional fence. The fence can be updated with a new
> + * fence, or be NULL.
> + *
> + * syncobj's can be export to fd's and back, these fd's are opaque and
> + * have no other use case, except passing the syncobj between processes.
> + *
> + * TODO: sync_file interactions, waiting
> + *
> + * Their primary use-case is to implement Vulkan fences and semaphores.
> + *
> + * syncobj have a kref reference count, but also have an optional file.
> + * The file is only created once the syncobj is exported.
> + * The file takes a reference on the kref.
> + */
> +
> +#include <drm/drmP.h>
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/anon_inodes.h>
> +
> +#include "drm_internal.h"
> +#include <drm/drm_syncobj.h>
> +
> +static struct drm_syncobj *drm_syncobj_get(struct drm_file *file_private,
> +					   u32 handle)
> +{
> +	struct drm_syncobj *syncobj;
> +
> +	spin_lock(&file_private->syncobj_table_lock);
> +
> +	/* Check if we currently have a reference on the object */
> +	syncobj = idr_find(&file_private->syncobj_idr, handle);
> +	if (syncobj)
> +		drm_syncobj_reference(syncobj);
> +
> +	spin_unlock(&file_private->syncobj_table_lock);
> +
> +	return syncobj;
> +}
> +
> +/**
> + * drm_syncobj_replace_fence - lookup and replace fence in a sync object.
> + * @file_private - drm file private pointer.
> + * @handle - syncobj handle to lookup
> + * @fence - fence to install in sync file.
> + * Returns:
> + * 0 on success, or -EINVAL when the handle doesn't point at a valid sem file.
> + *
> + * This looks up a sync object and replaces the fence on it, freeing
> + * the old one.
> + */
> +int drm_syncobj_replace_fence(struct drm_file *file_private,
> +			      u32 handle,
> +			      struct dma_fence *fence)
> +{
> +	struct drm_syncobj *syncobj = drm_syncobj_get(file_private, handle);
> +	struct dma_fence *old_fence = NULL;
> +
> +	if (!syncobj)
> +		return -EINVAL;
> +
> +	if (fence)
> +		dma_fence_get(fence);
> +	old_fence = xchg(&syncobj->fence, fence);
> +
> +	dma_fence_put(old_fence);
> +	drm_syncobj_unreference(syncobj);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_syncobj_replace_fence);
> +
> +int drm_syncobj_fence_get(struct drm_file *file_private,
> +			  u32 handle,
> +			  struct dma_fence **fence)
> +{
> +	struct drm_syncobj *syncobj = drm_syncobj_get(file_private, handle);
> +	int ret = 0;
> +
> +	if (!syncobj)
> +		return -EINVAL;
> +
> +	*fence = dma_fence_get(syncobj->fence);
> +	if (!*fence) {
> +		ret = -EINVAL;
> +	}
> +	drm_syncobj_unreference(syncobj);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_syncobj_fence_get);
> +
> +void drm_syncobj_free(struct kref *kref)
> +{
> +	struct drm_syncobj *syncobj = container_of(kref,
> +						   struct drm_syncobj,
> +						   refcount);
here losts fence leak, should be:
drm_fence_put(syncobj->fence);

Regards,
David Zhou
> +	kfree(syncobj);
> +}
> +
> +static int drm_syncobj_create(struct drm_file *file_private,
> +			      u32 *handle)
> +{
> +	int ret;
> +	struct drm_syncobj *syncobj;
> +
> +	syncobj = kzalloc(sizeof(struct drm_syncobj), GFP_KERNEL);
> +	if (!syncobj)
> +		return -ENOMEM;
> +
> +	mutex_init(&syncobj->file_lock);
> +	kref_init(&syncobj->refcount);
> +
> +	idr_preload(GFP_KERNEL);
> +	spin_lock(&file_private->syncobj_table_lock);
> +	ret = idr_alloc(&file_private->syncobj_idr, syncobj, 1, 0, GFP_NOWAIT);
> +	spin_unlock(&file_private->syncobj_table_lock);
> +
> +	idr_preload_end();
> +
> +	if (ret < 0) {
> +		drm_syncobj_unreference(syncobj);
> +		return ret;
> +	}
> +
> +	*handle = ret;
> +	return 0;
> +}
> +
> +static int drm_syncobj_destroy(struct drm_file *file_private,
> +			       u32 handle)
> +{
> +	struct drm_syncobj *syncobj;
> +
> +	spin_lock(&file_private->syncobj_table_lock);
> +	syncobj = idr_remove(&file_private->syncobj_idr, handle);
> +	spin_unlock(&file_private->syncobj_table_lock);
> +
> +	if (!syncobj)
> +		return -EINVAL;
> +
> +	drm_syncobj_unreference(syncobj);
> +	return 0;
> +}
> +
> +static int drm_syncobj_file_release(struct inode *inode, struct file *file)
> +{
> +	struct drm_syncobj *syncobj = file->private_data;
> +
> +	drm_syncobj_unreference(syncobj);
> +	return 0;
> +}
> +
> +static const struct file_operations drm_syncobj_file_fops = {
> +	.release = drm_syncobj_file_release,
> +};
> +
> +static int drm_syncobj_alloc_file_locked(struct drm_syncobj *syncobj)
> +{
> +	struct file *file = anon_inode_getfile("syncobj_file",
> +					       &drm_syncobj_file_fops,
> +					       syncobj, 0);
> +	if (IS_ERR(file))
> +		return PTR_ERR(file);
> +
> +	/* the file holds a reference to the object */
> +	drm_syncobj_reference(syncobj);
> +	syncobj->file = file;
> +
> +	return 0;
> +}
> +
> +static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
> +				    u32 handle, int *p_fd)
> +{
> +	struct drm_syncobj *syncobj = drm_syncobj_get(file_private, handle);
> +	int ret;
> +	int fd;
> +
> +	if (!syncobj)
> +		return -EINVAL;
> +
> +	fd = get_unused_fd_flags(O_CLOEXEC);
> +	if (fd < 0) {
> +		drm_syncobj_unreference(syncobj);
> +		return fd;
> +	}
> +
> +	mutex_lock(&syncobj->file_lock);
> +	if (!syncobj->file) {
> +		ret = drm_syncobj_alloc_file_locked(syncobj);
> +		if (ret)
> +			goto out_unlock;
> +	}
> +	fd_install(fd, syncobj->file);
> +	mutex_unlock(&syncobj->file_lock);
> +	drm_syncobj_unreference(syncobj);
> +	*p_fd = fd;
> +	return 0;
> +out_unlock:
> +	put_unused_fd(fd);
> +	mutex_unlock(&syncobj->file_lock);
> +	drm_syncobj_unreference(syncobj);
> +	return ret;
> +}
> +
> +static struct drm_syncobj *drm_syncobj_fdget(int fd)
> +{
> +	struct file *file = fget(fd);
> +
> +	if (!file)
> +		return NULL;
> +	if (file->f_op != &drm_syncobj_file_fops)
> +		goto err;
> +
> +	return file->private_data;
> +err:
> +	fput(file);
> +	return NULL;
> +};
> +
> +static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
> +				    int fd, u32 *handle)
> +{
> +	struct drm_syncobj *syncobj = drm_syncobj_fdget(fd);
> +	int ret;
> +
> +	if (!syncobj)
> +		return -EINVAL;
> +
> +	/* take a reference to put in the idr */
> +	drm_syncobj_reference(syncobj);
> +
> +	idr_preload(GFP_KERNEL);
> +	spin_lock(&file_private->syncobj_table_lock);
> +	ret = idr_alloc(&file_private->syncobj_idr, syncobj, 1, 0, GFP_NOWAIT);
> +	spin_unlock(&file_private->syncobj_table_lock);
> +	idr_preload_end();
> +
> +	if (ret < 0) {
> +		fput(syncobj->file);
> +		return ret;
> +	}
> +	*handle = ret;
> +	return 0;
> +}
> +
> +/**
> + * drm_syncobj_open - initalizes syncobj file-private structures at devnode open time
> + * @dev: drm_device which is being opened by userspace
> + * @file_private: drm file-private structure to set up
> + *
> + * Called at device open time, sets up the structure for handling refcounting
> + * of sync objects.
> + */
> +void
> +drm_syncobj_open(struct drm_file *file_private)
> +{
> +	idr_init(&file_private->syncobj_idr);
> +	spin_lock_init(&file_private->syncobj_table_lock);
> +}
> +
> +static int
> +drm_syncobj_release_handle(int id, void *ptr, void *data)
> +{
> +	struct drm_syncobj *syncobj = ptr;
> +
> +	drm_syncobj_unreference(syncobj);
> +	return 0;
> +}
> +
> +/**
> + * drm_syncobj_release - release file-private sync object resources
> + * @dev: drm_device which is being closed by userspace
> + * @file_private: drm file-private structure to clean up
> + *
> + * Called at close time when the filp is going away.
> + *
> + * Releases any remaining references on objects by this filp.
> + */
> +void
> +drm_syncobj_release(struct drm_file *file_private)
> +{
> +	idr_for_each(&file_private->syncobj_idr,
> +		     &drm_syncobj_release_handle, file_private);
> +	idr_destroy(&file_private->syncobj_idr);
> +}
> +
> +int
> +drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
> +			 struct drm_file *file_private)
> +{
> +	struct drm_syncobj_create *args = data;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> +		return -ENODEV;
> +
> +	/* no valid flags yet */
> +	if (args->flags)
> +		return -EINVAL;
> +
> +	return drm_syncobj_create(file_private,
> +				  &args->handle);
> +}
> +
> +int
> +drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
> +			  struct drm_file *file_private)
> +{
> +	struct drm_syncobj_destroy *args = data;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> +		return -ENODEV;
> +
> +	/* make sure padding is empty */
> +	if (args->pad)
> +		return -EINVAL;
> +	return drm_syncobj_destroy(file_private, args->handle);
> +}
> +
> +int
> +drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> +				   struct drm_file *file_private)
> +{
> +	struct drm_syncobj_handle *args = data;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> +		return -ENODEV;
> +
> +	return drm_syncobj_handle_to_fd(file_private, args->handle,
> +					&args->fd);
> +}
> +
> +int
> +drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> +				   struct drm_file *file_private)
> +{
> +	struct drm_syncobj_handle *args = data;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> +		return -ENODEV;
> +
> +	return drm_syncobj_fd_to_handle(file_private, args->fd,
> +					&args->handle);
> +}
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 6105c05..1d48f6f 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -405,6 +405,11 @@ struct drm_file {
>   	/** Lock for synchronization of access to object_idr. */
>   	spinlock_t table_lock;
>   
> +	/** Mapping of sync object handles to object pointers. */
> +	struct idr syncobj_idr;
> +	/** Lock for synchronization of access to syncobj_idr. */
> +	spinlock_t syncobj_table_lock;
> +
>   	struct file *filp;
>   	void *driver_priv;
>   
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 5699f42..48ff06b 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -53,6 +53,7 @@ struct drm_mode_create_dumb;
>   #define DRIVER_RENDER			0x8000
>   #define DRIVER_ATOMIC			0x10000
>   #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
> +#define DRIVER_SYNCOBJ                  0x40000
>   
>   /**
>    * struct drm_driver - DRM driver structure
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> new file mode 100644
> index 0000000..3cc42b7
> --- /dev/null
> +++ b/include/drm/drm_syncobj.h
> @@ -0,0 +1,104 @@
> +/*
> + * 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__
> +
> +#include "linux/dma-fence.h"
> +
> +/**
> + * struct drm_syncobj - sync object.
> + *
> + * This structure defines a generic sync object which wraps a dma fence.
> + */
> +struct drm_syncobj {
> +	/**
> +	 * @refcount:
> +	 *
> +	 * Reference count of this object.
> +	 */
> +	struct kref refcount;
> +	/**
> +	 * @fence:
> +	 * NULL or a pointer to the fence bound to this object.
> +	 */
> +	struct dma_fence *fence;
> +	/**
> +	 * @file_lock
> +	 * mutex protecting the struct file pointer.
> +	 */
> +	struct mutex file_lock;
> +	/**
> +	 * @file
> +	 * a file backing for this syncobj.
> +	 */
> +	struct file *file;
> +};
> +
> +void drm_syncobj_free(struct kref *kref);
> +
> +/**
> + * drm_gem_object_reference - acquire a GEM BO reference
> + * @obj: GEM buffer object
> + *
> + * This acquires additional reference to @obj. It is illegal to call this
> + * without already holding a reference. No locks required.
> + */
> +static inline void
> +drm_syncobj_reference(struct drm_syncobj *obj)
> +{
> +	kref_get(&obj->refcount);
> +}
> +
> +/**
> + * __drm_gem_object_unreference - raw function to release a GEM BO reference
> + * @obj: GEM buffer object
> + *
> + * This function is meant to be used by drivers which are not encumbered with
> + * &drm_device.struct_mutex legacy locking and which are using the
> + * gem_free_object_unlocked callback. It avoids all the locking checks and
> + * locking overhead of drm_gem_object_unreference() and
> + * drm_gem_object_unreference_unlocked().
> + *
> + * Drivers should never call this directly in their code. Instead they should
> + * wrap it up into a ``driver_gem_object_unreference(struct driver_gem_object
> + * *obj)`` wrapper function, and use that. Shared code should never call this, to
> + * avoid breaking drivers by accident which still depend upon
> + * &drm_device.struct_mutex locking.
> + */
> +static inline void
> +drm_syncobj_unreference(struct drm_syncobj *obj)
> +{
> +	kref_put(&obj->refcount, drm_syncobj_free);
> +}
> +
> +int drm_syncobj_fence_get(struct drm_file *file_private,
> +			  u32 handle,
> +			  struct dma_fence **fence);
> +int drm_syncobj_replace_fence(struct drm_file *file_private,
> +			      u32 handle,
> +			      struct dma_fence *fence);
> +
> +#endif
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b2c5284..dd0b99c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -647,6 +647,7 @@ struct drm_gem_open {
>   #define DRM_CAP_CURSOR_HEIGHT		0x9
>   #define DRM_CAP_ADDFB2_MODIFIERS	0x10
>   #define DRM_CAP_PAGE_FLIP_TARGET	0x11
> +#define DRM_CAP_SYNCOBJ		0x13
>   
>   /** DRM_IOCTL_GET_CAP ioctl argument type */
>   struct drm_get_cap {
> @@ -696,6 +697,25 @@ struct drm_prime_handle {
>   	__s32 fd;
>   };
>   
> +struct drm_syncobj_create {
> +	__u32 handle;
> +	__u32 flags;
> +};
> +
> +struct drm_syncobj_destroy {
> +	__u32 handle;
> +	__u32 pad;
> +};
> +
> +struct drm_syncobj_handle {
> +	__u32 handle;
> +	/** Flags.. only applicable for handle->fd */
> +	__u32 flags;
> +
> +	__s32 fd;
> +	__u32 pad;
> +};
> +
>   #if defined(__cplusplus)
>   }
>   #endif
> @@ -814,6 +834,11 @@ extern "C" {
>   #define DRM_IOCTL_MODE_CREATEPROPBLOB	DRM_IOWR(0xBD, struct drm_mode_create_blob)
>   #define DRM_IOCTL_MODE_DESTROYPROPBLOB	DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
>   
> +#define DRM_IOCTL_SYNCOBJ_CREATE	DRM_IOWR(0xBF, struct drm_syncobj_create)
> +#define DRM_IOCTL_SYNCOBJ_DESTROY	DRM_IOWR(0xC0, struct drm_syncobj_destroy)
> +#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD	DRM_IOWR(0xC1, struct drm_syncobj_handle)
> +#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE	DRM_IOWR(0xC2, struct drm_syncobj_handle)
> +
>   /**
>    * Device specific ioctls should only be in their respective headers
>    * The device specific ioctl range is from 0x40 to 0x9f.

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

Re: [PATCH 1/5] drm: introduce sync objects

[zhoucm1]

On 2017年04月26日 11:28, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> Sync objects are new toplevel drm object, that contain a
> pointer to a fence. This fence can be updated via command
> submission ioctls via drivers.
>
> There is also a generic wait obj API modelled on the vulkan
> wait API (with code modelled on some amdgpu code).
>
> These objects can be converted to an opaque fd that can be
> passes between processes.
>
> TODO: define sync_file interaction.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>   Documentation/gpu/drm-internals.rst |   3 +
>   Documentation/gpu/drm-mm.rst        |   6 +
>   drivers/gpu/drm/Makefile            |   2 +-
>   drivers/gpu/drm/drm_fops.c          |   8 +
>   drivers/gpu/drm/drm_internal.h      |  13 ++
>   drivers/gpu/drm/drm_ioctl.c         |  12 ++
>   drivers/gpu/drm/drm_syncobj.c       | 374 ++++++++++++++++++++++++++++++++++++
>   include/drm/drmP.h                  |   5 +
>   include/drm/drm_drv.h               |   1 +
>   include/drm/drm_syncobj.h           | 104 ++++++++++
>   include/uapi/drm/drm.h              |  25 +++
>   11 files changed, 552 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/drm_syncobj.c
>   create mode 100644 include/drm/drm_syncobj.h
>
> diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
> index e35920d..2ea3bce 100644
> --- a/Documentation/gpu/drm-internals.rst
> +++ b/Documentation/gpu/drm-internals.rst
> @@ -98,6 +98,9 @@ DRIVER_ATOMIC
>       implement appropriate obj->atomic_get_property() vfuncs for any
>       modeset objects with driver specific properties.
>   
> +DRIVER_SYNCOBJ
> +    Driver support drm sync objects.
> +
>   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..44ef903 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -142,4 +142,17 @@ static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
>   {
>   	return 0;
>   }
> +
>   #endif
> +
> +/* drm_syncobj.c */
> +void drm_syncobj_open(struct drm_file *file_private);
> +void drm_syncobj_release(struct drm_file *file_private);
> +int drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
> +			     struct drm_file *file_private);
> +int drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
> +			      struct drm_file *file_private);
> +int drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> +				   struct drm_file *file_private);
> +int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> +				   struct drm_file *file_private);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index a7c61c2..6da7adc 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -240,6 +240,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>   		req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0;
>   		req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0;
>   		return 0;
> +	case DRM_CAP_SYNCOBJ:
> +		req->value = drm_core_check_feature(dev, DRIVER_SYNCOBJ);
> +		return 0;
>   	}
>   
>   	/* Other caps only work with KMS drivers */
> @@ -641,6 +644,15 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +
> +	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_CREATE, drm_syncobj_create_ioctl,
> +		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_DESTROY, drm_syncobj_destroy_ioctl,
> +		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD, drm_syncobj_handle_to_fd_ioctl,
> +		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
> +		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
>   };
>   
>   #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> new file mode 100644
> index 0000000..e6a99d4
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -0,0 +1,374 @@
> +/*
> + * 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 a persistent objects,
> + * that contain an optional fence. The fence can be updated with a new
> + * fence, or be NULL.
> + *
> + * syncobj's can be export to fd's and back, these fd's are opaque and
> + * have no other use case, except passing the syncobj between processes.
> + *
> + * TODO: sync_file interactions, waiting
> + *
> + * Their primary use-case is to implement Vulkan fences and semaphores.
> + *
> + * syncobj have a kref reference count, but also have an optional file.
> + * The file is only created once the syncobj is exported.
> + * The file takes a reference on the kref.
> + */
> +
> +#include <drm/drmP.h>
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/anon_inodes.h>
> +
> +#include "drm_internal.h"
> +#include <drm/drm_syncobj.h>
> +
> +static struct drm_syncobj *drm_syncobj_get(struct drm_file *file_private,
> +					   u32 handle)
> +{
> +	struct drm_syncobj *syncobj;
> +
> +	spin_lock(&file_private->syncobj_table_lock);
> +
> +	/* Check if we currently have a reference on the object */
> +	syncobj = idr_find(&file_private->syncobj_idr, handle);
> +	if (syncobj)
> +		drm_syncobj_reference(syncobj);
> +
> +	spin_unlock(&file_private->syncobj_table_lock);
> +
> +	return syncobj;
> +}
> +
> +/**
> + * drm_syncobj_replace_fence - lookup and replace fence in a sync object.
> + * @file_private - drm file private pointer.
> + * @handle - syncobj handle to lookup
> + * @fence - fence to install in sync file.
> + * Returns:
> + * 0 on success, or -EINVAL when the handle doesn't point at a valid sem file.
> + *
> + * This looks up a sync object and replaces the fence on it, freeing
> + * the old one.
> + */
> +int drm_syncobj_replace_fence(struct drm_file *file_private,
> +			      u32 handle,
> +			      struct dma_fence *fence)
> +{
> +	struct drm_syncobj *syncobj = drm_syncobj_get(file_private, handle);
> +	struct dma_fence *old_fence = NULL;
> +
> +	if (!syncobj)
> +		return -EINVAL;
> +
> +	if (fence)
> +		dma_fence_get(fence);
> +	old_fence = xchg(&syncobj->fence, fence);
> +
> +	dma_fence_put(old_fence);
> +	drm_syncobj_unreference(syncobj);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_syncobj_replace_fence);
> +
> +int drm_syncobj_fence_get(struct drm_file *file_private,
> +			  u32 handle,
> +			  struct dma_fence **fence)
> +{
> +	struct drm_syncobj *syncobj = drm_syncobj_get(file_private, handle);
> +	int ret = 0;
> +
> +	if (!syncobj)
> +		return -EINVAL;
> +
> +	*fence = dma_fence_get(syncobj->fence);
> +	if (!*fence) {
> +		ret = -EINVAL;
> +	}
> +	drm_syncobj_unreference(syncobj);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_syncobj_fence_get);
> +
> +void drm_syncobj_free(struct kref *kref)
> +{
> +	struct drm_syncobj *syncobj = container_of(kref,
> +						   struct drm_syncobj,
> +						   refcount);
here fence will leak, should be:
drm_fence_put(syncobj->fence);

Regards,
David Zhou
> +	kfree(syncobj);
> +}
> +
> +static int drm_syncobj_create(struct drm_file *file_private,
> +			      u32 *handle)
> +{
> +	int ret;
> +	struct drm_syncobj *syncobj;
> +
> +	syncobj = kzalloc(sizeof(struct drm_syncobj), GFP_KERNEL);
> +	if (!syncobj)
> +		return -ENOMEM;
> +
> +	mutex_init(&syncobj->file_lock);
> +	kref_init(&syncobj->refcount);
> +
> +	idr_preload(GFP_KERNEL);
> +	spin_lock(&file_private->syncobj_table_lock);
> +	ret = idr_alloc(&file_private->syncobj_idr, syncobj, 1, 0, GFP_NOWAIT);
> +	spin_unlock(&file_private->syncobj_table_lock);
> +
> +	idr_preload_end();
> +
> +	if (ret < 0) {
> +		drm_syncobj_unreference(syncobj);
> +		return ret;
> +	}
> +
> +	*handle = ret;
> +	return 0;
> +}
> +
> +static int drm_syncobj_destroy(struct drm_file *file_private,
> +			       u32 handle)
> +{
> +	struct drm_syncobj *syncobj;
> +
> +	spin_lock(&file_private->syncobj_table_lock);
> +	syncobj = idr_remove(&file_private->syncobj_idr, handle);
> +	spin_unlock(&file_private->syncobj_table_lock);
> +
> +	if (!syncobj)
> +		return -EINVAL;
> +
> +	drm_syncobj_unreference(syncobj);
> +	return 0;
> +}
> +
> +static int drm_syncobj_file_release(struct inode *inode, struct file *file)
> +{
> +	struct drm_syncobj *syncobj = file->private_data;
> +
> +	drm_syncobj_unreference(syncobj);
> +	return 0;
> +}
> +
> +static const struct file_operations drm_syncobj_file_fops = {
> +	.release = drm_syncobj_file_release,
> +};
> +
> +static int drm_syncobj_alloc_file_locked(struct drm_syncobj *syncobj)
> +{
> +	struct file *file = anon_inode_getfile("syncobj_file",
> +					       &drm_syncobj_file_fops,
> +					       syncobj, 0);
> +	if (IS_ERR(file))
> +		return PTR_ERR(file);
> +
> +	/* the file holds a reference to the object */
> +	drm_syncobj_reference(syncobj);
> +	syncobj->file = file;
> +
> +	return 0;
> +}
> +
> +static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
> +				    u32 handle, int *p_fd)
> +{
> +	struct drm_syncobj *syncobj = drm_syncobj_get(file_private, handle);
> +	int ret;
> +	int fd;
> +
> +	if (!syncobj)
> +		return -EINVAL;
> +
> +	fd = get_unused_fd_flags(O_CLOEXEC);
> +	if (fd < 0) {
> +		drm_syncobj_unreference(syncobj);
> +		return fd;
> +	}
> +
> +	mutex_lock(&syncobj->file_lock);
> +	if (!syncobj->file) {
> +		ret = drm_syncobj_alloc_file_locked(syncobj);
> +		if (ret)
> +			goto out_unlock;
> +	}
> +	fd_install(fd, syncobj->file);
> +	mutex_unlock(&syncobj->file_lock);
> +	drm_syncobj_unreference(syncobj);
> +	*p_fd = fd;
> +	return 0;
> +out_unlock:
> +	put_unused_fd(fd);
> +	mutex_unlock(&syncobj->file_lock);
> +	drm_syncobj_unreference(syncobj);
> +	return ret;
> +}
> +
> +static struct drm_syncobj *drm_syncobj_fdget(int fd)
> +{
> +	struct file *file = fget(fd);
> +
> +	if (!file)
> +		return NULL;
> +	if (file->f_op != &drm_syncobj_file_fops)
> +		goto err;
> +
> +	return file->private_data;
> +err:
> +	fput(file);
> +	return NULL;
> +};
> +
> +static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
> +				    int fd, u32 *handle)
> +{
> +	struct drm_syncobj *syncobj = drm_syncobj_fdget(fd);
> +	int ret;
> +
> +	if (!syncobj)
> +		return -EINVAL;
> +
> +	/* take a reference to put in the idr */
> +	drm_syncobj_reference(syncobj);
> +
> +	idr_preload(GFP_KERNEL);
> +	spin_lock(&file_private->syncobj_table_lock);
> +	ret = idr_alloc(&file_private->syncobj_idr, syncobj, 1, 0, GFP_NOWAIT);
> +	spin_unlock(&file_private->syncobj_table_lock);
> +	idr_preload_end();
> +
> +	if (ret < 0) {
> +		fput(syncobj->file);
> +		return ret;
> +	}
> +	*handle = ret;
> +	return 0;
> +}
> +
> +/**
> + * drm_syncobj_open - initalizes syncobj file-private structures at devnode open time
> + * @dev: drm_device which is being opened by userspace
> + * @file_private: drm file-private structure to set up
> + *
> + * Called at device open time, sets up the structure for handling refcounting
> + * of sync objects.
> + */
> +void
> +drm_syncobj_open(struct drm_file *file_private)
> +{
> +	idr_init(&file_private->syncobj_idr);
> +	spin_lock_init(&file_private->syncobj_table_lock);
> +}
> +
> +static int
> +drm_syncobj_release_handle(int id, void *ptr, void *data)
> +{
> +	struct drm_syncobj *syncobj = ptr;
> +
> +	drm_syncobj_unreference(syncobj);
> +	return 0;
> +}
> +
> +/**
> + * drm_syncobj_release - release file-private sync object resources
> + * @dev: drm_device which is being closed by userspace
> + * @file_private: drm file-private structure to clean up
> + *
> + * Called at close time when the filp is going away.
> + *
> + * Releases any remaining references on objects by this filp.
> + */
> +void
> +drm_syncobj_release(struct drm_file *file_private)
> +{
> +	idr_for_each(&file_private->syncobj_idr,
> +		     &drm_syncobj_release_handle, file_private);
> +	idr_destroy(&file_private->syncobj_idr);
> +}
> +
> +int
> +drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
> +			 struct drm_file *file_private)
> +{
> +	struct drm_syncobj_create *args = data;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> +		return -ENODEV;
> +
> +	/* no valid flags yet */
> +	if (args->flags)
> +		return -EINVAL;
> +
> +	return drm_syncobj_create(file_private,
> +				  &args->handle);
> +}
> +
> +int
> +drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
> +			  struct drm_file *file_private)
> +{
> +	struct drm_syncobj_destroy *args = data;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> +		return -ENODEV;
> +
> +	/* make sure padding is empty */
> +	if (args->pad)
> +		return -EINVAL;
> +	return drm_syncobj_destroy(file_private, args->handle);
> +}
> +
> +int
> +drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> +				   struct drm_file *file_private)
> +{
> +	struct drm_syncobj_handle *args = data;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> +		return -ENODEV;
> +
> +	return drm_syncobj_handle_to_fd(file_private, args->handle,
> +					&args->fd);
> +}
> +
> +int
> +drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> +				   struct drm_file *file_private)
> +{
> +	struct drm_syncobj_handle *args = data;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> +		return -ENODEV;
> +
> +	return drm_syncobj_fd_to_handle(file_private, args->fd,
> +					&args->handle);
> +}
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 6105c05..1d48f6f 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -405,6 +405,11 @@ struct drm_file {
>   	/** Lock for synchronization of access to object_idr. */
>   	spinlock_t table_lock;
>   
> +	/** Mapping of sync object handles to object pointers. */
> +	struct idr syncobj_idr;
> +	/** Lock for synchronization of access to syncobj_idr. */
> +	spinlock_t syncobj_table_lock;
> +
>   	struct file *filp;
>   	void *driver_priv;
>   
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 5699f42..48ff06b 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -53,6 +53,7 @@ struct drm_mode_create_dumb;
>   #define DRIVER_RENDER			0x8000
>   #define DRIVER_ATOMIC			0x10000
>   #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
> +#define DRIVER_SYNCOBJ                  0x40000
>   
>   /**
>    * struct drm_driver - DRM driver structure
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> new file mode 100644
> index 0000000..3cc42b7
> --- /dev/null
> +++ b/include/drm/drm_syncobj.h
> @@ -0,0 +1,104 @@
> +/*
> + * 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__
> +
> +#include "linux/dma-fence.h"
> +
> +/**
> + * struct drm_syncobj - sync object.
> + *
> + * This structure defines a generic sync object which wraps a dma fence.
> + */
> +struct drm_syncobj {
> +	/**
> +	 * @refcount:
> +	 *
> +	 * Reference count of this object.
> +	 */
> +	struct kref refcount;
> +	/**
> +	 * @fence:
> +	 * NULL or a pointer to the fence bound to this object.
> +	 */
> +	struct dma_fence *fence;
> +	/**
> +	 * @file_lock
> +	 * mutex protecting the struct file pointer.
> +	 */
> +	struct mutex file_lock;
> +	/**
> +	 * @file
> +	 * a file backing for this syncobj.
> +	 */
> +	struct file *file;
> +};
> +
> +void drm_syncobj_free(struct kref *kref);
> +
> +/**
> + * drm_gem_object_reference - acquire a GEM BO reference
> + * @obj: GEM buffer object
> + *
> + * This acquires additional reference to @obj. It is illegal to call this
> + * without already holding a reference. No locks required.
> + */
> +static inline void
> +drm_syncobj_reference(struct drm_syncobj *obj)
> +{
> +	kref_get(&obj->refcount);
> +}
> +
> +/**
> + * __drm_gem_object_unreference - raw function to release a GEM BO reference
> + * @obj: GEM buffer object
> + *
> + * This function is meant to be used by drivers which are not encumbered with
> + * &drm_device.struct_mutex legacy locking and which are using the
> + * gem_free_object_unlocked callback. It avoids all the locking checks and
> + * locking overhead of drm_gem_object_unreference() and
> + * drm_gem_object_unreference_unlocked().
> + *
> + * Drivers should never call this directly in their code. Instead they should
> + * wrap it up into a ``driver_gem_object_unreference(struct driver_gem_object
> + * *obj)`` wrapper function, and use that. Shared code should never call this, to
> + * avoid breaking drivers by accident which still depend upon
> + * &drm_device.struct_mutex locking.
> + */
> +static inline void
> +drm_syncobj_unreference(struct drm_syncobj *obj)
> +{
> +	kref_put(&obj->refcount, drm_syncobj_free);
> +}
> +
> +int drm_syncobj_fence_get(struct drm_file *file_private,
> +			  u32 handle,
> +			  struct dma_fence **fence);
> +int drm_syncobj_replace_fence(struct drm_file *file_private,
> +			      u32 handle,
> +			      struct dma_fence *fence);
> +
> +#endif
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b2c5284..dd0b99c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -647,6 +647,7 @@ struct drm_gem_open {
>   #define DRM_CAP_CURSOR_HEIGHT		0x9
>   #define DRM_CAP_ADDFB2_MODIFIERS	0x10
>   #define DRM_CAP_PAGE_FLIP_TARGET	0x11
> +#define DRM_CAP_SYNCOBJ		0x13
>   
>   /** DRM_IOCTL_GET_CAP ioctl argument type */
>   struct drm_get_cap {
> @@ -696,6 +697,25 @@ struct drm_prime_handle {
>   	__s32 fd;
>   };
>   
> +struct drm_syncobj_create {
> +	__u32 handle;
> +	__u32 flags;
> +};
> +
> +struct drm_syncobj_destroy {
> +	__u32 handle;
> +	__u32 pad;
> +};
> +
> +struct drm_syncobj_handle {
> +	__u32 handle;
> +	/** Flags.. only applicable for handle->fd */
> +	__u32 flags;
> +
> +	__s32 fd;
> +	__u32 pad;
> +};
> +
>   #if defined(__cplusplus)
>   }
>   #endif
> @@ -814,6 +834,11 @@ extern "C" {
>   #define DRM_IOCTL_MODE_CREATEPROPBLOB	DRM_IOWR(0xBD, struct drm_mode_create_blob)
>   #define DRM_IOCTL_MODE_DESTROYPROPBLOB	DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
>   
> +#define DRM_IOCTL_SYNCOBJ_CREATE	DRM_IOWR(0xBF, struct drm_syncobj_create)
> +#define DRM_IOCTL_SYNCOBJ_DESTROY	DRM_IOWR(0xC0, struct drm_syncobj_destroy)
> +#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD	DRM_IOWR(0xC1, struct drm_syncobj_handle)
> +#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE	DRM_IOWR(0xC2, struct drm_syncobj_handle)
> +
>   /**
>    * Device specific ioctls should only be in their respective headers
>    * The device specific ioctl range is from 0x40 to 0x9f.

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

Re: [PATCH 1/5] drm: introduce sync objects

[Chris Wilson]

On Wed, Apr 26, 2017 at 01:28:29PM +1000, Dave Airlie wrote:
> +#include <drm/drmP.h>

I wonder if Daniel has already split everything used here into its own
headers?

> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/anon_inodes.h>
> +
> +#include "drm_internal.h"
> +#include <drm/drm_syncobj.h>
> +
> +static struct drm_syncobj *drm_syncobj_get(struct drm_file *file_private,
> +					   u32 handle)

I would like this exposed to the driver as well, so I can do handle to
syncobj conversion once. (drm_syncobj_find() if we go with kref_get style
naming.)

> +static struct drm_syncobj *drm_syncobj_fdget(int fd)

It will aslo be useful to export the fd -> syncobj function for drivers. In
the interface Jason has for execbuf, we can substitute the handle for an
fd + flag, which may be more convenient for the user.

> +static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
> +				    u32 handle, int *p_fd)
> +{
> +	struct drm_syncobj *syncobj = drm_syncobj_get(file_private, handle);
> +	int ret;
> +	int fd;
> +
> +	if (!syncobj)
> +		return -EINVAL;
> +
> +	fd = get_unused_fd_flags(O_CLOEXEC);
> +	if (fd < 0) {
> +		drm_syncobj_unreference(syncobj);
> +		return fd;
> +	}
> +
> +	mutex_lock(&syncobj->file_lock);
> +	if (!syncobj->file) {

Mutex only being used to ensure syncobj->file is set once? Is the race
likely? If not, we can spare the mutex and just use a try instead:

	if (!syncobj->file) {
		struct file *file;

		file = anon_inode_getfile("syncobj_file",
					  &drm_syncobj_file_fops,
					  syncobj, 0);
		if (IS_ERR(file))
			return PTR_ERR(file);

		drm_syncobj_get(syncobj);
		if (cmpxchg(&syncobj->file, NULL, file) {
			/* lost the race */
			fput(file);
		}
	}

> +	fd_install(fd, syncobj->file);
> +	mutex_unlock(&syncobj->file_lock);
> +	drm_syncobj_unreference(syncobj);
> +	*p_fd = fd;
> +	return 0;
> +out_unlock:
> +	put_unused_fd(fd);
> +	mutex_unlock(&syncobj->file_lock);
> +	drm_syncobj_unreference(syncobj);
> +	return ret;
> +}
> +

> +/**
> + * drm_gem_object_reference - acquire a GEM BO reference
> + * @obj: GEM buffer object
> + *
> + * This acquires additional reference to @obj. It is illegal to call this
> + * without already holding a reference. No locks required.
> + */
> +static inline void
> +drm_syncobj_reference(struct drm_syncobj *obj)
> +{
> +	kref_get(&obj->refcount);

We've been steadily converting to the kref_get style of nomenclature for
drm object reference handling (i.e. drm_syncobj_get, drm_syncobj_put)
-Chris

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

Re: [PATCH 1/5] drm: introduce sync objects

[Dave Airlie]

On 4 May 2017 at 18:16, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Apr 26, 2017 at 01:28:29PM +1000, Dave Airlie wrote:
>> +#include <drm/drmP.h>
>
> I wonder if Daniel has already split everything used here into its own
> headers?

not sure, if drm_file is out there yet. I'll find out when I rebase
this onto something newer I expect.
>> +
>> +static struct drm_syncobj *drm_syncobj_get(struct drm_file *file_private,
>> +                                        u32 handle)
>
> I would like this exposed to the driver as well, so I can do handle to
> syncobj conversion once. (drm_syncobj_find() if we go with kref_get style
> naming.)

Where do you expect to need two lookups? At least for the semaphore APIs,
you have a list of wait and list of signals, the lists should be different, I've
no objections to exporting this later, but it would be easier to just do that
with the first user.

>
>> +static struct drm_syncobj *drm_syncobj_fdget(int fd)
>
> It will aslo be useful to export the fd -> syncobj function for drivers. In
> the interface Jason has for execbuf, we can substitute the handle for an
> fd + flag, which may be more convenient for the user.

Happy once we have a use case for it, I'd rather we didn't expose the syncobj
fd to userspace for anything but sending a syncobj between processes, avoiding
using fd's is one of the main points of this, I'd hate for an API to
demand we use
fd's.

>> +     mutex_lock(&syncobj->file_lock);
>> +     if (!syncobj->file) {
>
> Mutex only being used to ensure syncobj->file is set once? Is the race
> likely? If not, we can spare the mutex and just use a try instead:

It's pretty unlikely, so I've gone ahead and used your code.
>
> We've been steadily converting to the kref_get style of nomenclature for
> drm object reference handling (i.e. drm_syncobj_get, drm_syncobj_put)

Done locally, will resend series soon.

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

Re: [PATCH 1/5] drm: introduce sync objects

[Chris Wilson]

On Tue, May 09, 2017 at 12:26:34PM +1000, Dave Airlie wrote:
> On 4 May 2017 at 18:16, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Wed, Apr 26, 2017 at 01:28:29PM +1000, Dave Airlie wrote:
> >> +#include <drm/drmP.h>
> >
> > I wonder if Daniel has already split everything used here into its own
> > headers?
> 
> not sure, if drm_file is out there yet. I'll find out when I rebase
> this onto something newer I expect.
> >> +
> >> +static struct drm_syncobj *drm_syncobj_get(struct drm_file *file_private,
> >> +                                        u32 handle)
> >
> > I would like this exposed to the driver as well, so I can do handle to
> > syncobj conversion once. (drm_syncobj_find() if we go with kref_get style
> > naming.)
> 
> Where do you expect to need two lookups? At least for the semaphore APIs,
> you have a list of wait and list of signals, the lists should be different, I've
> no objections to exporting this later, but it would be easier to just do that
> with the first user.

i915 being a laggard with an inflexible ioctl struct, we've repurposed a
single array to hold both in/out. From the kernel point of view we have
a handle + a flag to wait/signal/unsignal. In the first pass we add the
waits, in the second pass we clear in-semaphores, signal out-semaphores.
Userspace is able to express its requirements as a set of operations -
I was looking at reusing this api for fence handling as well. The
difference being that we didn't want to clear the in-fences after
waiting.

So i915, one array of mixed in/out, 2 passes (before/after submit).
-Chris

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

Re: [PATCH 1/5] drm: introduce sync objects

[Jason Ekstrand]

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

On Mon, May 8, 2017 at 7:26 PM, Dave Airlie <airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On 4 May 2017 at 18:16, Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org> wrote:
> > On Wed, Apr 26, 2017 at 01:28:29PM +1000, Dave Airlie wrote:
> >> +#include <drm/drmP.h>
> >
> > I wonder if Daniel has already split everything used here into its own
> > headers?
>
> not sure, if drm_file is out there yet. I'll find out when I rebase
> this onto something newer I expect.
> >> +
> >> +static struct drm_syncobj *drm_syncobj_get(struct drm_file
> *file_private,
> >> +                                        u32 handle)
> >
> > I would like this exposed to the driver as well, so I can do handle to
> > syncobj conversion once. (drm_syncobj_find() if we go with kref_get style
> > naming.)
>
> Where do you expect to need two lookups? At least for the semaphore APIs,
> you have a list of wait and list of signals, the lists should be
> different, I've
> no objections to exporting this later, but it would be easier to just do
> that
> with the first user.
>
> >
> >> +static struct drm_syncobj *drm_syncobj_fdget(int fd)
> >
> > It will aslo be useful to export the fd -> syncobj function for drivers.
> In
> > the interface Jason has for execbuf, we can substitute the handle for an
> > fd + flag, which may be more convenient for the user.
>
> Happy once we have a use case for it, I'd rather we didn't expose the
> syncobj
> fd to userspace for anything but sending a syncobj between processes,
> avoiding
> using fd's is one of the main points of this, I'd hate for an API to
> demand we use
> fd's.
>

I'm not sure how useful Chris' use-case is here.  From a userspace
perspective, I don't want to burn any more FDs than I absolutely have to,
so I'll do the FD -> syncobj conversion on import and use the handle from
there on.  For sync_file, using the FD isn't a big deal as it's a one-shot
and we close it as soon as execbuf() is completed.  Permanently
exported/imported VkSemaphores, on the other hand, are re-usable long-lived
objects and the ioctl on import is a smal

[-- Attachment #1.2: Type: text/html, Size: 2878 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: [PATCH 1/5] drm: introduce sync objects

[Sean Paul]

On Wed, Apr 26, 2017 at 01:28:29PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> Sync objects are new toplevel drm object, that contain a
> pointer to a fence. This fence can be updated via command
> submission ioctls via drivers.
> 
> There is also a generic wait obj API modelled on the vulkan
> wait API (with code modelled on some amdgpu code).
> 
> These objects can be converted to an opaque fd that can be
> passes between processes.
> 
> TODO: define sync_file interaction.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

<snip>

> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> new file mode 100644
> index 0000000..3cc42b7
> --- /dev/null
> +++ b/include/drm/drm_syncobj.h

<snip>

> +/**
> + * drm_gem_object_reference - acquire a GEM BO reference
> + * @obj: GEM buffer object
> + *
> + * This acquires additional reference to @obj. It is illegal to call this
> + * without already holding a reference. No locks required.
> + */
> +static inline void
> +drm_syncobj_reference(struct drm_syncobj *obj)
> +{
> +	kref_get(&obj->refcount);
> +}
> +
> +/**
> + * __drm_gem_object_unreference - raw function to release a GEM BO reference
> + * @obj: GEM buffer object
> + *
> + * This function is meant to be used by drivers which are not encumbered with
> + * &drm_device.struct_mutex legacy locking and which are using the
> + * gem_free_object_unlocked callback. It avoids all the locking checks and
> + * locking overhead of drm_gem_object_unreference() and
> + * drm_gem_object_unreference_unlocked().
> + *
> + * Drivers should never call this directly in their code. Instead they should
> + * wrap it up into a ``driver_gem_object_unreference(struct driver_gem_object
> + * *obj)`` wrapper function, and use that. Shared code should never call this, to
> + * avoid breaking drivers by accident which still depend upon
> + * &drm_device.struct_mutex locking.

Lot's of gem_obj copypasta to clean up in the comment here and above

> + */
> +static inline void
> +drm_syncobj_unreference(struct drm_syncobj *obj)
> +{
> +	kref_put(&obj->refcount, drm_syncobj_free);
> +}
> +
> +int drm_syncobj_fence_get(struct drm_file *file_private,
> +			  u32 handle,
> +			  struct dma_fence **fence);
> +int drm_syncobj_replace_fence(struct drm_file *file_private,
> +			      u32 handle,
> +			      struct dma_fence *fence);
> +
> +#endif
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b2c5284..dd0b99c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -647,6 +647,7 @@ struct drm_gem_open {
>  #define DRM_CAP_CURSOR_HEIGHT		0x9
>  #define DRM_CAP_ADDFB2_MODIFIERS	0x10
>  #define DRM_CAP_PAGE_FLIP_TARGET	0x11
> +#define DRM_CAP_SYNCOBJ		0x13
>  
>  /** DRM_IOCTL_GET_CAP ioctl argument type */
>  struct drm_get_cap {
> @@ -696,6 +697,25 @@ struct drm_prime_handle {
>  	__s32 fd;
>  };
>  
> +struct drm_syncobj_create {
> +	__u32 handle;
> +	__u32 flags;
> +};
> +
> +struct drm_syncobj_destroy {
> +	__u32 handle;
> +	__u32 pad;
> +};
> +
> +struct drm_syncobj_handle {
> +	__u32 handle;
> +	/** Flags.. only applicable for handle->fd */
> +	__u32 flags;
> +
> +	__s32 fd;
> +	__u32 pad;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> @@ -814,6 +834,11 @@ extern "C" {
>  #define DRM_IOCTL_MODE_CREATEPROPBLOB	DRM_IOWR(0xBD, struct drm_mode_create_blob)
>  #define DRM_IOCTL_MODE_DESTROYPROPBLOB	DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
>  
> +#define DRM_IOCTL_SYNCOBJ_CREATE	DRM_IOWR(0xBF, struct drm_syncobj_create)
> +#define DRM_IOCTL_SYNCOBJ_DESTROY	DRM_IOWR(0xC0, struct drm_syncobj_destroy)
> +#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD	DRM_IOWR(0xC1, struct drm_syncobj_handle)
> +#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE	DRM_IOWR(0xC2, struct drm_syncobj_handle)
> +
>  /**
>   * Device specific ioctls should only be in their respective headers
>   * The device specific ioctl range is from 0x40 to 0x9f.
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH 2/5] drm/syncobj: add sync obj wait interface.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This interface will allow sync object to be used to back
Vulkan fences. This API is pretty much the vulkan fence waiting
API, and I've ported the code from amdgpu.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_internal.h |   2 +
 drivers/gpu/drm/drm_ioctl.c    |   2 +
 drivers/gpu/drm/drm_syncobj.c  | 162 ++++++++++++++++++++++++++++++++++++++++-
 include/uapi/drm/drm.h         |  11 +++
 4 files changed, 176 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 44ef903..a508ad9 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -156,3 +156,5 @@ 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_wait_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 6da7adc..b142466 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -653,6 +653,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
 		      DRM_UNLOCKED|DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_syncobj_wait_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
index e6a99d4..c24fea0 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1,5 +1,7 @@
 /*
  * Copyright 2017 Red Hat
+ * Parts ported from amdgpu (fence wait code).
+ * Copyright 2016 Advanced Micro Devices, Inc.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -31,10 +33,13 @@
  * that contain an optional fence. The fence can be updated with a new
  * fence, or be NULL.
  *
+ * syncobj's can be waited upon, where it will wait for the underlying
+ * fence.
+ *
  * syncobj's can be export to fd's and back, these fd's are opaque and
  * have no other use case, except passing the syncobj between processes.
  *
- * TODO: sync_file interactions, waiting
+ * TODO: sync_file interactions.
  *
  * Their primary use-case is to implement Vulkan fences and semaphores.
  *
@@ -372,3 +377,158 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 	return drm_syncobj_fd_to_handle(file_private, args->fd,
 					&args->handle);
 }
+
+/**
+ * calc_timeout - calculate jiffies timeout from absolute value
+ *
+ * @timeout_ns: timeout in ns
+ *
+ * Calculate the timeout in jiffies from an absolute timeout in ns.
+ */
+unsigned long calc_wait_timeout(uint64_t timeout_ns)
+{
+        unsigned long timeout_jiffies;
+        ktime_t timeout;
+
+        /* clamp timeout if it's to large */
+        if (((int64_t)timeout_ns) < 0)
+                return MAX_SCHEDULE_TIMEOUT;
+
+        timeout = ktime_sub(ns_to_ktime(timeout_ns), ktime_get());
+        if (ktime_to_ns(timeout) < 0)
+                return 0;
+
+        timeout_jiffies = nsecs_to_jiffies(ktime_to_ns(timeout));
+        /*  clamp timeout to avoid unsigned-> signed overflow */
+        if (timeout_jiffies > MAX_SCHEDULE_TIMEOUT )
+                return MAX_SCHEDULE_TIMEOUT - 1;
+
+        return timeout_jiffies;
+}
+
+static int drm_syncobj_wait_all_fences(struct drm_device *dev,
+				       struct drm_file *file_private,
+				       struct drm_syncobj_wait *wait,
+				       uint32_t *handles)
+{
+	uint32_t i;
+	int ret;
+
+	for (i = 0; i < wait->count_handles; i++) {
+		unsigned long timeout = calc_wait_timeout(wait->timeout_ns);
+		struct dma_fence *fence;
+
+		ret = drm_syncobj_fence_get(file_private, handles[i],
+					    &fence);
+		if (ret)
+			return ret;
+
+		if (!fence)
+			continue;
+
+		ret = dma_fence_wait_timeout(fence, true, timeout);
+
+		dma_fence_put(fence);
+		if (ret < 0)
+			return ret;
+		if (ret == 0)
+			break;
+	}
+
+	wait->out_status = (ret > 0);
+	wait->first_signaled = 0;
+	return 0;
+}
+
+static int drm_syncobj_wait_any_fence(struct drm_device *dev,
+				      struct drm_file *file_private,
+				      struct drm_syncobj_wait *wait,
+				      uint32_t *handles)
+{
+	unsigned long timeout = calc_wait_timeout(wait->timeout_ns);
+	struct dma_fence **array;
+	uint32_t i;
+	int ret;
+	uint32_t first = ~0;
+
+	/* Prepare the fence array */
+	array = kcalloc(wait->count_handles,
+			sizeof(struct dma_fence *), GFP_KERNEL);
+
+	if (array == NULL)
+		return -ENOMEM;
+
+	for (i = 0; i < wait->count_handles; i++) {
+		struct dma_fence *fence;
+
+		ret = drm_syncobj_fence_get(file_private, handles[i],
+					    &fence);
+		if (ret)
+			goto err_free_fence_array;
+		else if (fence)
+			array[i] = fence;
+		else { /* NULL, the fence has been already signaled */
+			ret = 1;
+			goto out;
+		}
+	}
+
+	ret = dma_fence_wait_any_timeout(array, wait->count_handles, true, timeout,
+					 &first);
+	if (ret < 0)
+		goto err_free_fence_array;
+out:
+	wait->out_status = (ret > 0);
+	wait->first_signaled = first;
+	/* set return value 0 to indicate success */
+	ret = 0;
+
+err_free_fence_array:
+	for (i = 0; i < wait->count_handles; i++)
+		dma_fence_put(array[i]);
+	kfree(array);
+
+	return ret;
+}
+
+int
+drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
+		       struct drm_file *file_private)
+{
+	struct drm_syncobj_wait *args = data;
+	uint32_t *handles;
+	int ret = 0;
+
+	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+		return -ENODEV;
+
+	if (args->flags != 0 && args->flags != DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)
+		return -EINVAL;
+
+	if (args->count_handles == 0)
+		return 0;
+
+	/* Get the handles from userspace */
+	handles = kmalloc_array(args->count_handles, sizeof(uint32_t),
+				GFP_KERNEL);
+	if (handles == NULL)
+		return -ENOMEM;
+
+	if (copy_from_user(handles,
+			   (void __user *)(unsigned long)(args->handles),
+			   sizeof(uint32_t) * args->count_handles)) {
+		ret = -EFAULT;
+		goto err_free_handles;
+	}
+
+	if (args->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)
+		ret = drm_syncobj_wait_all_fences(dev, file_private,
+						  args, handles);
+	else
+		ret = drm_syncobj_wait_any_fence(dev, file_private,
+						 args, handles);
+err_free_handles:
+	kfree(handles);
+
+	return ret;
+}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index dd0b99c..7c508d0 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -716,6 +716,16 @@ struct drm_syncobj_handle {
 	__u32 pad;
 };
 
+#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
+struct drm_syncobj_wait {
+	__u64 handles;
+	__u64 timeout_ns;
+	__u32 count_handles;
+	__u32 flags;
+	__u32 out_status;
+	__u32 first_signaled; /* on valid when not waiting all */
+};
+
 #if defined(__cplusplus)
 }
 #endif
@@ -838,6 +848,7 @@ extern "C" {
 #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_WAIT		DRM_IOWR(0xC3, struct drm_syncobj_wait)
 
 /**
  * Device specific ioctls should only be in their respective headers
-- 
2.9.3

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

Re: [PATCH 2/5] drm/syncobj: add sync obj wait interface.

[Chris Wilson]

On Wed, Apr 26, 2017 at 01:28:30PM +1000, Dave Airlie wrote:
> +/**
> + * calc_timeout - calculate jiffies timeout from absolute value
> + *
> + * @timeout_ns: timeout in ns
> + *
> + * Calculate the timeout in jiffies from an absolute timeout in ns.
> + */
> +unsigned long calc_wait_timeout(uint64_t timeout_ns)
> +{
> +        unsigned long timeout_jiffies;
> +        ktime_t timeout;
> +
> +        /* clamp timeout if it's to large */
> +        if (((int64_t)timeout_ns) < 0)
> +                return MAX_SCHEDULE_TIMEOUT;
> +
> +        timeout = ktime_sub(ns_to_ktime(timeout_ns), ktime_get());
> +        if (ktime_to_ns(timeout) < 0)
> +                return 0;
> +
> +        timeout_jiffies = nsecs_to_jiffies(ktime_to_ns(timeout));
> +        /*  clamp timeout to avoid unsigned-> signed overflow */
> +        if (timeout_jiffies > MAX_SCHEDULE_TIMEOUT )
> +                return MAX_SCHEDULE_TIMEOUT - 1;
> +
> +        return timeout_jiffies;
> +}
> +
> +static int drm_syncobj_wait_all_fences(struct drm_device *dev,
> +				       struct drm_file *file_private,
> +				       struct drm_syncobj_wait *wait,
> +				       uint32_t *handles)
> +{
> +	uint32_t i;
> +	int ret;
> +
> +	for (i = 0; i < wait->count_handles; i++) {
> +		unsigned long timeout = calc_wait_timeout(wait->timeout_ns);
> +		struct dma_fence *fence;
> +
> +		ret = drm_syncobj_fence_get(file_private, handles[i],
> +					    &fence);
> +		if (ret)
> +			return ret;
> +
> +		if (!fence)
> +			continue;
> +
> +		ret = dma_fence_wait_timeout(fence, true, timeout);

We are resetting the timeout for each wait, and do not report the
remaining time back to the user. Useful for handling SIGINT and keeping
the maximum waittime intact.

> +		dma_fence_put(fence);
> +		if (ret < 0)
> +			return ret;
> +		if (ret == 0)
> +			break;
> +	}
> +
> +	wait->out_status = (ret > 0);
> +	wait->first_signaled = 0;
> +	return 0;
> +}

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

[PATCH 3/5] drm/syncobj: add sync_file interaction.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This interface allows importing the fence from a sync_file into
an existing drm sync object, or exporting the fence attached to
an existing drm sync object into a new sync file object.

This should only be used to interact with sync files where necessary.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_syncobj.c | 56 +++++++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/drm.h        |  6 +++--
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index c24fea0..89bf120 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -52,6 +52,7 @@
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/anon_inodes.h>
+#include <linux/sync_file.h>
 
 #include "drm_internal.h"
 #include <drm/drm_syncobj.h>
@@ -279,6 +280,48 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
 	return 0;
 }
 
+int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
+				       int fd, int handle)
+{
+	struct dma_fence *fence = sync_file_get_fence(fd);
+	if (!fence)
+		return -EINVAL;
+
+	return drm_syncobj_replace_fence(file_private, handle, fence);
+}
+
+int drm_syncobj_export_sync_file(struct drm_file *file_private,
+				 int handle, int *p_fd)
+{
+	int ret;
+	struct dma_fence *fence;
+	struct sync_file *sync_file;
+	int fd = get_unused_fd_flags(O_CLOEXEC);
+
+	if (fd < 0)
+		return fd;
+
+	ret = drm_syncobj_fence_get(file_private, handle, &fence);
+	if (ret)
+		goto err_put_fd;
+
+	sync_file = sync_file_create(fence);
+	if (!sync_file) {
+		ret = -EINVAL;
+		goto err_fence_put;
+	}
+
+	fd_install(fd, sync_file->file);
+
+	dma_fence_put(fence);
+	*p_fd = fd;
+	return 0;
+err_fence_put:
+	dma_fence_put(fence);
+err_put_fd:
+	put_unused_fd(fd);
+	return ret;
+}
 /**
  * drm_syncobj_open - initalizes syncobj file-private structures at devnode open time
  * @dev: drm_device which is being opened by userspace
@@ -361,6 +404,12 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
 		return -ENODEV;
 
+	if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_FENCE_SYNC_FILE)
+		return drm_syncobj_export_sync_file(file_private, args->handle,
+						    &args->fd);
+	else if (args->flags)
+		return -EINVAL;
+
 	return drm_syncobj_handle_to_fd(file_private, args->handle,
 					&args->fd);
 }
@@ -374,6 +423,13 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
 		return -ENODEV;
 
+	if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE_FENCE)
+		return drm_syncobj_import_sync_file_fence(file_private,
+							  args->fd,
+							  args->handle);
+	else if (args->flags)
+		return -EINVAL;
+
 	return drm_syncobj_fd_to_handle(file_private, args->fd,
 					&args->handle);
 }
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 7c508d0..a06d370 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -707,13 +707,15 @@ struct drm_syncobj_destroy {
 	__u32 pad;
 };
 
+#define DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE_FENCE (1 << 0)
+#define DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_FENCE_SYNC_FILE (1 << 0)
 struct drm_syncobj_handle {
 	__u32 handle;
 	/** Flags.. only applicable for handle->fd */
-	__u32 flags;
+	__u32 fd_flags;
 
 	__s32 fd;
-	__u32 pad;
+	__u32 flags;
 };
 
 #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
-- 
2.9.3

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

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

Re: [PATCH 4/5] amdgpu/cs: split out fence dependency checking

[zhoucm1]

On 2017年04月26日 11:28, Dave Airlie wrote:
> 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,
To consistent, we'd like amdgpu_cs prefix in amdgpu_cs.c, same as other 
patches.

Regards,
David Zhou
> +				    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;
>   		}
>   	}
>   

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

Re: [rfc] drm sync objects (search for spock)

[Christian König]

Am 26.04.2017 um 05:28 schrieb Dave Airlie:
> Okay I've gone around the sun with these a few times, and
> pretty much implemented what I said last week.
>
> This is pretty much a complete revamp.
>
> 1. sync objects are self contained drm objects, they
> have a file reference so can be passed between processes.
>
> 2. Added a sync object wait interface modelled on the vulkan
> fence waiting API.
>
> 3. sync_file interaction is explicitly different than
> opaque fd passing, you import a sync file state into an
> existing syncobj, or create a new sync_file from an
> existing syncobj. This means no touching the sync file code
> at all. \o/

Doesn't that break the Vulkan requirement that if a sync_obj is exported 
to an fd and imported on the other side we get the same sync_obj again?

In other words the fd is exported and imported only once and the 
expectation is that we fence containing it will change on each signaling 
operation.

As far as I can see with the current implementation you get two 
sync_objects on in the exporting process and one in the importing one.

Regards,
Christian.

>
> I haven't used rcu anywhere here, I've used xchg to swap
> fence pointers in the hope that's safe. If this does need
> rcu'ing I suggest we do it in a follow on patch to minimise
> the review pain.
>
> 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: [rfc] drm sync objects (search for spock)

[Dave Airlie]

On 26 April 2017 at 18:45, Christian König <deathsimple@vodafone.de> wrote:
> Am 26.04.2017 um 05:28 schrieb Dave Airlie:
>>
>> Okay I've gone around the sun with these a few times, and
>> pretty much implemented what I said last week.
>>
>> This is pretty much a complete revamp.
>>
>> 1. sync objects are self contained drm objects, they
>> have a file reference so can be passed between processes.
>>
>> 2. Added a sync object wait interface modelled on the vulkan
>> fence waiting API.
>>
>> 3. sync_file interaction is explicitly different than
>> opaque fd passing, you import a sync file state into an
>> existing syncobj, or create a new sync_file from an
>> existing syncobj. This means no touching the sync file code
>> at all. \o/
>
>
> Doesn't that break the Vulkan requirement that if a sync_obj is exported to
> an fd and imported on the other side we get the same sync_obj again?
>
> In other words the fd is exported and imported only once and the expectation
> is that we fence containing it will change on each signaling operation.
>
> As far as I can see with the current implementation you get two sync_objects
> on in the exporting process and one in the importing one.

Are you talking about using sync_file interop for vulkan, then yes
that would be wrong.

But that isn't how this works, see 1. above the sync obj has a 1:1
mapping with an
opaque fd (non-sync_file) that is only used for interprocess passing
of the syncobj.

You can interoperate with sync_files by importing/exporting the
syncobj fence into
and out of them but that aren't meant for passing the fds around, more
for where you
get a sync_file fd from somewhere else and you want to use it and vice-versa.

I'd like to move any drm APIs away from sync_file to avoid the fd wastages, I'd
also like to move the driver specific fences to syncobj where I can.

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

Re: [rfc] drm sync objects (search for spock)

[Christian König]

Am 26.04.2017 um 11:57 schrieb Dave Airlie:
> On 26 April 2017 at 18:45, Christian König <deathsimple@vodafone.de> wrote:
>> Am 26.04.2017 um 05:28 schrieb Dave Airlie:
>>> Okay I've gone around the sun with these a few times, and
>>> pretty much implemented what I said last week.
>>>
>>> This is pretty much a complete revamp.
>>>
>>> 1. sync objects are self contained drm objects, they
>>> have a file reference so can be passed between processes.
>>>
>>> 2. Added a sync object wait interface modelled on the vulkan
>>> fence waiting API.
>>>
>>> 3. sync_file interaction is explicitly different than
>>> opaque fd passing, you import a sync file state into an
>>> existing syncobj, or create a new sync_file from an
>>> existing syncobj. This means no touching the sync file code
>>> at all. \o/
>>
>> Doesn't that break the Vulkan requirement that if a sync_obj is exported to
>> an fd and imported on the other side we get the same sync_obj again?
>>
>> In other words the fd is exported and imported only once and the expectation
>> is that we fence containing it will change on each signaling operation.
>>
>> As far as I can see with the current implementation you get two sync_objects
>> on in the exporting process and one in the importing one.
> Are you talking about using sync_file interop for vulkan, then yes
> that would be wrong.
>
> But that isn't how this works, see 1. above the sync obj has a 1:1
> mapping with an
> opaque fd (non-sync_file) that is only used for interprocess passing
> of the syncobj.

Ah, ok that makes some more sense.

> You can interoperate with sync_files by importing/exporting the
> syncobj fence into
> and out of them but that aren't meant for passing the fds around, more
> for where you
> get a sync_file fd from somewhere else and you want to use it and vice-versa.

I would prefer dealing only with one type of fd in userspace, but that 
approach should work as well.

> I'd like to move any drm APIs away from sync_file to avoid the fd wastages,

That sounds like it make sense, but I would still rather vote for 
extending the sync_file interface than deprecating it with another one.

> I'd also like to move the driver specific fences to syncobj where I can.

I'm pretty sure that is not a good idea.

Beating the sequence based fence values we currently use for CPU sync in 
performance would be pretty hard. E.g. at least on amdgpu we can even 
check those by doing a simple 64bit read from a memory address, without 
IOCTL or any other overhead involved.

Regards,
Christian.

>
> Dave.


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

Re: [rfc] drm sync objects (search for spock)

[Jason Ekstrand]

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

On Wed, Apr 26, 2017 at 7:57 AM, Christian König <deathsimple@vodafone.de>
wrote:

> Am 26.04.2017 um 11:57 schrieb Dave Airlie:
>
>> On 26 April 2017 at 18:45, Christian König <deathsimple@vodafone.de>
>> wrote:
>>
>>> Am 26.04.2017 um 05:28 schrieb Dave Airlie:
>>>
>>>> Okay I've gone around the sun with these a few times, and
>>>> pretty much implemented what I said last week.
>>>>
>>>> This is pretty much a complete revamp.
>>>>
>>>> 1. sync objects are self contained drm objects, they
>>>> have a file reference so can be passed between processes.
>>>>
>>>> 2. Added a sync object wait interface modelled on the vulkan
>>>> fence waiting API.
>>>>
>>>> 3. sync_file interaction is explicitly different than
>>>> opaque fd passing, you import a sync file state into an
>>>> existing syncobj, or create a new sync_file from an
>>>> existing syncobj. This means no touching the sync file code
>>>> at all. \o/
>>>>
>>>
I've done a quick scan over the patches and I like the API.  It almost
looks as if Santa read my wish list. :-)

That said, it was a "quick scan" so don't take this as any sort of actual
code review.  It'll probably be a little while before I get a chance to sit
down and look at i915 again but things seem reasonable.


> Doesn't that break the Vulkan requirement that if a sync_obj is exported to
>>> an fd and imported on the other side we get the same sync_obj again?
>>>
>>> In other words the fd is exported and imported only once and the
>>> expectation
>>> is that we fence containing it will change on each signaling operation.
>>>
>>> As far as I can see with the current implementation you get two
>>> sync_objects
>>> on in the exporting process and one in the importing one.
>>>
>> Are you talking about using sync_file interop for vulkan, then yes
>> that would be wrong.
>>
>> But that isn't how this works, see 1. above the sync obj has a 1:1
>> mapping with an
>> opaque fd (non-sync_file) that is only used for interprocess passing
>> of the syncobj.
>>
>
> Ah, ok that makes some more sense.
>
> You can interoperate with sync_files by importing/exporting the
>> syncobj fence into
>> and out of them but that aren't meant for passing the fds around, more
>> for where you
>> get a sync_file fd from somewhere else and you want to use it and
>> vice-versa.
>>
>
> I would prefer dealing only with one type of fd in userspace, but that
> approach should work as well.
>
> I'd like to move any drm APIs away from sync_file to avoid the fd wastages,
>>
>
> That sounds like it make sense, but I would still rather vote for
> extending the sync_file interface than deprecating it with another one.
>
> I'd also like to move the driver specific fences to syncobj where I can.
>>
>
> I'm pretty sure that is not a good idea.
>
> Beating the sequence based fence values we currently use for CPU sync in
> performance would be pretty hard. E.g. at least on amdgpu we can even check
> those by doing a simple 64bit read from a memory address, without IOCTL or
> any other overhead involved.
>
> Regards,
> Christian.
>
>
>
>> Dave.
>>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

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

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

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

[PATCH 5/5] amdgpu: use drm sync objects for shared semaphores (v4)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This creates a new command submission chunk for amdgpu
to add in and out 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 syncobj pre submission dependencies,
and one for post submission sync obj 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.

In theory VkFences could be backed with sync objects and
just get passed into the cs as syncobj handles as well.

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

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

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 81 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
 include/uapi/drm/amdgpu_drm.h           |  6 +++
 3 files changed, 87 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..e86c832 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_SYNCOBJ_IN:
+		case AMDGPU_CHUNK_ID_SYNCOBJ_OUT:
 			break;
 
 		default:
@@ -1008,6 +1011,40 @@ static int amdgpu_process_fence_dep(struct amdgpu_cs_parser *p,
 	return 0;
 }
 
+static int amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
+						 uint32_t handle)
+{
+	int r;
+	struct dma_fence *fence;
+	r = drm_syncobj_fence_get(p->filp, handle, &fence);
+	if (r)
+		return r;
+
+	r = amdgpu_sync_fence(p->adev, &p->job->sync, fence);
+	dma_fence_put(fence);
+
+	return r;
+}
+
+static int amdgpu_process_syncobj_in_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_syncobj_lookup_and_add_to_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 +1059,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_SYNCOBJ_IN) {
+			r = amdgpu_process_syncobj_in_dep(p, chunk);
+			if (r)
+				return r;
 		}
 	}
 
 	return 0;
 }
 
+static int amdgpu_process_syncobj_out_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_SYNCOBJ_OUT) {
+			r = amdgpu_process_syncobj_out_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 +1134,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..9881e27 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_SYNCOBJ_IN      0x04
+#define AMDGPU_CHUNK_ID_SYNCOBJ_OUT     0x05
 
 struct drm_amdgpu_cs_chunk {
 	__u32		chunk_id;
@@ -454,6 +456,10 @@ struct drm_amdgpu_cs_chunk_fence {
 	__u32 offset;
 };
 
+struct drm_amdgpu_cs_chunk_sem {
+	__u32 handle;
+};
+
 struct drm_amdgpu_cs_chunk_data {
 	union {
 		struct drm_amdgpu_cs_chunk_ib		ib_data;
-- 
2.9.3

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

drm syncobj - can we get some r-b/a-bs?

[Dave Airlie]

Okay I'm not convinced this is going to get any better out of tree,
I've polished what I can, and fixed up the last few comments from people,

I'd like to rebase on drm-misc probably at some point and send a pull
request for it.

This mostly just addresses things around naming that Chris pointed out.

Dave.

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

[PATCH 5/5] amdgpu: use drm sync objects for shared semaphores (v4)

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This creates a new command submission chunk for amdgpu
to add in and out 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 syncobj pre submission dependencies,
and one for post submission sync obj 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.

In theory VkFences could be backed with sync objects and
just get passed into the cs as syncobj handles as well.

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

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

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 81 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
 include/uapi/drm/amdgpu_drm.h           |  6 +++
 3 files changed, 87 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..e86c832 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_SYNCOBJ_IN:
+		case AMDGPU_CHUNK_ID_SYNCOBJ_OUT:
 			break;
 
 		default:
@@ -1008,6 +1011,40 @@ static int amdgpu_process_fence_dep(struct amdgpu_cs_parser *p,
 	return 0;
 }
 
+static int amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
+						 uint32_t handle)
+{
+	int r;
+	struct dma_fence *fence;
+	r = drm_syncobj_fence_get(p->filp, handle, &fence);
+	if (r)
+		return r;
+
+	r = amdgpu_sync_fence(p->adev, &p->job->sync, fence);
+	dma_fence_put(fence);
+
+	return r;
+}
+
+static int amdgpu_process_syncobj_in_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_syncobj_lookup_and_add_to_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 +1059,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_SYNCOBJ_IN) {
+			r = amdgpu_process_syncobj_in_dep(p, chunk);
+			if (r)
+				return r;
 		}
 	}
 
 	return 0;
 }
 
+static int amdgpu_process_syncobj_out_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_SYNCOBJ_OUT) {
+			r = amdgpu_process_syncobj_out_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 +1134,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..9881e27 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_SYNCOBJ_IN      0x04
+#define AMDGPU_CHUNK_ID_SYNCOBJ_OUT     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 5/5] amdgpu: use drm sync objects for shared semaphores (v4)

[zhoucm1]

1. generally, functions in amdgpu_cs.c should be with amdgpu_cs_ as prefix.
2. If I'm not wrong to your proposal, SYNCOBJ_IN is to semaphore wait 
while SYNCOBJ_OUT is to semaphore signal. SYNCOBJ_IN/OUT both are based 
on command submission ioctl, that means user space must generate CS when 
using semaphore? but with my understand, they should not be dependent 
with that, they can be used independently, right?

Anything I missed, if yes, pls correct me.

+		case AMDGPU_CHUNK_ID_SYNCOBJ_IN:
+		case AMDGPU_CHUNK_ID_SYNCOBJ_OUT:

Regards,
David Zhou
On 2017年05月12日 08:34, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This creates a new command submission chunk for amdgpu
> to add in and out 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 syncobj pre submission dependencies,
> and one for post submission sync obj 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.
>
> In theory VkFences could be backed with sync objects and
> just get passed into the cs as syncobj handles as well.
>
> NOTE: this interface addition needs a version bump to expose
> it to userspace.
>
> v1.1: keep file reference on import.
> v2: move to using syncobjs
> v2.1: change some APIs to just use p pointer.
> v3: make more robust against CS failures, we now add the
> wait sems but only remove them once the CS job has been
> submitted.
> v4: rewrite names of API and base on new syncobj code.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 81 ++++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
>   include/uapi/drm/amdgpu_drm.h           |  6 +++
>   3 files changed, 87 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..e86c832 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_SYNCOBJ_IN:
> +		case AMDGPU_CHUNK_ID_SYNCOBJ_OUT:
>   			break;
>   
>   		default:
> @@ -1008,6 +1011,40 @@ static int amdgpu_process_fence_dep(struct amdgpu_cs_parser *p,
>   	return 0;
>   }
>   
> +static int amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
> +						 uint32_t handle)
> +{
> +	int r;
> +	struct dma_fence *fence;
> +	r = drm_syncobj_fence_get(p->filp, handle, &fence);
> +	if (r)
> +		return r;
> +
> +	r = amdgpu_sync_fence(p->adev, &p->job->sync, fence);
> +	dma_fence_put(fence);
> +
> +	return r;
> +}
> +
> +static int amdgpu_process_syncobj_in_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_syncobj_lookup_and_add_to_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 +1059,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_SYNCOBJ_IN) {
> +			r = amdgpu_process_syncobj_in_dep(p, chunk);
> +			if (r)
> +				return r;
>   		}
>   	}
>   
>   	return 0;
>   }
>   
> +static int amdgpu_process_syncobj_out_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_SYNCOBJ_OUT) {
> +			r = amdgpu_process_syncobj_out_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 +1134,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..9881e27 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_SYNCOBJ_IN      0x04
> +#define AMDGPU_CHUNK_ID_SYNCOBJ_OUT     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;

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

Re: [PATCH 5/5] amdgpu: use drm sync objects for shared semaphores (v4)

[Dave Airlie]

On 12 May 2017 at 13:34, zhoucm1 <david1.zhou@amd.com> wrote:
> 1. generally, functions in amdgpu_cs.c should be with amdgpu_cs_ as prefix.

Okay I've fixed this and previous patch up locally.

> 2. If I'm not wrong to your proposal, SYNCOBJ_IN is to semaphore wait while
> SYNCOBJ_OUT is to semaphore signal. SYNCOBJ_IN/OUT both are based on command
> submission ioctl, that means user space must generate CS when using
> semaphore?  but with my understand, they should not be dependent with that,
> they can be used independently, right?

Yes in is WAIT and out is signal, however OUT could also be used to
write a syncobj as a fence if needed, hence why I moved away from
semaphore naming.

The only place I can see them being used independently is a possible
signal operation after present, due not being able to pass the
semaphores over dri3 yet. I think I've said this before and Christian
has confirmed that doing anything with semaphores not via the command
submission ioctl is going to be messy as they have to queue jobs in
the scheduler, so if we need to tune the command submission ioctl to
take empty CS or add a flag to just do semaphore operations we should
do so in the future when we have a clear use case for it (and we see
the need to optimise for it).

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

Re: [PATCH 5/5] amdgpu: use drm sync objects for shared semaphores (v4)

[zhoucm1]

On 2017年05月12日 12:17, Dave Airlie wrote:
> On 12 May 2017 at 13:34, zhoucm1 <david1.zhou@amd.com> wrote:
>> 1. generally, functions in amdgpu_cs.c should be with amdgpu_cs_ as prefix.
> Okay I've fixed this and previous patch up locally.
>
>> 2. If I'm not wrong to your proposal, SYNCOBJ_IN is to semaphore wait while
>> SYNCOBJ_OUT is to semaphore signal. SYNCOBJ_IN/OUT both are based on command
>> submission ioctl, that means user space must generate CS when using
>> semaphore?  but with my understand, they should not be dependent with that,
>> they can be used independently, right?
> Yes in is WAIT and out is signal, however OUT could also be used to
> write a syncobj as a fence if needed, hence why I moved away from
> semaphore naming.
>
> The only place I can see them being used independently is a possible
> signal operation after present, due not being able to pass the
> semaphores over dri3 yet. I think I've said this before and Christian
> has confirmed that doing anything with semaphores not via the command
> submission ioctl is going to be messy as they have to queue jobs in
> the scheduler, so if we need to tune the command submission ioctl to
> take empty CS or add a flag to just do semaphore operations we should
> do so in the future when we have a clear use case for it (and we see
> the need to optimise for it).
I see.
+David Mao and Jacob to aware, they are expert of Vulkan, if they have 
no concern, It's ok.

Regards,
David Zhou
>
> Dave.

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

Re: [PATCH 5/5] amdgpu: use drm sync objects for shared semaphores (v4)

[Christian König]

Am 12.05.2017 um 02:34 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
>
> This creates a new command submission chunk for amdgpu
> to add in and out 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 syncobj pre submission dependencies,
> and one for post submission sync obj 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.
>
> In theory VkFences could be backed with sync objects and
> just get passed into the cs as syncobj handles as well.
>
> NOTE: this interface addition needs a version bump to expose
> it to userspace.
>
> v1.1: keep file reference on import.
> v2: move to using syncobjs
> v2.1: change some APIs to just use p pointer.
> v3: make more robust against CS failures, we now add the
> wait sems but only remove them once the CS job has been
> submitted.
> v4: rewrite names of API and base on new syncobj code.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 81 ++++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
>   include/uapi/drm/amdgpu_drm.h           |  6 +++
>   3 files changed, 87 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..e86c832 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_SYNCOBJ_IN:
> +		case AMDGPU_CHUNK_ID_SYNCOBJ_OUT:
>   			break;
>   
>   		default:
> @@ -1008,6 +1011,40 @@ static int amdgpu_process_fence_dep(struct amdgpu_cs_parser *p,
>   	return 0;
>   }
>   
> +static int amdgpu_syncobj_lookup_and_add_to_sync(struct amdgpu_cs_parser *p,
> +						 uint32_t handle)
> +{
> +	int r;
> +	struct dma_fence *fence;
> +	r = drm_syncobj_fence_get(p->filp, handle, &fence);
> +	if (r)
> +		return r;
> +
> +	r = amdgpu_sync_fence(p->adev, &p->job->sync, fence);
> +	dma_fence_put(fence);
> +
> +	return r;
> +}
> +
> +static int amdgpu_process_syncobj_in_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_syncobj_lookup_and_add_to_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 +1059,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_SYNCOBJ_IN) {
> +			r = amdgpu_process_syncobj_in_dep(p, chunk);
> +			if (r)
> +				return r;
>   		}
>   	}
>   
>   	return 0;
>   }
>   
> +static int amdgpu_process_syncobj_out_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_SYNCOBJ_OUT) {
> +			r = amdgpu_process_syncobj_out_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 +1134,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);

When userspace provided an invalid out sync handle we return an error 
here, but the job would have been pushed to the hardware already.

Better move this before amd_sched_entity_push_job() and abort gracefully 
when this happens.

Apart from that the patch looks good to me,
Christian.

>   }
>   
>   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..9881e27 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_SYNCOBJ_IN      0x04
> +#define AMDGPU_CHUNK_ID_SYNCOBJ_OUT     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;


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