Re: [Intel-gfx] [PATCH 27/38] drm/i915: Introduce the i915_user_extension_method
On 22/01/2019 10:47, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-01-22 09:31:31)
On 18/01/2019 14:00, Chris Wilson wrote:
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2018 Intel Corporation
+ */
+
+#include
+#include
+#include
+
+#include "i915_user_extensions.h"
+
+int i915_user_extensions(struct i915_user_extension __user *ext,
+ const i915_user_extension_fn *tbl,
+ unsigned long count,
I would be tempted to limit the count to unsigned int. 4 billion
extensions should be enough for everyone. :)
I just picked the natural type, thinking having it the same width as
ARRAY_SIZE might save a few questions from semantic analysers.
+{
+ while (ext) {
+ int err;
+ u64 x;
+
+ cond_resched();
+ if (signal_pending(current))
+ return -EINTR;
What was your thinking behind this? It feels like, since here we are not
doing any explicit wait/sleeps, that at this level the code shouldn't
bother with it.
This ties into the discussion vvv
+ if (get_user(x, >name))
+ return -EFAULT;
+
+ err = -EINVAL;
+ if (x < count && tbl[x])
+ err = tbl[x](ext, data);
+ if (err)
+ return err;
We talked about providing unwind on failure ie. option for destructor
call backs. You gave up on that?
The patch is simply called 'merde'. Yes, unwind on failure does not lend
itself to a simple tail call implementation :) And it doesn't lend
itself nicely to writing the stacked cleanup handlers either. (So I
stuck with the solution of just doing a single cleanup on failure, safe
in the knowledge that the most complicated case in this series is
trivial.)
Thinking about the issues with providing a stack for unwind, leads to
the nasty question of how big a stack exactly do we want to provide.
Limiting the chain is required for defense against misuse, but what
depth? For the chained parameter setup of a single shot context create,
we could easily be into the dozens of parameters and extensions blocks.
The extreme I've been contemplating is a multi-batch execbuf setup (with
all the fancy extensions for e.g. semi-privileged fancy register setup),
for that I've been thinking about how this interface would extend to
supporting many chained chunks. What makes a good upper bound for stack
depth? 32? 64? 512? Pick a number, it won't be enough for someone. (Now,
really passing that much information through an ioctl means our design
is broken ;)
So... The break on interrupt there was for the silly protection against
recursion, if it doesn't result in an invalid command.
Another reason the patch was called merde.
I think the chained API extension is very powerful. Being able to do
arbitrary things like a single-shot context creation ioctl and still be
able to redefine/extend the interface as needs demands, is compelling.
I did not think about unwind implementation details. :)
We could make the ABI double linked list? But the feeling is bad..
Or have some reasonable stack size and if overflows fall back to a very
inefficient lookup from root to walk backwards. Sounds okay for this use
case since we would never overflow even a small stack, and even the
inefficient walk on unwind would be a) fast and b) rare.
But since you mention stack sizes like 512 I wonder I did not quite
grasp how much you'd want to use extensions in the future. :)
+/*
+ * 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;
s/name/id/ ?
I think name is common parlance for extensions/parameters? At least I've
been using it like it was :) And I was trying to retrospectively restrict
'id' for handles tracked by an idr! :)
I don't know if it is common, it is completely new to me. Maybe it is..
Or how about type?
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 27/38] drm/i915: Introduce the i915_user_extension_method
Quoting Tvrtko Ursulin (2019-01-22 09:31:31)
>
> On 18/01/2019 14:00, Chris Wilson wrote:
> > +/*
> > + * SPDX-License-Identifier: MIT
> > + *
> > + * Copyright © 2018 Intel Corporation
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include "i915_user_extensions.h"
> > +
> > +int i915_user_extensions(struct i915_user_extension __user *ext,
> > + const i915_user_extension_fn *tbl,
> > + unsigned long count,
>
> I would be tempted to limit the count to unsigned int. 4 billion
> extensions should be enough for everyone. :)
I just picked the natural type, thinking having it the same width as
ARRAY_SIZE might save a few questions from semantic analysers.
> > +{
> > + while (ext) {
> > + int err;
> > + u64 x;
> > +
> > + cond_resched();
> > + if (signal_pending(current))
> > + return -EINTR;
>
> What was your thinking behind this? It feels like, since here we are not
> doing any explicit wait/sleeps, that at this level the code shouldn't
> bother with it.
This ties into the discussion vvv
> > + if (get_user(x, >name))
> > + return -EFAULT;
> > +
> > + err = -EINVAL;
> > + if (x < count && tbl[x])
> > + err = tbl[x](ext, data);
> > + if (err)
> > + return err;
>
> We talked about providing unwind on failure ie. option for destructor
> call backs. You gave up on that?
The patch is simply called 'merde'. Yes, unwind on failure does not lend
itself to a simple tail call implementation :) And it doesn't lend
itself nicely to writing the stacked cleanup handlers either. (So I
stuck with the solution of just doing a single cleanup on failure, safe
in the knowledge that the most complicated case in this series is
trivial.)
Thinking about the issues with providing a stack for unwind, leads to
the nasty question of how big a stack exactly do we want to provide.
Limiting the chain is required for defense against misuse, but what
depth? For the chained parameter setup of a single shot context create,
we could easily be into the dozens of parameters and extensions blocks.
The extreme I've been contemplating is a multi-batch execbuf setup (with
all the fancy extensions for e.g. semi-privileged fancy register setup),
for that I've been thinking about how this interface would extend to
supporting many chained chunks. What makes a good upper bound for stack
depth? 32? 64? 512? Pick a number, it won't be enough for someone. (Now,
really passing that much information through an ioctl means our design
is broken ;)
So... The break on interrupt there was for the silly protection against
recursion, if it doesn't result in an invalid command.
Another reason the patch was called merde.
I think the chained API extension is very powerful. Being able to do
arbitrary things like a single-shot context creation ioctl and still be
able to redefine/extend the interface as needs demands, is compelling.
> > +/*
> > + * 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;
>
> s/name/id/ ?
I think name is common parlance for extensions/parameters? At least I've
been using it like it was :) And I was trying to retrospectively restrict
'id' for handles tracked by an idr! :)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 27/38] drm/i915: Introduce the i915_user_extension_method
On 18/01/2019 14:00, 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
s/similar/suffer/ I think.
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.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/i915_user_extensions.c | 42 +
drivers/gpu/drm/i915/i915_user_extensions.h | 20 ++
include/uapi/drm/i915_drm.h | 20 ++
4 files changed, 83 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 65ed00db..f206fbc85cee 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -45,6 +45,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 ..5d90c368f185
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_user_extensions.c
@@ -0,0 +1,42 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2018 Intel Corporation
+ */
+
+#include
+#include
+#include
+
+#include "i915_user_extensions.h"
+
+int i915_user_extensions(struct i915_user_extension __user *ext,
+const i915_user_extension_fn *tbl,
+unsigned long count,
I would be tempted to limit the count to unsigned int. 4 billion
extensions should be enough for everyone. :)
ABI can remain u64, but implementation I think in this form does not
need it.
+void *data)
+{
+ while (ext) {
+ int err;
+ u64 x;
+
+ cond_resched();
+ if (signal_pending(current))
+ return -EINTR;
What was your thinking behind this? It feels like, since here we are not
doing any explicit wait/sleeps, that at this level the code shouldn't
bother with it.
+
+ if (get_user(x, >name))
+ return -EFAULT;
+
+ err = -EINVAL;
+ if (x < count && tbl[x])
+ err = tbl[x](ext, data);
+ if (err)
+ return err;
We talked about providing unwind on failure ie. option for destructor
call backs. You gave up on that?
+
+ if (get_user(x, >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 ..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/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index
[Intel-gfx] [PATCH 27/38] drm/i915: Introduce the i915_user_extension_method
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.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/i915_user_extensions.c | 42 +
drivers/gpu/drm/i915/i915_user_extensions.h | 20 ++
include/uapi/drm/i915_drm.h | 20 ++
4 files changed, 83 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 65ed00db..f206fbc85cee 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -45,6 +45,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 ..5d90c368f185
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_user_extensions.c
@@ -0,0 +1,42 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2018 Intel Corporation
+ */
+
+#include
+#include
+#include
+
+#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)
+{
+ while (ext) {
+ int err;
+ u64 x;
+
+ cond_resched();
+ if (signal_pending(current))
+ return -EINTR;
+
+ if (get_user(x, >name))
+ return -EFAULT;
+
+ err = -EINVAL;
+ if (x < count && tbl[x])
+ err = tbl[x](ext, data);
+ if (err)
+ return err;
+
+ if (get_user(x, >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 ..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/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 298b2e197744..6ee2221838da 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
