| Message ID | 20181004113248.11986-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7] drm/i915: Engine discovery query | expand |
On 04/10/2018 13:32, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Engine discovery query allows userspace to enumerate engines, probe their > configuration features, all without needing to maintain the internal PCI > ID based database. > > A new query for the generic i915 query ioctl is added named > DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure > drm_i915_query_engine_info. The address of latter should be passed to the > kernel in the query.data_ptr field, and should be large enough for the > kernel to fill out all known engines as struct drm_i915_engine_info > elements trailing the query. > > As with other queries, setting the item query length to zero allows > userspace to query minimum required buffer size. > > Enumerated engines have common type mask which can be used to query all > hardware engines, versus engines userspace can submit to using the execbuf > uAPI. > > Engines also have capabilities which are per engine class namespace of > bits describing features not present on all engine instances. > > v2: > * Fixed HEVC assignment. > * Reorder some fields, rename type to flags, increase width. (Lionel) > * No need to allocate temporary storage if we do it engine by engine. > (Lionel) > > v3: > * Describe engine flags and mark mbz fields. (Lionel) > * HEVC only applies to VCS. > > v4: > * Squash SFC flag into main patch. > * Tidy some comments. > > v5: > * Add uabi_ prefix to engine capabilities. (Chris Wilson) > * Report exact size of engine info array. (Chris Wilson) > * Drop the engine flags. (Joonas Lahtinen) > * Added some more reserved fields. > * Move flags after class/instance. > > v6: > * Do not check engine info array was zeroed by userspace but zero the > unused fields for them instead. > > v7: > * Simplify length calculation loop. (Lionel Landwerlin) > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Tony Ye <tony.ye@intel.com> Looks good to me (might want someone else to rubberstamp the uapi structs) : Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/i915/i915_query.c | 54 +++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_engine_cs.c | 12 ++++++ > drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++ > include/uapi/drm/i915_drm.h | 47 +++++++++++++++++++++ > 4 files changed, 116 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > index 5821002cad42..c374075850a6 100644 > --- a/drivers/gpu/drm/i915/i915_query.c > +++ b/drivers/gpu/drm/i915/i915_query.c > @@ -84,9 +84,63 @@ static int query_topology_info(struct drm_i915_private *dev_priv, > return total_length; > } > > +static int > +query_engine_info(struct drm_i915_private *i915, > + struct drm_i915_query_item *query_item) > +{ > + struct drm_i915_query_engine_info __user *query_ptr = > + u64_to_user_ptr(query_item->data_ptr); > + struct drm_i915_engine_info __user *info_ptr = &query_ptr->engines[0]; > + struct drm_i915_query_engine_info query; > + struct drm_i915_engine_info info = { }; > + struct intel_engine_cs *engine; > + enum intel_engine_id id; > + int len; > + > + if (query_item->flags) > + return -EINVAL; > + > + len = sizeof(struct drm_i915_query_engine_info); > + for_each_engine(engine, i915, id) > + len += sizeof(struct drm_i915_engine_info); > + > + if (!query_item->length) > + return len; > + else if (query_item->length < len) > + return -EINVAL; > + > + if (copy_from_user(&query, query_ptr, sizeof(query))) > + return -EFAULT; > + > + if (query.num_engines || query.rsvd[0] || query.rsvd[1] || > + query.rsvd[2]) > + return -EINVAL; > + > + if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length)) > + return -EFAULT; > + > + for_each_engine(engine, i915, id) { > + info.class = engine->uabi_class; > + info.instance = engine->instance; > + info.capabilities = engine->uabi_capabilities; > + > + if (__copy_to_user(info_ptr, &info, sizeof(info))) > + return -EFAULT; > + > + query.num_engines++; > + info_ptr++; > + } > + > + if (__copy_to_user(query_ptr, &query, sizeof(query))) > + return -EFAULT; > + > + return len; > +} > + > static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv, > struct drm_i915_query_item *query_item) = { > query_topology_info, > + query_engine_info, > }; > > int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 1c6143bdf5a4..134f0cec724c 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -298,6 +298,18 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > engine->uabi_id = info->uabi_id; > engine->uabi_class = intel_engine_classes[info->class].uabi_class; > > + if (engine->class == VIDEO_DECODE_CLASS) { > + /* HEVC support is present only on vcs0. */ > + if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0) > + engine->uabi_capabilities = > + I915_VCS_CLASS_CAPABILITY_HEVC; > + > + /* SFC support is wired only to even VCS instances. */ > + if (INTEL_GEN(dev_priv) >= 9 && !(info->instance & 1)) > + engine->uabi_capabilities |= > + I915_VCS_CLASS_CAPABILITY_SFC; > + } > + > engine->context_size = __intel_engine_context_size(dev_priv, > engine->class); > if (WARN_ON(engine->context_size > BIT(20))) > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index f6ec48a75a69..9dc738f1b175 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -370,6 +370,9 @@ struct intel_engine_cs { > > u8 class; > u8 instance; > + > + u32 uabi_capabilities; > + > u32 context_size; > u32 mmio_base; > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 298b2e197744..3b0373fb0a93 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1650,6 +1650,7 @@ struct drm_i915_perf_oa_config { > struct drm_i915_query_item { > __u64 query_id; > #define DRM_I915_QUERY_TOPOLOGY_INFO 1 > +#define DRM_I915_QUERY_ENGINE_INFO 2 > > /* > * When set to zero by userspace, this is filled with the size of the > @@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info { > __u8 data[]; > }; > > +/** > + * struct drm_i915_engine_info > + * > + * Describes one engine known to the driver, whether or not it is an user- > + * accessible or hardware only engine, and what are it's capabilities where > + * applicable. > + */ > +struct drm_i915_engine_info { > + /** Engine class as in enum drm_i915_gem_engine_class. */ > + __u16 class; > + > + /** Engine instance number. */ > + __u16 instance; > + > + /** Reserved field must be cleared to zero. */ > + __u32 rsvd0; > + > + /** Engine flags. */ > + __u64 flags; > + > + /** Capabilities of this engine. */ > + __u64 capabilities; > +#define I915_VCS_CLASS_CAPABILITY_HEVC (1 << 0) > +#define I915_VCS_CLASS_CAPABILITY_SFC (1 << 1) > + > + /** Reserved fields must be cleared to zero. */ > + __u64 rsvd1[4]; > +}; > + > +/** > + * struct drm_i915_query_engine_info > + * > + * Engine info query enumerates all engines known to the driver by filling in > + * an array of struct drm_i915_engine_info structures. > + */ > +struct drm_i915_query_engine_info { > + /** Number of struct drm_i915_engine_info structs following. */ > + __u32 num_engines; > + > + /** MBZ */ > + __u32 rsvd[3]; > + > + /** Marker for drm_i915_engine_info structures. */ > + struct drm_i915_engine_info engines[]; > +}; > + > #if defined(__cplusplus) > } > #endif
Some comments below, mostly related to trying to keep the uapi header nice and tidy. Quoting Tvrtko Ursulin (2018-10-04 14:32:48) > @@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info { > __u8 data[]; > }; > > +/** > + * struct drm_i915_engine_info > + * > + * Describes one engine known to the driver, whether or not it is an user- > + * accessible or hardware only engine, and what are it's capabilities where > + * applicable. > + */ > +struct drm_i915_engine_info { > + /** Engine class as in enum drm_i915_gem_engine_class. */ > + __u16 class; > + > + /** Engine instance number. */ > + __u16 instance; > + > + /** Reserved field must be cleared to zero. */ > + __u32 rsvd0; u32 class, u32 instance just to put the padding to good use? > + > + /** Engine flags. */ > + __u64 flags; > + > + /** Capabilities of this engine. */ > + __u64 capabilities; > +#define I915_VCS_CLASS_CAPABILITY_HEVC (1 << 0) > +#define I915_VCS_CLASS_CAPABILITY_SFC (1 << 1) > + > + /** Reserved fields must be cleared to zero. */ > + __u64 rsvd1[4]; Why this at the end of the struct? We have flags which we should be able to use for new versions. > +}; > + > +/** > + * struct drm_i915_query_engine_info > + * > + * Engine info query enumerates all engines known to the driver by filling in > + * an array of struct drm_i915_engine_info structures. > + */ > +struct drm_i915_query_engine_info { > + /** Number of struct drm_i915_engine_info structs following. */ > + __u32 num_engines; > + > + /** MBZ */ Just copy the non-abbreviated comment from other reserved fields. > + __u32 rsvd[3]; > + Might as well do just __u32 flags, which must be zero? I don't think we need to get too excited about adding the reserved fields in every spot :) Regards, Joonas > + /** Marker for drm_i915_engine_info structures. */ > + struct drm_i915_engine_info engines[]; > +}; > + > #if defined(__cplusplus) > } > #endif > -- > 2.17.1 >
On 04/10/2018 15:32, Joonas Lahtinen wrote: > Some comments below, mostly related to trying to keep the uapi header > nice and tidy. > > Quoting Tvrtko Ursulin (2018-10-04 14:32:48) >> @@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info { >> __u8 data[]; >> }; >> >> +/** >> + * struct drm_i915_engine_info >> + * >> + * Describes one engine known to the driver, whether or not it is an user- >> + * accessible or hardware only engine, and what are it's capabilities where >> + * applicable. >> + */ >> +struct drm_i915_engine_info { >> + /** Engine class as in enum drm_i915_gem_engine_class. */ >> + __u16 class; >> + >> + /** Engine instance number. */ >> + __u16 instance; >> + >> + /** Reserved field must be cleared to zero. */ >> + __u32 rsvd0; > > u32 class, u32 instance just to put the padding to good use? There is some attractiveness to lose the padding, but I think in general we trashed it out to be u16:u16. So it is a question of consistency vs elegance and I give preference to consistency. Chris, is your recollection also that we said u16:u16 for class:instance in all uAPI? > >> + >> + /** Engine flags. */ >> + __u64 flags; >> + >> + /** Capabilities of this engine. */ >> + __u64 capabilities; >> +#define I915_VCS_CLASS_CAPABILITY_HEVC (1 << 0) >> +#define I915_VCS_CLASS_CAPABILITY_SFC (1 << 1) >> + >> + /** Reserved fields must be cleared to zero. */ >> + __u64 rsvd1[4]; > > Why this at the end of the struct? We have flags which we should be able > to use for new versions. This is to allow some growing space without complicating the userspace array member start calculation. If we have to grow struct engine_info itself when adding a new member, then we have to define the array (via documentation) as potentially not aligned to sizeof(struct engine_info) as known by userspace. I don't have such a big aversion to rsvd fields so for me it seems easier to have some, with the benefit of simpler userspace code. But if the consensus will be to change it, no problem. > >> +}; >> + >> +/** >> + * struct drm_i915_query_engine_info >> + * >> + * Engine info query enumerates all engines known to the driver by filling in >> + * an array of struct drm_i915_engine_info structures. >> + */ >> +struct drm_i915_query_engine_info { >> + /** Number of struct drm_i915_engine_info structs following. */ >> + __u32 num_engines; >> + >> + /** MBZ */ > > Just copy the non-abbreviated comment from other reserved fields. I need to nuke this comment since from v6 code is not mandating MBZ for the array. > >> + __u32 rsvd[3]; >> + > > Might as well do just __u32 flags, which must be zero? Could do flags.. > > I don't think we need to get too excited about adding the reserved > fields in every spot :) ... but I do like my rsvd! :)) Regards, Tvrtko > Regards, Joonas > >> + /** Marker for drm_i915_engine_info structures. */ >> + struct drm_i915_engine_info engines[]; >> +}; >> + >> #if defined(__cplusplus) >> } >> #endif >> -- >> 2.17.1 >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
Quoting Tvrtko Ursulin (2018-10-05 09:34:35) > > On 04/10/2018 15:32, Joonas Lahtinen wrote: > > Some comments below, mostly related to trying to keep the uapi header > > nice and tidy. > > > > Quoting Tvrtko Ursulin (2018-10-04 14:32:48) > >> @@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info { > >> __u8 data[]; > >> }; > >> > >> +/** > >> + * struct drm_i915_engine_info > >> + * > >> + * Describes one engine known to the driver, whether or not it is an user- > >> + * accessible or hardware only engine, and what are it's capabilities where > >> + * applicable. > >> + */ > >> +struct drm_i915_engine_info { > >> + /** Engine class as in enum drm_i915_gem_engine_class. */ > >> + __u16 class; > >> + > >> + /** Engine instance number. */ > >> + __u16 instance; > >> + > >> + /** Reserved field must be cleared to zero. */ > >> + __u32 rsvd0; > > > > u32 class, u32 instance just to put the padding to good use? > > There is some attractiveness to lose the padding, but I think in general > we trashed it out to be u16:u16. So it is a question of consistency vs > elegance and I give preference to consistency. > > Chris, is your recollection also that we said u16:u16 for class:instance > in all uAPI? Yes, that is the conclusion we came to. I've changed my uABI to u16:u16 as well. u8:u8 too tight, u32:u32 very unlikely. u16 is goldilocks. -Chris
Quoting Chris Wilson (2018-10-05 12:21:12) > Quoting Tvrtko Ursulin (2018-10-05 09:34:35) > > > > On 04/10/2018 15:32, Joonas Lahtinen wrote: > > > Some comments below, mostly related to trying to keep the uapi header > > > nice and tidy. > > > > > > Quoting Tvrtko Ursulin (2018-10-04 14:32:48) > > >> @@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info { > > >> __u8 data[]; > > >> }; > > >> > > >> +/** > > >> + * struct drm_i915_engine_info > > >> + * > > >> + * Describes one engine known to the driver, whether or not it is an user- > > >> + * accessible or hardware only engine, and what are it's capabilities where > > >> + * applicable. > > >> + */ > > >> +struct drm_i915_engine_info { > > >> + /** Engine class as in enum drm_i915_gem_engine_class. */ > > >> + __u16 class; > > >> + > > >> + /** Engine instance number. */ > > >> + __u16 instance; > > >> + > > >> + /** Reserved field must be cleared to zero. */ > > >> + __u32 rsvd0; > > > > > > u32 class, u32 instance just to put the padding to good use? > > > > There is some attractiveness to lose the padding, but I think in general > > we trashed it out to be u16:u16. So it is a question of consistency vs > > elegance and I give preference to consistency. > > > > Chris, is your recollection also that we said u16:u16 for class:instance > > in all uAPI? > > Yes, that is the conclusion we came to. I've changed my uABI to u16:u16 > as well. > > u8:u8 too tight, u32:u32 very unlikely. u16 is goldilocks. u32:u32 nicely aligns with u64 which is required in all structs ;) As mentioned in IRC, I'd try to keep the uAPI structs lean and simple as possible, but it's not a blocker to sprinkle some rsvd if others think they are beneficial/pessimism is required. Regards, Joonas
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index 5821002cad42..c374075850a6 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -84,9 +84,63 @@ static int query_topology_info(struct drm_i915_private *dev_priv, return total_length; } +static int +query_engine_info(struct drm_i915_private *i915, + struct drm_i915_query_item *query_item) +{ + struct drm_i915_query_engine_info __user *query_ptr = + u64_to_user_ptr(query_item->data_ptr); + struct drm_i915_engine_info __user *info_ptr = &query_ptr->engines[0]; + struct drm_i915_query_engine_info query; + struct drm_i915_engine_info info = { }; + struct intel_engine_cs *engine; + enum intel_engine_id id; + int len; + + if (query_item->flags) + return -EINVAL; + + len = sizeof(struct drm_i915_query_engine_info); + for_each_engine(engine, i915, id) + len += sizeof(struct drm_i915_engine_info); + + if (!query_item->length) + return len; + else if (query_item->length < len) + return -EINVAL; + + if (copy_from_user(&query, query_ptr, sizeof(query))) + return -EFAULT; + + if (query.num_engines || query.rsvd[0] || query.rsvd[1] || + query.rsvd[2]) + return -EINVAL; + + if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length)) + return -EFAULT; + + for_each_engine(engine, i915, id) { + info.class = engine->uabi_class; + info.instance = engine->instance; + info.capabilities = engine->uabi_capabilities; + + if (__copy_to_user(info_ptr, &info, sizeof(info))) + return -EFAULT; + + query.num_engines++; + info_ptr++; + } + + if (__copy_to_user(query_ptr, &query, sizeof(query))) + return -EFAULT; + + return len; +} + static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv, struct drm_i915_query_item *query_item) = { query_topology_info, + query_engine_info, }; int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 1c6143bdf5a4..134f0cec724c 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -298,6 +298,18 @@ intel_engine_setup(struct drm_i915_private *dev_priv, engine->uabi_id = info->uabi_id; engine->uabi_class = intel_engine_classes[info->class].uabi_class; + if (engine->class == VIDEO_DECODE_CLASS) { + /* HEVC support is present only on vcs0. */ + if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0) + engine->uabi_capabilities = + I915_VCS_CLASS_CAPABILITY_HEVC; + + /* SFC support is wired only to even VCS instances. */ + if (INTEL_GEN(dev_priv) >= 9 && !(info->instance & 1)) + engine->uabi_capabilities |= + I915_VCS_CLASS_CAPABILITY_SFC; + } + engine->context_size = __intel_engine_context_size(dev_priv, engine->class); if (WARN_ON(engine->context_size > BIT(20))) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index f6ec48a75a69..9dc738f1b175 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -370,6 +370,9 @@ struct intel_engine_cs { u8 class; u8 instance; + + u32 uabi_capabilities; + u32 context_size; u32 mmio_base; diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 298b2e197744..3b0373fb0a93 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1650,6 +1650,7 @@ struct drm_i915_perf_oa_config { struct drm_i915_query_item { __u64 query_id; #define DRM_I915_QUERY_TOPOLOGY_INFO 1 +#define DRM_I915_QUERY_ENGINE_INFO 2 /* * When set to zero by userspace, this is filled with the size of the @@ -1747,6 +1748,52 @@ struct drm_i915_query_topology_info { __u8 data[]; }; +/** + * struct drm_i915_engine_info + * + * Describes one engine known to the driver, whether or not it is an user- + * accessible or hardware only engine, and what are it's capabilities where + * applicable. + */ +struct drm_i915_engine_info { + /** Engine class as in enum drm_i915_gem_engine_class. */ + __u16 class; + + /** Engine instance number. */ + __u16 instance; + + /** Reserved field must be cleared to zero. */ + __u32 rsvd0; + + /** Engine flags. */ + __u64 flags; + + /** Capabilities of this engine. */ + __u64 capabilities; +#define I915_VCS_CLASS_CAPABILITY_HEVC (1 << 0) +#define I915_VCS_CLASS_CAPABILITY_SFC (1 << 1) + + /** Reserved fields must be cleared to zero. */ + __u64 rsvd1[4]; +}; + +/** + * struct drm_i915_query_engine_info + * + * Engine info query enumerates all engines known to the driver by filling in + * an array of struct drm_i915_engine_info structures. + */ +struct drm_i915_query_engine_info { + /** Number of struct drm_i915_engine_info structs following. */ + __u32 num_engines; + + /** MBZ */ + __u32 rsvd[3]; + + /** Marker for drm_i915_engine_info structures. */ + struct drm_i915_engine_info engines[]; +}; + #if defined(__cplusplus) } #endif