On 04/04/2017 11:52 AM, Daniel Vetter wrote:
> Also unify/merge with the existing stuff.
>
> I was a bit torn where to put this, but in the end I decided to put
> all the ioctl/sysfs/debugfs stuff into drm-uapi.rst. That means we
> have a bit a split with the other uapi related stuff used internally,
> like drm_file.[hc], but I think overall this makes more sense.
>
> If it's too confusing we can always add more cross-links to make it
> more discoverable. But the auto-sprinkling of links kernel-doc already
> does seems sufficient.
>
> Also for prettier docs and more cross-links, switch the internal
> defines over to an enum, as usual.
>
> v2: Update kerneldoc fro drm_compat_ioctl too (caught by 0day), plus a
> bit more drive-by polish.
>
> v3: Fix typo, spotted by xerpi on irc (Sergi).
>
> Cc: Sergi Granell <xerpi.g.12@xxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> Documentation/gpu/drm-internals.rst | 50 ----------------
> Documentation/gpu/drm-uapi.rst | 14 +++++
> drivers/gpu/drm/drm_ioc32.c | 76 ++++++++++++-----------
> drivers/gpu/drm/drm_ioctl.c | 50 +++++++++++++++-
> include/drm/drm_ioctl.h | 116 +++++++++++++++++++++++++++++++-----
> 5 files changed, 203 insertions(+), 103 deletions(-)
>
> diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
> index a09c721f9e89..babfb6143bd9 100644
> --- a/Documentation/gpu/drm-internals.rst
> +++ b/Documentation/gpu/drm-internals.rst
> @@ -255,56 +255,6 @@ File Operations
> .. kernel-doc:: drivers/gpu/drm/drm_file.c
> :export:
>
> -IOCTLs
> -------
> -
> -struct drm_ioctl_desc \*ioctls; int num_ioctls;
> - Driver-specific ioctls descriptors table.
> -
> -Driver-specific ioctls numbers start at DRM_COMMAND_BASE. The ioctls
> -descriptors table is indexed by the ioctl number offset from the base
> -value. Drivers can use the DRM_IOCTL_DEF_DRV() macro to initialize
> -the table entries.
> -
> -::
> -
> - DRM_IOCTL_DEF_DRV(ioctl, func, flags)
> -
> -``ioctl`` is the ioctl name. Drivers must define the DRM_##ioctl and
> -DRM_IOCTL_##ioctl macros to the ioctl number offset from
> -DRM_COMMAND_BASE and the ioctl number respectively. The first macro is
> -private to the device while the second must be exposed to userspace in a
> -public header.
> -
> -``func`` is a pointer to the ioctl handler function compatible with the
> -``drm_ioctl_t`` type.
> -
> -::
> -
> - typedef int drm_ioctl_t(struct drm_device *dev, void *data,
> - struct drm_file *file_priv);
> -
> -``flags`` is a bitmask combination of the following values. It restricts
> -how the ioctl is allowed to be called.
> -
> -- DRM_AUTH - Only authenticated callers allowed
> -
> -- DRM_MASTER - The ioctl can only be called on the master file handle
> -
> -- DRM_ROOT_ONLY - Only callers with the SYSADMIN capability allowed
> -
> -- DRM_CONTROL_ALLOW - The ioctl can only be called on a control
> - device
> -
> -- DRM_UNLOCKED - The ioctl handler will be called without locking the
> - DRM global mutex. This is the enforced default for kms drivers (i.e.
> - using the DRIVER_MODESET flag) and hence shouldn't be used any more
> - for new drivers.
> -
> -.. kernel-doc:: drivers/gpu/drm/drm_ioctl.c
> - :export:
> -
> -
> Misc Utilities
> ==============
>
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 8b0f0e403f0c..858457567d3d 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -160,6 +160,20 @@ other hand, a driver requires shared state between clients which is
> visible to user-space and accessible beyond open-file boundaries, they
> cannot support render nodes.
>
> +IOCTL Support on Device Nodes
> +=============================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_ioctl.c
> + :doc: driver specific ioctls
> +
> +.. kernel-doc:: include/drm/drm_ioctl.h
> + :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_ioctl.c
> + :export:
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_ioc32.c
> + :export:
>
> Testing and validation
> ======================
> diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c
> index b134482f4022..0fa7827cb0fd 100644
> --- a/drivers/gpu/drm/drm_ioc32.c
> +++ b/drivers/gpu/drm/drm_ioc32.c
> @@ -1,4 +1,4 @@
> -/**
> +/*
> * \file drm_ioc32.c
> *
> * 32-bit ioctl compatibility routines for the DRM.
> @@ -72,15 +72,15 @@
> #define DRM_IOCTL_MODE_ADDFB232 DRM_IOWR(0xb8, drm_mode_fb_cmd232_t)
>
> typedef struct drm_version_32 {
> - int version_major; /**< Major version */
> - int version_minor; /**< Minor version */
> - int version_patchlevel; /**< Patch level */
> - u32 name_len; /**< Length of name buffer */
> - u32 name; /**< Name of driver */
> - u32 date_len; /**< Length of date buffer */
> - u32 date; /**< User-space buffer to hold date */
> - u32 desc_len; /**< Length of desc buffer */
> - u32 desc; /**< User-space buffer to hold desc */
> + int version_major; /* Major version */
> + int version_minor; /* Minor version */
> + int version_patchlevel; /* Patch level */
> + u32 name_len; /* Length of name buffer */
> + u32 name; /* Name of driver */
> + u32 date_len; /* Length of date buffer */
> + u32 date; /* User-space buffer to hold date */
> + u32 desc_len; /* Length of desc buffer */
> + u32 desc; /* User-space buffer to hold desc */
> } drm_version32_t;
>
> static int compat_drm_version(struct file *file, unsigned int cmd,
> @@ -126,8 +126,8 @@ static int compat_drm_version(struct file *file, unsigned int cmd,
> }
>
> typedef struct drm_unique32 {
> - u32 unique_len; /**< Length of unique */
> - u32 unique; /**< Unique name for driver instantiation */
> + u32 unique_len; /* Length of unique */
> + u32 unique; /* Unique name for driver instantiation */
> } drm_unique32_t;
>
> static int compat_drm_getunique(struct file *file, unsigned int cmd,
> @@ -180,12 +180,12 @@ static int compat_drm_setunique(struct file *file, unsigned int cmd,
> }
>
> typedef struct drm_map32 {
> - u32 offset; /**< Requested physical address (0 for SAREA)*/
> - u32 size; /**< Requested physical size (bytes) */
> - enum drm_map_type type; /**< Type of memory to map */
> - enum drm_map_flags flags; /**< Flags */
> - u32 handle; /**< User-space: "Handle" to pass to mmap() */
> - int mtrr; /**< MTRR slot used */
> + u32 offset; /* Requested physical address (0 for SAREA)*/
/\
Missing space here
> + u32 size; /* Requested physical size (bytes) */
> + enum drm_map_type type; /* Type of memory to map */
> + enum drm_map_flags flags; /* Flags */
> + u32 handle; /* User-space: "Handle" to pass to mmap() */
> + int mtrr; /* MTRR slot used */
> } drm_map32_t;
>
> static int compat_drm_getmap(struct file *file, unsigned int cmd,
> @@ -286,12 +286,12 @@ static int compat_drm_rmmap(struct file *file, unsigned int cmd,
> }
>
> typedef struct drm_client32 {
> - int idx; /**< Which client desired? */
> - int auth; /**< Is client authenticated? */
> - u32 pid; /**< Process ID */
> - u32 uid; /**< User ID */
> - u32 magic; /**< Magic */
> - u32 iocs; /**< Ioctl count */
> + int idx; /* Which client desired? */
> + int auth; /* Is client authenticated? */
> + u32 pid; /* Process ID */
> + u32 uid; /* User ID */
> + u32 magic; /* Magic */
> + u32 iocs; /* Ioctl count */
> } drm_client32_t;
>
> static int compat_drm_getclient(struct file *file, unsigned int cmd,
> @@ -366,12 +366,12 @@ static int compat_drm_getstats(struct file *file, unsigned int cmd,
> }
>
> typedef struct drm_buf_desc32 {
> - int count; /**< Number of buffers of this size */
> - int size; /**< Size in bytes */
> - int low_mark; /**< Low water mark */
> - int high_mark; /**< High water mark */
> + int count; /* Number of buffers of this size */
> + int size; /* Size in bytes */
> + int low_mark; /* Low water mark */
> + int high_mark; /* High water mark */
> int flags;
> - u32 agp_start; /**< Start address in the AGP aperture */
> + u32 agp_start; /* Start address in the AGP aperture */
> } drm_buf_desc32_t;
>
> static int compat_drm_addbufs(struct file *file, unsigned int cmd,
> @@ -1111,13 +1111,18 @@ static drm_ioctl_compat_t *drm_compat_ioctls[] = {
> };
>
> /**
> - * Called whenever a 32-bit process running under a 64-bit kernel
> - * performs an ioctl on /dev/drm.
> + * drm_compat_ioctl - 32bit IOCTL compatibility handler for DRM drivers
> + * @filp: file this ioctl is called on
> + * @cmd: ioctl cmd number
> + * @arg: user argument
> + *
> + * Compatibility handler for 32 bit userspace running on 64 kernels. All actual
> + * IOCTL handling is forwarded to drm_ioctl(), while marshalling structures as
> + * appropriate. Note that this only handles DRM core IOCTLs, if the driver has
> + * botched IOCTL itself, it must handle those by wrapping this function.
> *
> - * \param file_priv DRM file private.
> - * \param cmd command.
> - * \param arg user argument.
> - * \return zero on success or negative number on failure.
> + * Returns:
> + * Zero on success, negative error code on failure.
> */
> long drm_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> @@ -1141,5 +1146,4 @@ long drm_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>
> return ret;
> }
> -
> EXPORT_SYMBOL(drm_compat_ioctl);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 7f4f4f48e390..f908ea94b784 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -647,13 +647,59 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
>
> /**
> + * DOC: driver specific ioctls
> + *
> + * First things first, driver private IOCTLs should only be needed for drivers
> + * supporting rendering. Kernel modesetting is all standardized, and extended
> + * through properties. There are a few exceptions in some existing drivers,
> + * which define IOCTL for use by the display DRM master, but they all predate
> + * properties.
> + *
> + * Now if you do have a render driver you always have to support it through
> + * driver private properties. There's a few steps needed to wire all the things
> + * up.
> + *
> + * First you need to define the structure for your IOCTL in your driver private
> + * UAPI header in ``include/uapi/drm/my_driver_drm.h``::
> + *
> + * struct my_driver_operation {
> + * u32 some_thing;
> + * u32 another_thing;
> + * };
> + *
> + * Please make sure that you follow all the best practices from
> + * ``Documentation/ioctl/botching-up-ioctls.txt``. Note that drm_ioctl()
> + * automatically zero-extends structures, hence make sure you can add more stuff
> + * at the end, i.e. don't put a variable sized array there.
> + *
> + * Then you need to define your IOCTL number, using one of DRM_IO(), DRM_IOR(),
> + * DRM_IOW() or DRM_IOWR(). It must start with the DRM_IOCTL\_ prefix::
> + *
> + * ##define DRM_IOCTL_MY_DRIVER_OPERATION \
> + * DRM_IOW(DRM_COMMAND_BASE, struct my_driver_operation)
> + *
> + * DRM driver private IOCTL must be in the range from DRM_COMMAND_BASE to
> + * DRM_COMMAND_END. Finally you need an array of &struct drm_ioctl_desc to wire
> + * up the handlers and set the access rights:
> + *
> + * static const struct drm_ioctl_desc my_driver_ioctls[] = {
> + * DRM_IOCTL_DEF_DRV(MY_DRIVER_OPERATION, my_driver_operation,
> + * DRM_AUTH|DRM_RENDER_ALLOW),
> + * };
> + *
> + * And then assign this to the &drm_driver.ioctls field in your driver
> + * structure.
> + */
> +
> +/**
> * drm_ioctl - ioctl callback implementation for DRM drivers
> * @filp: file this ioctl is called on
> * @cmd: ioctl cmd number
> * @arg: user argument
> *
> - * Looks up the ioctl function in the ::ioctls table, checking for root
> - * previleges if so required, and dispatches to the respective function.
> + * Looks up the ioctl function in the DRM core and the driver dispatch table,
> + * stored in &drm_driver.ioctls. It checks for necessary permission by calling
> + * drm_ioctl_permit(), and dispatches to the respective function.
> *
> * Returns:
> * Zero on success, negative error code on failure.
> diff --git a/include/drm/drm_ioctl.h b/include/drm/drm_ioctl.h
> index f17ee077f649..ee03b3c44b3b 100644
> --- a/include/drm/drm_ioctl.h
> +++ b/include/drm/drm_ioctl.h
> @@ -33,6 +33,7 @@
> #define _DRM_IOCTL_H_
>
> #include <linux/types.h>
> +#include <linux/bitops.h>
>
> #include <asm/ioctl.h>
>
> @@ -41,41 +42,126 @@ struct drm_file;
> struct file;
>
> /**
> - * Ioctl function type.
> + * drm_ioctl_t - DRM ioctl function type.
> + * @dev: DRM device inode
> + * @data: private pointer of the ioctl call
> + * @file_priv: DRM file this ioctl was made on
> *
> - * \param inode device inode.
> - * \param file_priv DRM file private pointer.
> - * \param cmd command.
> - * \param arg argument.
> + * This is the DRM ioctl typedef. Note that drm_ioctl() has alrady copied @data
> + * into kernel-space, and will also copy it back, depending upon the read/write
> + * settings in the ioctl command code.
> */
> typedef int drm_ioctl_t(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
>
> +/**
> + * drm_ioctl_compat_t - compatibility DRM ioctl function type.
> + * @filp: file pointer
> + * @cmd: ioctl command code
> + * @arg: DRM file this ioctl was made on
> + *
> + * Just a typedef to make declaring an array of compatibility handlers easier.
> + * New drivers shouldn't screw up the structure layout for their ioctl
> + * structures and hence never need this.
> + */
> typedef int drm_ioctl_compat_t(struct file *filp, unsigned int cmd,
> unsigned long arg);
>
> #define DRM_IOCTL_NR(n) _IOC_NR(n)
> #define DRM_MAJOR 226
>
> -#define DRM_AUTH 0x1
> -#define DRM_MASTER 0x2
> -#define DRM_ROOT_ONLY 0x4
> -#define DRM_CONTROL_ALLOW 0x8
> -#define DRM_UNLOCKED 0x10
> -#define DRM_RENDER_ALLOW 0x20
> +/**
> + * enum drm_ioctl_flags - DRM ioctl flags
> + *
> + * Various flags that can be set in &drm_ioctl_desc.flags to control how
> + * userspace can use a given ioctl.
> + */
> +enum drm_ioctl_flags {
> + /**
> + * @DRM_AUTH:
> + *
> + * This is for ioctl which are used for rendering, and require that the
> + * file descriptor is either for a render node, or if it's a
> + * legacy/primary node, then it must be authenticated.
> + */
> + DRM_AUTH = BIT(0),
> + /**
> + * @DRM_MASTER:
> + *
> + * This must be set for any ioctl which can change the modeset or
> + * display state. Userspace must call the ioctl through a primary node,
> + * while it is the active master.
> + *
> + * Note that read-only modeset ioctl can also be called by
> + * unauthenticated clients, or when a master is not the currently active
> + * one.
> + */
> + DRM_MASTER = BIT(1),
> + /**
> + * @DRM_ROOT_ONLY:
> + *
> + * Anything that could potentially wreak a master file descriptor needs
> + * to have this flag set. Current that's only for the SETMASTER and
> + * DROPMASTER ioctl, which e.g. logind can call to force a non-behaving
> + * master (display compositor) into compliance.
> + *
> + * This is equivalent to callers with the SYSADMIN capability.
> + */
> + DRM_ROOT_ONLY = BIT(2),
> + /**
> + * @DRM_CONTROL_ALLOW:
> + *
> + * Deprecated, do not use. Control nodes are in the process of getting
> + * removed.
> + */
> + DRM_CONTROL_ALLOW = BIT(3),
> + /**
> + * @DRM_UNLOCKED:
> + *
> + * Whether &drm_ioctl_desc.func should be called with the DRM BKL held
> + * or not. Enforced as the default for all modern drivers, hence there
> + * should never be a need to set this flag.
> + */
> + DRM_UNLOCKED = BIT(4),
> + /**
> + * @DRM_RENDER_ALLOW:
> + *
> + * This is used for all ioctl needed for rendering only, for drivers
> + * which support render nodes. This should be all new render drivers,
> + * and hence it should be always set for any ioctl with DRM_AUTH set.
> + * Note though that read-only query ioctl might have this set, but have
> + * not set DRM_AUTH because they do not require authentication.
> + */
> + DRM_RENDER_ALLOW = BIT(5),
> +};
>
> +/**
> + * struct drm_ioctl_desc - DRM driver ioctl entry
> + * @cmd: ioctl command number, without flags
> + * @flags: a bitmask of &enum drm_ioctl_flags
> + * @func: handler for this ioctl
> + * @name: user-readable name for debug output
> + *
> + * For convenience it's easier to create these using the DRM_IOCTL_DEF_DRV()
> + * macro.
> + */
> struct drm_ioctl_desc {
> unsigned int cmd;
> - int flags;
> + enum drm_ioctl_flags flags;
> drm_ioctl_t *func;
> const char *name;
> };
>
> /**
> - * Creates a driver or general drm_ioctl_desc array entry for the given
> - * ioctl, for use by drm_ioctl().
> + * DRM_IOCTL_DEF_DRV() - helper macro to fill out a &struct drm_ioctl_desc
> + * @ioctl: ioctl command suffix
> + * @_func: handler for the ioctl
> + * @_flags: a bitmask of &enum drm_ioctl_flags
> + *
> + * Small helper macro to create a &struct drm_ioctl_desc entry. The ioctl
> + * command number is constructed by prepending ``DRM_IOCTL\_`` and passing that
> + * to DRM_IOCTL_NR().
> */
> -
> #define DRM_IOCTL_DEF_DRV(ioctl, _func, _flags) \
> [DRM_IOCTL_NR(DRM_IOCTL_##ioctl) - DRM_COMMAND_BASE] = { \
> .cmd = DRM_IOCTL_##ioctl, \
>
Reviewed-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
