On Tue, Apr 04, 2017 at 02:27:28PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxx>
> +
> 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@xxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
