* [RFC] DRM synchronisation objects
@ 2017-04-04 4:27 Dave Airlie
2017-04-04 4:27 ` [PATCH 1/8] sync_file: add type/flags to sync file object creation Dave Airlie
` (10 more replies)
0 siblings, 11 replies; 31+ messages in thread
From: Dave Airlie @ 2017-04-04 4:27 UTC (permalink / raw)
To: dri-devel
This series enhances my previous semaphore work on for amdgpu,
with a generic DRM sync object. (drm_syncobj).
It first enhances sync_file to have a type/flags so we can have
different semantics for different sync files, and a wait
to retrieve the type of sync_file for userspace.
Then it adds drm sync objects which are just a drm wrapper around
a sync_file object, allowing creation/info/destroy and import/export
of the objects.
Next it enhances sync_file to have semaphore semantics for Vulkan.
Finally it adds amdgpu support to it's command submission paths to
use the new code.
I've hopefully fixed up the things pointed out in the last
review of the sync_file fence changes, I do wonder if we should
just block poll on semaphore objects as currently I've no use
case for this.
Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 1/8] sync_file: add type/flags to sync file object creation.
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
@ 2017-04-04 4:27 ` Dave Airlie
2017-04-04 7:08 ` Daniel Vetter
2017-04-04 4:27 ` [PATCH 2/8] sync_file: export some interfaces needed by drm sync objects Dave Airlie
` (9 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Dave Airlie @ 2017-04-04 4:27 UTC (permalink / raw)
To: dri-devel
From: Dave Airlie <airlied@redhat.com>
This allows us to create sync files with different semantics,
and clearly define the interoperation between them it also
provides flags to allow for tweaks on those semantics.
This provides a validation interface for drivers that accept
types from userspace so they can return EINVAL instead of ENOMEM.
This provides an ioctl for userspace to retrieve the type/flags
of an object it may recieve from somewhere else.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/dma-buf/sw_sync.c | 2 +-
drivers/dma-buf/sync_file.c | 62 +++++++++++++++++++++++++++++++++++++++---
drivers/gpu/drm/drm_atomic.c | 2 +-
include/linux/sync_file.h | 9 +++++-
include/uapi/linux/sync_file.h | 27 ++++++++++++++++++
5 files changed, 95 insertions(+), 7 deletions(-)
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 69c5ff3..1c47de6 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -315,7 +315,7 @@ static long sw_sync_ioctl_create_fence(struct sync_timeline *obj,
goto err;
}
- sync_file = sync_file_create(&pt->base);
+ sync_file = sync_file_create(&pt->base, SYNC_FILE_TYPE_FENCE, 0);
dma_fence_put(&pt->base);
if (!sync_file) {
err = -ENOMEM;
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2321035..b33af9d 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -28,9 +28,32 @@
static const struct file_operations sync_file_fops;
-static struct sync_file *sync_file_alloc(void)
+/**
+ * sync_file_validate_type_flags - validate type/flags for support
+ * @type: type of sync file object
+ * @flags: flags to sync object.
+ *
+ * Validates the flags are correct so userspace can get a more
+ * detailed error type.
+ */
+int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
+{
+ if (flags)
+ return -EINVAL;
+ if (type != SYNC_FILE_TYPE_FENCE)
+ return -EINVAL;
+ return 0;
+}
+EXPORT_SYMBOL(sync_file_validate_type_flags);
+
+static struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
{
struct sync_file *sync_file;
+ int ret;
+
+ ret = sync_file_validate_type_flags(type, flags);
+ if (ret)
+ return NULL;
sync_file = kzalloc(sizeof(*sync_file), GFP_KERNEL);
if (!sync_file)
@@ -47,6 +70,8 @@ static struct sync_file *sync_file_alloc(void)
INIT_LIST_HEAD(&sync_file->cb.node);
+ sync_file->type = type;
+ sync_file->flags = flags;
return sync_file;
err:
@@ -72,11 +97,13 @@ static void fence_check_cb_func(struct dma_fence *f, struct dma_fence_cb *cb)
* sync_file can be released with fput(sync_file->file). Returns the
* sync_file or NULL in case of error.
*/
-struct sync_file *sync_file_create(struct dma_fence *fence)
+struct sync_file *sync_file_create(struct dma_fence *fence,
+ uint32_t type,
+ uint32_t flags)
{
struct sync_file *sync_file;
- sync_file = sync_file_alloc();
+ sync_file = sync_file_alloc(type, flags);
if (!sync_file)
return NULL;
@@ -200,7 +227,10 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
struct dma_fence **fences, **nfences, **a_fences, **b_fences;
int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
- sync_file = sync_file_alloc();
+ if (a->type != b->type)
+ return NULL;
+
+ sync_file = sync_file_alloc(a->type, a->flags);
if (!sync_file)
return NULL;
@@ -437,6 +467,27 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
return ret;
}
+static long sync_file_ioctl_type(struct sync_file *sync_file,
+ unsigned long arg)
+{
+ struct sync_file_type type;
+ int ret;
+ if (copy_from_user(&type, (void __user *)arg, sizeof(type)))
+ return -EFAULT;
+
+ if (type.flags || type.type)
+ return -EINVAL;
+
+ type.type = sync_file->type;
+ type.flags = sync_file->flags;
+
+ if (copy_to_user((void __user *)arg, &type, sizeof(type)))
+ ret = -EFAULT;
+ else
+ ret = 0;
+ return ret;
+}
+
static long sync_file_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
@@ -449,6 +500,9 @@ static long sync_file_ioctl(struct file *file, unsigned int cmd,
case SYNC_IOC_FILE_INFO:
return sync_file_ioctl_fence_info(sync_file, arg);
+ case SYNC_IOC_TYPE:
+ return sync_file_ioctl_type(sync_file, arg);
+
default:
return -ENOTTY;
}
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a567310..bb5a740 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1917,7 +1917,7 @@ static int setup_out_fence(struct drm_out_fence_state *fence_state,
if (put_user(fence_state->fd, fence_state->out_fence_ptr))
return -EFAULT;
- fence_state->sync_file = sync_file_create(fence);
+ fence_state->sync_file = sync_file_create(fence, SYNC_FILE_TYPE_FENCE, 0);
if (!fence_state->sync_file)
return -ENOMEM;
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 3e3ab84..ede4182 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -20,6 +20,8 @@
#include <linux/spinlock.h>
#include <linux/dma-fence.h>
#include <linux/dma-fence-array.h>
+#include <uapi/linux/sync_file.h>
+
/**
* struct sync_file - sync file to export to the userspace
@@ -30,6 +32,8 @@
* @wq: wait queue for fence signaling
* @fence: fence with the fences in the sync_file
* @cb: fence callback information
+ * @type: sync file type
+ * @flags: flags used to create sync file
*/
struct sync_file {
struct file *file;
@@ -43,11 +47,14 @@ struct sync_file {
struct dma_fence *fence;
struct dma_fence_cb cb;
+ uint32_t type;
+ uint32_t flags;
};
#define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
-struct sync_file *sync_file_create(struct dma_fence *fence);
+int sync_file_validate_type_flags(uint32_t type, uint32_t flags);
+struct sync_file *sync_file_create(struct dma_fence *fence, uint32_t type, uint32_t flags);
struct dma_fence *sync_file_get_fence(int fd);
#endif /* _LINUX_SYNC_H */
diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
index 5b287d6..f439cda 100644
--- a/include/uapi/linux/sync_file.h
+++ b/include/uapi/linux/sync_file.h
@@ -69,6 +69,26 @@ struct sync_file_info {
#define SYNC_IOC_MAGIC '>'
/**
+ * DOC: SYNC_FILE_TYPE_FENCE - fence sync file object
+ *
+ * This sync file is a wrapper around a dma fence or a dma fence array.
+ * It can be merged with another fence sync file object to create a new
+ * merged object.
+ * The fence backing this object cannot be replaced.
+ * This is useful for shared fences.
+ */
+#define SYNC_FILE_TYPE_FENCE 0
+
+/**
+ * struct sync_file_type - data returned from sync file type ioctl
+ * @type: sync_file type
+ * @flags: sync_file creation flags
+ */
+struct sync_file_type {
+ __u32 type;
+ __u32 flags;
+};
+/**
* Opcodes 0, 1 and 2 were burned during a API change to avoid users of the
* old API to get weird errors when trying to handling sync_files. The API
* change happened during the de-stage of the Sync Framework when there was
@@ -94,4 +114,11 @@ struct sync_file_info {
*/
#define SYNC_IOC_FILE_INFO _IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
+/**
+ * DOC: SYNC_IOC_TYPE - get creation type and flags of sync_file.
+ *
+ * Takes a struct sync_file_type. Returns the created values of type and flags.
+ */
+#define SYNC_IOC_TYPE _IOWR(SYNC_IOC_MAGIC, 5, struct sync_file_type)
+
#endif /* _UAPI_LINUX_SYNC_H */
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 2/8] sync_file: export some interfaces needed by drm sync objects.
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
2017-04-04 4:27 ` [PATCH 1/8] sync_file: add type/flags to sync file object creation Dave Airlie
@ 2017-04-04 4:27 ` Dave Airlie
2017-04-04 7:10 ` Daniel Vetter
2017-04-04 4:27 ` [PATCH 3/8] drm: introduce sync objects as sync file objects with no fd Dave Airlie
` (8 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Dave Airlie @ 2017-04-04 4:27 UTC (permalink / raw)
To: dri-devel
From: Dave Airlie <airlied@redhat.com>
These are just alloc and fdget interfaces needed by the drm sync
objects code.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/dma-buf/sync_file.c | 21 +++++++++++++++++++--
include/linux/sync_file.h | 3 ++-
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index b33af9d..153bf03 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -46,7 +46,16 @@ int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
}
EXPORT_SYMBOL(sync_file_validate_type_flags);
-static struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
+/**
+ * sync_file_alloc() - allocate an unfenced sync file
+ * @type: type of sync file object
+ * @flags: creation flags for sync file object
+ *
+ * Creates a sync_file.
+ * The sync_file can be released with fput(sync_file->file).
+ * Returns the sync_file or NULL in case of error.
+ */
+struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
{
struct sync_file *sync_file;
int ret;
@@ -78,6 +87,7 @@ static struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
kfree(sync_file);
return NULL;
}
+EXPORT_SYMBOL(sync_file_alloc);
static void fence_check_cb_func(struct dma_fence *f, struct dma_fence_cb *cb)
{
@@ -118,7 +128,13 @@ struct sync_file *sync_file_create(struct dma_fence *fence,
}
EXPORT_SYMBOL(sync_file_create);
-static struct sync_file *sync_file_fdget(int fd)
+/**
+ * sync_file_fdget - get a reference to the file and return sync_file.
+ * @fd - file descriptor pointing at a sync_file.
+ *
+ * Returns the corresponding sync_file object.
+ */
+struct sync_file *sync_file_fdget(int fd)
{
struct file *file = fget(fd);
@@ -134,6 +150,7 @@ static struct sync_file *sync_file_fdget(int fd)
fput(file);
return NULL;
}
+EXPORT_SYMBOL(sync_file_fdget);
/**
* sync_file_get_fence - get the fence related to the sync_file fd
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index ede4182..e683dd1 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -54,7 +54,8 @@ struct sync_file {
#define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
int sync_file_validate_type_flags(uint32_t type, uint32_t flags);
+struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags);
struct sync_file *sync_file_create(struct dma_fence *fence, uint32_t type, uint32_t flags);
struct dma_fence *sync_file_get_fence(int fd);
-
+struct sync_file *sync_file_fdget(int fd);
#endif /* _LINUX_SYNC_H */
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 3/8] drm: introduce sync objects as sync file objects with no fd
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
2017-04-04 4:27 ` [PATCH 1/8] sync_file: add type/flags to sync file object creation Dave Airlie
2017-04-04 4:27 ` [PATCH 2/8] sync_file: export some interfaces needed by drm sync objects Dave Airlie
@ 2017-04-04 4:27 ` Dave Airlie
2017-04-04 7:42 ` Daniel Vetter
2017-04-04 4:27 ` [PATCH 4/8] sync_file: add a mutex to protect fence and callback members. (v4) Dave Airlie
` (7 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Dave Airlie @ 2017-04-04 4:27 UTC (permalink / raw)
To: dri-devel
From: Dave Airlie <airlied@redhat.com>
Sync objects are new toplevel drm object, that have the same
semantics as sync_file objects, but without requiring an fd
to be consumed to support them.
This patch just enables the DRM interface to create these
objects, it doesn't actually provide any semaphore objects
or new features.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
Documentation/gpu/drm-internals.rst | 5 +
Documentation/gpu/drm-mm.rst | 6 +
drivers/gpu/drm/Makefile | 2 +-
drivers/gpu/drm/drm_fops.c | 8 ++
drivers/gpu/drm/drm_internal.h | 15 +++
drivers/gpu/drm/drm_ioctl.c | 14 ++
drivers/gpu/drm/drm_syncobj.c | 257 ++++++++++++++++++++++++++++++++++++
include/drm/drmP.h | 5 +
include/drm/drm_drv.h | 1 +
include/uapi/drm/drm.h | 27 ++++
10 files changed, 339 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/drm_syncobj.c
diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
index e35920d..743b751 100644
--- a/Documentation/gpu/drm-internals.rst
+++ b/Documentation/gpu/drm-internals.rst
@@ -98,6 +98,11 @@ DRIVER_ATOMIC
implement appropriate obj->atomic_get_property() vfuncs for any
modeset objects with driver specific properties.
+DRIVER_SYNCOBJ
+ Driver support sync objects. These are just sync files that don't
+ use a file descriptor. They can be used to implement Vulkan shared
+ semaphores.
+
Major, Minor and Patchlevel
~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
index f5760b1..28aebe8 100644
--- a/Documentation/gpu/drm-mm.rst
+++ b/Documentation/gpu/drm-mm.rst
@@ -483,3 +483,9 @@ DRM Cache Handling
.. kernel-doc:: drivers/gpu/drm/drm_cache.c
:export:
+
+DRM Sync Objects
+===========================
+
+.. kernel-doc:: drivers/gpu/drm/drm_syncobj.c
+ :export:
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 3ee9579..b5e565c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -16,7 +16,7 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \
drm_framebuffer.o drm_connector.o drm_blend.o \
drm_encoder.o drm_mode_object.o drm_property.o \
drm_plane.o drm_color_mgmt.o drm_print.o \
- drm_dumb_buffers.o drm_mode_config.o
+ drm_dumb_buffers.o drm_mode_config.o drm_syncobj.o
drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
drm-$(CONFIG_DRM_VM) += drm_vm.o
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index afdf5b1..9a61df2 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -219,6 +219,9 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
if (drm_core_check_feature(dev, DRIVER_GEM))
drm_gem_open(dev, priv);
+ if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ drm_syncobj_open(priv);
+
if (drm_core_check_feature(dev, DRIVER_PRIME))
drm_prime_init_file_private(&priv->prime);
@@ -266,6 +269,8 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
out_prime_destroy:
if (drm_core_check_feature(dev, DRIVER_PRIME))
drm_prime_destroy_file_private(&priv->prime);
+ if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ drm_syncobj_release(priv);
if (drm_core_check_feature(dev, DRIVER_GEM))
drm_gem_release(dev, priv);
put_pid(priv->pid);
@@ -400,6 +405,9 @@ int drm_release(struct inode *inode, struct file *filp)
drm_property_destroy_user_blobs(dev, file_priv);
}
+ if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ drm_syncobj_release(file_priv);
+
if (drm_core_check_feature(dev, DRIVER_GEM))
drm_gem_release(dev, file_priv);
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index f37388c..70c27a0 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -142,4 +142,19 @@ static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
{
return 0;
}
+
#endif
+
+/* drm_syncobj.c */
+void drm_syncobj_open(struct drm_file *file_private);
+void drm_syncobj_release(struct drm_file *file_private);
+int drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
+int drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
+int drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
+int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
+int drm_syncobj_info_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index a7c61c2..4a8ed66 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -240,6 +240,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0;
req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0;
return 0;
+ case DRM_CAP_SYNCOBJ:
+ req->value = drm_core_check_feature(dev, DRIVER_SYNCOBJ);
+ return 0;
}
/* Other caps only work with KMS drivers */
@@ -641,6 +644,17 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_CREATE, drm_syncobj_create_ioctl,
+ DRM_UNLOCKED|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_DESTROY, drm_syncobj_destroy_ioctl,
+ DRM_UNLOCKED|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD, drm_syncobj_handle_to_fd_ioctl,
+ DRM_UNLOCKED|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
+ DRM_UNLOCKED|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_INFO, drm_syncobj_info_ioctl,
+ DRM_UNLOCKED|DRM_RENDER_ALLOW),
};
#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
new file mode 100644
index 0000000..a3a1ace
--- /dev/null
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -0,0 +1,257 @@
+/*
+ * Copyright © 2017 Red Hat
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *
+ */
+
+/**
+ * DOC: Overview
+ *
+ * DRM synchronisation objects (syncobj) are wrappers for sync_file objects,
+ * that don't use fd space, and can be converted to/from sync_file fds.
+ *
+ * Depending on the sync object type, they can expose different sync_file
+ * semantics and restrictions.
+ *
+ * Their primary use-case is to implement Vulkan shared semaphores.
+ */
+
+#include <drm/drmP.h>
+#include <linux/sync_file.h>
+#include "drm_internal.h"
+
+static struct sync_file *drm_syncobj_file_lookup(struct drm_file *file_private,
+ u32 handle)
+{
+ struct sync_file *sync_file;
+
+ spin_lock(&file_private->syncobj_table_lock);
+
+ /* Check if we currently have a reference on the object */
+ sync_file = idr_find(&file_private->syncobj_idr, handle);
+
+ spin_unlock(&file_private->syncobj_table_lock);
+
+ return sync_file;
+}
+
+static int drm_syncobj_create(struct drm_file *file_private,
+ unsigned type,
+ unsigned flags, u32 *handle)
+{
+ struct sync_file *sync_file;
+ int ret;
+
+ ret = sync_file_validate_type_flags(type, flags);
+ if (ret)
+ return ret;
+
+ sync_file = sync_file_alloc(type, flags);
+ if (!sync_file)
+ return -ENOMEM;
+
+ idr_preload(GFP_KERNEL);
+ spin_lock(&file_private->syncobj_table_lock);
+ ret = idr_alloc(&file_private->syncobj_idr, sync_file, 1, 0, GFP_NOWAIT);
+ spin_unlock(&file_private->syncobj_table_lock);
+
+ idr_preload_end();
+
+ if (ret < 0)
+ return ret;
+
+ *handle = ret;
+ return 0;
+}
+
+static void drm_syncobj_destroy(struct drm_file *file_private,
+ u32 handle)
+{
+ struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
+ if (!sync_file)
+ return;
+
+ spin_lock(&file_private->syncobj_table_lock);
+ idr_remove(&file_private->syncobj_idr, handle);
+ spin_unlock(&file_private->syncobj_table_lock);
+
+ fput(sync_file->file);
+}
+
+static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
+ u32 handle, int *fd)
+{
+ struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
+ int ret;
+ if (!sync_file)
+ return -EINVAL;
+
+ ret = get_unused_fd_flags(O_CLOEXEC);
+ if (ret < 0)
+ return ret;
+ fd_install(ret, sync_file->file);
+ *fd = ret;
+ return 0;
+}
+
+static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
+ int fd, u32 *handle)
+{
+ struct sync_file *sync_file = sync_file_fdget(fd);
+ int ret;
+ if (!sync_file)
+ return -EINVAL;
+ idr_preload(GFP_KERNEL);
+ spin_lock(&file_private->syncobj_table_lock);
+ ret = idr_alloc(&file_private->syncobj_idr, sync_file, 1, 0, GFP_NOWAIT);
+ spin_unlock(&file_private->syncobj_table_lock);
+ idr_preload_end();
+
+ if (ret < 0) {
+ fput(sync_file->file);
+ return ret;
+ }
+ *handle = ret;
+ return 0;
+}
+
+static int drm_syncobj_info(struct drm_file *file_private,
+ u32 handle, u32 *type, u32 *flags)
+{
+ struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
+
+ if (!sync_file)
+ return -EINVAL;
+ *type = sync_file->type;
+ *flags = sync_file->flags;
+ return 0;
+}
+
+/**
+ * drm_syncobj_open - initalizes syncobj file-private structures at devnode open time
+ * @dev: drm_device which is being opened by userspace
+ * @file_private: drm file-private structure to set up
+ *
+ * Called at device open time, sets up the structure for handling refcounting
+ * of sync objects.
+ */
+void
+drm_syncobj_open(struct drm_file *file_private)
+{
+ idr_init(&file_private->syncobj_idr);
+ spin_lock_init(&file_private->syncobj_table_lock);
+}
+
+static int
+drm_syncobj_release_handle(int id, void *ptr, void *data)
+{
+ struct sync_file *sync_file = ptr;
+
+ fput(sync_file->file);
+ return 0;
+}
+
+/**
+ * drm_syncobj_release - release file-private sync object resources
+ * @dev: drm_device which is being closed by userspace
+ * @file_private: drm file-private structure to clean up
+ *
+ * Called at close time when the filp is going away.
+ *
+ * Releases any remaining references on objects by this filp.
+ */
+void
+drm_syncobj_release(struct drm_file *file_private)
+{
+ idr_for_each(&file_private->syncobj_idr,
+ &drm_syncobj_release_handle, file_private);
+ idr_destroy(&file_private->syncobj_idr);
+}
+
+int
+drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private)
+{
+ struct drm_syncobj_create_info *args = data;
+
+ if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ return -ENODEV;
+
+ return drm_syncobj_create(file_private, args->type,
+ args->flags, &args->handle);
+}
+
+int
+drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private)
+{
+ struct drm_syncobj_destroy *args = data;
+
+ if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ return -ENODEV;
+
+ drm_syncobj_destroy(file_private, args->handle);
+ return 0;
+}
+
+
+int
+drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private)
+{
+ struct drm_syncobj_handle *args = data;
+
+ if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ return -ENODEV;
+
+ return drm_syncobj_handle_to_fd(file_private, args->handle,
+ &args->fd);
+
+}
+
+int
+drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private)
+{
+ struct drm_syncobj_handle *args = data;
+
+ if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ return -ENODEV;
+
+ return drm_syncobj_fd_to_handle(file_private, args->fd,
+ &args->handle);
+
+}
+
+int
+drm_syncobj_info_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private)
+{
+ struct drm_syncobj_create_info *args = data;
+
+ if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ return -ENODEV;
+
+ return drm_syncobj_info(file_private, args->handle,
+ &args->type, &args->flags);
+}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 6105c05..1d48f6f 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -405,6 +405,11 @@ struct drm_file {
/** Lock for synchronization of access to object_idr. */
spinlock_t table_lock;
+ /** Mapping of sync object handles to object pointers. */
+ struct idr syncobj_idr;
+ /** Lock for synchronization of access to syncobj_idr. */
+ spinlock_t syncobj_table_lock;
+
struct file *filp;
void *driver_priv;
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 5699f42..48ff06b 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -53,6 +53,7 @@ struct drm_mode_create_dumb;
#define DRIVER_RENDER 0x8000
#define DRIVER_ATOMIC 0x10000
#define DRIVER_KMS_LEGACY_CONTEXT 0x20000
+#define DRIVER_SYNCOBJ 0x40000
/**
* struct drm_driver - DRM driver structure
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index b2c5284..26b7e86 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -647,6 +647,7 @@ struct drm_gem_open {
#define DRM_CAP_CURSOR_HEIGHT 0x9
#define DRM_CAP_ADDFB2_MODIFIERS 0x10
#define DRM_CAP_PAGE_FLIP_TARGET 0x11
+#define DRM_CAP_SYNCOBJ 0x12
/** DRM_IOCTL_GET_CAP ioctl argument type */
struct drm_get_cap {
@@ -696,6 +697,26 @@ struct drm_prime_handle {
__s32 fd;
};
+struct drm_syncobj_create_info {
+ __u32 handle;
+ __u32 type;
+ __u32 flags;
+ __u32 pad;
+};
+
+struct drm_syncobj_destroy {
+ __u32 handle;
+ __u32 pad;
+};
+
+struct drm_syncobj_handle {
+ __u32 handle;
+ /** Flags.. only applicable for handle->fd */
+ __u32 flags;
+
+ __s32 fd;
+};
+
#if defined(__cplusplus)
}
#endif
@@ -814,6 +835,12 @@ extern "C" {
#define DRM_IOCTL_MODE_CREATEPROPBLOB DRM_IOWR(0xBD, struct drm_mode_create_blob)
#define DRM_IOCTL_MODE_DESTROYPROPBLOB DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
+#define DRM_IOCTL_SYNCOBJ_CREATE DRM_IOWR(0xBF, struct drm_syncobj_create_info)
+#define DRM_IOCTL_SYNCOBJ_DESTROY DRM_IOWR(0xC0, struct drm_syncobj_destroy)
+#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD DRM_IOWR(0xC1, struct drm_syncobj_handle)
+#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE DRM_IOWR(0xC2, struct drm_syncobj_handle)
+#define DRM_IOCTL_SYNCOBJ_INFO DRM_IOWR(0xC3, struct drm_syncobj_create_info)
+
/**
* Device specific ioctls should only be in their respective headers
* The device specific ioctl range is from 0x40 to 0x9f.
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 4/8] sync_file: add a mutex to protect fence and callback members. (v4)
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
` (2 preceding siblings ...)
2017-04-04 4:27 ` [PATCH 3/8] drm: introduce sync objects as sync file objects with no fd Dave Airlie
@ 2017-04-04 4:27 ` Dave Airlie
2017-04-04 7:52 ` Daniel Vetter
2017-04-04 8:07 ` Christian König
2017-04-04 4:27 ` [PATCH 5/8] sync_file: add support for a semaphore object Dave Airlie
` (6 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: Dave Airlie @ 2017-04-04 4:27 UTC (permalink / raw)
To: dri-devel
From: Dave Airlie <airlied@redhat.com>
This patch allows the underlying fence in a sync_file to be changed
or set to NULL. This isn't currently required but for Vulkan
semaphores we need to be able to swap and reset the fence.
In order to faciliate this, it uses rcu to protect the fence,
along with a new mutex. The mutex also protects the callback.
It also checks for NULL when retrieving the rcu protected
fence in case it has been reset.
v1.1: fix the locking (Julia Lawall).
v2: use rcu try one
v3: fix poll to use proper rcu, fixup merge/fill ioctls
to not crash on NULL fence cases.
v4: use rcu in even more places, add missing fput
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/dma-buf/sync_file.c | 135 +++++++++++++++++++++++++++++++++++---------
include/linux/sync_file.h | 6 +-
2 files changed, 112 insertions(+), 29 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 153bf03..6376f6f 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -28,6 +28,10 @@
static const struct file_operations sync_file_fops;
+#define sync_file_held(obj) lockdep_is_held(&(obj)->lock)
+#define sync_file_assert_held(obj) \
+ lockdep_assert_held(&(obj)->lock)
+
/**
* sync_file_validate_type_flags - validate type/flags for support
* @type: type of sync file object
@@ -81,6 +85,10 @@ struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
sync_file->type = type;
sync_file->flags = flags;
+
+ RCU_INIT_POINTER(sync_file->fence, NULL);
+
+ mutex_init(&sync_file->lock);
return sync_file;
err:
@@ -117,7 +125,9 @@ struct sync_file *sync_file_create(struct dma_fence *fence,
if (!sync_file)
return NULL;
- sync_file->fence = dma_fence_get(fence);
+ dma_fence_get(fence);
+
+ RCU_INIT_POINTER(sync_file->fence, fence);
snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
fence->ops->get_driver_name(fence),
@@ -168,13 +178,28 @@ struct dma_fence *sync_file_get_fence(int fd)
if (!sync_file)
return NULL;
- fence = dma_fence_get(sync_file->fence);
+ if (!rcu_access_pointer(sync_file->fence)) {
+ fput(sync_file->file);
+ return NULL;
+ }
+
+ rcu_read_lock();
+ fence = dma_fence_get_rcu_safe(&sync_file->fence);
+ rcu_read_unlock();
+
fput(sync_file->file);
return fence;
}
EXPORT_SYMBOL(sync_file_get_fence);
+static inline struct dma_fence *
+sync_file_get_fence_locked(struct sync_file *sync_file)
+{
+ return rcu_dereference_protected(sync_file->fence,
+ sync_file_held(sync_file));
+}
+
static int sync_file_set_fence(struct sync_file *sync_file,
struct dma_fence **fences, int num_fences)
{
@@ -187,7 +212,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
* we own the reference of the dma_fence_array creation.
*/
if (num_fences == 1) {
- sync_file->fence = fences[0];
+ RCU_INIT_POINTER(sync_file->fence, fences[0]);
kfree(fences);
} else {
array = dma_fence_array_create(num_fences, fences,
@@ -196,24 +221,30 @@ static int sync_file_set_fence(struct sync_file *sync_file,
if (!array)
return -ENOMEM;
- sync_file->fence = &array->base;
+ RCU_INIT_POINTER(sync_file->fence, &array->base);
}
return 0;
}
-static struct dma_fence **get_fences(struct sync_file *sync_file,
+/* must be called with rcu read lock taken */
+static struct dma_fence **get_fences(struct dma_fence **fence,
int *num_fences)
{
- if (dma_fence_is_array(sync_file->fence)) {
- struct dma_fence_array *array = to_dma_fence_array(sync_file->fence);
+ if (!*fence) {
+ *num_fences = 0;
+ return NULL;
+ }
+
+ if (dma_fence_is_array(*fence)) {
+ struct dma_fence_array *array = to_dma_fence_array(*fence);
*num_fences = array->num_fences;
return array->fences;
}
*num_fences = 1;
- return &sync_file->fence;
+ return fence;
}
static void add_fence(struct dma_fence **fences,
@@ -243,18 +274,31 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
struct sync_file *sync_file;
struct dma_fence **fences, **nfences, **a_fences, **b_fences;
int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
+ struct dma_fence *a_fence, *b_fence;
if (a->type != b->type)
return NULL;
- sync_file = sync_file_alloc(a->type, a->flags);
- if (!sync_file)
+ if (!rcu_access_pointer(a->fence) ||
+ !rcu_access_pointer(b->fence))
return NULL;
- a_fences = get_fences(a, &a_num_fences);
- b_fences = get_fences(b, &b_num_fences);
+ rcu_read_lock();
+ a_fence = dma_fence_get_rcu_safe(&a->fence);
+ b_fence = dma_fence_get_rcu_safe(&b->fence);
+ rcu_read_unlock();
+
+ a_fences = get_fences(&a_fence, &a_num_fences);
+ b_fences = get_fences(&b_fence, &b_num_fences);
+ if (!a_num_fences || !b_num_fences)
+ goto put_src_fences;
+
if (a_num_fences > INT_MAX - b_num_fences)
- return NULL;
+ goto put_src_fences;
+
+ sync_file = sync_file_alloc(a->type, a->flags);
+ if (!sync_file)
+ goto put_src_fences;
num_fences = a_num_fences + b_num_fences;
@@ -315,11 +359,16 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
goto err;
}
+ dma_fence_put(a_fence);
+ dma_fence_put(b_fence);
strlcpy(sync_file->name, name, sizeof(sync_file->name));
return sync_file;
err:
fput(sync_file->file);
+put_src_fences:
+ dma_fence_put(a_fence);
+ dma_fence_put(b_fence);
return NULL;
}
@@ -328,10 +377,15 @@ static void sync_file_free(struct kref *kref)
{
struct sync_file *sync_file = container_of(kref, struct sync_file,
kref);
+ struct dma_fence *fence;
+
+ fence = rcu_dereference_protected(sync_file->fence, 1);
+ if (fence) {
+ if (test_bit(POLL_ENABLED, &fence->flags))
+ dma_fence_remove_callback(fence, &sync_file->cb);
+ dma_fence_put(fence);
+ }
- if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
- dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
- dma_fence_put(sync_file->fence);
kfree(sync_file);
}
@@ -346,16 +400,25 @@ static int sync_file_release(struct inode *inode, struct file *file)
static unsigned int sync_file_poll(struct file *file, poll_table *wait)
{
struct sync_file *sync_file = file->private_data;
+ unsigned int ret_val = 0;
+ struct dma_fence *fence;
poll_wait(file, &sync_file->wq, wait);
- if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) {
- if (dma_fence_add_callback(sync_file->fence, &sync_file->cb,
- fence_check_cb_func) < 0)
- wake_up_all(&sync_file->wq);
+ mutex_lock(&sync_file->lock);
+
+ fence = sync_file_get_fence_locked(sync_file);
+ if (fence) {
+ if (!test_and_set_bit(POLL_ENABLED, &fence->flags)) {
+ if (dma_fence_add_callback(fence, &sync_file->cb,
+ fence_check_cb_func) < 0)
+ wake_up_all(&sync_file->wq);
+ }
+ ret_val = dma_fence_is_signaled(fence) ? POLLIN : 0;
}
+ mutex_unlock(&sync_file->lock);
- return dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
+ return ret_val;
}
static long sync_file_ioctl_merge(struct sync_file *sync_file,
@@ -431,6 +494,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
struct sync_file_info info;
struct sync_fence_info *fence_info = NULL;
struct dma_fence **fences;
+ struct dma_fence *fence;
__u32 size;
int num_fences, ret, i;
@@ -440,7 +504,15 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
if (info.flags || info.pad)
return -EINVAL;
- fences = get_fences(sync_file, &num_fences);
+ rcu_read_lock();
+ fence = dma_fence_get_rcu_safe(&sync_file->fence);
+ rcu_read_unlock();
+
+ fences = get_fences(&fence, &num_fences);
+
+ /* if there are no fences in the sync_file just return */
+ if (!num_fences)
+ goto no_fences;
/*
* Passing num_fences = 0 means that userspace doesn't want to
@@ -451,13 +523,17 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
if (!info.num_fences)
goto no_fences;
- if (info.num_fences < num_fences)
- return -EINVAL;
+ if (info.num_fences < num_fences) {
+ ret = -EINVAL;
+ goto out;
+ }
size = num_fences * sizeof(*fence_info);
fence_info = kzalloc(size, GFP_KERNEL);
- if (!fence_info)
- return -ENOMEM;
+ if (!fence_info) {
+ ret = -ENOMEM;
+ goto out;
+ }
for (i = 0; i < num_fences; i++)
sync_fill_fence_info(fences[i], &fence_info[i]);
@@ -470,7 +546,10 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
no_fences:
strlcpy(info.name, sync_file->name, sizeof(info.name));
- info.status = dma_fence_is_signaled(sync_file->fence);
+ if (num_fences)
+ info.status = dma_fence_is_signaled(sync_file->fence);
+ else
+ info.status = -ENOENT;
info.num_fences = num_fences;
if (copy_to_user((void __user *)arg, &info, sizeof(info)))
@@ -480,7 +559,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
out:
kfree(fence_info);
-
+ dma_fence_put(fence);
return ret;
}
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index e683dd1..4bf661b 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -34,6 +34,7 @@
* @cb: fence callback information
* @type: sync file type
* @flags: flags used to create sync file
+ * @lock: mutex to protect fence/cb - used for semaphores
*/
struct sync_file {
struct file *file;
@@ -45,10 +46,13 @@ struct sync_file {
wait_queue_head_t wq;
- struct dma_fence *fence;
+ struct dma_fence __rcu *fence;
struct dma_fence_cb cb;
uint32_t type;
uint32_t flags;
+
+ /* protects the fence pointer and cb */
+ struct mutex lock;
};
#define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 5/8] sync_file: add support for a semaphore object
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
` (3 preceding siblings ...)
2017-04-04 4:27 ` [PATCH 4/8] sync_file: add a mutex to protect fence and callback members. (v4) Dave Airlie
@ 2017-04-04 4:27 ` Dave Airlie
2017-04-04 7:59 ` Daniel Vetter
2017-04-04 11:52 ` Chris Wilson
2017-04-04 4:27 ` [PATCH 6/8] drm/syncobj: add semaphore support helpers Dave Airlie
` (5 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: Dave Airlie @ 2017-04-04 4:27 UTC (permalink / raw)
To: dri-devel
From: Dave Airlie <airlied@redhat.com>
This object can be used to implement the Vulkan semaphores.
The object behaviour differs from fence, in that you can
replace the underlying fence, and you cannot merge semaphores.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/dma-buf/sync_file.c | 36 +++++++++++++++++++++++++++++++++++-
include/linux/sync_file.h | 2 ++
include/uapi/linux/sync_file.h | 14 ++++++++++++++
3 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 6376f6f..a82f6d8 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -44,7 +44,7 @@ int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
{
if (flags)
return -EINVAL;
- if (type != SYNC_FILE_TYPE_FENCE)
+ if (type != SYNC_FILE_TYPE_FENCE && type != SYNC_FILE_TYPE_SEMAPHORE)
return -EINVAL;
return 0;
}
@@ -200,6 +200,38 @@ sync_file_get_fence_locked(struct sync_file *sync_file)
sync_file_held(sync_file));
}
+/**
+ * sync_file_replace_fence - replace the fence related to the sync_file
+ * @sync_file: sync file to replace fence in
+ * @fence: fence to replace with (or NULL for no fence).
+ * Returns previous fence.
+ */
+struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
+ struct dma_fence *fence)
+{
+ struct dma_fence *ret_fence = NULL;
+
+ if (sync_file->type != SYNC_FILE_TYPE_SEMAPHORE)
+ return NULL;
+
+ if (fence)
+ dma_fence_get(fence);
+
+ mutex_lock(&sync_file->lock);
+
+ ret_fence = sync_file_get_fence_locked(sync_file);
+ if (ret_fence) {
+ if (test_bit(POLL_ENABLED, &ret_fence->flags))
+ dma_fence_remove_callback(ret_fence, &sync_file->cb);
+ }
+
+ RCU_INIT_POINTER(sync_file->fence, fence);
+
+ mutex_unlock(&sync_file->lock);
+ return ret_fence;
+}
+EXPORT_SYMBOL(sync_file_replace_fence);
+
static int sync_file_set_fence(struct sync_file *sync_file,
struct dma_fence **fences, int num_fences)
{
@@ -278,6 +310,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
if (a->type != b->type)
return NULL;
+ if (a->type != SYNC_FILE_TYPE_FENCE)
+ return NULL;
if (!rcu_access_pointer(a->fence) ||
!rcu_access_pointer(b->fence))
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 4bf661b..245c7da 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -62,4 +62,6 @@ struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags);
struct sync_file *sync_file_create(struct dma_fence *fence, uint32_t type, uint32_t flags);
struct dma_fence *sync_file_get_fence(int fd);
struct sync_file *sync_file_fdget(int fd);
+struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
+ struct dma_fence *fence);
#endif /* _LINUX_SYNC_H */
diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
index f439cda..5f266e0 100644
--- a/include/uapi/linux/sync_file.h
+++ b/include/uapi/linux/sync_file.h
@@ -80,6 +80,20 @@ struct sync_file_info {
#define SYNC_FILE_TYPE_FENCE 0
/**
+ * DOC: SYNC_FILE_TYPE_SEMAPHORE - semaphore sync file object
+ *
+ * This is a sync file that operates like a Vulkan semaphore.
+ * The object should just be imported/exported but not use the
+ * sync file ioctls (except info).
+ * This object can have it's backing fence replaced multiple times.
+ * Each signal operation assigns a backing fence.
+ * Each wait operation waits on the current fence, and removes it.
+ * These operations should happen via driver command submission interfaces.
+ * This is useful for shared vulkan semaphores.
+ */
+#define SYNC_FILE_TYPE_SEMAPHORE 1
+
+/**
* struct sync_file_type - data returned from sync file type ioctl
* @type: sync_file type
* @flags: sync_file creation flags
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 6/8] drm/syncobj: add semaphore support helpers.
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
` (4 preceding siblings ...)
2017-04-04 4:27 ` [PATCH 5/8] sync_file: add support for a semaphore object Dave Airlie
@ 2017-04-04 4:27 ` Dave Airlie
2017-04-04 8:07 ` Daniel Vetter
2017-04-04 4:27 ` [PATCH 7/8] amdgpu/cs: split out fence dependency checking Dave Airlie
` (4 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Dave Airlie @ 2017-04-04 4:27 UTC (permalink / raw)
To: dri-devel
From: Dave Airlie <airlied@redhat.com>
This just adds two helper interfaces to bridge the gap from
drivers to sync_file for the semaphore objects.
These will be used by the amdgpu driver.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/drm/drm_syncobj.c | 72 +++++++++++++++++++++++++++++++++++++++++++
include/drm/drm_syncobj.h | 37 ++++++++++++++++++++++
2 files changed, 109 insertions(+)
create mode 100644 include/drm/drm_syncobj.h
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index a3a1ace..6f1b61f 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -39,6 +39,7 @@
#include <drm/drmP.h>
#include <linux/sync_file.h>
#include "drm_internal.h"
+#include <drm/drm_syncobj.h>
static struct sync_file *drm_syncobj_file_lookup(struct drm_file *file_private,
u32 handle)
@@ -55,6 +56,77 @@ static struct sync_file *drm_syncobj_file_lookup(struct drm_file *file_private,
return sync_file;
}
+static int drm_syncobj_swap_fences_nocheck(struct drm_file *file_private,
+ uint32_t handle,
+ struct dma_fence *fence,
+ struct dma_fence **old_fence)
+{
+ struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
+
+ if (!sync_file)
+ return -EINVAL;
+
+ *old_fence = sync_file_replace_fence(sync_file, fence);
+ return 0;
+}
+
+/**
+ * drm_syncobj_swap_fences - lookup and replace fence in a sync object.
+ * @file_private - drm file private pointer.
+ * @handle - sync object handle
+ * @fence - new fence (or NULL) to back sync object.
+ * @old_fence - old fence backing sync object.
+ * Returns: Old fence
+ *
+ * This function is used to swap the fence backing the sync object
+ * with a new one, it returns the old fence in old_fence;
+ */
+int drm_syncobj_swap_fences(struct drm_file *file_private,
+ uint32_t handle,
+ struct dma_fence *fence,
+ struct dma_fence **old_fence)
+{
+ int r;
+
+ r = drm_syncobj_swap_fences_nocheck(file_private,
+ handle,
+ fence,
+ old_fence);
+ if (r)
+ return r;
+
+
+ if (!*old_fence)
+ return -EINVAL;
+ return 0;
+}
+EXPORT_SYMBOL(drm_syncobj_swap_fences);
+
+/**
+ * drm_syncobj_replace_fence - lookup and replace fence in a sync object.
+ * @file_private - drm file private pointer.
+ * @handle - syncobj handle to lookup
+ * @fence - fence to install in sync file.
+ * Returns: 0 on success or -EINVAL on error
+ *
+ * This looks up a sync object and replaces the fence on it, freeing
+ * the old one.
+ */
+int drm_syncobj_replace_fence(struct drm_file *file_private,
+ u32 handle,
+ struct dma_fence *fence)
+{
+ struct dma_fence *old_fence;
+ int r;
+
+ r = drm_syncobj_swap_fences_nocheck(file_private, handle, fence, &old_fence);
+ if (r)
+ return r;
+ dma_fence_put(old_fence);
+ return 0;
+}
+EXPORT_SYMBOL(drm_syncobj_replace_fence);
+
static int drm_syncobj_create(struct drm_file *file_private,
unsigned type,
unsigned flags, u32 *handle)
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
new file mode 100644
index 0000000..f614e06
--- /dev/null
+++ b/include/drm/drm_syncobj.h
@@ -0,0 +1,37 @@
+/*
+ * Copyright © 2017 Red Hat
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *
+ */
+#ifndef __DRM_SYNCOBJ_H__
+#define __DRM_SYNCOBJ_H__
+
+int drm_syncobj_swap_fences(struct drm_file *file_private,
+ uint32_t handle,
+ struct dma_fence *fence,
+ struct dma_fence **old_fence);
+int drm_syncobj_replace_fence(struct drm_file *file_private,
+ u32 handle,
+ struct dma_fence *fence);
+
+#endif
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 7/8] amdgpu/cs: split out fence dependency checking
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
` (5 preceding siblings ...)
2017-04-04 4:27 ` [PATCH 6/8] drm/syncobj: add semaphore support helpers Dave Airlie
@ 2017-04-04 4:27 ` Dave Airlie
2017-04-04 7:37 ` Christian König
2017-04-04 4:27 ` [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2) Dave Airlie
` (3 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Dave Airlie @ 2017-04-04 4:27 UTC (permalink / raw)
To: dri-devel
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.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 86 +++++++++++++++++++---------------
1 file changed, 48 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..4671432 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -963,56 +963,66 @@ 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_device *adev,
+ 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(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(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(adev, p, chunk);
+ if (r)
+ return r;
}
}
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2)
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
` (6 preceding siblings ...)
2017-04-04 4:27 ` [PATCH 7/8] amdgpu/cs: split out fence dependency checking Dave Airlie
@ 2017-04-04 4:27 ` Dave Airlie
2017-04-04 7:40 ` Christian König
2017-04-04 4:35 ` [RFC] DRM synchronisation objects Dave Airlie
` (2 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Dave Airlie @ 2017-04-04 4:27 UTC (permalink / raw)
To: dri-devel
From: Dave Airlie <airlied@redhat.com>
This creates a new interface for amdgpu with ioctls to
create/destroy/import and export shared semaphores using
sem object backed by the sync_file code. The semaphores
are not installed as fd (except for export), but rather
like other driver internal objects in an idr. The idr
holds the initial reference on the sync file.
The command submission interface is enhanced with two new
chunks, one for semaphore waiting, one for semaphore signalling
and just takes a list of handles for each.
This is based on work originally done by David Zhou at AMD,
with input from Christian Konig on what things should look like.
NOTE: this interface addition needs a version bump to expose
it to userspace.
v1.1: keep file reference on import.
v2: move to using syncobjs
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 86 ++++++++++++++++++++++++++++++++-
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
include/uapi/drm/amdgpu_drm.h | 6 +++
3 files changed, 92 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 4671432..4dd210b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -27,6 +27,7 @@
#include <linux/pagemap.h>
#include <drm/drmP.h>
#include <drm/amdgpu_drm.h>
+#include <drm/drm_syncobj.h>
#include "amdgpu.h"
#include "amdgpu_trace.h"
@@ -217,6 +218,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
break;
case AMDGPU_CHUNK_ID_DEPENDENCIES:
+ case AMDGPU_CHUNK_ID_SEM_WAIT:
+ case AMDGPU_CHUNK_ID_SEM_SIGNAL:
break;
default:
@@ -1009,6 +1012,44 @@ static int amdgpu_process_fence_dep(struct amdgpu_device *adev,
return 0;
}
+static int amdgpu_sem_lookup_and_sync(struct amdgpu_device *adev,
+ struct drm_file *file_private,
+ struct amdgpu_sync *sync,
+ uint32_t handle)
+{
+ int r;
+ struct dma_fence *old_fence;
+ r = drm_syncobj_swap_fences(file_private, handle, NULL, &old_fence);
+ if (r)
+ return r;
+
+ r = amdgpu_sync_fence(adev, sync, old_fence);
+ dma_fence_put(old_fence);
+
+ return r;
+}
+
+static int amdgpu_process_sem_wait_dep(struct amdgpu_device *adev,
+ struct amdgpu_cs_parser *p,
+ struct amdgpu_cs_chunk *chunk)
+{
+ unsigned num_deps;
+ int i, r;
+ struct drm_amdgpu_cs_chunk_sem *deps;
+
+ deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
+ num_deps = chunk->length_dw * 4 /
+ sizeof(struct drm_amdgpu_cs_chunk_sem);
+
+ for (i = 0; i < num_deps; ++i) {
+ r = amdgpu_sem_lookup_and_sync(adev, p->filp, &p->job->sync,
+ deps[i].handle);
+ if (r)
+ return r;
+ }
+ return 0;
+}
+
static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
struct amdgpu_cs_parser *p)
{
@@ -1023,12 +1064,55 @@ static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
r = amdgpu_process_fence_dep(adev, p, chunk);
if (r)
return r;
+ } else if (chunk->chunk_id == AMDGPU_CHUNK_ID_SEM_WAIT) {
+ r = amdgpu_process_sem_wait_dep(adev, p, chunk);
+ if (r)
+ return r;
}
}
return 0;
}
+static int amdgpu_process_sem_signal_dep(struct amdgpu_cs_parser *p,
+ struct amdgpu_cs_chunk *chunk,
+ struct dma_fence *fence)
+{
+ 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,
+ fence);
+ if (r)
+ return r;
+ }
+ return 0;
+}
+
+static int amdgpu_cs_post_dependencies(struct amdgpu_cs_parser *p)
+{
+ int i, r;
+
+ for (i = 0; i < p->nchunks; ++i) {
+ struct amdgpu_cs_chunk *chunk;
+
+ chunk = &p->chunks[i];
+
+ if (chunk->chunk_id == AMDGPU_CHUNK_ID_SEM_SIGNAL) {
+ r = amdgpu_process_sem_signal_dep(p, chunk, p->fence);
+ if (r)
+ return r;
+ }
+ }
+ return 0;
+}
+
static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
union drm_amdgpu_cs *cs)
{
@@ -1056,7 +1140,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
trace_amdgpu_cs_ioctl(job);
amd_sched_entity_push_job(&job->base);
- return 0;
+ return amdgpu_cs_post_dependencies(p);
}
int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b76cd69..e95951e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -683,7 +683,7 @@ static struct drm_driver kms_driver = {
.driver_features =
DRIVER_USE_AGP |
DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
- DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET,
+ DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
.load = amdgpu_driver_load_kms,
.open = amdgpu_driver_open_kms,
.preclose = amdgpu_driver_preclose_kms,
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 5797283..647c520 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -390,6 +390,8 @@ struct drm_amdgpu_gem_va {
#define AMDGPU_CHUNK_ID_IB 0x01
#define AMDGPU_CHUNK_ID_FENCE 0x02
#define AMDGPU_CHUNK_ID_DEPENDENCIES 0x03
+#define AMDGPU_CHUNK_ID_SEM_WAIT 0x04
+#define AMDGPU_CHUNK_ID_SEM_SIGNAL 0x05
struct drm_amdgpu_cs_chunk {
__u32 chunk_id;
@@ -454,6 +456,10 @@ struct drm_amdgpu_cs_chunk_fence {
__u32 offset;
};
+struct drm_amdgpu_cs_chunk_sem {
+ __u32 handle;
+};
+
struct drm_amdgpu_cs_chunk_data {
union {
struct drm_amdgpu_cs_chunk_ib ib_data;
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [RFC] DRM synchronisation objects
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
` (7 preceding siblings ...)
2017-04-04 4:27 ` [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2) Dave Airlie
@ 2017-04-04 4:35 ` Dave Airlie
2017-04-04 8:02 ` Christian König
2017-04-04 8:11 ` Daniel Vetter
10 siblings, 0 replies; 31+ messages in thread
From: Dave Airlie @ 2017-04-04 4:35 UTC (permalink / raw)
To: dri-devel
On 4 April 2017 at 14:27, Dave Airlie <airlied@gmail.com> wrote:
> This series enhances my previous semaphore work on for amdgpu,
> with a generic DRM sync object. (drm_syncobj).
>
> It first enhances sync_file to have a type/flags so we can have
> different semantics for different sync files, and a wait
> to retrieve the type of sync_file for userspace.
>
> Then it adds drm sync objects which are just a drm wrapper around
> a sync_file object, allowing creation/info/destroy and import/export
> of the objects.
>
> Next it enhances sync_file to have semaphore semantics for Vulkan.
>
> Finally it adds amdgpu support to it's command submission paths to
> use the new code.
>
> I've hopefully fixed up the things pointed out in the last
> review of the sync_file fence changes, I do wonder if we should
> just block poll on semaphore objects as currently I've no use
> case for this.
Also available in my drm-syncobj branch.
Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/8] sync_file: add type/flags to sync file object creation.
2017-04-04 4:27 ` [PATCH 1/8] sync_file: add type/flags to sync file object creation Dave Airlie
@ 2017-04-04 7:08 ` Daniel Vetter
0 siblings, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2017-04-04 7:08 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
On Tue, Apr 04, 2017 at 02:27:26PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This allows us to create sync files with different semantics,
> and clearly define the interoperation between them it also
> provides flags to allow for tweaks on those semantics.
>
> This provides a validation interface for drivers that accept
> types from userspace so they can return EINVAL instead of ENOMEM.
>
> This provides an ioctl for userspace to retrieve the type/flags
> of an object it may recieve from somewhere else.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
Since you've bothered to type the docs for the ioctl structs too, please
squash in the below (and double-check that kernel-doc is still happy):
diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst
index 31671b469627..375848c590ce 100644
--- a/Documentation/driver-api/dma-buf.rst
+++ b/Documentation/driver-api/dma-buf.rst
@@ -163,3 +163,8 @@ DMA Fence uABI/Sync File
.. kernel-doc:: include/linux/sync_file.h
:internal:
+Sync File IOCTL Definitions
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+.. kernel-doc:: include/uapi/linux/sync_file.h
+ :internal:
With that: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
One open question: Do we expect the current sync_file_get_fence to only
ever work for a FENCE type sync_file? Might be good to clarify that in the
kernel-doc for sync_file_get_fence().
-Daniel
> ---
> drivers/dma-buf/sw_sync.c | 2 +-
> drivers/dma-buf/sync_file.c | 62 +++++++++++++++++++++++++++++++++++++++---
> drivers/gpu/drm/drm_atomic.c | 2 +-
> include/linux/sync_file.h | 9 +++++-
> include/uapi/linux/sync_file.h | 27 ++++++++++++++++++
> 5 files changed, 95 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 69c5ff3..1c47de6 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -315,7 +315,7 @@ static long sw_sync_ioctl_create_fence(struct sync_timeline *obj,
> goto err;
> }
>
> - sync_file = sync_file_create(&pt->base);
> + sync_file = sync_file_create(&pt->base, SYNC_FILE_TYPE_FENCE, 0);
> dma_fence_put(&pt->base);
> if (!sync_file) {
> err = -ENOMEM;
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 2321035..b33af9d 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -28,9 +28,32 @@
>
> static const struct file_operations sync_file_fops;
>
> -static struct sync_file *sync_file_alloc(void)
> +/**
> + * sync_file_validate_type_flags - validate type/flags for support
> + * @type: type of sync file object
> + * @flags: flags to sync object.
> + *
> + * Validates the flags are correct so userspace can get a more
> + * detailed error type.
> + */
> +int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
> +{
> + if (flags)
> + return -EINVAL;
> + if (type != SYNC_FILE_TYPE_FENCE)
> + return -EINVAL;
> + return 0;
> +}
> +EXPORT_SYMBOL(sync_file_validate_type_flags);
> +
> +static struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
> {
> struct sync_file *sync_file;
> + int ret;
> +
> + ret = sync_file_validate_type_flags(type, flags);
> + if (ret)
> + return NULL;
>
> sync_file = kzalloc(sizeof(*sync_file), GFP_KERNEL);
> if (!sync_file)
> @@ -47,6 +70,8 @@ static struct sync_file *sync_file_alloc(void)
>
> INIT_LIST_HEAD(&sync_file->cb.node);
>
> + sync_file->type = type;
> + sync_file->flags = flags;
> return sync_file;
>
> err:
> @@ -72,11 +97,13 @@ static void fence_check_cb_func(struct dma_fence *f, struct dma_fence_cb *cb)
> * sync_file can be released with fput(sync_file->file). Returns the
> * sync_file or NULL in case of error.
> */
> -struct sync_file *sync_file_create(struct dma_fence *fence)
> +struct sync_file *sync_file_create(struct dma_fence *fence,
> + uint32_t type,
> + uint32_t flags)
> {
> struct sync_file *sync_file;
>
> - sync_file = sync_file_alloc();
> + sync_file = sync_file_alloc(type, flags);
> if (!sync_file)
> return NULL;
>
> @@ -200,7 +227,10 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
> struct dma_fence **fences, **nfences, **a_fences, **b_fences;
> int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
>
> - sync_file = sync_file_alloc();
> + if (a->type != b->type)
> + return NULL;
> +
> + sync_file = sync_file_alloc(a->type, a->flags);
> if (!sync_file)
> return NULL;
>
> @@ -437,6 +467,27 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> return ret;
> }
>
> +static long sync_file_ioctl_type(struct sync_file *sync_file,
> + unsigned long arg)
> +{
> + struct sync_file_type type;
> + int ret;
> + if (copy_from_user(&type, (void __user *)arg, sizeof(type)))
> + return -EFAULT;
> +
> + if (type.flags || type.type)
> + return -EINVAL;
> +
> + type.type = sync_file->type;
> + type.flags = sync_file->flags;
> +
> + if (copy_to_user((void __user *)arg, &type, sizeof(type)))
> + ret = -EFAULT;
> + else
> + ret = 0;
> + return ret;
> +}
> +
> static long sync_file_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> @@ -449,6 +500,9 @@ static long sync_file_ioctl(struct file *file, unsigned int cmd,
> case SYNC_IOC_FILE_INFO:
> return sync_file_ioctl_fence_info(sync_file, arg);
>
> + case SYNC_IOC_TYPE:
> + return sync_file_ioctl_type(sync_file, arg);
> +
> default:
> return -ENOTTY;
> }
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..bb5a740 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1917,7 +1917,7 @@ static int setup_out_fence(struct drm_out_fence_state *fence_state,
> if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> return -EFAULT;
>
> - fence_state->sync_file = sync_file_create(fence);
> + fence_state->sync_file = sync_file_create(fence, SYNC_FILE_TYPE_FENCE, 0);
> if (!fence_state->sync_file)
> return -ENOMEM;
>
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index 3e3ab84..ede4182 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -20,6 +20,8 @@
> #include <linux/spinlock.h>
> #include <linux/dma-fence.h>
> #include <linux/dma-fence-array.h>
> +#include <uapi/linux/sync_file.h>
> +
>
> /**
> * struct sync_file - sync file to export to the userspace
> @@ -30,6 +32,8 @@
> * @wq: wait queue for fence signaling
> * @fence: fence with the fences in the sync_file
> * @cb: fence callback information
> + * @type: sync file type
> + * @flags: flags used to create sync file
> */
> struct sync_file {
> struct file *file;
> @@ -43,11 +47,14 @@ struct sync_file {
>
> struct dma_fence *fence;
> struct dma_fence_cb cb;
> + uint32_t type;
> + uint32_t flags;
> };
>
> #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
>
> -struct sync_file *sync_file_create(struct dma_fence *fence);
> +int sync_file_validate_type_flags(uint32_t type, uint32_t flags);
> +struct sync_file *sync_file_create(struct dma_fence *fence, uint32_t type, uint32_t flags);
> struct dma_fence *sync_file_get_fence(int fd);
>
> #endif /* _LINUX_SYNC_H */
> diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
> index 5b287d6..f439cda 100644
> --- a/include/uapi/linux/sync_file.h
> +++ b/include/uapi/linux/sync_file.h
> @@ -69,6 +69,26 @@ struct sync_file_info {
> #define SYNC_IOC_MAGIC '>'
>
> /**
> + * DOC: SYNC_FILE_TYPE_FENCE - fence sync file object
> + *
> + * This sync file is a wrapper around a dma fence or a dma fence array.
> + * It can be merged with another fence sync file object to create a new
> + * merged object.
> + * The fence backing this object cannot be replaced.
> + * This is useful for shared fences.
> + */
> +#define SYNC_FILE_TYPE_FENCE 0
> +
> +/**
> + * struct sync_file_type - data returned from sync file type ioctl
> + * @type: sync_file type
> + * @flags: sync_file creation flags
> + */
> +struct sync_file_type {
> + __u32 type;
> + __u32 flags;
> +};
> +/**
> * Opcodes 0, 1 and 2 were burned during a API change to avoid users of the
> * old API to get weird errors when trying to handling sync_files. The API
> * change happened during the de-stage of the Sync Framework when there was
> @@ -94,4 +114,11 @@ struct sync_file_info {
> */
> #define SYNC_IOC_FILE_INFO _IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
>
> +/**
> + * DOC: SYNC_IOC_TYPE - get creation type and flags of sync_file.
> + *
> + * Takes a struct sync_file_type. Returns the created values of type and flags.
> + */
> +#define SYNC_IOC_TYPE _IOWR(SYNC_IOC_MAGIC, 5, struct sync_file_type)
> +
> #endif /* _UAPI_LINUX_SYNC_H */
> --
> 2.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/8] sync_file: export some interfaces needed by drm sync objects.
2017-04-04 4:27 ` [PATCH 2/8] sync_file: export some interfaces needed by drm sync objects Dave Airlie
@ 2017-04-04 7:10 ` Daniel Vetter
0 siblings, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2017-04-04 7:10 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
On Tue, Apr 04, 2017 at 02:27:27PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> These are just alloc and fdget interfaces needed by the drm sync
> objects code.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/dma-buf/sync_file.c | 21 +++++++++++++++++++--
> include/linux/sync_file.h | 3 ++-
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index b33af9d..153bf03 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -46,7 +46,16 @@ int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
> }
> EXPORT_SYMBOL(sync_file_validate_type_flags);
>
> -static struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
> +/**
> + * sync_file_alloc() - allocate an unfenced sync file
> + * @type: type of sync file object
> + * @flags: creation flags for sync file object
> + *
> + * Creates a sync_file.
> + * The sync_file can be released with fput(sync_file->file).
> + * Returns the sync_file or NULL in case of error.
> + */
> +struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
> {
> struct sync_file *sync_file;
> int ret;
> @@ -78,6 +87,7 @@ static struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
> kfree(sync_file);
> return NULL;
> }
> +EXPORT_SYMBOL(sync_file_alloc);
>
> static void fence_check_cb_func(struct dma_fence *f, struct dma_fence_cb *cb)
> {
> @@ -118,7 +128,13 @@ struct sync_file *sync_file_create(struct dma_fence *fence,
> }
> EXPORT_SYMBOL(sync_file_create);
>
> -static struct sync_file *sync_file_fdget(int fd)
> +/**
> + * sync_file_fdget - get a reference to the file and return sync_file.
> + * @fd - file descriptor pointing at a sync_file.
> + *
> + * Returns the corresponding sync_file object.
> + */
> +struct sync_file *sync_file_fdget(int fd)
> {
> struct file *file = fget(fd);
>
> @@ -134,6 +150,7 @@ static struct sync_file *sync_file_fdget(int fd)
> fput(file);
> return NULL;
> }
> +EXPORT_SYMBOL(sync_file_fdget);
>
> /**
> * sync_file_get_fence - get the fence related to the sync_file fd
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index ede4182..e683dd1 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -54,7 +54,8 @@ struct sync_file {
> #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
>
> int sync_file_validate_type_flags(uint32_t type, uint32_t flags);
> +struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags);
> struct sync_file *sync_file_create(struct dma_fence *fence, uint32_t type, uint32_t flags);
> struct dma_fence *sync_file_get_fence(int fd);
> -
> +struct sync_file *sync_file_fdget(int fd);
> #endif /* _LINUX_SYNC_H */
> --
> 2.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 7/8] amdgpu/cs: split out fence dependency checking
2017-04-04 4:27 ` [PATCH 7/8] amdgpu/cs: split out fence dependency checking Dave Airlie
@ 2017-04-04 7:37 ` Christian König
0 siblings, 0 replies; 31+ messages in thread
From: Christian König @ 2017-04-04 7:37 UTC (permalink / raw)
To: Dave Airlie, dri-devel
Am 04.04.2017 um 06:27 schrieb 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.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 86 +++++++++++++++++++---------------
> 1 file changed, 48 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..4671432 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -963,56 +963,66 @@ 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_device *adev,
> + struct amdgpu_cs_parser *p,
> + struct amdgpu_cs_chunk *chunk)
adev is actually also available as p->adev.
But the old code got this wrong as well, so either way the patch is
Reviewed-by: Christian König <christian.koenig@amd.com>.
Regards,
Christian.
> {
> 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(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(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(adev, p, chunk);
> + if (r)
> + return r;
> }
> }
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2)
2017-04-04 4:27 ` [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2) Dave Airlie
@ 2017-04-04 7:40 ` Christian König
2017-04-04 8:10 ` Daniel Vetter
0 siblings, 1 reply; 31+ messages in thread
From: Christian König @ 2017-04-04 7:40 UTC (permalink / raw)
To: Dave Airlie, dri-devel
Am 04.04.2017 um 06:27 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
>
> This creates a new interface for amdgpu with ioctls to
> create/destroy/import and export shared semaphores using
> sem object backed by the sync_file code. The semaphores
> are not installed as fd (except for export), but rather
> like other driver internal objects in an idr. The idr
> holds the initial reference on the sync file.
>
> The command submission interface is enhanced with two new
> chunks, one for semaphore waiting, one for semaphore signalling
> and just takes a list of handles for each.
>
> This is based on work originally done by David Zhou at AMD,
> with input from Christian Konig on what things should look like.
>
> NOTE: this interface addition needs a version bump to expose
> it to userspace.
>
> v1.1: keep file reference on import.
> v2: move to using syncobjs
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
Looks good to me in general, but one note below.
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 86 ++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
> include/uapi/drm/amdgpu_drm.h | 6 +++
> 3 files changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 4671432..4dd210b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -27,6 +27,7 @@
> #include <linux/pagemap.h>
> #include <drm/drmP.h>
> #include <drm/amdgpu_drm.h>
> +#include <drm/drm_syncobj.h>
> #include "amdgpu.h"
> #include "amdgpu_trace.h"
>
> @@ -217,6 +218,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
> break;
>
> case AMDGPU_CHUNK_ID_DEPENDENCIES:
> + case AMDGPU_CHUNK_ID_SEM_WAIT:
> + case AMDGPU_CHUNK_ID_SEM_SIGNAL:
> break;
>
> default:
> @@ -1009,6 +1012,44 @@ static int amdgpu_process_fence_dep(struct amdgpu_device *adev,
> return 0;
> }
>
> +static int amdgpu_sem_lookup_and_sync(struct amdgpu_device *adev,
> + struct drm_file *file_private,
> + struct amdgpu_sync *sync,
> + uint32_t handle)
> +{
> + int r;
> + struct dma_fence *old_fence;
> + r = drm_syncobj_swap_fences(file_private, handle, NULL, &old_fence);
What happens when we the CS fails later on? Would the semaphore then not
contain the fence any more?
Christian.
> + if (r)
> + return r;
> +
> + r = amdgpu_sync_fence(adev, sync, old_fence);
> + dma_fence_put(old_fence);
> +
> + return r;
> +}
> +
> +static int amdgpu_process_sem_wait_dep(struct amdgpu_device *adev,
> + struct amdgpu_cs_parser *p,
> + struct amdgpu_cs_chunk *chunk)
> +{
> + unsigned num_deps;
> + int i, r;
> + struct drm_amdgpu_cs_chunk_sem *deps;
> +
> + deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
> + num_deps = chunk->length_dw * 4 /
> + sizeof(struct drm_amdgpu_cs_chunk_sem);
> +
> + for (i = 0; i < num_deps; ++i) {
> + r = amdgpu_sem_lookup_and_sync(adev, p->filp, &p->job->sync,
> + deps[i].handle);
> + if (r)
> + return r;
> + }
> + return 0;
> +}
> +
> static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
> struct amdgpu_cs_parser *p)
> {
> @@ -1023,12 +1064,55 @@ static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
> r = amdgpu_process_fence_dep(adev, p, chunk);
> if (r)
> return r;
> + } else if (chunk->chunk_id == AMDGPU_CHUNK_ID_SEM_WAIT) {
> + r = amdgpu_process_sem_wait_dep(adev, p, chunk);
> + if (r)
> + return r;
> }
> }
>
> return 0;
> }
>
> +static int amdgpu_process_sem_signal_dep(struct amdgpu_cs_parser *p,
> + struct amdgpu_cs_chunk *chunk,
> + struct dma_fence *fence)
> +{
> + 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,
> + fence);
> + if (r)
> + return r;
> + }
> + return 0;
> +}
> +
> +static int amdgpu_cs_post_dependencies(struct amdgpu_cs_parser *p)
> +{
> + int i, r;
> +
> + for (i = 0; i < p->nchunks; ++i) {
> + struct amdgpu_cs_chunk *chunk;
> +
> + chunk = &p->chunks[i];
> +
> + if (chunk->chunk_id == AMDGPU_CHUNK_ID_SEM_SIGNAL) {
> + r = amdgpu_process_sem_signal_dep(p, chunk, p->fence);
> + if (r)
> + return r;
> + }
> + }
> + return 0;
> +}
> +
> static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
> union drm_amdgpu_cs *cs)
> {
> @@ -1056,7 +1140,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
> trace_amdgpu_cs_ioctl(job);
> amd_sched_entity_push_job(&job->base);
>
> - return 0;
> + return amdgpu_cs_post_dependencies(p);
> }
>
> int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index b76cd69..e95951e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -683,7 +683,7 @@ static struct drm_driver kms_driver = {
> .driver_features =
> DRIVER_USE_AGP |
> DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
> - DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET,
> + DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
> .load = amdgpu_driver_load_kms,
> .open = amdgpu_driver_open_kms,
> .preclose = amdgpu_driver_preclose_kms,
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index 5797283..647c520 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -390,6 +390,8 @@ struct drm_amdgpu_gem_va {
> #define AMDGPU_CHUNK_ID_IB 0x01
> #define AMDGPU_CHUNK_ID_FENCE 0x02
> #define AMDGPU_CHUNK_ID_DEPENDENCIES 0x03
> +#define AMDGPU_CHUNK_ID_SEM_WAIT 0x04
> +#define AMDGPU_CHUNK_ID_SEM_SIGNAL 0x05
>
> struct drm_amdgpu_cs_chunk {
> __u32 chunk_id;
> @@ -454,6 +456,10 @@ struct drm_amdgpu_cs_chunk_fence {
> __u32 offset;
> };
>
> +struct drm_amdgpu_cs_chunk_sem {
> + __u32 handle;
> +};
> +
> struct drm_amdgpu_cs_chunk_data {
> union {
> struct drm_amdgpu_cs_chunk_ib ib_data;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/8] drm: introduce sync objects as sync file objects with no fd
2017-04-04 4:27 ` [PATCH 3/8] drm: introduce sync objects as sync file objects with no fd Dave Airlie
@ 2017-04-04 7:42 ` Daniel Vetter
[not found] ` <CAPM=9tyj6k4hqJWrwDW8Ch+TZCOoXRuAK2g71ciUm5vxpwmkuw@mail.gmail.com>
0 siblings, 1 reply; 31+ messages in thread
From: Daniel Vetter @ 2017-04-04 7:42 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
On Tue, Apr 04, 2017 at 02:27:28PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> Sync objects are new toplevel drm object, that have the same
> semantics as sync_file objects, but without requiring an fd
> to be consumed to support them.
>
> This patch just enables the DRM interface to create these
> objects, it doesn't actually provide any semaphore objects
> or new features.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> Documentation/gpu/drm-internals.rst | 5 +
> Documentation/gpu/drm-mm.rst | 6 +
> drivers/gpu/drm/Makefile | 2 +-
> drivers/gpu/drm/drm_fops.c | 8 ++
> drivers/gpu/drm/drm_internal.h | 15 +++
> drivers/gpu/drm/drm_ioctl.c | 14 ++
> drivers/gpu/drm/drm_syncobj.c | 257 ++++++++++++++++++++++++++++++++++++
> include/drm/drmP.h | 5 +
> include/drm/drm_drv.h | 1 +
> include/uapi/drm/drm.h | 27 ++++
> 10 files changed, 339 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/drm_syncobj.c
>
> diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
> index e35920d..743b751 100644
> --- a/Documentation/gpu/drm-internals.rst
> +++ b/Documentation/gpu/drm-internals.rst
> @@ -98,6 +98,11 @@ DRIVER_ATOMIC
> implement appropriate obj->atomic_get_property() vfuncs for any
> modeset objects with driver specific properties.
>
> +DRIVER_SYNCOBJ
> + Driver support sync objects. These are just sync files that don't
> + use a file descriptor. They can be used to implement Vulkan shared
> + semaphores.
Bikeshed: I'm kinda leaning towards not adding driver flags and having
drivers call _init() functions in their probe code. Instead of magic flags
that make the core init stuff for you. Feels a bit much mid-layer-y.
But since it's such a common pattern, and since our load/unload stuff is
still an impressive mess, meh.
A few more things I spotted below, with those addressed:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> +
> Major, Minor and Patchlevel
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
> index f5760b1..28aebe8 100644
> --- a/Documentation/gpu/drm-mm.rst
> +++ b/Documentation/gpu/drm-mm.rst
> @@ -483,3 +483,9 @@ DRM Cache Handling
>
> .. kernel-doc:: drivers/gpu/drm/drm_cache.c
> :export:
> +
> +DRM Sync Objects
> +===========================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_syncobj.c
> + :export:
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 3ee9579..b5e565c 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -16,7 +16,7 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \
> drm_framebuffer.o drm_connector.o drm_blend.o \
> drm_encoder.o drm_mode_object.o drm_property.o \
> drm_plane.o drm_color_mgmt.o drm_print.o \
> - drm_dumb_buffers.o drm_mode_config.o
> + drm_dumb_buffers.o drm_mode_config.o drm_syncobj.o
>
> drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> drm-$(CONFIG_DRM_VM) += drm_vm.o
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index afdf5b1..9a61df2 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -219,6 +219,9 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
> if (drm_core_check_feature(dev, DRIVER_GEM))
> drm_gem_open(dev, priv);
>
> + if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> + drm_syncobj_open(priv);
> +
> if (drm_core_check_feature(dev, DRIVER_PRIME))
> drm_prime_init_file_private(&priv->prime);
>
> @@ -266,6 +269,8 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
> out_prime_destroy:
> if (drm_core_check_feature(dev, DRIVER_PRIME))
> drm_prime_destroy_file_private(&priv->prime);
> + if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> + drm_syncobj_release(priv);
> if (drm_core_check_feature(dev, DRIVER_GEM))
> drm_gem_release(dev, priv);
> put_pid(priv->pid);
> @@ -400,6 +405,9 @@ int drm_release(struct inode *inode, struct file *filp)
> drm_property_destroy_user_blobs(dev, file_priv);
> }
>
> + if (drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> + drm_syncobj_release(file_priv);
> +
> if (drm_core_check_feature(dev, DRIVER_GEM))
> drm_gem_release(dev, file_priv);
>
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index f37388c..70c27a0 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -142,4 +142,19 @@ static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
> {
> return 0;
> }
> +
> #endif
> +
> +/* drm_syncobj.c */
> +void drm_syncobj_open(struct drm_file *file_private);
> +void drm_syncobj_release(struct drm_file *file_private);
> +int drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private);
> +int drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private);
> +int drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private);
> +int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private);
> +int drm_syncobj_info_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index a7c61c2..4a8ed66 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -240,6 +240,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> req->value |= dev->driver->prime_fd_to_handle ? DRM_PRIME_CAP_IMPORT : 0;
> req->value |= dev->driver->prime_handle_to_fd ? DRM_PRIME_CAP_EXPORT : 0;
> return 0;
> + case DRM_CAP_SYNCOBJ:
> + req->value = drm_core_check_feature(dev, DRIVER_SYNCOBJ);
> + return 0;
> }
>
> /* Other caps only work with KMS drivers */
> @@ -641,6 +644,17 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> +
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_CREATE, drm_syncobj_create_ioctl,
> + DRM_UNLOCKED|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_DESTROY, drm_syncobj_destroy_ioctl,
> + DRM_UNLOCKED|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD, drm_syncobj_handle_to_fd_ioctl,
> + DRM_UNLOCKED|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE, drm_syncobj_fd_to_handle_ioctl,
> + DRM_UNLOCKED|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_INFO, drm_syncobj_info_ioctl,
> + DRM_UNLOCKED|DRM_RENDER_ALLOW),
You can drop the DRM_UNLOCKED, it's the enforced standard for non-legacy
drivers since:
commit ea487835e8876abf7ad909636e308c801a2bcda6
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Mon Sep 28 21:42:40 2015 +0200
drm: Enforce unlocked ioctl operation for kms driver ioctls
> };
>
> #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..a3a1ace
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -0,0 +1,257 @@
> +/*
> + * Copyright © 2017 Red Hat
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *
> + */
> +
> +/**
> + * DOC: Overview
> + *
> + * DRM synchronisation objects (syncobj) are wrappers for sync_file objects,
> + * that don't use fd space, and can be converted to/from sync_file fds.
> + *
> + * Depending on the sync object type, they can expose different sync_file
> + * semantics and restrictions.
> + *
> + * Their primary use-case is to implement Vulkan shared semaphores.
> + */
> +
> +#include <drm/drmP.h>
> +#include <linux/sync_file.h>
> +#include "drm_internal.h"
> +
> +static struct sync_file *drm_syncobj_file_lookup(struct drm_file *file_private,
> + u32 handle)
> +{
> + struct sync_file *sync_file;
> +
> + spin_lock(&file_private->syncobj_table_lock);
> +
> + /* Check if we currently have a reference on the object */
> + sync_file = idr_find(&file_private->syncobj_idr, handle);
You need to get a reference for the sync_file (hm, should we have
sync_file_get/put helpers?), since otherwise someone might sneak in a
destroy ioctl call right after you drop the lock here and boom.
Means ofc that all the callers must explicitly drop that additional
reference.
> +
> + spin_unlock(&file_private->syncobj_table_lock);
> +
> + return sync_file;
> +}
> +
> +static int drm_syncobj_create(struct drm_file *file_private,
> + unsigned type,
> + unsigned flags, u32 *handle)
> +{
> + struct sync_file *sync_file;
> + int ret;
> +
> + ret = sync_file_validate_type_flags(type, flags);
> + if (ret)
> + return ret;
> +
> + sync_file = sync_file_alloc(type, flags);
> + if (!sync_file)
> + return -ENOMEM;
> +
> + idr_preload(GFP_KERNEL);
> + spin_lock(&file_private->syncobj_table_lock);
> + ret = idr_alloc(&file_private->syncobj_idr, sync_file, 1, 0, GFP_NOWAIT);
> + spin_unlock(&file_private->syncobj_table_lock);
> +
> + idr_preload_end();
> +
> + if (ret < 0)
> + return ret;
> +
> + *handle = ret;
> + return 0;
> +}
> +
> +static void drm_syncobj_destroy(struct drm_file *file_private,
> + u32 handle)
> +{
> + struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
> + if (!sync_file)
> + return;
> +
> + spin_lock(&file_private->syncobj_table_lock);
> + idr_remove(&file_private->syncobj_idr, handle);
> + spin_unlock(&file_private->syncobj_table_lock);
> +
> + fput(sync_file->file);
> +}
> +
> +static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
> + u32 handle, int *fd)
> +{
> + struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
> + int ret;
> + if (!sync_file)
> + return -EINVAL;
> +
> + ret = get_unused_fd_flags(O_CLOEXEC);
> + if (ret < 0)
> + return ret;
> + fd_install(ret, sync_file->file);
> + *fd = ret;
> + return 0;
> +}
> +
> +static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
> + int fd, u32 *handle)
> +{
> + struct sync_file *sync_file = sync_file_fdget(fd);
> + int ret;
> + if (!sync_file)
> + return -EINVAL;
> + idr_preload(GFP_KERNEL);
> + spin_lock(&file_private->syncobj_table_lock);
> + ret = idr_alloc(&file_private->syncobj_idr, sync_file, 1, 0, GFP_NOWAIT);
> + spin_unlock(&file_private->syncobj_table_lock);
> + idr_preload_end();
> +
> + if (ret < 0) {
> + fput(sync_file->file);
> + return ret;
> + }
> + *handle = ret;
> + return 0;
> +}
> +
> +static int drm_syncobj_info(struct drm_file *file_private,
> + u32 handle, u32 *type, u32 *flags)
> +{
> + struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
> +
> + if (!sync_file)
> + return -EINVAL;
> + *type = sync_file->type;
> + *flags = sync_file->flags;
> + return 0;
> +}
Do we really need this? I'd have assumed that all the drm_syncobj are
guaranteed to be of type sema. Why else would you want to store them in an
idr for future reuse?
-Daniel
> +
> +/**
> + * drm_syncobj_open - initalizes syncobj file-private structures at devnode open time
> + * @dev: drm_device which is being opened by userspace
> + * @file_private: drm file-private structure to set up
> + *
> + * Called at device open time, sets up the structure for handling refcounting
> + * of sync objects.
> + */
> +void
> +drm_syncobj_open(struct drm_file *file_private)
> +{
> + idr_init(&file_private->syncobj_idr);
> + spin_lock_init(&file_private->syncobj_table_lock);
> +}
> +
> +static int
> +drm_syncobj_release_handle(int id, void *ptr, void *data)
> +{
> + struct sync_file *sync_file = ptr;
> +
> + fput(sync_file->file);
> + return 0;
> +}
> +
> +/**
> + * drm_syncobj_release - release file-private sync object resources
> + * @dev: drm_device which is being closed by userspace
> + * @file_private: drm file-private structure to clean up
> + *
> + * Called at close time when the filp is going away.
> + *
> + * Releases any remaining references on objects by this filp.
> + */
> +void
> +drm_syncobj_release(struct drm_file *file_private)
> +{
> + idr_for_each(&file_private->syncobj_idr,
> + &drm_syncobj_release_handle, file_private);
> + idr_destroy(&file_private->syncobj_idr);
> +}
> +
> +int
> +drm_syncobj_create_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private)
> +{
> + struct drm_syncobj_create_info *args = data;
> +
> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> + return -ENODEV;
> +
> + return drm_syncobj_create(file_private, args->type,
> + args->flags, &args->handle);
> +}
> +
> +int
> +drm_syncobj_destroy_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private)
> +{
> + struct drm_syncobj_destroy *args = data;
> +
> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> + return -ENODEV;
> +
> + drm_syncobj_destroy(file_private, args->handle);
> + return 0;
> +}
> +
> +
> +int
> +drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private)
> +{
> + struct drm_syncobj_handle *args = data;
> +
> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> + return -ENODEV;
> +
> + return drm_syncobj_handle_to_fd(file_private, args->handle,
> + &args->fd);
> +
> +}
> +
> +int
> +drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private)
> +{
> + struct drm_syncobj_handle *args = data;
> +
> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> + return -ENODEV;
> +
> + return drm_syncobj_fd_to_handle(file_private, args->fd,
> + &args->handle);
> +
> +}
> +
> +int
> +drm_syncobj_info_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_private)
> +{
> + struct drm_syncobj_create_info *args = data;
> +
> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> + return -ENODEV;
> +
> + return drm_syncobj_info(file_private, args->handle,
> + &args->type, &args->flags);
> +}
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 6105c05..1d48f6f 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -405,6 +405,11 @@ struct drm_file {
struct drm_file is now in drm_file.h, and with fixed up kernel-doc. You
need @memeber: before the text to make it work.
> /** Lock for synchronization of access to object_idr. */
> spinlock_t table_lock;
>
> + /** Mapping of sync object handles to object pointers. */
> + struct idr syncobj_idr;
> + /** Lock for synchronization of access to syncobj_idr. */
> + spinlock_t syncobj_table_lock;
> +
> struct file *filp;
> void *driver_priv;
>
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 5699f42..48ff06b 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -53,6 +53,7 @@ struct drm_mode_create_dumb;
> #define DRIVER_RENDER 0x8000
> #define DRIVER_ATOMIC 0x10000
> #define DRIVER_KMS_LEGACY_CONTEXT 0x20000
> +#define DRIVER_SYNCOBJ 0x40000
>
> /**
> * struct drm_driver - DRM driver structure
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b2c5284..26b7e86 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -647,6 +647,7 @@ struct drm_gem_open {
> #define DRM_CAP_CURSOR_HEIGHT 0x9
> #define DRM_CAP_ADDFB2_MODIFIERS 0x10
> #define DRM_CAP_PAGE_FLIP_TARGET 0x11
> +#define DRM_CAP_SYNCOBJ 0x12
>
> /** DRM_IOCTL_GET_CAP ioctl argument type */
> struct drm_get_cap {
> @@ -696,6 +697,26 @@ struct drm_prime_handle {
> __s32 fd;
> };
>
> +struct drm_syncobj_create_info {
> + __u32 handle;
> + __u32 type;
> + __u32 flags;
> + __u32 pad;
> +};
> +
> +struct drm_syncobj_destroy {
> + __u32 handle;
> + __u32 pad;
> +};
> +
> +struct drm_syncobj_handle {
> + __u32 handle;
> + /** Flags.. only applicable for handle->fd */
> + __u32 flags;
> +
> + __s32 fd;
> +};
> +
> #if defined(__cplusplus)
> }
> #endif
> @@ -814,6 +835,12 @@ extern "C" {
> #define DRM_IOCTL_MODE_CREATEPROPBLOB DRM_IOWR(0xBD, struct drm_mode_create_blob)
> #define DRM_IOCTL_MODE_DESTROYPROPBLOB DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
>
> +#define DRM_IOCTL_SYNCOBJ_CREATE DRM_IOWR(0xBF, struct drm_syncobj_create_info)
> +#define DRM_IOCTL_SYNCOBJ_DESTROY DRM_IOWR(0xC0, struct drm_syncobj_destroy)
> +#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD DRM_IOWR(0xC1, struct drm_syncobj_handle)
> +#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE DRM_IOWR(0xC2, struct drm_syncobj_handle)
> +#define DRM_IOCTL_SYNCOBJ_INFO DRM_IOWR(0xC3, struct drm_syncobj_create_info)
> +
> /**
> * Device specific ioctls should only be in their respective headers
> * The device specific ioctl range is from 0x40 to 0x9f.
> --
> 2.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/8] sync_file: add a mutex to protect fence and callback members. (v4)
2017-04-04 4:27 ` [PATCH 4/8] sync_file: add a mutex to protect fence and callback members. (v4) Dave Airlie
@ 2017-04-04 7:52 ` Daniel Vetter
2017-04-04 8:07 ` Christian König
1 sibling, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2017-04-04 7:52 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
On Tue, Apr 04, 2017 at 02:27:29PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This patch allows the underlying fence in a sync_file to be changed
> or set to NULL. This isn't currently required but for Vulkan
> semaphores we need to be able to swap and reset the fence.
>
> In order to faciliate this, it uses rcu to protect the fence,
> along with a new mutex. The mutex also protects the callback.
> It also checks for NULL when retrieving the rcu protected
> fence in case it has been reset.
>
> v1.1: fix the locking (Julia Lawall).
> v2: use rcu try one
> v3: fix poll to use proper rcu, fixup merge/fill ioctls
> to not crash on NULL fence cases.
> v4: use rcu in even more places, add missing fput
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> drivers/dma-buf/sync_file.c | 135 +++++++++++++++++++++++++++++++++++---------
> include/linux/sync_file.h | 6 +-
> 2 files changed, 112 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 153bf03..6376f6f 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -28,6 +28,10 @@
>
> static const struct file_operations sync_file_fops;
>
> +#define sync_file_held(obj) lockdep_is_held(&(obj)->lock)
> +#define sync_file_assert_held(obj) \
> + lockdep_assert_held(&(obj)->lock)
> +
> /**
> * sync_file_validate_type_flags - validate type/flags for support
> * @type: type of sync file object
> @@ -81,6 +85,10 @@ struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
>
> sync_file->type = type;
> sync_file->flags = flags;
> +
> + RCU_INIT_POINTER(sync_file->fence, NULL);
> +
> + mutex_init(&sync_file->lock);
> return sync_file;
>
> err:
> @@ -117,7 +125,9 @@ struct sync_file *sync_file_create(struct dma_fence *fence,
> if (!sync_file)
> return NULL;
>
> - sync_file->fence = dma_fence_get(fence);
> + dma_fence_get(fence);
> +
> + RCU_INIT_POINTER(sync_file->fence, fence);
>
> snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
> fence->ops->get_driver_name(fence),
> @@ -168,13 +178,28 @@ struct dma_fence *sync_file_get_fence(int fd)
> if (!sync_file)
> return NULL;
>
> - fence = dma_fence_get(sync_file->fence);
> + if (!rcu_access_pointer(sync_file->fence)) {
> + fput(sync_file->file);
> + return NULL;
> + }
> +
> + rcu_read_lock();
> + fence = dma_fence_get_rcu_safe(&sync_file->fence);
> + rcu_read_unlock();
Looks like room for a dma_fence_get_rcu_unlocked variant. Optional
bikeshed :-)
> +
> fput(sync_file->file);
>
> return fence;
> }
> EXPORT_SYMBOL(sync_file_get_fence);
>
> +static inline struct dma_fence *
> +sync_file_get_fence_locked(struct sync_file *sync_file)
> +{
> + return rcu_dereference_protected(sync_file->fence,
> + sync_file_held(sync_file));
> +}
> +
> static int sync_file_set_fence(struct sync_file *sync_file,
> struct dma_fence **fences, int num_fences)
> {
> @@ -187,7 +212,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
> * we own the reference of the dma_fence_array creation.
> */
> if (num_fences == 1) {
> - sync_file->fence = fences[0];
> + RCU_INIT_POINTER(sync_file->fence, fences[0]);
> kfree(fences);
> } else {
> array = dma_fence_array_create(num_fences, fences,
> @@ -196,24 +221,30 @@ static int sync_file_set_fence(struct sync_file *sync_file,
> if (!array)
> return -ENOMEM;
>
> - sync_file->fence = &array->base;
> + RCU_INIT_POINTER(sync_file->fence, &array->base);
> }
>
> return 0;
> }
>
> -static struct dma_fence **get_fences(struct sync_file *sync_file,
> +/* must be called with rcu read lock taken */
> +static struct dma_fence **get_fences(struct dma_fence **fence,
> int *num_fences)
> {
> - if (dma_fence_is_array(sync_file->fence)) {
> - struct dma_fence_array *array = to_dma_fence_array(sync_file->fence);
> + if (!*fence) {
> + *num_fences = 0;
> + return NULL;
> + }
> +
> + if (dma_fence_is_array(*fence)) {
> + struct dma_fence_array *array = to_dma_fence_array(*fence);
>
> *num_fences = array->num_fences;
> return array->fences;
> }
>
> *num_fences = 1;
> - return &sync_file->fence;
> + return fence;
> }
>
> static void add_fence(struct dma_fence **fences,
> @@ -243,18 +274,31 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
> struct sync_file *sync_file;
> struct dma_fence **fences, **nfences, **a_fences, **b_fences;
> int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
> + struct dma_fence *a_fence, *b_fence;
>
> if (a->type != b->type)
> return NULL;
>
> - sync_file = sync_file_alloc(a->type, a->flags);
> - if (!sync_file)
> + if (!rcu_access_pointer(a->fence) ||
> + !rcu_access_pointer(b->fence))
> return NULL;
>
> - a_fences = get_fences(a, &a_num_fences);
> - b_fences = get_fences(b, &b_num_fences);
> + rcu_read_lock();
> + a_fence = dma_fence_get_rcu_safe(&a->fence);
> + b_fence = dma_fence_get_rcu_safe(&b->fence);
> + rcu_read_unlock();
> +
> + a_fences = get_fences(&a_fence, &a_num_fences);
> + b_fences = get_fences(&b_fence, &b_num_fences);
> + if (!a_num_fences || !b_num_fences)
> + goto put_src_fences;
> +
> if (a_num_fences > INT_MAX - b_num_fences)
> - return NULL;
> + goto put_src_fences;
> +
> + sync_file = sync_file_alloc(a->type, a->flags);
> + if (!sync_file)
> + goto put_src_fences;
>
> num_fences = a_num_fences + b_num_fences;
>
> @@ -315,11 +359,16 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
> goto err;
> }
>
> + dma_fence_put(a_fence);
> + dma_fence_put(b_fence);
> strlcpy(sync_file->name, name, sizeof(sync_file->name));
> return sync_file;
>
> err:
> fput(sync_file->file);
> +put_src_fences:
> + dma_fence_put(a_fence);
> + dma_fence_put(b_fence);
> return NULL;
>
> }
> @@ -328,10 +377,15 @@ static void sync_file_free(struct kref *kref)
> {
> struct sync_file *sync_file = container_of(kref, struct sync_file,
> kref);
> + struct dma_fence *fence;
> +
> + fence = rcu_dereference_protected(sync_file->fence, 1);
> + if (fence) {
> + if (test_bit(POLL_ENABLED, &fence->flags))
> + dma_fence_remove_callback(fence, &sync_file->cb);
> + dma_fence_put(fence);
> + }
>
> - if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
> - dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
> - dma_fence_put(sync_file->fence);
> kfree(sync_file);
> }
>
> @@ -346,16 +400,25 @@ static int sync_file_release(struct inode *inode, struct file *file)
> static unsigned int sync_file_poll(struct file *file, poll_table *wait)
> {
> struct sync_file *sync_file = file->private_data;
> + unsigned int ret_val = 0;
> + struct dma_fence *fence;
>
> poll_wait(file, &sync_file->wq, wait);
>
> - if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) {
> - if (dma_fence_add_callback(sync_file->fence, &sync_file->cb,
> - fence_check_cb_func) < 0)
> - wake_up_all(&sync_file->wq);
> + mutex_lock(&sync_file->lock);
> +
> + fence = sync_file_get_fence_locked(sync_file);
> + if (fence) {
> + if (!test_and_set_bit(POLL_ENABLED, &fence->flags)) {
This seems to be a pre-existing bug, but if someone else already enabled
polling (e.g. driver uses this as an in-fence, later on userspace polls)
we seem to fail to install our wakeup cb for sync_file. Seems buggy.
Besides this one I didn't spot anything wrong, and since it's not one of
your own doing:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> + if (dma_fence_add_callback(fence, &sync_file->cb,
> + fence_check_cb_func) < 0)
> + wake_up_all(&sync_file->wq);
> + }
> + ret_val = dma_fence_is_signaled(fence) ? POLLIN : 0;
> }
> + mutex_unlock(&sync_file->lock);
>
> - return dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
> + return ret_val;
> }
>
> static long sync_file_ioctl_merge(struct sync_file *sync_file,
> @@ -431,6 +494,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> struct sync_file_info info;
> struct sync_fence_info *fence_info = NULL;
> struct dma_fence **fences;
> + struct dma_fence *fence;
> __u32 size;
> int num_fences, ret, i;
>
> @@ -440,7 +504,15 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> if (info.flags || info.pad)
> return -EINVAL;
>
> - fences = get_fences(sync_file, &num_fences);
> + rcu_read_lock();
> + fence = dma_fence_get_rcu_safe(&sync_file->fence);
> + rcu_read_unlock();
> +
> + fences = get_fences(&fence, &num_fences);
> +
> + /* if there are no fences in the sync_file just return */
> + if (!num_fences)
> + goto no_fences;
>
> /*
> * Passing num_fences = 0 means that userspace doesn't want to
> @@ -451,13 +523,17 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> if (!info.num_fences)
> goto no_fences;
>
> - if (info.num_fences < num_fences)
> - return -EINVAL;
> + if (info.num_fences < num_fences) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> size = num_fences * sizeof(*fence_info);
> fence_info = kzalloc(size, GFP_KERNEL);
> - if (!fence_info)
> - return -ENOMEM;
> + if (!fence_info) {
> + ret = -ENOMEM;
> + goto out;
> + }
>
> for (i = 0; i < num_fences; i++)
> sync_fill_fence_info(fences[i], &fence_info[i]);
> @@ -470,7 +546,10 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>
> no_fences:
> strlcpy(info.name, sync_file->name, sizeof(info.name));
> - info.status = dma_fence_is_signaled(sync_file->fence);
> + if (num_fences)
> + info.status = dma_fence_is_signaled(sync_file->fence);
> + else
> + info.status = -ENOENT;
> info.num_fences = num_fences;
>
> if (copy_to_user((void __user *)arg, &info, sizeof(info)))
> @@ -480,7 +559,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>
> out:
> kfree(fence_info);
> -
> + dma_fence_put(fence);
> return ret;
> }
>
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index e683dd1..4bf661b 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -34,6 +34,7 @@
> * @cb: fence callback information
> * @type: sync file type
> * @flags: flags used to create sync file
> + * @lock: mutex to protect fence/cb - used for semaphores
> */
> struct sync_file {
> struct file *file;
> @@ -45,10 +46,13 @@ struct sync_file {
>
> wait_queue_head_t wq;
>
> - struct dma_fence *fence;
> + struct dma_fence __rcu *fence;
> struct dma_fence_cb cb;
> uint32_t type;
> uint32_t flags;
> +
> + /* protects the fence pointer and cb */
> + struct mutex lock;
> };
>
> #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
> --
> 2.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/8] sync_file: add support for a semaphore object
2017-04-04 4:27 ` [PATCH 5/8] sync_file: add support for a semaphore object Dave Airlie
@ 2017-04-04 7:59 ` Daniel Vetter
2017-04-04 11:52 ` Chris Wilson
1 sibling, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2017-04-04 7:59 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
On Tue, Apr 04, 2017 at 02:27:30PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This object can be used to implement the Vulkan semaphores.
>
> The object behaviour differs from fence, in that you can
> replace the underlying fence, and you cannot merge semaphores.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
Since you're going to all the trouble of having distinct types of
sync_file:
- I think we should reject non-TYPE_FENCE sync_file in
sync_file_get_fence. This is to make sure no one can pull existing
userspace over the table. Fence vs. sema also have different semantics,
so I expect we'll have separate cs flags anyway.
- Already mentioned in the drm_syncobj patch, but I'd reorder that one
after this one here and restrict drm_syncobj to only TYPE_SEMA. At least
I can't see any reason why you'd want to make a plain fence persistent
using an idr, they're kinda ephemeral.
A few more minor nits below. Patch itself looks good, but I want to figure
the type confusion semantics out first.
Cheers, Daniel
> ---
> drivers/dma-buf/sync_file.c | 36 +++++++++++++++++++++++++++++++++++-
> include/linux/sync_file.h | 2 ++
> include/uapi/linux/sync_file.h | 14 ++++++++++++++
> 3 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 6376f6f..a82f6d8 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -44,7 +44,7 @@ int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
> {
> if (flags)
> return -EINVAL;
> - if (type != SYNC_FILE_TYPE_FENCE)
> + if (type != SYNC_FILE_TYPE_FENCE && type != SYNC_FILE_TYPE_SEMAPHORE)
> return -EINVAL;
> return 0;
> }
> @@ -200,6 +200,38 @@ sync_file_get_fence_locked(struct sync_file *sync_file)
> sync_file_held(sync_file));
> }
>
> +/**
> + * sync_file_replace_fence - replace the fence related to the sync_file
> + * @sync_file: sync file to replace fence in
> + * @fence: fence to replace with (or NULL for no fence).
Maybe mention here that this is only valid for TYPE_SEMA?
> + * Returns previous fence.
> + */
> +struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
> + struct dma_fence *fence)
> +{
> + struct dma_fence *ret_fence = NULL;
> +
> + if (sync_file->type != SYNC_FILE_TYPE_SEMAPHORE)
I think this could hide bugs, should we wrap it into a if(WARN_ON())?
> + return NULL;
> +
> + if (fence)
> + dma_fence_get(fence);
> +
> + mutex_lock(&sync_file->lock);
> +
> + ret_fence = sync_file_get_fence_locked(sync_file);
> + if (ret_fence) {
> + if (test_bit(POLL_ENABLED, &ret_fence->flags))
> + dma_fence_remove_callback(ret_fence, &sync_file->cb);
> + }
> +
> + RCU_INIT_POINTER(sync_file->fence, fence);
> +
> + mutex_unlock(&sync_file->lock);
> + return ret_fence;
> +}
> +EXPORT_SYMBOL(sync_file_replace_fence);
> +
> static int sync_file_set_fence(struct sync_file *sync_file,
> struct dma_fence **fences, int num_fences)
> {
> @@ -278,6 +310,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
>
> if (a->type != b->type)
> return NULL;
> + if (a->type != SYNC_FILE_TYPE_FENCE)
> + return NULL;
>
> if (!rcu_access_pointer(a->fence) ||
> !rcu_access_pointer(b->fence))
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index 4bf661b..245c7da 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -62,4 +62,6 @@ struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags);
> struct sync_file *sync_file_create(struct dma_fence *fence, uint32_t type, uint32_t flags);
> struct dma_fence *sync_file_get_fence(int fd);
> struct sync_file *sync_file_fdget(int fd);
> +struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
> + struct dma_fence *fence);
> #endif /* _LINUX_SYNC_H */
> diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
> index f439cda..5f266e0 100644
> --- a/include/uapi/linux/sync_file.h
> +++ b/include/uapi/linux/sync_file.h
> @@ -80,6 +80,20 @@ struct sync_file_info {
> #define SYNC_FILE_TYPE_FENCE 0
>
> /**
> + * DOC: SYNC_FILE_TYPE_SEMAPHORE - semaphore sync file object
Iirc you don't need the DOC: - kerneldoc should pick up #defines as-is.
You can iirc reference them from within kernel-doc using %DEFINED_THING.
> + *
> + * This is a sync file that operates like a Vulkan semaphore.
> + * The object should just be imported/exported but not use the
> + * sync file ioctls (except info).
> + * This object can have it's backing fence replaced multiple times.
> + * Each signal operation assigns a backing fence.
> + * Each wait operation waits on the current fence, and removes it.
> + * These operations should happen via driver command submission interfaces.
> + * This is useful for shared vulkan semaphores.
> + */
> +#define SYNC_FILE_TYPE_SEMAPHORE 1
> +
> +/**
> * struct sync_file_type - data returned from sync file type ioctl
> * @type: sync_file type
> * @flags: sync_file creation flags
> --
> 2.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] DRM synchronisation objects
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
` (8 preceding siblings ...)
2017-04-04 4:35 ` [RFC] DRM synchronisation objects Dave Airlie
@ 2017-04-04 8:02 ` Christian König
2017-04-04 8:11 ` Daniel Vetter
10 siblings, 0 replies; 31+ messages in thread
From: Christian König @ 2017-04-04 8:02 UTC (permalink / raw)
To: Dave Airlie, dri-devel
Am 04.04.2017 um 06:27 schrieb Dave Airlie:
> This series enhances my previous semaphore work on for amdgpu,
> with a generic DRM sync object. (drm_syncobj).
>
> It first enhances sync_file to have a type/flags so we can have
> different semantics for different sync files, and a wait
> to retrieve the type of sync_file for userspace.
>
> Then it adds drm sync objects which are just a drm wrapper around
> a sync_file object, allowing creation/info/destroy and import/export
> of the objects.
>
> Next it enhances sync_file to have semaphore semantics for Vulkan.
>
> Finally it adds amdgpu support to it's command submission paths to
> use the new code.
>
> I've hopefully fixed up the things pointed out in the last
> review of the sync_file fence changes, I do wonder if we should
> just block poll on semaphore objects as currently I've no use
> case for this.
Patches 1-3 are Reviewed-by: Christian König <christian.koenig@amd.com>.
A few more individual comments on the rest.
Christian.
>
> Dave.
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 6/8] drm/syncobj: add semaphore support helpers.
2017-04-04 4:27 ` [PATCH 6/8] drm/syncobj: add semaphore support helpers Dave Airlie
@ 2017-04-04 8:07 ` Daniel Vetter
0 siblings, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2017-04-04 8:07 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
On Tue, Apr 04, 2017 at 02:27:31PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This just adds two helper interfaces to bridge the gap from
> drivers to sync_file for the semaphore objects.
>
> These will be used by the amdgpu driver.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> drivers/gpu/drm/drm_syncobj.c | 72 +++++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_syncobj.h | 37 ++++++++++++++++++++++
> 2 files changed, 109 insertions(+)
> create mode 100644 include/drm/drm_syncobj.h
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index a3a1ace..6f1b61f 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
Kernel-doc stanza for you to pull in:
diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
index 96b9c34c21e4..d55a22efb7c3 100644
--- a/Documentation/gpu/drm-mm.rst
+++ b/Documentation/gpu/drm-mm.rst
@@ -455,6 +455,12 @@ PRIME Function References
.. kernel-doc:: drivers/gpu/drm/drm_prime.c
:export:
+DRM Syncobjects
+===============
+
+.. kernel-doc:: drivers/gpu/drm/drm_syncobj.c
+ :export:
+
DRM MM Range Allocator
======================
A few links to &struct sync_file and &SYNC_FILE_TYPE_SEMAPHORE sprinkled
into the kerneldoc of the two functions would spice it up a bit :-)
> @@ -39,6 +39,7 @@
> #include <drm/drmP.h>
> #include <linux/sync_file.h>
> #include "drm_internal.h"
> +#include <drm/drm_syncobj.h>
>
> static struct sync_file *drm_syncobj_file_lookup(struct drm_file *file_private,
> u32 handle)
> @@ -55,6 +56,77 @@ static struct sync_file *drm_syncobj_file_lookup(struct drm_file *file_private,
> return sync_file;
> }
>
> +static int drm_syncobj_swap_fences_nocheck(struct drm_file *file_private,
> + uint32_t handle,
> + struct dma_fence *fence,
> + struct dma_fence **old_fence)
> +{
> + struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
Needs the refcount dance I mentioned in the previous drm_syncobj patch.
> +
> + if (!sync_file)
> + return -EINVAL;
> +
> + *old_fence = sync_file_replace_fence(sync_file, fence);
> + return 0;
> +}
If we restrict drm_syncobj to semas, then we can add the WARN_ON in the
previous patch and forgo this unchecked version here.
> +
> +/**
> + * drm_syncobj_swap_fences - lookup and replace fence in a sync object.
> + * @file_private - drm file private pointer.
> + * @handle - sync object handle
> + * @fence - new fence (or NULL) to back sync object.
> + * @old_fence - old fence backing sync object.
> + * Returns: Old fence
> + *
> + * This function is used to swap the fence backing the sync object
> + * with a new one, it returns the old fence in old_fence;
Please add.
Returns:
0 on success, or -EINVAL when the handle doesn't point at a valid
sync_file with %SYNC_FILE_TYPE_SEMAPHORE.
Same below.
Cheers, Daniel
> + */
> +int drm_syncobj_swap_fences(struct drm_file *file_private,
> + uint32_t handle,
> + struct dma_fence *fence,
> + struct dma_fence **old_fence)
> +{
> + int r;
> +
> + r = drm_syncobj_swap_fences_nocheck(file_private,
> + handle,
> + fence,
> + old_fence);
> + if (r)
> + return r;
> +
> +
> + if (!*old_fence)
> + return -EINVAL;
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_syncobj_swap_fences);
> +
> +/**
> + * drm_syncobj_replace_fence - lookup and replace fence in a sync object.
> + * @file_private - drm file private pointer.
> + * @handle - syncobj handle to lookup
> + * @fence - fence to install in sync file.
> + * Returns: 0 on success or -EINVAL on error
> + *
> + * This looks up a sync object and replaces the fence on it, freeing
> + * the old one.
> + */
> +int drm_syncobj_replace_fence(struct drm_file *file_private,
> + u32 handle,
> + struct dma_fence *fence)
> +{
> + struct dma_fence *old_fence;
> + int r;
> +
> + r = drm_syncobj_swap_fences_nocheck(file_private, handle, fence, &old_fence);
> + if (r)
> + return r;
> + dma_fence_put(old_fence);
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_syncobj_replace_fence);
> +
> static int drm_syncobj_create(struct drm_file *file_private,
> unsigned type,
> unsigned flags, u32 *handle)
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> new file mode 100644
> index 0000000..f614e06
> --- /dev/null
> +++ b/include/drm/drm_syncobj.h
> @@ -0,0 +1,37 @@
> +/*
> + * Copyright © 2017 Red Hat
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *
> + */
> +#ifndef __DRM_SYNCOBJ_H__
> +#define __DRM_SYNCOBJ_H__
> +
> +int drm_syncobj_swap_fences(struct drm_file *file_private,
> + uint32_t handle,
> + struct dma_fence *fence,
> + struct dma_fence **old_fence);
> +int drm_syncobj_replace_fence(struct drm_file *file_private,
> + u32 handle,
> + struct dma_fence *fence);
> +
> +#endif
> --
> 2.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 4/8] sync_file: add a mutex to protect fence and callback members. (v4)
2017-04-04 4:27 ` [PATCH 4/8] sync_file: add a mutex to protect fence and callback members. (v4) Dave Airlie
2017-04-04 7:52 ` Daniel Vetter
@ 2017-04-04 8:07 ` Christian König
2017-04-11 2:57 ` Dave Airlie
1 sibling, 1 reply; 31+ messages in thread
From: Christian König @ 2017-04-04 8:07 UTC (permalink / raw)
To: Dave Airlie, dri-devel
Am 04.04.2017 um 06:27 schrieb Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
>
> This patch allows the underlying fence in a sync_file to be changed
> or set to NULL. This isn't currently required but for Vulkan
> semaphores we need to be able to swap and reset the fence.
>
> In order to faciliate this, it uses rcu to protect the fence,
> along with a new mutex. The mutex also protects the callback.
> It also checks for NULL when retrieving the rcu protected
> fence in case it has been reset.
>
> v1.1: fix the locking (Julia Lawall).
> v2: use rcu try one
> v3: fix poll to use proper rcu, fixup merge/fill ioctls
> to not crash on NULL fence cases.
> v4: use rcu in even more places, add missing fput
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> drivers/dma-buf/sync_file.c | 135 +++++++++++++++++++++++++++++++++++---------
> include/linux/sync_file.h | 6 +-
> 2 files changed, 112 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 153bf03..6376f6f 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -28,6 +28,10 @@
>
> static const struct file_operations sync_file_fops;
>
> +#define sync_file_held(obj) lockdep_is_held(&(obj)->lock)
> +#define sync_file_assert_held(obj) \
> + lockdep_assert_held(&(obj)->lock)
> +
> /**
> * sync_file_validate_type_flags - validate type/flags for support
> * @type: type of sync file object
> @@ -81,6 +85,10 @@ struct sync_file *sync_file_alloc(uint32_t type, uint32_t flags)
>
> sync_file->type = type;
> sync_file->flags = flags;
> +
> + RCU_INIT_POINTER(sync_file->fence, NULL);
> +
> + mutex_init(&sync_file->lock);
> return sync_file;
>
> err:
> @@ -117,7 +125,9 @@ struct sync_file *sync_file_create(struct dma_fence *fence,
> if (!sync_file)
> return NULL;
>
> - sync_file->fence = dma_fence_get(fence);
> + dma_fence_get(fence);
> +
> + RCU_INIT_POINTER(sync_file->fence, fence);
>
> snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
> fence->ops->get_driver_name(fence),
> @@ -168,13 +178,28 @@ struct dma_fence *sync_file_get_fence(int fd)
> if (!sync_file)
> return NULL;
>
> - fence = dma_fence_get(sync_file->fence);
> + if (!rcu_access_pointer(sync_file->fence)) {
> + fput(sync_file->file);
> + return NULL;
> + }
> +
> + rcu_read_lock();
> + fence = dma_fence_get_rcu_safe(&sync_file->fence);
> + rcu_read_unlock();
> +
> fput(sync_file->file);
>
> return fence;
> }
> EXPORT_SYMBOL(sync_file_get_fence);
>
> +static inline struct dma_fence *
> +sync_file_get_fence_locked(struct sync_file *sync_file)
> +{
> + return rcu_dereference_protected(sync_file->fence,
> + sync_file_held(sync_file));
> +}
> +
> static int sync_file_set_fence(struct sync_file *sync_file,
> struct dma_fence **fences, int num_fences)
> {
> @@ -187,7 +212,7 @@ static int sync_file_set_fence(struct sync_file *sync_file,
> * we own the reference of the dma_fence_array creation.
> */
> if (num_fences == 1) {
> - sync_file->fence = fences[0];
> + RCU_INIT_POINTER(sync_file->fence, fences[0]);
> kfree(fences);
> } else {
> array = dma_fence_array_create(num_fences, fences,
> @@ -196,24 +221,30 @@ static int sync_file_set_fence(struct sync_file *sync_file,
> if (!array)
> return -ENOMEM;
>
> - sync_file->fence = &array->base;
> + RCU_INIT_POINTER(sync_file->fence, &array->base);
> }
>
> return 0;
> }
>
> -static struct dma_fence **get_fences(struct sync_file *sync_file,
> +/* must be called with rcu read lock taken */
> +static struct dma_fence **get_fences(struct dma_fence **fence,
> int *num_fences)
> {
> - if (dma_fence_is_array(sync_file->fence)) {
> - struct dma_fence_array *array = to_dma_fence_array(sync_file->fence);
> + if (!*fence) {
> + *num_fences = 0;
> + return NULL;
> + }
> +
> + if (dma_fence_is_array(*fence)) {
> + struct dma_fence_array *array = to_dma_fence_array(*fence);
>
> *num_fences = array->num_fences;
> return array->fences;
> }
>
> *num_fences = 1;
> - return &sync_file->fence;
> + return fence;
> }
>
> static void add_fence(struct dma_fence **fences,
> @@ -243,18 +274,31 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
> struct sync_file *sync_file;
> struct dma_fence **fences, **nfences, **a_fences, **b_fences;
> int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
> + struct dma_fence *a_fence, *b_fence;
>
> if (a->type != b->type)
> return NULL;
>
> - sync_file = sync_file_alloc(a->type, a->flags);
> - if (!sync_file)
> + if (!rcu_access_pointer(a->fence) ||
> + !rcu_access_pointer(b->fence))
> return NULL;
>
> - a_fences = get_fences(a, &a_num_fences);
> - b_fences = get_fences(b, &b_num_fences);
> + rcu_read_lock();
> + a_fence = dma_fence_get_rcu_safe(&a->fence);
> + b_fence = dma_fence_get_rcu_safe(&b->fence);
> + rcu_read_unlock();
> +
> + a_fences = get_fences(&a_fence, &a_num_fences);
> + b_fences = get_fences(&b_fence, &b_num_fences);
> + if (!a_num_fences || !b_num_fences)
> + goto put_src_fences;
> +
> if (a_num_fences > INT_MAX - b_num_fences)
> - return NULL;
> + goto put_src_fences;
> +
> + sync_file = sync_file_alloc(a->type, a->flags);
> + if (!sync_file)
> + goto put_src_fences;
>
> num_fences = a_num_fences + b_num_fences;
>
> @@ -315,11 +359,16 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
> goto err;
> }
>
> + dma_fence_put(a_fence);
> + dma_fence_put(b_fence);
> strlcpy(sync_file->name, name, sizeof(sync_file->name));
> return sync_file;
>
> err:
> fput(sync_file->file);
> +put_src_fences:
> + dma_fence_put(a_fence);
> + dma_fence_put(b_fence);
> return NULL;
>
> }
> @@ -328,10 +377,15 @@ static void sync_file_free(struct kref *kref)
> {
> struct sync_file *sync_file = container_of(kref, struct sync_file,
> kref);
> + struct dma_fence *fence;
> +
> + fence = rcu_dereference_protected(sync_file->fence, 1);
> + if (fence) {
> + if (test_bit(POLL_ENABLED, &fence->flags))
> + dma_fence_remove_callback(fence, &sync_file->cb);
> + dma_fence_put(fence);
> + }
>
> - if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
> - dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
> - dma_fence_put(sync_file->fence);
> kfree(sync_file);
> }
>
> @@ -346,16 +400,25 @@ static int sync_file_release(struct inode *inode, struct file *file)
> static unsigned int sync_file_poll(struct file *file, poll_table *wait)
> {
> struct sync_file *sync_file = file->private_data;
> + unsigned int ret_val = 0;
> + struct dma_fence *fence;
>
> poll_wait(file, &sync_file->wq, wait);
>
> - if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) {
> - if (dma_fence_add_callback(sync_file->fence, &sync_file->cb,
> - fence_check_cb_func) < 0)
> - wake_up_all(&sync_file->wq);
> + mutex_lock(&sync_file->lock);
> +
> + fence = sync_file_get_fence_locked(sync_file);
> + if (fence) {
> + if (!test_and_set_bit(POLL_ENABLED, &fence->flags)) {
> + if (dma_fence_add_callback(fence, &sync_file->cb,
> + fence_check_cb_func) < 0)
> + wake_up_all(&sync_file->wq);
> + }
> + ret_val = dma_fence_is_signaled(fence) ? POLLIN : 0;
> }
> + mutex_unlock(&sync_file->lock);
Maybe I'm a bit confused, but why exactly are you taking the lock here?
It isn't protecting anything anymore as far as I can see.
Or is it to prevent concurrent adding of the same callback? That would
have failed before if I'm not completely mistaken.
Christian.
>
> - return dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
> + return ret_val;
> }
>
> static long sync_file_ioctl_merge(struct sync_file *sync_file,
> @@ -431,6 +494,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> struct sync_file_info info;
> struct sync_fence_info *fence_info = NULL;
> struct dma_fence **fences;
> + struct dma_fence *fence;
> __u32 size;
> int num_fences, ret, i;
>
> @@ -440,7 +504,15 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> if (info.flags || info.pad)
> return -EINVAL;
>
> - fences = get_fences(sync_file, &num_fences);
> + rcu_read_lock();
> + fence = dma_fence_get_rcu_safe(&sync_file->fence);
> + rcu_read_unlock();
> +
> + fences = get_fences(&fence, &num_fences);
> +
> + /* if there are no fences in the sync_file just return */
> + if (!num_fences)
> + goto no_fences;
>
> /*
> * Passing num_fences = 0 means that userspace doesn't want to
> @@ -451,13 +523,17 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> if (!info.num_fences)
> goto no_fences;
>
> - if (info.num_fences < num_fences)
> - return -EINVAL;
> + if (info.num_fences < num_fences) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> size = num_fences * sizeof(*fence_info);
> fence_info = kzalloc(size, GFP_KERNEL);
> - if (!fence_info)
> - return -ENOMEM;
> + if (!fence_info) {
> + ret = -ENOMEM;
> + goto out;
> + }
>
> for (i = 0; i < num_fences; i++)
> sync_fill_fence_info(fences[i], &fence_info[i]);
> @@ -470,7 +546,10 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>
> no_fences:
> strlcpy(info.name, sync_file->name, sizeof(info.name));
> - info.status = dma_fence_is_signaled(sync_file->fence);
> + if (num_fences)
> + info.status = dma_fence_is_signaled(sync_file->fence);
> + else
> + info.status = -ENOENT;
> info.num_fences = num_fences;
>
> if (copy_to_user((void __user *)arg, &info, sizeof(info)))
> @@ -480,7 +559,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>
> out:
> kfree(fence_info);
> -
> + dma_fence_put(fence);
> return ret;
> }
>
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index e683dd1..4bf661b 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -34,6 +34,7 @@
> * @cb: fence callback information
> * @type: sync file type
> * @flags: flags used to create sync file
> + * @lock: mutex to protect fence/cb - used for semaphores
> */
> struct sync_file {
> struct file *file;
> @@ -45,10 +46,13 @@ struct sync_file {
>
> wait_queue_head_t wq;
>
> - struct dma_fence *fence;
> + struct dma_fence __rcu *fence;
> struct dma_fence_cb cb;
> uint32_t type;
> uint32_t flags;
> +
> + /* protects the fence pointer and cb */
> + struct mutex lock;
> };
>
> #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2)
2017-04-04 7:40 ` Christian König
@ 2017-04-04 8:10 ` Daniel Vetter
2017-04-04 11:05 ` Christian König
0 siblings, 1 reply; 31+ messages in thread
From: Daniel Vetter @ 2017-04-04 8:10 UTC (permalink / raw)
To: Christian König; +Cc: dri-devel
On Tue, Apr 04, 2017 at 09:40:57AM +0200, Christian König wrote:
> Am 04.04.2017 um 06:27 schrieb Dave Airlie:
> > From: Dave Airlie <airlied@redhat.com>
> >
> > This creates a new interface for amdgpu with ioctls to
> > create/destroy/import and export shared semaphores using
> > sem object backed by the sync_file code. The semaphores
> > are not installed as fd (except for export), but rather
> > like other driver internal objects in an idr. The idr
> > holds the initial reference on the sync file.
> >
> > The command submission interface is enhanced with two new
> > chunks, one for semaphore waiting, one for semaphore signalling
> > and just takes a list of handles for each.
> >
> > This is based on work originally done by David Zhou at AMD,
> > with input from Christian Konig on what things should look like.
> >
> > NOTE: this interface addition needs a version bump to expose
> > it to userspace.
> >
> > v1.1: keep file reference on import.
> > v2: move to using syncobjs
> >
> > Signed-off-by: Dave Airlie <airlied@redhat.com>
>
> Looks good to me in general, but one note below.
>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 86 ++++++++++++++++++++++++++++++++-
> > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
> > include/uapi/drm/amdgpu_drm.h | 6 +++
> > 3 files changed, 92 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > index 4671432..4dd210b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > @@ -27,6 +27,7 @@
> > #include <linux/pagemap.h>
> > #include <drm/drmP.h>
> > #include <drm/amdgpu_drm.h>
> > +#include <drm/drm_syncobj.h>
> > #include "amdgpu.h"
> > #include "amdgpu_trace.h"
> > @@ -217,6 +218,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
> > break;
> > case AMDGPU_CHUNK_ID_DEPENDENCIES:
> > + case AMDGPU_CHUNK_ID_SEM_WAIT:
> > + case AMDGPU_CHUNK_ID_SEM_SIGNAL:
> > break;
> > default:
> > @@ -1009,6 +1012,44 @@ static int amdgpu_process_fence_dep(struct amdgpu_device *adev,
> > return 0;
> > }
> > +static int amdgpu_sem_lookup_and_sync(struct amdgpu_device *adev,
> > + struct drm_file *file_private,
> > + struct amdgpu_sync *sync,
> > + uint32_t handle)
> > +{
> > + int r;
> > + struct dma_fence *old_fence;
> > + r = drm_syncobj_swap_fences(file_private, handle, NULL, &old_fence);
>
> What happens when we the CS fails later on? Would the semaphore then not
> contain the fence any more?
Atm rules are that you're not allowed to publish a dma_fence before you're
committed to executing whatever it represents (i.e. guaranteed to get
signalled).
Publish depends upon context, but can include: Install an fd for a type
fence sync_file, swap the sync_file in a sema type, assign it to the
reservation_object for implicit sync.
I guess we need to have drm_syncobj_lookup exported (or whatever it was),
keep a temporary pointer, and then swap it only at the end (with
sync_file_swap_fence or whatever it was).
-Daniel
>
> Christian.
>
> > + if (r)
> > + return r;
> > +
> > + r = amdgpu_sync_fence(adev, sync, old_fence);
> > + dma_fence_put(old_fence);
> > +
> > + return r;
> > +}
> > +
> > +static int amdgpu_process_sem_wait_dep(struct amdgpu_device *adev,
> > + struct amdgpu_cs_parser *p,
> > + struct amdgpu_cs_chunk *chunk)
> > +{
> > + unsigned num_deps;
> > + int i, r;
> > + struct drm_amdgpu_cs_chunk_sem *deps;
> > +
> > + deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
> > + num_deps = chunk->length_dw * 4 /
> > + sizeof(struct drm_amdgpu_cs_chunk_sem);
> > +
> > + for (i = 0; i < num_deps; ++i) {
> > + r = amdgpu_sem_lookup_and_sync(adev, p->filp, &p->job->sync,
> > + deps[i].handle);
> > + if (r)
> > + return r;
> > + }
> > + return 0;
> > +}
> > +
> > static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
> > struct amdgpu_cs_parser *p)
> > {
> > @@ -1023,12 +1064,55 @@ static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
> > r = amdgpu_process_fence_dep(adev, p, chunk);
> > if (r)
> > return r;
> > + } else if (chunk->chunk_id == AMDGPU_CHUNK_ID_SEM_WAIT) {
> > + r = amdgpu_process_sem_wait_dep(adev, p, chunk);
> > + if (r)
> > + return r;
> > }
> > }
> > return 0;
> > }
> > +static int amdgpu_process_sem_signal_dep(struct amdgpu_cs_parser *p,
> > + struct amdgpu_cs_chunk *chunk,
> > + struct dma_fence *fence)
> > +{
> > + 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,
> > + fence);
> > + if (r)
> > + return r;
> > + }
> > + return 0;
> > +}
> > +
> > +static int amdgpu_cs_post_dependencies(struct amdgpu_cs_parser *p)
> > +{
> > + int i, r;
> > +
> > + for (i = 0; i < p->nchunks; ++i) {
> > + struct amdgpu_cs_chunk *chunk;
> > +
> > + chunk = &p->chunks[i];
> > +
> > + if (chunk->chunk_id == AMDGPU_CHUNK_ID_SEM_SIGNAL) {
> > + r = amdgpu_process_sem_signal_dep(p, chunk, p->fence);
> > + if (r)
> > + return r;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
> > union drm_amdgpu_cs *cs)
> > {
> > @@ -1056,7 +1140,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
> > trace_amdgpu_cs_ioctl(job);
> > amd_sched_entity_push_job(&job->base);
> > - return 0;
> > + return amdgpu_cs_post_dependencies(p);
> > }
> > int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > index b76cd69..e95951e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > @@ -683,7 +683,7 @@ static struct drm_driver kms_driver = {
> > .driver_features =
> > DRIVER_USE_AGP |
> > DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
> > - DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET,
> > + DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
> > .load = amdgpu_driver_load_kms,
> > .open = amdgpu_driver_open_kms,
> > .preclose = amdgpu_driver_preclose_kms,
> > diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> > index 5797283..647c520 100644
> > --- a/include/uapi/drm/amdgpu_drm.h
> > +++ b/include/uapi/drm/amdgpu_drm.h
> > @@ -390,6 +390,8 @@ struct drm_amdgpu_gem_va {
> > #define AMDGPU_CHUNK_ID_IB 0x01
> > #define AMDGPU_CHUNK_ID_FENCE 0x02
> > #define AMDGPU_CHUNK_ID_DEPENDENCIES 0x03
> > +#define AMDGPU_CHUNK_ID_SEM_WAIT 0x04
> > +#define AMDGPU_CHUNK_ID_SEM_SIGNAL 0x05
> > struct drm_amdgpu_cs_chunk {
> > __u32 chunk_id;
> > @@ -454,6 +456,10 @@ struct drm_amdgpu_cs_chunk_fence {
> > __u32 offset;
> > };
> > +struct drm_amdgpu_cs_chunk_sem {
> > + __u32 handle;
> > +};
> > +
> > struct drm_amdgpu_cs_chunk_data {
> > union {
> > struct drm_amdgpu_cs_chunk_ib ib_data;
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] DRM synchronisation objects
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
` (9 preceding siblings ...)
2017-04-04 8:02 ` Christian König
@ 2017-04-04 8:11 ` Daniel Vetter
10 siblings, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2017-04-04 8:11 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
On Tue, Apr 04, 2017 at 02:27:25PM +1000, Dave Airlie wrote:
> This series enhances my previous semaphore work on for amdgpu,
> with a generic DRM sync object. (drm_syncobj).
>
> It first enhances sync_file to have a type/flags so we can have
> different semantics for different sync files, and a wait
> to retrieve the type of sync_file for userspace.
>
> Then it adds drm sync objects which are just a drm wrapper around
> a sync_file object, allowing creation/info/destroy and import/export
> of the objects.
>
> Next it enhances sync_file to have semaphore semantics for Vulkan.
>
> Finally it adds amdgpu support to it's command submission paths to
> use the new code.
>
> I've hopefully fixed up the things pointed out in the last
> review of the sync_file fence changes, I do wonder if we should
> just block poll on semaphore objects as currently I've no use
> case for this.
Looking pretty good, some minor polish and ack from Jason that this seems
ok for anv too. Pls also add igt testcases for some of the silly cases
(trying to import a non-sync_file fd, bad handle for export, stuff like
that).
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2)
2017-04-04 8:10 ` Daniel Vetter
@ 2017-04-04 11:05 ` Christian König
2017-04-11 3:18 ` Dave Airlie
0 siblings, 1 reply; 31+ messages in thread
From: Christian König @ 2017-04-04 11:05 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
Am 04.04.2017 um 10:10 schrieb Daniel Vetter:
> On Tue, Apr 04, 2017 at 09:40:57AM +0200, Christian König wrote:
>> Am 04.04.2017 um 06:27 schrieb Dave Airlie:
>>> From: Dave Airlie <airlied@redhat.com>
>>>
>>> This creates a new interface for amdgpu with ioctls to
>>> create/destroy/import and export shared semaphores using
>>> sem object backed by the sync_file code. The semaphores
>>> are not installed as fd (except for export), but rather
>>> like other driver internal objects in an idr. The idr
>>> holds the initial reference on the sync file.
>>>
>>> The command submission interface is enhanced with two new
>>> chunks, one for semaphore waiting, one for semaphore signalling
>>> and just takes a list of handles for each.
>>>
>>> This is based on work originally done by David Zhou at AMD,
>>> with input from Christian Konig on what things should look like.
>>>
>>> NOTE: this interface addition needs a version bump to expose
>>> it to userspace.
>>>
>>> v1.1: keep file reference on import.
>>> v2: move to using syncobjs
>>>
>>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> Looks good to me in general, but one note below.
>>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 86 ++++++++++++++++++++++++++++++++-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
>>> include/uapi/drm/amdgpu_drm.h | 6 +++
>>> 3 files changed, 92 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index 4671432..4dd210b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -27,6 +27,7 @@
>>> #include <linux/pagemap.h>
>>> #include <drm/drmP.h>
>>> #include <drm/amdgpu_drm.h>
>>> +#include <drm/drm_syncobj.h>
>>> #include "amdgpu.h"
>>> #include "amdgpu_trace.h"
>>> @@ -217,6 +218,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
>>> break;
>>> case AMDGPU_CHUNK_ID_DEPENDENCIES:
>>> + case AMDGPU_CHUNK_ID_SEM_WAIT:
>>> + case AMDGPU_CHUNK_ID_SEM_SIGNAL:
>>> break;
>>> default:
>>> @@ -1009,6 +1012,44 @@ static int amdgpu_process_fence_dep(struct amdgpu_device *adev,
>>> return 0;
>>> }
>>> +static int amdgpu_sem_lookup_and_sync(struct amdgpu_device *adev,
>>> + struct drm_file *file_private,
>>> + struct amdgpu_sync *sync,
>>> + uint32_t handle)
>>> +{
>>> + int r;
>>> + struct dma_fence *old_fence;
>>> + r = drm_syncobj_swap_fences(file_private, handle, NULL, &old_fence);
>> What happens when we the CS fails later on? Would the semaphore then not
>> contain the fence any more?
> Atm rules are that you're not allowed to publish a dma_fence before you're
> committed to executing whatever it represents (i.e. guaranteed to get
> signalled).
Yeah, I know. But this isn't the signaling case, but rather the waiting
case.
If I'm not completely mistaken what Dave does here is retrieving the
fence from a semaphore object we need to wait for and setting it to NULL.
If the command submission fails with -ERESTARTSYS after this is done the
retried IOCTL will behave incorrectly.
Regards,
Christian.
>
> Publish depends upon context, but can include: Install an fd for a type
> fence sync_file, swap the sync_file in a sema type, assign it to the
> reservation_object for implicit sync.
>
> I guess we need to have drm_syncobj_lookup exported (or whatever it was),
> keep a temporary pointer, and then swap it only at the end (with
> sync_file_swap_fence or whatever it was).
>
> -Daniel
>
>> Christian.
>>
>>> + if (r)
>>> + return r;
>>> +
>>> + r = amdgpu_sync_fence(adev, sync, old_fence);
>>> + dma_fence_put(old_fence);
>>> +
>>> + return r;
>>> +}
>>> +
>>> +static int amdgpu_process_sem_wait_dep(struct amdgpu_device *adev,
>>> + struct amdgpu_cs_parser *p,
>>> + struct amdgpu_cs_chunk *chunk)
>>> +{
>>> + unsigned num_deps;
>>> + int i, r;
>>> + struct drm_amdgpu_cs_chunk_sem *deps;
>>> +
>>> + deps = (struct drm_amdgpu_cs_chunk_sem *)chunk->kdata;
>>> + num_deps = chunk->length_dw * 4 /
>>> + sizeof(struct drm_amdgpu_cs_chunk_sem);
>>> +
>>> + for (i = 0; i < num_deps; ++i) {
>>> + r = amdgpu_sem_lookup_and_sync(adev, p->filp, &p->job->sync,
>>> + deps[i].handle);
>>> + if (r)
>>> + return r;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
>>> struct amdgpu_cs_parser *p)
>>> {
>>> @@ -1023,12 +1064,55 @@ static int amdgpu_cs_dependencies(struct amdgpu_device *adev,
>>> r = amdgpu_process_fence_dep(adev, p, chunk);
>>> if (r)
>>> return r;
>>> + } else if (chunk->chunk_id == AMDGPU_CHUNK_ID_SEM_WAIT) {
>>> + r = amdgpu_process_sem_wait_dep(adev, p, chunk);
>>> + if (r)
>>> + return r;
>>> }
>>> }
>>> return 0;
>>> }
>>> +static int amdgpu_process_sem_signal_dep(struct amdgpu_cs_parser *p,
>>> + struct amdgpu_cs_chunk *chunk,
>>> + struct dma_fence *fence)
>>> +{
>>> + 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,
>>> + fence);
>>> + if (r)
>>> + return r;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static int amdgpu_cs_post_dependencies(struct amdgpu_cs_parser *p)
>>> +{
>>> + int i, r;
>>> +
>>> + for (i = 0; i < p->nchunks; ++i) {
>>> + struct amdgpu_cs_chunk *chunk;
>>> +
>>> + chunk = &p->chunks[i];
>>> +
>>> + if (chunk->chunk_id == AMDGPU_CHUNK_ID_SEM_SIGNAL) {
>>> + r = amdgpu_process_sem_signal_dep(p, chunk, p->fence);
>>> + if (r)
>>> + return r;
>>> + }
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>>> union drm_amdgpu_cs *cs)
>>> {
>>> @@ -1056,7 +1140,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>>> trace_amdgpu_cs_ioctl(job);
>>> amd_sched_entity_push_job(&job->base);
>>> - return 0;
>>> + return amdgpu_cs_post_dependencies(p);
>>> }
>>> int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> index b76cd69..e95951e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> @@ -683,7 +683,7 @@ static struct drm_driver kms_driver = {
>>> .driver_features =
>>> DRIVER_USE_AGP |
>>> DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM |
>>> - DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET,
>>> + DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
>>> .load = amdgpu_driver_load_kms,
>>> .open = amdgpu_driver_open_kms,
>>> .preclose = amdgpu_driver_preclose_kms,
>>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>>> index 5797283..647c520 100644
>>> --- a/include/uapi/drm/amdgpu_drm.h
>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>> @@ -390,6 +390,8 @@ struct drm_amdgpu_gem_va {
>>> #define AMDGPU_CHUNK_ID_IB 0x01
>>> #define AMDGPU_CHUNK_ID_FENCE 0x02
>>> #define AMDGPU_CHUNK_ID_DEPENDENCIES 0x03
>>> +#define AMDGPU_CHUNK_ID_SEM_WAIT 0x04
>>> +#define AMDGPU_CHUNK_ID_SEM_SIGNAL 0x05
>>> struct drm_amdgpu_cs_chunk {
>>> __u32 chunk_id;
>>> @@ -454,6 +456,10 @@ struct drm_amdgpu_cs_chunk_fence {
>>> __u32 offset;
>>> };
>>> +struct drm_amdgpu_cs_chunk_sem {
>>> + __u32 handle;
>>> +};
>>> +
>>> struct drm_amdgpu_cs_chunk_data {
>>> union {
>>> struct drm_amdgpu_cs_chunk_ib ib_data;
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/8] sync_file: add support for a semaphore object
2017-04-04 4:27 ` [PATCH 5/8] sync_file: add support for a semaphore object Dave Airlie
2017-04-04 7:59 ` Daniel Vetter
@ 2017-04-04 11:52 ` Chris Wilson
2017-04-04 11:59 ` Chris Wilson
1 sibling, 1 reply; 31+ messages in thread
From: Chris Wilson @ 2017-04-04 11:52 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
On Tue, Apr 04, 2017 at 02:27:30PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This object can be used to implement the Vulkan semaphores.
>
> The object behaviour differs from fence, in that you can
> replace the underlying fence, and you cannot merge semaphores.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> drivers/dma-buf/sync_file.c | 36 +++++++++++++++++++++++++++++++++++-
> include/linux/sync_file.h | 2 ++
> include/uapi/linux/sync_file.h | 14 ++++++++++++++
> 3 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 6376f6f..a82f6d8 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -44,7 +44,7 @@ int sync_file_validate_type_flags(uint32_t type, uint32_t flags)
> {
> if (flags)
> return -EINVAL;
> - if (type != SYNC_FILE_TYPE_FENCE)
> + if (type != SYNC_FILE_TYPE_FENCE && type != SYNC_FILE_TYPE_SEMAPHORE)
> return -EINVAL;
> return 0;
> }
> @@ -200,6 +200,38 @@ sync_file_get_fence_locked(struct sync_file *sync_file)
> sync_file_held(sync_file));
> }
>
> +/**
> + * sync_file_replace_fence - replace the fence related to the sync_file
> + * @sync_file: sync file to replace fence in
> + * @fence: fence to replace with (or NULL for no fence).
> + * Returns previous fence.
> + */
> +struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
> + struct dma_fence *fence)
> +{
> + struct dma_fence *ret_fence = NULL;
> +
> + if (sync_file->type != SYNC_FILE_TYPE_SEMAPHORE)
> + return NULL;
> +
> + if (fence)
> + dma_fence_get(fence);
> +
> + mutex_lock(&sync_file->lock);
> +
> + ret_fence = sync_file_get_fence_locked(sync_file);
> + if (ret_fence) {
> + if (test_bit(POLL_ENABLED, &ret_fence->flags))
> + dma_fence_remove_callback(ret_fence, &sync_file->cb);
This is racy with sync_file_poll. And sync_file_poll now needs rcu
protection (as does all access to sync_file->fence), I need to check
whether the previous patches are complete. Also needs to handle, or
forbid, the caller passing in the same fence.
> + }
> +
> + RCU_INIT_POINTER(sync_file->fence, fence);
> +
> + mutex_unlock(&sync_file->lock);
> + return ret_fence;
> +}
> +EXPORT_SYMBOL(sync_file_replace_fence);
> +
> static int sync_file_set_fence(struct sync_file *sync_file,
> struct dma_fence **fences, int num_fences)
> {
> @@ -278,6 +310,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
>
> if (a->type != b->type)
> return NULL;
> + if (a->type != SYNC_FILE_TYPE_FENCE)
> + return NULL;
No rules for containerisation? Being able to pass in an array of current
semaphore in-fences through a single fd? It will have a bigger window
for change than doing everything inside the kernel, but since the kernel
cannot take a simultaneous snaphot of all in-semaphores either (could
use ww_mutex?) it seems harsh to exclude current ABI interop.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/8] sync_file: add support for a semaphore object
2017-04-04 11:52 ` Chris Wilson
@ 2017-04-04 11:59 ` Chris Wilson
0 siblings, 0 replies; 31+ messages in thread
From: Chris Wilson @ 2017-04-04 11:59 UTC (permalink / raw)
To: Dave Airlie, dri-devel
On Tue, Apr 04, 2017 at 12:52:32PM +0100, Chris Wilson wrote:
> On Tue, Apr 04, 2017 at 02:27:30PM +1000, Dave Airlie wrote:
> > +struct dma_fence *sync_file_replace_fence(struct sync_file *sync_file,
> > + struct dma_fence *fence)
> > +{
> > + struct dma_fence *ret_fence = NULL;
> > +
> > + if (sync_file->type != SYNC_FILE_TYPE_SEMAPHORE)
> > + return NULL;
> > +
> > + if (fence)
> > + dma_fence_get(fence);
> > +
> > + mutex_lock(&sync_file->lock);
> > +
> > + ret_fence = sync_file_get_fence_locked(sync_file);
> > + if (ret_fence) {
> > + if (test_bit(POLL_ENABLED, &ret_fence->flags))
> > + dma_fence_remove_callback(ret_fence, &sync_file->cb);
>
> This is racy with sync_file_poll. And sync_file_poll now needs rcu
> protection (as does all access to sync_file->fence), I need to check
> whether the previous patches are complete. Also needs to handle, or
> forbid, the caller passing in the same fence.
Race is serialised by sync_file_poll also be under the mutex, so that's
ok. Just the possibility of being passed in its own fence.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/8] sync_file: add a mutex to protect fence and callback members. (v4)
2017-04-04 8:07 ` Christian König
@ 2017-04-11 2:57 ` Dave Airlie
0 siblings, 0 replies; 31+ messages in thread
From: Dave Airlie @ 2017-04-11 2:57 UTC (permalink / raw)
To: Christian König; +Cc: dri-devel
On 4 April 2017 at 18:07, Christian König <deathsimple@vodafone.de> wrote:
> Am 04.04.2017 um 06:27 schrieb Dave Airlie:
>>
>> From: Dave Airlie <airlied@redhat.com>
>>
>> This patch allows the underlying fence in a sync_file to be changed
>> or set to NULL. This isn't currently required but for Vulkan
>> semaphores we need to be able to swap and reset the fence.
>>
>> In order to faciliate this, it uses rcu to protect the fence,
>> along with a new mutex. The mutex also protects the callback.
>> It also checks for NULL when retrieving the rcu protected
>> fence in case it has been reset.
>>
>> v1.1: fix the locking (Julia Lawall).
>> v2: use rcu try one
>> v3: fix poll to use proper rcu, fixup merge/fill ioctls
>> to not crash on NULL fence cases.
>> v4: use rcu in even more places, add missing fput
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> ---
>> drivers/dma-buf/sync_file.c | 135
>> +++++++++++++++++++++++++++++++++++---------
>> include/linux/sync_file.h | 6 +-
>> 2 files changed, 112 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
>> index 153bf03..6376f6f 100644
>> --- a/drivers/dma-buf/sync_file.c
>> +++ b/drivers/dma-buf/sync_file.c
>> @@ -28,6 +28,10 @@
>> static const struct file_operations sync_file_fops;
>> +#define sync_file_held(obj) lockdep_is_held(&(obj)->lock)
>> +#define sync_file_assert_held(obj) \
>> + lockdep_assert_held(&(obj)->lock)
>> +
>> /**
>> * sync_file_validate_type_flags - validate type/flags for support
>> * @type: type of sync file object
>> @@ -81,6 +85,10 @@ struct sync_file *sync_file_alloc(uint32_t type,
>> uint32_t flags)
>> sync_file->type = type;
>> sync_file->flags = flags;
>> +
>> + RCU_INIT_POINTER(sync_file->fence, NULL);
>> +
>> + mutex_init(&sync_file->lock);
>> return sync_file;
>> err:
>> @@ -117,7 +125,9 @@ struct sync_file *sync_file_create(struct dma_fence
>> *fence,
>> if (!sync_file)
>> return NULL;
>> - sync_file->fence = dma_fence_get(fence);
>> + dma_fence_get(fence);
>> +
>> + RCU_INIT_POINTER(sync_file->fence, fence);
>> snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
>> fence->ops->get_driver_name(fence),
>> @@ -168,13 +178,28 @@ struct dma_fence *sync_file_get_fence(int fd)
>> if (!sync_file)
>> return NULL;
>> - fence = dma_fence_get(sync_file->fence);
>> + if (!rcu_access_pointer(sync_file->fence)) {
>> + fput(sync_file->file);
>> + return NULL;
>> + }
>> +
>> + rcu_read_lock();
>> + fence = dma_fence_get_rcu_safe(&sync_file->fence);
>> + rcu_read_unlock();
>> +
>> fput(sync_file->file);
>> return fence;
>> }
>> EXPORT_SYMBOL(sync_file_get_fence);
>> +static inline struct dma_fence *
>> +sync_file_get_fence_locked(struct sync_file *sync_file)
>> +{
>> + return rcu_dereference_protected(sync_file->fence,
>> + sync_file_held(sync_file));
>> +}
>> +
>> static int sync_file_set_fence(struct sync_file *sync_file,
>> struct dma_fence **fences, int num_fences)
>> {
>> @@ -187,7 +212,7 @@ static int sync_file_set_fence(struct sync_file
>> *sync_file,
>> * we own the reference of the dma_fence_array creation.
>> */
>> if (num_fences == 1) {
>> - sync_file->fence = fences[0];
>> + RCU_INIT_POINTER(sync_file->fence, fences[0]);
>> kfree(fences);
>> } else {
>> array = dma_fence_array_create(num_fences, fences,
>> @@ -196,24 +221,30 @@ static int sync_file_set_fence(struct sync_file
>> *sync_file,
>> if (!array)
>> return -ENOMEM;
>> - sync_file->fence = &array->base;
>> + RCU_INIT_POINTER(sync_file->fence, &array->base);
>> }
>> return 0;
>> }
>> -static struct dma_fence **get_fences(struct sync_file *sync_file,
>> +/* must be called with rcu read lock taken */
>> +static struct dma_fence **get_fences(struct dma_fence **fence,
>> int *num_fences)
>> {
>> - if (dma_fence_is_array(sync_file->fence)) {
>> - struct dma_fence_array *array =
>> to_dma_fence_array(sync_file->fence);
>> + if (!*fence) {
>> + *num_fences = 0;
>> + return NULL;
>> + }
>> +
>> + if (dma_fence_is_array(*fence)) {
>> + struct dma_fence_array *array =
>> to_dma_fence_array(*fence);
>> *num_fences = array->num_fences;
>> return array->fences;
>> }
>> *num_fences = 1;
>> - return &sync_file->fence;
>> + return fence;
>> }
>> static void add_fence(struct dma_fence **fences,
>> @@ -243,18 +274,31 @@ static struct sync_file *sync_file_merge(const char
>> *name, struct sync_file *a,
>> struct sync_file *sync_file;
>> struct dma_fence **fences, **nfences, **a_fences, **b_fences;
>> int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
>> + struct dma_fence *a_fence, *b_fence;
>> if (a->type != b->type)
>> return NULL;
>> - sync_file = sync_file_alloc(a->type, a->flags);
>> - if (!sync_file)
>> + if (!rcu_access_pointer(a->fence) ||
>> + !rcu_access_pointer(b->fence))
>> return NULL;
>> - a_fences = get_fences(a, &a_num_fences);
>> - b_fences = get_fences(b, &b_num_fences);
>> + rcu_read_lock();
>> + a_fence = dma_fence_get_rcu_safe(&a->fence);
>> + b_fence = dma_fence_get_rcu_safe(&b->fence);
>> + rcu_read_unlock();
>> +
>> + a_fences = get_fences(&a_fence, &a_num_fences);
>> + b_fences = get_fences(&b_fence, &b_num_fences);
>> + if (!a_num_fences || !b_num_fences)
>> + goto put_src_fences;
>> +
>> if (a_num_fences > INT_MAX - b_num_fences)
>> - return NULL;
>> + goto put_src_fences;
>> +
>> + sync_file = sync_file_alloc(a->type, a->flags);
>> + if (!sync_file)
>> + goto put_src_fences;
>> num_fences = a_num_fences + b_num_fences;
>> @@ -315,11 +359,16 @@ static struct sync_file *sync_file_merge(const
>> char *name, struct sync_file *a,
>> goto err;
>> }
>> + dma_fence_put(a_fence);
>> + dma_fence_put(b_fence);
>> strlcpy(sync_file->name, name, sizeof(sync_file->name));
>> return sync_file;
>> err:
>> fput(sync_file->file);
>> +put_src_fences:
>> + dma_fence_put(a_fence);
>> + dma_fence_put(b_fence);
>> return NULL;
>> }
>> @@ -328,10 +377,15 @@ static void sync_file_free(struct kref *kref)
>> {
>> struct sync_file *sync_file = container_of(kref, struct sync_file,
>> kref);
>> + struct dma_fence *fence;
>> +
>> + fence = rcu_dereference_protected(sync_file->fence, 1);
>> + if (fence) {
>> + if (test_bit(POLL_ENABLED, &fence->flags))
>> + dma_fence_remove_callback(fence, &sync_file->cb);
>> + dma_fence_put(fence);
>> + }
>> - if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
>> - dma_fence_remove_callback(sync_file->fence,
>> &sync_file->cb);
>> - dma_fence_put(sync_file->fence);
>> kfree(sync_file);
>> }
>> @@ -346,16 +400,25 @@ static int sync_file_release(struct inode *inode,
>> struct file *file)
>> static unsigned int sync_file_poll(struct file *file, poll_table *wait)
>> {
>> struct sync_file *sync_file = file->private_data;
>> + unsigned int ret_val = 0;
>> + struct dma_fence *fence;
>> poll_wait(file, &sync_file->wq, wait);
>> - if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) {
>> - if (dma_fence_add_callback(sync_file->fence,
>> &sync_file->cb,
>> - fence_check_cb_func) < 0)
>> - wake_up_all(&sync_file->wq);
>> + mutex_lock(&sync_file->lock);
>> +
>> + fence = sync_file_get_fence_locked(sync_file);
>> + if (fence) {
>> + if (!test_and_set_bit(POLL_ENABLED, &fence->flags)) {
>> + if (dma_fence_add_callback(fence, &sync_file->cb,
>> + fence_check_cb_func) <
>> 0)
>> + wake_up_all(&sync_file->wq);
>> + }
>> + ret_val = dma_fence_is_signaled(fence) ? POLLIN : 0;
>> }
>> + mutex_unlock(&sync_file->lock);
>
>
> Maybe I'm a bit confused, but why exactly are you taking the lock here? It
> isn't protecting anything anymore as far as I can see.
>
> Or is it to prevent concurrent adding of the same callback? That would have
> failed before if I'm not completely mistaken.
The lock is to protect the callback change, the fence lookup is opportunistic
since we already have the lock.
I'm not sure what the old code would do here, but it looks like it
would have just
been racy, not sure adding the lock will cause it to do anything different.
Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2)
2017-04-04 11:05 ` Christian König
@ 2017-04-11 3:18 ` Dave Airlie
2017-04-11 6:55 ` Christian König
0 siblings, 1 reply; 31+ messages in thread
From: Dave Airlie @ 2017-04-11 3:18 UTC (permalink / raw)
To: Christian König; +Cc: dri-devel
On 4 April 2017 at 21:05, Christian König <deathsimple@vodafone.de> wrote:
> Am 04.04.2017 um 10:10 schrieb Daniel Vetter:
>>
>> On Tue, Apr 04, 2017 at 09:40:57AM +0200, Christian König wrote:
>>>
>>> Am 04.04.2017 um 06:27 schrieb Dave Airlie:
>>>>
>>>> From: Dave Airlie <airlied@redhat.com>
>>>>
>>>> This creates a new interface for amdgpu with ioctls to
>>>> create/destroy/import and export shared semaphores using
>>>> sem object backed by the sync_file code. The semaphores
>>>> are not installed as fd (except for export), but rather
>>>> like other driver internal objects in an idr. The idr
>>>> holds the initial reference on the sync file.
>>>>
>>>> The command submission interface is enhanced with two new
>>>> chunks, one for semaphore waiting, one for semaphore signalling
>>>> and just takes a list of handles for each.
>>>>
>>>> This is based on work originally done by David Zhou at AMD,
>>>> with input from Christian Konig on what things should look like.
>>>>
>>>> NOTE: this interface addition needs a version bump to expose
>>>> it to userspace.
>>>>
>>>> v1.1: keep file reference on import.
>>>> v2: move to using syncobjs
>>>>
>>>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>>>
>>> Looks good to me in general, but one note below.
>>>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 86
>>>> ++++++++++++++++++++++++++++++++-
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
>>>> include/uapi/drm/amdgpu_drm.h | 6 +++
>>>> 3 files changed, 92 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index 4671432..4dd210b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -27,6 +27,7 @@
>>>> #include <linux/pagemap.h>
>>>> #include <drm/drmP.h>
>>>> #include <drm/amdgpu_drm.h>
>>>> +#include <drm/drm_syncobj.h>
>>>> #include "amdgpu.h"
>>>> #include "amdgpu_trace.h"
>>>> @@ -217,6 +218,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser
>>>> *p, void *data)
>>>> break;
>>>> case AMDGPU_CHUNK_ID_DEPENDENCIES:
>>>> + case AMDGPU_CHUNK_ID_SEM_WAIT:
>>>> + case AMDGPU_CHUNK_ID_SEM_SIGNAL:
>>>> break;
>>>> default:
>>>> @@ -1009,6 +1012,44 @@ static int amdgpu_process_fence_dep(struct
>>>> amdgpu_device *adev,
>>>> return 0;
>>>> }
>>>> +static int amdgpu_sem_lookup_and_sync(struct amdgpu_device *adev,
>>>> + struct drm_file *file_private,
>>>> + struct amdgpu_sync *sync,
>>>> + uint32_t handle)
>>>> +{
>>>> + int r;
>>>> + struct dma_fence *old_fence;
>>>> + r = drm_syncobj_swap_fences(file_private, handle, NULL,
>>>> &old_fence);
>>>
>>> What happens when we the CS fails later on? Would the semaphore then not
>>> contain the fence any more?
>>
>> Atm rules are that you're not allowed to publish a dma_fence before you're
>> committed to executing whatever it represents (i.e. guaranteed to get
>> signalled).
>
>
> Yeah, I know. But this isn't the signaling case, but rather the waiting
> case.
>
> If I'm not completely mistaken what Dave does here is retrieving the fence
> from a semaphore object we need to wait for and setting it to NULL.
>
> If the command submission fails with -ERESTARTSYS after this is done the
> retried IOCTL will behave incorrectly.
I don't think it will since the return ioctl will already have waited won't it?
So once it's done the waiting, there is no point in waiting on resubmit.
Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/8] drm: introduce sync objects as sync file objects with no fd
[not found] ` <CAKMK7uFoFvsREVtSxsoOeM6OPDM-iGOATtcAK6p65LzG39D6oQ@mail.gmail.com>
@ 2017-04-11 6:00 ` Daniel Vetter
0 siblings, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2017-04-11 6:00 UTC (permalink / raw)
To: Dave Airlie, dri-devel
re-adding dri-devel, somehow got lost.
On Tue, Apr 11, 2017 at 8:00 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Apr 11, 2017 at 5:06 AM, Dave Airlie <airlied@gmail.com> wrote:
>>> You can drop the DRM_UNLOCKED, it's the enforced standard for non-legacy
>>> drivers since:
>>>
>>> commit ea487835e8876abf7ad909636e308c801a2bcda6
>>> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Date: Mon Sep 28 21:42:40 2015 +0200
>>>
>>> drm: Enforce unlocked ioctl operation for kms driver ioctls
>>
>> This is a core ioctl. not a driver one, so I think I still need UNLOCKED.
>
> Oh right, we need that still for nouveau's legacy ctx usage.
>
>>>> + /* Check if we currently have a reference on the object */
>>>> + sync_file = idr_find(&file_private->syncobj_idr, handle);
>>>
>>> You need to get a reference for the sync_file (hm, should we have
>>> sync_file_get/put helpers?), since otherwise someone might sneak in a
>>> destroy ioctl call right after you drop the lock here and boom.
>>
>> Indeed, fixed locally now.
>>
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + sync_file = sync_file_alloc(type, flags);
>>>> + if (!sync_file)
>>>> + return -ENOMEM;
>>>> +
>>>> + idr_preload(GFP_KERNEL);
>>>> + spin_lock(&file_private->syncobj_table_lock);
>>>> + ret = idr_alloc(&file_private->syncobj_idr, sync_file, 1, 0, GFP_NOWAIT);
>>>> + spin_unlock(&file_private->syncobj_table_lock);
>>>> +
>>>> + idr_preload_end();
>>>> +
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + *handle = ret;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void drm_syncobj_destroy(struct drm_file *file_private,
>>>> + u32 handle)
>>>> +{
>>>> + struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
>>>> + if (!sync_file)
>>>> + return;
>>>> +
>>>> + spin_lock(&file_private->syncobj_table_lock);
>>>> + idr_remove(&file_private->syncobj_idr, handle);
>>>> + spin_unlock(&file_private->syncobj_table_lock);
>>>> +
>>>> + fput(sync_file->file);
>>>> +}
>>>> +
>>>> +static int drm_syncobj_handle_to_fd(struct drm_file *file_private,
>>>> + u32 handle, int *fd)
>>>> +{
>>>> + struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
>>>> + int ret;
>>>> + if (!sync_file)
>>>> + return -EINVAL;
>>>> +
>>>> + ret = get_unused_fd_flags(O_CLOEXEC);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + fd_install(ret, sync_file->file);
>>>> + *fd = ret;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int drm_syncobj_fd_to_handle(struct drm_file *file_private,
>>>> + int fd, u32 *handle)
>>>> +{
>>>> + struct sync_file *sync_file = sync_file_fdget(fd);
>>>> + int ret;
>>>> + if (!sync_file)
>>>> + return -EINVAL;
>>>> + idr_preload(GFP_KERNEL);
>>>> + spin_lock(&file_private->syncobj_table_lock);
>>>> + ret = idr_alloc(&file_private->syncobj_idr, sync_file, 1, 0, GFP_NOWAIT);
>>>> + spin_unlock(&file_private->syncobj_table_lock);
>>>> + idr_preload_end();
>>>> +
>>>> + if (ret < 0) {
>>>> + fput(sync_file->file);
>>>> + return ret;
>>>> + }
>>>> + *handle = ret;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int drm_syncobj_info(struct drm_file *file_private,
>>>> + u32 handle, u32 *type, u32 *flags)
>>>> +{
>>>> + struct sync_file *sync_file = drm_syncobj_file_lookup(file_private, handle);
>>>> +
>>>> + if (!sync_file)
>>>> + return -EINVAL;
>>>> + *type = sync_file->type;
>>>> + *flags = sync_file->flags;
>>>> + return 0;
>>>> +}
>>>
>>> Do we really need this? I'd have assumed that all the drm_syncobj are
>>> guaranteed to be of type sema. Why else would you want to store them in an
>>> idr for future reuse?
>>
>> I'm allowing for future changes to the API, for now we could probably
>> limit syncobj
>> to SEMA, but there is nothing stopping you from importing a sync_file fence
>> into a syncobj and exporting it again, we should consider if we should restrict
>> the drm sync obj ioctls to just sema types for now. Hopefully someone has some
>> opinions here, I'm leaning towards just limiting the initial API to syncobj.
>
> Yeah, makes sense.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2)
2017-04-11 3:18 ` Dave Airlie
@ 2017-04-11 6:55 ` Christian König
0 siblings, 0 replies; 31+ messages in thread
From: Christian König @ 2017-04-11 6:55 UTC (permalink / raw)
To: Dave Airlie; +Cc: dri-devel
Am 11.04.2017 um 05:18 schrieb Dave Airlie:
> On 4 April 2017 at 21:05, Christian König <deathsimple@vodafone.de> wrote:
>> Am 04.04.2017 um 10:10 schrieb Daniel Vetter:
>>> On Tue, Apr 04, 2017 at 09:40:57AM +0200, Christian König wrote:
>>>> Am 04.04.2017 um 06:27 schrieb Dave Airlie:
>>>>> From: Dave Airlie <airlied@redhat.com>
>>>>>
>>>>> This creates a new interface for amdgpu with ioctls to
>>>>> create/destroy/import and export shared semaphores using
>>>>> sem object backed by the sync_file code. The semaphores
>>>>> are not installed as fd (except for export), but rather
>>>>> like other driver internal objects in an idr. The idr
>>>>> holds the initial reference on the sync file.
>>>>>
>>>>> The command submission interface is enhanced with two new
>>>>> chunks, one for semaphore waiting, one for semaphore signalling
>>>>> and just takes a list of handles for each.
>>>>>
>>>>> This is based on work originally done by David Zhou at AMD,
>>>>> with input from Christian Konig on what things should look like.
>>>>>
>>>>> NOTE: this interface addition needs a version bump to expose
>>>>> it to userspace.
>>>>>
>>>>> v1.1: keep file reference on import.
>>>>> v2: move to using syncobjs
>>>>>
>>>>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>>>> Looks good to me in general, but one note below.
>>>>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 86
>>>>> ++++++++++++++++++++++++++++++++-
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
>>>>> include/uapi/drm/amdgpu_drm.h | 6 +++
>>>>> 3 files changed, 92 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> index 4671432..4dd210b 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> @@ -27,6 +27,7 @@
>>>>> #include <linux/pagemap.h>
>>>>> #include <drm/drmP.h>
>>>>> #include <drm/amdgpu_drm.h>
>>>>> +#include <drm/drm_syncobj.h>
>>>>> #include "amdgpu.h"
>>>>> #include "amdgpu_trace.h"
>>>>> @@ -217,6 +218,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser
>>>>> *p, void *data)
>>>>> break;
>>>>> case AMDGPU_CHUNK_ID_DEPENDENCIES:
>>>>> + case AMDGPU_CHUNK_ID_SEM_WAIT:
>>>>> + case AMDGPU_CHUNK_ID_SEM_SIGNAL:
>>>>> break;
>>>>> default:
>>>>> @@ -1009,6 +1012,44 @@ static int amdgpu_process_fence_dep(struct
>>>>> amdgpu_device *adev,
>>>>> return 0;
>>>>> }
>>>>> +static int amdgpu_sem_lookup_and_sync(struct amdgpu_device *adev,
>>>>> + struct drm_file *file_private,
>>>>> + struct amdgpu_sync *sync,
>>>>> + uint32_t handle)
>>>>> +{
>>>>> + int r;
>>>>> + struct dma_fence *old_fence;
>>>>> + r = drm_syncobj_swap_fences(file_private, handle, NULL,
>>>>> &old_fence);
>>>> What happens when we the CS fails later on? Would the semaphore then not
>>>> contain the fence any more?
>>> Atm rules are that you're not allowed to publish a dma_fence before you're
>>> committed to executing whatever it represents (i.e. guaranteed to get
>>> signalled).
>>
>> Yeah, I know. But this isn't the signaling case, but rather the waiting
>> case.
>>
>> If I'm not completely mistaken what Dave does here is retrieving the fence
>> from a semaphore object we need to wait for and setting it to NULL.
>>
>> If the command submission fails with -ERESTARTSYS after this is done the
>> retried IOCTL will behave incorrectly.
> I don't think it will since the return ioctl will already have waited won't it?
No, it the IOCTL doesn't wait for anything. If you added a wait here
that would clearly be a NAK for the patch.
> So once it's done the waiting, there is no point in waiting on resubmit.
We don't wait in the IOCTL, instead all the fences are put as
dependencies into the job and submit that to the GPU scheduler for
execution when all fences are done.
Christian.
>
> Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 7/8] amdgpu/cs: split out fence dependency checking
[not found] ` <20170412045726.13689-1-airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-04-12 4:57 ` Dave Airlie
0 siblings, 0 replies; 31+ messages in thread
From: Dave Airlie @ 2017-04-12 4:57 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 7/8] amdgpu/cs: split out fence dependency checking
[not found] ` <20170411032220.21101-1-airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-04-11 3:22 ` Dave Airlie
0 siblings, 0 replies; 31+ messages in thread
From: Dave Airlie @ 2017-04-11 3:22 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
end of thread, other threads:[~2017-04-12 4:57 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-04 4:27 [RFC] DRM synchronisation objects Dave Airlie
2017-04-04 4:27 ` [PATCH 1/8] sync_file: add type/flags to sync file object creation Dave Airlie
2017-04-04 7:08 ` Daniel Vetter
2017-04-04 4:27 ` [PATCH 2/8] sync_file: export some interfaces needed by drm sync objects Dave Airlie
2017-04-04 7:10 ` Daniel Vetter
2017-04-04 4:27 ` [PATCH 3/8] drm: introduce sync objects as sync file objects with no fd Dave Airlie
2017-04-04 7:42 ` Daniel Vetter
[not found] ` <CAPM=9tyj6k4hqJWrwDW8Ch+TZCOoXRuAK2g71ciUm5vxpwmkuw@mail.gmail.com>
[not found] ` <CAKMK7uFoFvsREVtSxsoOeM6OPDM-iGOATtcAK6p65LzG39D6oQ@mail.gmail.com>
2017-04-11 6:00 ` Daniel Vetter
2017-04-04 4:27 ` [PATCH 4/8] sync_file: add a mutex to protect fence and callback members. (v4) Dave Airlie
2017-04-04 7:52 ` Daniel Vetter
2017-04-04 8:07 ` Christian König
2017-04-11 2:57 ` Dave Airlie
2017-04-04 4:27 ` [PATCH 5/8] sync_file: add support for a semaphore object Dave Airlie
2017-04-04 7:59 ` Daniel Vetter
2017-04-04 11:52 ` Chris Wilson
2017-04-04 11:59 ` Chris Wilson
2017-04-04 4:27 ` [PATCH 6/8] drm/syncobj: add semaphore support helpers Dave Airlie
2017-04-04 8:07 ` Daniel Vetter
2017-04-04 4:27 ` [PATCH 7/8] amdgpu/cs: split out fence dependency checking Dave Airlie
2017-04-04 7:37 ` Christian König
2017-04-04 4:27 ` [PATCH 8/8] amdgpu: use sync file for shared semaphores (v2) Dave Airlie
2017-04-04 7:40 ` Christian König
2017-04-04 8:10 ` Daniel Vetter
2017-04-04 11:05 ` Christian König
2017-04-11 3:18 ` Dave Airlie
2017-04-11 6:55 ` Christian König
2017-04-04 4:35 ` [RFC] DRM synchronisation objects Dave Airlie
2017-04-04 8:02 ` Christian König
2017-04-04 8:11 ` Daniel Vetter
2017-04-11 3:22 [repost] drm sync objects cleaned up Dave Airlie
[not found] ` <20170411032220.21101-1-airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-04-11 3:22 ` [PATCH 7/8] amdgpu/cs: split out fence dependency checking Dave Airlie
2017-04-12 4:57 drm sync objects (vn+1) Dave Airlie
[not found] ` <20170412045726.13689-1-airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-04-12 4:57 ` [PATCH 7/8] amdgpu/cs: split out fence dependency checking Dave Airlie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.