| Message ID | 20181001161524.8104-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Engine discovery query | expand |
Hi, Media experts please scroll down... On 01/10/2018 17:15, 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. > > 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> > --- > drivers/gpu/drm/i915/i915_query.c | 63 +++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_engine_cs.c | 9 ++++ > drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++ > include/uapi/drm/i915_drm.h | 54 +++++++++++++++++++++ > 4 files changed, 129 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > index 5821002cad42..294f8195efa7 100644 > --- a/drivers/gpu/drm/i915/i915_query.c > +++ b/drivers/gpu/drm/i915/i915_query.c > @@ -84,9 +84,72 @@ 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 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) + > + I915_NUM_ENGINES * 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) { > + struct drm_i915_engine_info info; > + > + if (__copy_from_user(&info, info_ptr, sizeof(info))) > + return -EFAULT; > + > + if (info.flags || info.class || info.instance || > + info.capabilities || info.rsvd0 || info.rsvd1[0] || > + info.rsvd1[1]) > + return -EINVAL; > + > + info.class = engine->uabi_class; > + info.instance = engine->instance; > + info.flags = I915_ENGINE_FLAG_PHYSICAL | I915_ENGINE_FLAG_ABI; > + info.capabilities = engine->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..97b4acf8af5c 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -294,6 +294,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases); > engine->class = info->class; > engine->instance = info->instance; > + if (engine->class == VIDEO_DECODE_CLASS) { > + /* HEVC support is present only on vcs0. */ > + if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0) > + engine->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->capabilities |= I915_VCS_CLASS_CAPABILITY_SFC; > + } ... to here! :) I need help with the above two capabilities assignments. Primarily are they correct? HEVC situation might be different on Gen11 for instance? Otherwise it is basically the same patch I sent months ago so essentially sending it again just for another review pass, if someone will have some new comments. Regards, Tvrtko > > engine->uabi_id = info->uabi_id; > engine->uabi_class = intel_engine_classes[info->class].uabi_class; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 1534de5bb852..90cf4eea0de2 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 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..c4292e5fed52 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,59 @@ 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 flags. > + * > + * I915_ENGINE_FLAG_PHYSICAL - engine exists in the hardware > + * I915_ENGINE_FLAG_ABI - engine can be submitted to via execbuf > + */ > + __u64 flags; > +#define I915_ENGINE_FLAG_PHYSICAL (1 << 0) > +#define I915_ENGINE_FLAG_ABI (1 << 1) > + > + /** 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; > + > + /** 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[2]; > +}; > + > +/** > + * 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 >
Hi, Media experts please scroll down... On 01/10/2018 17:15, 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. > > 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> > --- > drivers/gpu/drm/i915/i915_query.c | 63 +++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_engine_cs.c | 9 ++++ > drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++ > include/uapi/drm/i915_drm.h | 54 +++++++++++++++++++++ > 4 files changed, 129 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > index 5821002cad42..294f8195efa7 100644 > --- a/drivers/gpu/drm/i915/i915_query.c > +++ b/drivers/gpu/drm/i915/i915_query.c > @@ -84,9 +84,72 @@ 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 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) + > + I915_NUM_ENGINES * 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) { > + struct drm_i915_engine_info info; > + > + if (__copy_from_user(&info, info_ptr, sizeof(info))) > + return -EFAULT; > + > + if (info.flags || info.class || info.instance || > + info.capabilities || info.rsvd0 || info.rsvd1[0] || > + info.rsvd1[1]) > + return -EINVAL; > + > + info.class = engine->uabi_class; > + info.instance = engine->instance; > + info.flags = I915_ENGINE_FLAG_PHYSICAL | I915_ENGINE_FLAG_ABI; > + info.capabilities = engine->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..97b4acf8af5c 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -294,6 +294,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases); > engine->class = info->class; > engine->instance = info->instance; > + if (engine->class == VIDEO_DECODE_CLASS) { > + /* HEVC support is present only on vcs0. */ > + if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0) > + engine->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->capabilities |= I915_VCS_CLASS_CAPABILITY_SFC; > + } ... to here! :) I need help with the above two capabilities assignments. Primarily are they correct? HEVC situation might be different on Gen11 for instance? Otherwise it is basically the same patch I sent months ago so essentially sending it again just for another review pass, if someone will have some new comments. Regards, Tvrtko > > engine->uabi_id = info->uabi_id; > engine->uabi_class = intel_engine_classes[info->class].uabi_class; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 1534de5bb852..90cf4eea0de2 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 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..c4292e5fed52 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,59 @@ 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 flags. > + * > + * I915_ENGINE_FLAG_PHYSICAL - engine exists in the hardware > + * I915_ENGINE_FLAG_ABI - engine can be submitted to via execbuf > + */ > + __u64 flags; > +#define I915_ENGINE_FLAG_PHYSICAL (1 << 0) > +#define I915_ENGINE_FLAG_ABI (1 << 1) > + > + /** 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; > + > + /** 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[2]; > +}; > + > +/** > + * 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 >
Quoting Tvrtko Ursulin (2018-10-01 17:15:24) > 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. > > 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> > --- > drivers/gpu/drm/i915/i915_query.c | 63 +++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_engine_cs.c | 9 ++++ > drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++ > include/uapi/drm/i915_drm.h | 54 +++++++++++++++++++++ > 4 files changed, 129 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c > index 5821002cad42..294f8195efa7 100644 > --- a/drivers/gpu/drm/i915/i915_query.c > +++ b/drivers/gpu/drm/i915/i915_query.c > @@ -84,9 +84,72 @@ 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 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) + > + I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info); Since I915_NUM_ENGINES is not ABI, and this will be using a 2-step query (1st to find length, 2nd to grab details), it should work with info->num_rings. If not, we have problems later ;) > + > + 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) { > + struct drm_i915_engine_info info; > + > + if (__copy_from_user(&info, info_ptr, sizeof(info))) > + return -EFAULT; > + > + if (info.flags || info.class || info.instance || > + info.capabilities || info.rsvd0 || info.rsvd1[0] || > + info.rsvd1[1]) > + return -EINVAL; > + > + info.class = engine->uabi_class; > + info.instance = engine->instance; > + info.flags = I915_ENGINE_FLAG_PHYSICAL | I915_ENGINE_FLAG_ABI; > + info.capabilities = engine->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..97b4acf8af5c 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -294,6 +294,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases); > engine->class = info->class; > engine->instance = info->instance; > + if (engine->class == VIDEO_DECODE_CLASS) { > + /* HEVC support is present only on vcs0. */ > + if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0) > + engine->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->capabilities |= I915_VCS_CLASS_CAPABILITY_SFC; > + } > > engine->uabi_id = info->uabi_id; > engine->uabi_class = intel_engine_classes[info->class].uabi_class; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 1534de5bb852..90cf4eea0de2 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 capabilities; I wonder if uabi_capabilities would be a helpful reminder that we can't change this field without wider repercussions. > + > u32 context_size; > u32 mmio_base; > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 298b2e197744..c4292e5fed52 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,59 @@ 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 flags. > + * > + * I915_ENGINE_FLAG_PHYSICAL - engine exists in the hardware > + * I915_ENGINE_FLAG_ABI - engine can be submitted to via execbuf > + */ > + __u64 flags; > +#define I915_ENGINE_FLAG_PHYSICAL (1 << 0) > +#define I915_ENGINE_FLAG_ABI (1 << 1) > + > + /** 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; > + > + /** 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[2]; > +}; > + > +/** > + * 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 > -- > 2.17.1 >
On 01/10/2018 17:24, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-10-01 17:15:24) >> 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. >> >> 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> >> --- >> drivers/gpu/drm/i915/i915_query.c | 63 +++++++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_engine_cs.c | 9 ++++ >> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++ >> include/uapi/drm/i915_drm.h | 54 +++++++++++++++++++++ >> 4 files changed, 129 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c >> index 5821002cad42..294f8195efa7 100644 >> --- a/drivers/gpu/drm/i915/i915_query.c >> +++ b/drivers/gpu/drm/i915/i915_query.c >> @@ -84,9 +84,72 @@ 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 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) + >> + I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info); > > Since I915_NUM_ENGINES is not ABI, and this will be using a 2-step > query (1st to find length, 2nd to grab details), it should work with > info->num_rings. If not, we have problems later ;) I don't follow, what do you plan to enumerate that would cause a problem? >> + >> + 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) { >> + struct drm_i915_engine_info info; >> + >> + if (__copy_from_user(&info, info_ptr, sizeof(info))) >> + return -EFAULT; >> + >> + if (info.flags || info.class || info.instance || >> + info.capabilities || info.rsvd0 || info.rsvd1[0] || >> + info.rsvd1[1]) >> + return -EINVAL; >> + >> + info.class = engine->uabi_class; >> + info.instance = engine->instance; >> + info.flags = I915_ENGINE_FLAG_PHYSICAL | I915_ENGINE_FLAG_ABI; >> + info.capabilities = engine->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..97b4acf8af5c 100644 >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >> @@ -294,6 +294,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv, >> engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases); >> engine->class = info->class; >> engine->instance = info->instance; >> + if (engine->class == VIDEO_DECODE_CLASS) { >> + /* HEVC support is present only on vcs0. */ >> + if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0) >> + engine->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->capabilities |= I915_VCS_CLASS_CAPABILITY_SFC; >> + } >> >> engine->uabi_id = info->uabi_id; >> engine->uabi_class = intel_engine_classes[info->class].uabi_class; >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h >> index 1534de5bb852..90cf4eea0de2 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 capabilities; > > I wonder if uabi_capabilities would be a helpful reminder that we can't > change this field without wider repercussions. Spot on! Regards, Tvrtko > >> + >> u32 context_size; >> u32 mmio_base; >> >> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >> index 298b2e197744..c4292e5fed52 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,59 @@ 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 flags. >> + * >> + * I915_ENGINE_FLAG_PHYSICAL - engine exists in the hardware >> + * I915_ENGINE_FLAG_ABI - engine can be submitted to via execbuf >> + */ >> + __u64 flags; >> +#define I915_ENGINE_FLAG_PHYSICAL (1 << 0) >> +#define I915_ENGINE_FLAG_ABI (1 << 1) >> + >> + /** 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; >> + >> + /** 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[2]; >> +}; >> + >> +/** >> + * 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 >> -- >> 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-01 17:41:00) > > On 01/10/2018 17:24, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-10-01 17:15:24) > >> + len = sizeof(struct drm_i915_query_engine_info) + > >> + I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info); > > > > Since I915_NUM_ENGINES is not ABI, and this will be using a 2-step > > query (1st to find length, 2nd to grab details), it should work with > > info->num_rings. If not, we have problems later ;) > > I don't follow, what do you plan to enumerate that would cause a problem? All I'm suggesting is to avoid any notion that this length is fixed indefinitely :) -Chris
On 01/10/2018 20:39, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-10-01 17:41:00) >> >> On 01/10/2018 17:24, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2018-10-01 17:15:24) >>>> + len = sizeof(struct drm_i915_query_engine_info) + >>>> + I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info); >>> >>> Since I915_NUM_ENGINES is not ABI, and this will be using a 2-step >>> query (1st to find length, 2nd to grab details), it should work with >>> info->num_rings. If not, we have problems later ;) >> >> I don't follow, what do you plan to enumerate that would cause a problem? > > All I'm suggesting is to avoid any notion that this length is fixed > indefinitely :) You mean extra paranoid step of making sure reported len exactly matches the number of reported engines * sizeof(engine_info)? uAPI is defined to query required memory block size and then read the reported num_engines so I think that's fine, but sure, I can make it match exactly. Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-10-02 10:05:11) > > On 01/10/2018 20:39, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-10-01 17:41:00) > >> > >> On 01/10/2018 17:24, Chris Wilson wrote: > >>> Quoting Tvrtko Ursulin (2018-10-01 17:15:24) > >>>> + len = sizeof(struct drm_i915_query_engine_info) + > >>>> + I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info); > >>> > >>> Since I915_NUM_ENGINES is not ABI, and this will be using a 2-step > >>> query (1st to find length, 2nd to grab details), it should work with > >>> info->num_rings. If not, we have problems later ;) > >> > >> I don't follow, what do you plan to enumerate that would cause a problem? > > > > All I'm suggesting is to avoid any notion that this length is fixed > > indefinitely :) > > You mean extra paranoid step of making sure reported len exactly matches > the number of reported engines * sizeof(engine_info)? uAPI is defined to > query required memory block size and then read the reported num_engines > so I think that's fine, but sure, I can make it match exactly. No, my paranoia is towards people seeing I915_NUM_ENGINES and so assuming this is constant. -Chris
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index 5821002cad42..294f8195efa7 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -84,9 +84,72 @@ 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 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) + + I915_NUM_ENGINES * 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) { + struct drm_i915_engine_info info; + + if (__copy_from_user(&info, info_ptr, sizeof(info))) + return -EFAULT; + + if (info.flags || info.class || info.instance || + info.capabilities || info.rsvd0 || info.rsvd1[0] || + info.rsvd1[1]) + return -EINVAL; + + info.class = engine->uabi_class; + info.instance = engine->instance; + info.flags = I915_ENGINE_FLAG_PHYSICAL | I915_ENGINE_FLAG_ABI; + info.capabilities = engine->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..97b4acf8af5c 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -294,6 +294,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv, engine->mmio_base = __engine_mmio_base(dev_priv, info->mmio_bases); engine->class = info->class; engine->instance = info->instance; + if (engine->class == VIDEO_DECODE_CLASS) { + /* HEVC support is present only on vcs0. */ + if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0) + engine->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->capabilities |= I915_VCS_CLASS_CAPABILITY_SFC; + } engine->uabi_id = info->uabi_id; engine->uabi_class = intel_engine_classes[info->class].uabi_class; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 1534de5bb852..90cf4eea0de2 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 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..c4292e5fed52 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,59 @@ 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 flags. + * + * I915_ENGINE_FLAG_PHYSICAL - engine exists in the hardware + * I915_ENGINE_FLAG_ABI - engine can be submitted to via execbuf + */ + __u64 flags; +#define I915_ENGINE_FLAG_PHYSICAL (1 << 0) +#define I915_ENGINE_FLAG_ABI (1 << 1) + + /** 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; + + /** 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[2]; +}; + +/** + * 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