[Intel-gfx] [PATCH 14/38] drm/i915: Introduce the i915_user_extension_method
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Mar 1 15:39:13 UTC 2019
On 01/03/2019 14:03, Chris Wilson wrote:
> An idea for extending uABI inspired by Vulkan's extension chains.
> Instead of expanding the data struct for each ioctl every time we need
> to add a new feature, define an extension chain instead. As we add
> optional interfaces to control the ioctl, we define a new extension
> struct that can be linked into the ioctl data only when required by the
> user. The key advantage being able to ignore large control structs for
> optional interfaces/extensions, while being able to process them in a
> consistent manner.
>
> In comparison to other extensible ioctls, the key difference is the
> use of a linked chain of extension structs vs an array of tagged
> pointers. For example,
>
> struct drm_amdgpu_cs_chunk {
> __u32 chunk_id;
> __u32 length_dw;
> __u64 chunk_data;
> };
>
> struct drm_amdgpu_cs_in {
> __u32 ctx_id;
> __u32 bo_list_handle;
> __u32 num_chunks;
> __u32 _pad;
> __u64 chunks;
> };
>
> allows userspace to pass in array of pointers to extension structs, but
> must therefore keep constructing that array along side the command stream.
> In dynamic situations like that, a linked list is preferred and does not
> similar from extra cache line misses as the extension structs themselves
> must still be loaded separate to the chunks array.
>
> v2: Apply the tail call optimisation directly to nip the worry of stack
> overflow in the bud.
> v3: Defend against recursion.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/i915_user_extensions.c | 43 +++++++++++++++++++++
> drivers/gpu/drm/i915/i915_user_extensions.h | 20 ++++++++++
> drivers/gpu/drm/i915/i915_utils.h | 7 ++++
> include/uapi/drm/i915_drm.h | 20 ++++++++++
> 5 files changed, 91 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/i915_user_extensions.c
> create mode 100644 drivers/gpu/drm/i915/i915_user_extensions.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index a1d834068765..89105b1aaf12 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -46,6 +46,7 @@ i915-y := i915_drv.o \
> i915_sw_fence.o \
> i915_syncmap.o \
> i915_sysfs.o \
> + i915_user_extensions.o \
> intel_csr.o \
> intel_device_info.o \
> intel_pm.o \
> diff --git a/drivers/gpu/drm/i915/i915_user_extensions.c b/drivers/gpu/drm/i915/i915_user_extensions.c
> new file mode 100644
> index 000000000000..879b4094b2d7
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_user_extensions.c
> @@ -0,0 +1,43 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2018 Intel Corporation
> + */
> +
> +#include <linux/sched/signal.h>
> +#include <linux/uaccess.h>
> +#include <uapi/drm/i915_drm.h>
> +
> +#include "i915_user_extensions.h"
> +
> +int i915_user_extensions(struct i915_user_extension __user *ext,
> + const i915_user_extension_fn *tbl,
> + unsigned long count,
> + void *data)
> +{
> + unsigned int stackdepth = 512;
> +
> + while (ext) {
> + int err;
> + u64 x;
> +
> + if (!stackdepth--) /* recursion vs useful flexibility */
> + return -EINVAL;
I don't get this. What stack? Did you mean "static unsigned int
stackdepth" in case someone puts i915_user_extension into the extension
table? Or just a limit on number of chained extensions? But you are not
processing the recursively here.
Regards,
Tvrtko
> +
> + if (get_user(x, &ext->name))
> + return -EFAULT;
> +
> + err = -EINVAL;
> + if (x < count && tbl[x])
> + err = tbl[x](ext, data);
> + if (err)
> + return err;
> +
> + if (get_user(x, &ext->next_extension))
> + return -EFAULT;
> +
> + ext = u64_to_user_ptr(x);
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_user_extensions.h b/drivers/gpu/drm/i915/i915_user_extensions.h
> new file mode 100644
> index 000000000000..313a510b068a
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_user_extensions.h
> @@ -0,0 +1,20 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2018 Intel Corporation
> + */
> +
> +#ifndef I915_USER_EXTENSIONS_H
> +#define I915_USER_EXTENSIONS_H
> +
> +struct i915_user_extension;
> +
> +typedef int (*i915_user_extension_fn)(struct i915_user_extension __user *ext,
> + void *data);
> +
> +int i915_user_extensions(struct i915_user_extension __user *ext,
> + const i915_user_extension_fn *tbl,
> + unsigned long count,
> + void *data);
> +
> +#endif /* I915_USER_EXTENSIONS_H */
> diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h
> index 9726df37c4c4..fcc751aa1ea8 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -105,6 +105,13 @@
> __T; \
> })
>
> +#define container_of_user(ptr, type, member) ({ \
> + void __user *__mptr = (void __user *)(ptr); \
> + BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
> + !__same_type(*(ptr), void), \
> + "pointer type mismatch in container_of()"); \
> + ((type __user *)(__mptr - offsetof(type, member))); })
> +
> static inline u64 ptr_to_u64(const void *ptr)
> {
> return (uintptr_t)ptr;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index b10eea3f6d24..f4fa0825722a 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -62,6 +62,26 @@ extern "C" {
> #define I915_ERROR_UEVENT "ERROR"
> #define I915_RESET_UEVENT "RESET"
>
> +/*
> + * i915_user_extension: Base class for defining a chain of extensions
> + *
> + * Many interfaces need to grow over time. In most cases we can simply
> + * extend the struct and have userspace pass in more data. Another option,
> + * as demonstrated by Vulkan's approach to providing extensions for forward
> + * and backward compatibility, is to use a list of optional structs to
> + * provide those extra details.
> + *
> + * The key advantage to using an extension chain is that it allows us to
> + * redefine the interface more easily than an ever growing struct of
> + * increasing complexity, and for large parts of that interface to be
> + * entirely optional. The downside is more pointer chasing; chasing across
> + * the __user boundary with pointers encapsulated inside u64.
> + */
> +struct i915_user_extension {
> + __u64 next_extension;
> + __u64 name;
> +};
> +
> /*
> * MOCS indexes used for GPU surfaces, defining the cacheability of the
> * surface data and the coherency for this data wrt. CPU vs. GPU accesses.
>
More information about the Intel-gfx
mailing list