| Message ID | 20190308141244.16837-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] drm/i915: Suppress the "Failed to idle" warning for gem_eio | expand |
On 08/03/2019 14:12, 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@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 68fecf355471..60de05f3fa60 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; I have doubts about usefulness of trying to impose some limit now. And also reservations about using the name stack. But both are irrelevant implementation details at this stage so meh. > + > + while (ext) { > + int err; > + u64 x; > + > + if (!stackdepth--) /* recursion vs useful flexibility */ > + return -EINVAL; > + > + if (get_user(x, &ext->name)) > + return -EFAULT; > + > + err = -EINVAL; > + if (x < count && tbl[x]) > + err = tbl[x](ext, data); How about: put_user(err, &ext->result); And: struct i915_user_extension { __u64 next_extension; __u64 name; __u32 result; __u32 mbz; }; So we add the ability for each extension to store it's exit code giving userspace opportunity to know which one failed. With this I would be satisfied usability is future proof enough. Regards, Tvrtko > + 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 aa2d4c73a97d..39835793722b 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. >
Quoting Tvrtko Ursulin (2019-03-08 14:33:02) > > On 08/03/2019 14:12, Chris Wilson wrote: > > +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; > > I have doubts about usefulness of trying to impose some limit now. And > also reservations about using the name stack. But both are irrelevant > implementation details at this stage so meh. We need defence against malice userspace doing struct i915_user_extension ext = { .next_extension = &ext, }; so sadly some limit is required. > > + > > + while (ext) { > > + int err; > > + u64 x; > > + > > + if (!stackdepth--) /* recursion vs useful flexibility */ > > + return -EINVAL; > > + > > + if (get_user(x, &ext->name)) > > + return -EFAULT; > > + > > + err = -EINVAL; > > + if (x < count && tbl[x]) > > + err = tbl[x](ext, data); > > How about: > > put_user(err, &ext->result); > > And: > > struct i915_user_extension { > __u64 next_extension; > __u64 name; > __u32 result; > __u32 mbz; > }; > > So we add the ability for each extension to store it's exit code giving > userspace opportunity to know which one failed. > > With this I would be satisfied usability is future proof enough. I'm sorely tempted. The biggest objection I have is this defeats the elegance of a read-only chain. So who would actually use it? err = gem_context_create_ext(&chain); if (err) { struct i915_user_extension *ext = (struct i915_user_extension *)chain; while (ext && !ext->result) ext = (struct i915_user_extension *)ext->next_extension; if (ext) fprintf(stderr, "context creation failed at extension: %lld", ext->name); } What exactly are they going to do? They are not going to do anything like while (err) { ext = first_faulty_ext(&chain); switch (ext->name) { case ...: do_fixup_A(ext); } err = gem_context_create_ext(&chain); } I'm not really seeing how they benefit over, and above, handling the ioctl error by printing out the entire erroneous struct and chain, and falling back to avoiding that ioctl. I think what you really want is a per-application/fd debug log, so that we can dump the actual errors as they arise (without leaking them into the general syslog). -Chris
On 13/03/2019 10:50, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-03-08 14:33:02) >> >> On 08/03/2019 14:12, Chris Wilson wrote: >>> +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; >> >> I have doubts about usefulness of trying to impose some limit now. And >> also reservations about using the name stack. But both are irrelevant >> implementation details at this stage so meh. > > We need defence against malice userspace doing > struct i915_user_extension ext = { > .next_extension = &ext, > }; > so sadly some limit is required. Oh yes, good point. I wasn't thinking maliciously enough. S possible alternative solution could be, in conjunction with the result field from below, to only allow visiting any extension once. It would require reserving some value as meaning "not visited". Probably zero, so non-zero in result would immediately fail the chain, but would also I think mean we only support negative values in result as output, mapping zeros to one. >>> + >>> + while (ext) { >>> + int err; >>> + u64 x; >>> + >>> + if (!stackdepth--) /* recursion vs useful flexibility */ >>> + return -EINVAL; >>> + >>> + if (get_user(x, &ext->name)) >>> + return -EFAULT; >>> + >>> + err = -EINVAL; >>> + if (x < count && tbl[x]) >>> + err = tbl[x](ext, data); >> >> How about: >> >> put_user(err, &ext->result); >> >> And: >> >> struct i915_user_extension { >> __u64 next_extension; >> __u64 name; >> __u32 result; >> __u32 mbz; >> }; >> >> So we add the ability for each extension to store it's exit code giving >> userspace opportunity to know which one failed. >> >> With this I would be satisfied usability is future proof enough. > > I'm sorely tempted. The biggest objection I have is this defeats the > elegance of a read-only chain. So who would actually use it? > > err = gem_context_create_ext(&chain); > if (err) { > struct i915_user_extension *ext = (struct i915_user_extension *)chain; > while (ext && !ext->result) > ext = (struct i915_user_extension *)ext->next_extension; > if (ext) > fprintf(stderr, "context creation failed at extension: %lld", ext->name); > } > > What exactly are they going to do? They are not going to do anything > like > while (err) { > ext = first_faulty_ext(&chain); > switch (ext->name) { > case ...: do_fixup_A(ext); > } > err = gem_context_create_ext(&chain); > } > > I'm not really seeing how they benefit over, and above, handling the > ioctl error by printing out the entire erroneous struct and chain, and > falling back to avoiding that ioctl. > > I think what you really want is a per-application/fd debug log, so that > we can dump the actual errors as they arise (without leaking them into > the general syslog). Maybe.. could be an extension of the existing problem of "What EINVAL you mean exactly?" indeed. I don't see a problem with writing back though? Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-03-13 11:13:10) > > On 13/03/2019 10:50, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-03-08 14:33:02) > >> > >> On 08/03/2019 14:12, Chris Wilson wrote: > >>> +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; > >> > >> I have doubts about usefulness of trying to impose some limit now. And > >> also reservations about using the name stack. But both are irrelevant > >> implementation details at this stage so meh. > > > > We need defence against malice userspace doing > > struct i915_user_extension ext = { > > .next_extension = &ext, > > }; > > so sadly some limit is required. > > Oh yes, good point. I wasn't thinking maliciously enough. > > S possible alternative solution could be, in conjunction with the result > field from below, to only allow visiting any extension once. It would > require reserving some value as meaning "not visited". Probably zero, so > non-zero in result would immediately fail the chain, but would also I > think mean we only support negative values in result as output, mapping > zeros to one. I've avoided using the struct itself for markup so far. Ugh, it would also mean that userspace has to sanitize the extension chain between uses. > >>> + > >>> + while (ext) { > >>> + int err; > >>> + u64 x; > >>> + > >>> + if (!stackdepth--) /* recursion vs useful flexibility */ > >>> + return -EINVAL; > >>> + > >>> + if (get_user(x, &ext->name)) > >>> + return -EFAULT; > >>> + > >>> + err = -EINVAL; > >>> + if (x < count && tbl[x]) > >>> + err = tbl[x](ext, data); > >> > >> How about: > >> > >> put_user(err, &ext->result); > >> > >> And: > >> > >> struct i915_user_extension { > >> __u64 next_extension; > >> __u64 name; > >> __u32 result; > >> __u32 mbz; > >> }; > >> > >> So we add the ability for each extension to store it's exit code giving > >> userspace opportunity to know which one failed. > >> > >> With this I would be satisfied usability is future proof enough. > > > > I'm sorely tempted. The biggest objection I have is this defeats the > > elegance of a read-only chain. So who would actually use it? > > > > err = gem_context_create_ext(&chain); > > if (err) { > > struct i915_user_extension *ext = (struct i915_user_extension *)chain; > > while (ext && !ext->result) > > ext = (struct i915_user_extension *)ext->next_extension; > > if (ext) > > fprintf(stderr, "context creation failed at extension: %lld", ext->name); > > } > > > > What exactly are they going to do? They are not going to do anything > > like > > while (err) { > > ext = first_faulty_ext(&chain); > > switch (ext->name) { > > case ...: do_fixup_A(ext); > > } > > err = gem_context_create_ext(&chain); > > } > > > > I'm not really seeing how they benefit over, and above, handling the > > ioctl error by printing out the entire erroneous struct and chain, and > > falling back to avoiding that ioctl. > > > > I think what you really want is a per-application/fd debug log, so that > > we can dump the actual errors as they arise (without leaking them into > > the general syslog). > > Maybe.. could be an extension of the existing problem of "What EINVAL > you mean exactly?" indeed. > > I don't see a problem with writing back though? Writing anything gives me the heebie-jeebies. If we keep it a read-only struct, we can never be tricked into overwriting something important. It also makes it harder for userspace to reuse as they have to clear the result field? -Chris
On 13/03/2019 11:21, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-03-13 11:13:10) >> >> On 13/03/2019 10:50, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2019-03-08 14:33:02) >>>> >>>> On 08/03/2019 14:12, Chris Wilson wrote: >>>>> +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; >>>> >>>> I have doubts about usefulness of trying to impose some limit now. And >>>> also reservations about using the name stack. But both are irrelevant >>>> implementation details at this stage so meh. >>> >>> We need defence against malice userspace doing >>> struct i915_user_extension ext = { >>> .next_extension = &ext, >>> }; >>> so sadly some limit is required. >> >> Oh yes, good point. I wasn't thinking maliciously enough. >> >> S possible alternative solution could be, in conjunction with the result >> field from below, to only allow visiting any extension once. It would >> require reserving some value as meaning "not visited". Probably zero, so >> non-zero in result would immediately fail the chain, but would also I >> think mean we only support negative values in result as output, mapping >> zeros to one. > > I've avoided using the struct itself for markup so far. > Ugh, it would also mean that userspace has to sanitize the extension > chain between uses. > >>>>> + >>>>> + while (ext) { >>>>> + int err; >>>>> + u64 x; >>>>> + >>>>> + if (!stackdepth--) /* recursion vs useful flexibility */ >>>>> + return -EINVAL; >>>>> + >>>>> + if (get_user(x, &ext->name)) >>>>> + return -EFAULT; >>>>> + >>>>> + err = -EINVAL; >>>>> + if (x < count && tbl[x]) >>>>> + err = tbl[x](ext, data); >>>> >>>> How about: >>>> >>>> put_user(err, &ext->result); >>>> >>>> And: >>>> >>>> struct i915_user_extension { >>>> __u64 next_extension; >>>> __u64 name; >>>> __u32 result; >>>> __u32 mbz; >>>> }; >>>> >>>> So we add the ability for each extension to store it's exit code giving >>>> userspace opportunity to know which one failed. >>>> >>>> With this I would be satisfied usability is future proof enough. >>> >>> I'm sorely tempted. The biggest objection I have is this defeats the >>> elegance of a read-only chain. So who would actually use it? >>> >>> err = gem_context_create_ext(&chain); >>> if (err) { >>> struct i915_user_extension *ext = (struct i915_user_extension *)chain; >>> while (ext && !ext->result) >>> ext = (struct i915_user_extension *)ext->next_extension; >>> if (ext) >>> fprintf(stderr, "context creation failed at extension: %lld", ext->name); >>> } >>> >>> What exactly are they going to do? They are not going to do anything >>> like >>> while (err) { >>> ext = first_faulty_ext(&chain); >>> switch (ext->name) { >>> case ...: do_fixup_A(ext); >>> } >>> err = gem_context_create_ext(&chain); >>> } >>> >>> I'm not really seeing how they benefit over, and above, handling the >>> ioctl error by printing out the entire erroneous struct and chain, and >>> falling back to avoiding that ioctl. >>> >>> I think what you really want is a per-application/fd debug log, so that >>> we can dump the actual errors as they arise (without leaking them into >>> the general syslog). >> >> Maybe.. could be an extension of the existing problem of "What EINVAL >> you mean exactly?" indeed. >> >> I don't see a problem with writing back though? > > Writing anything gives me the heebie-jeebies. If we keep it a read-only > struct, we can never be tricked into overwriting something important. > > It also makes it harder for userspace to reuse as they have to clear the > result field? Yeah.. nothing then. Shall we only reserve some space with a flags and some rsvd fields just in case it will need to change/grow? Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-03-13 11:35:55) [snip] > Shall we only reserve some space with a flags and some rsvd fields just > in case it will need to change/grow? The only thing that occurs to me is to exchange the next pointer with a table of next[] (C++ here we come). But I ask myself, could any extension like not be part of the next layer? That is if any particular extension needs to chain up to more than one iface, it can call each itself: struct hypothetical_extension { struct i915_user_extension base; u64 iface1_extension; u64 iface2_extension; ... u64 ifaceN_extension; } ? So far I haven't thought of anything I can't weasel my way out by punting the problem to the caller :) -Chris
On 13/03/2019 11:46, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-03-13 11:35:55) > [snip] >> Shall we only reserve some space with a flags and some rsvd fields just >> in case it will need to change/grow? > > The only thing that occurs to me is to exchange the next pointer with a > table of next[] (C++ here we come). But I ask myself, could any > extension like not be part of the next layer? > > That is if any particular extension needs to chain up to more than one > iface, it can call each itself: > > struct hypothetical_extension { > struct i915_user_extension base; > > u64 iface1_extension; > u64 iface2_extension; > ... > u64 ifaceN_extension; > } > > ? So far I haven't thought of anything I can't weasel my way out by > punting the problem to the caller :) Just top make sure we are on the same page, I was thinking of: struct i915_user_extension { __u64 next_extension; __u64 name; __u32 flags; __u32 rsvd[7]; }; So we could add things like: /* Store each extension return code in rsvd[0]. */ #define I915_USER_EXTENSION_STORE_RESULT (1) /* Only check whether extensions are known by the driver. */ #define I915_USER_EXTENSION_DRY_RUN. (2) And things like that. Because we are putting in a generic extension mechanism I am worried that if it itself turns out to have some limitation we will not have wiggle room to extend it. Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-03-13 13:11:09) > > On 13/03/2019 11:46, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-03-13 11:35:55) > > [snip] > >> Shall we only reserve some space with a flags and some rsvd fields just > >> in case it will need to change/grow? > > > > The only thing that occurs to me is to exchange the next pointer with a > > table of next[] (C++ here we come). But I ask myself, could any > > extension like not be part of the next layer? > > > > That is if any particular extension needs to chain up to more than one > > iface, it can call each itself: > > > > struct hypothetical_extension { > > struct i915_user_extension base; > > > > u64 iface1_extension; > > u64 iface2_extension; > > ... > > u64 ifaceN_extension; > > } > > > > ? So far I haven't thought of anything I can't weasel my way out by > > punting the problem to the caller :) > > Just top make sure we are on the same page, I was thinking of: > > struct i915_user_extension { > __u64 next_extension; > __u64 name; > __u32 flags; > __u32 rsvd[7]; > }; > > So we could add things like: > > /* Store each extension return code in rsvd[0]. */ > #define I915_USER_EXTENSION_STORE_RESULT (1) > > /* Only check whether extensions are known by the driver. */ > #define I915_USER_EXTENSION_DRY_RUN. (2) > > And things like that. Because we are putting in a generic extension > mechanism I am worried that if it itself turns out to have some > limitation we will not have wiggle room to extend it. u64 next; u32 name; u32 flags; u32 rsvd[4]; Maybe... That's a cacheline. -Chris
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 68fecf355471..60de05f3fa60 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; + + 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 aa2d4c73a97d..39835793722b 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.
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@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