Quoting Tvrtko Ursulin (2019-02-11 19:00:59)
>
> On 06/02/2019 13: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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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 - which recursion? Variable is also a local automatic.
User recursion:
my_uapi_extension = {
.name = MY_UAPI_EXTENSION,
.next = to_user_pointer(&my_uapi_extension),
};
> > + if (get_user(x, &ext->name))
> > + return -EFAULT;
> > +
> > + err = -EINVAL;
> > + if (x < count && tbl[x])
> > + err = tbl[x](ext, data);
> > + if (err)
> > + return err;
>
> Do you plan to add the unwind as previously discussed? Or we define the
> interface as having undefined state if one extension failed? I would be
> a bit suboptimal for userspace since it would mean having to throw away
> and recreate the object in use cases for user extensions capable ioctl
> is executed post creation of some object.
I don't have a nice plan for adding unwind support, and it isn't
required to handle the current usecases. Real recursion would do the
trick, or manual callstacks. But each unwind callback basically has to
handle the struct in an unknown state, and in the end it all boils down
to a fini anyway (I think).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
