Re: [Intel-gfx] [PATCH v11] drm/i915: Engine discovery query
Quoting Tvrtko Ursulin (2019-05-01 16:51:28) > > On 01/05/2019 12:55, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-05-01 12:45:36) > >> Hmm.. probably manual check for no holes _and_ alignment is good enough > >> for uAPI since once it's in it's in. Will triple-check. > > > > Yeah, we actually need something more like > > offsetofend(previous_field) == offsetof(next_field) > > > > BUILD_BUG_ON(check_user_struct(info, previous_field, next_field)) ? > > How would you logistically do it? List all struct members for each uapi > struct you want to check? > > Maybe a variadic macro like: > > CHECK_USER_STRUCT_FUNCTION(type, member0, ... memberN); > > Which expands to a dedicated function to check this type, using > va_start/va_end to iterate all members checking for holes. So somewhere > in code we would also need: > > CHECK_USER_STRUCT(type); > > Which would call the function. But thats not build time.. Could be under > debug and selftests I guess. Could even be IGT in this case. > > But I am not to keen in listing each struct member with a > prev/next_field BUILD_BUG_ON. > > Perhaps IGT is indeed a better place to start testing for this. Since we > anyway require each new uAPI to have good IGT coverage. Definitely don't like the idea of doing it manually, I could have just about accepted it if we could have rolled it into a get_user wrapper. We should just go annoy Jani to whip up some Makefile magic to call pahole and check the structs defined in uapi.h -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v11] drm/i915: Engine discovery query
On 01/05/2019 12:55, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-05-01 12:45:36)
On 01/05/2019 12:10, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-05-01 11:52:28)
From: Tvrtko Ursulin
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)
v8:
* Remove MBZ comments where not applicable.
* Rename ABI flags to match engine class define naming.
* Rename SFC ABI flag to reflect it applies to VCS and VECS.
* SFC is wired to even _logical_ engine instances.
* SFC applies to VCS and VECS.
* HEVC is present on all instances on Gen11. (Tony)
* Simplify length calculation even more. (Chris Wilson)
* Move info_ptr assigment closer to loop for clarity. (Chris Wilson)
* Use vdbox_sfc_access from runtime info.
* Rebase for RUNTIME_INFO.
* Refactor for lower indentation.
* Rename uAPI class/instance to engine_class/instance to avoid C++
keyword.
v9:
* Rebase for s/num_rings/num_engines/ in RUNTIME_INFO.
v10:
* Use new copy_query_item.
v11:
* Consolidate with struct i915_engine_class_instnace.
Signed-off-by: Tvrtko Ursulin
Cc: Chris Wilson
Cc: Jon Bloomfield
Cc: Dmitry Rogozhkin
Cc: Lionel Landwerlin
Cc: Joonas Lahtinen
Cc: Tony Ye
Reviewed-by: Lionel Landwerlin # v7
Reviewed-by: Chris Wilson # v7
---
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine and it's capabilities as known to the driver.
+ */
+struct drm_i915_engine_info {
+ /** Engine class and instance. */
+ struct i915_engine_class_instance engine;
+
+ /** Reserved field. */
+ __u32 rsvd0;
+
+ /** Engine flags. */
+ __u64 flags;
Do you think we could do something like
BUILD_BUG_ON(!IS_ALIGNED(offsetof(*info, flags), sizeof(info->flags));
Will that work, and worthwhile? Maybe work into a
BUILD_BUG_ON(check_user_alignment(info, flags));
Hmm.. probably manual check for no holes _and_ alignment is good enough
for uAPI since once it's in it's in. Will triple-check.
Yeah, we actually need something more like
offsetofend(previous_field) == offsetof(next_field)
BUILD_BUG_ON(check_user_struct(info, previous_field, next_field)) ?
How would you logistically do it? List all struct members for each uapi
struct you want to check?
Maybe a variadic macro like:
CHECK_USER_STRUCT_FUNCTION(type, member0, ... memberN);
Which expands to a dedicated function to check this type, using
va_start/va_end to iterate all members checking for holes. So somewhere
in code we would also need:
CHECK_USER_STRUCT(type);
Which would call the function. But thats not build time.. Could be under
debug and selftests I guess. Could even be IGT in this case.
But I am not to keen in listing each struct member with a
prev/next_field BUILD_BUG_ON.
Perhaps IGT is indeed a better place to start testing for this. Since we
anyway require each new uAPI to have good IGT coverage.
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v11] drm/i915: Engine discovery query
Quoting Tvrtko Ursulin (2019-05-01 12:45:36)
>
> On 01/05/2019 12:10, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-05-01 11:52:28)
> >> From: Tvrtko Ursulin
> >>
> >> 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)
> >>
> >> v8:
> >> * Remove MBZ comments where not applicable.
> >> * Rename ABI flags to match engine class define naming.
> >> * Rename SFC ABI flag to reflect it applies to VCS and VECS.
> >> * SFC is wired to even _logical_ engine instances.
> >> * SFC applies to VCS and VECS.
> >> * HEVC is present on all instances on Gen11. (Tony)
> >> * Simplify length calculation even more. (Chris Wilson)
> >> * Move info_ptr assigment closer to loop for clarity. (Chris Wilson)
> >> * Use vdbox_sfc_access from runtime info.
> >> * Rebase for RUNTIME_INFO.
> >> * Refactor for lower indentation.
> >> * Rename uAPI class/instance to engine_class/instance to avoid C++
> >> keyword.
> >>
> >> v9:
> >> * Rebase for s/num_rings/num_engines/ in RUNTIME_INFO.
> >>
> >> v10:
> >> * Use new copy_query_item.
> >>
> >> v11:
> >> * Consolidate with struct i915_engine_class_instnace.
> >>
> >> Signed-off-by: Tvrtko Ursulin
> >> Cc: Chris Wilson
> >> Cc: Jon Bloomfield
> >> Cc: Dmitry Rogozhkin
> >> Cc: Lionel Landwerlin
> >> Cc: Joonas Lahtinen
> >> Cc: Tony Ye
> >> Reviewed-by: Lionel Landwerlin # v7
> >> Reviewed-by: Chris Wilson # v7
> >> ---
> >> +/**
> >> + * struct drm_i915_engine_info
> >> + *
> >> + * Describes one engine and it's capabilities as known to the driver.
> >> + */
> >> +struct drm_i915_engine_info {
> >> + /** Engine class and instance. */
> >> + struct i915_engine_class_instance engine;
> >> +
> >> + /** Reserved field. */
> >> + __u32 rsvd0;
> >> +
> >> + /** Engine flags. */
> >> + __u64 flags;
> >
> > Do you think we could do something like
> > BUILD_BUG_ON(!IS_ALIGNED(offsetof(*info, flags), sizeof(info->flags));
> >
> > Will that work, and worthwhile? Maybe work into a
> >
> > BUILD_BUG_ON(check_user_alignment(info, flags));
>
> Hmm.. probably manual check for no holes _and_ alignment is good enough
> for uAPI since once it's in it's in. Will triple-check.
Yeah, we actually need something more like
offsetofend(previous_field) == offsetof(next_field)
BUILD_BUG_ON(check_user_struct(info, previous_field, next_field)) ?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v11] drm/i915: Engine discovery query
On 01/05/2019 12:10, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-05-01 11:52:28)
From: Tvrtko Ursulin
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)
v8:
* Remove MBZ comments where not applicable.
* Rename ABI flags to match engine class define naming.
* Rename SFC ABI flag to reflect it applies to VCS and VECS.
* SFC is wired to even _logical_ engine instances.
* SFC applies to VCS and VECS.
* HEVC is present on all instances on Gen11. (Tony)
* Simplify length calculation even more. (Chris Wilson)
* Move info_ptr assigment closer to loop for clarity. (Chris Wilson)
* Use vdbox_sfc_access from runtime info.
* Rebase for RUNTIME_INFO.
* Refactor for lower indentation.
* Rename uAPI class/instance to engine_class/instance to avoid C++
keyword.
v9:
* Rebase for s/num_rings/num_engines/ in RUNTIME_INFO.
v10:
* Use new copy_query_item.
v11:
* Consolidate with struct i915_engine_class_instnace.
Signed-off-by: Tvrtko Ursulin
Cc: Chris Wilson
Cc: Jon Bloomfield
Cc: Dmitry Rogozhkin
Cc: Lionel Landwerlin
Cc: Joonas Lahtinen
Cc: Tony Ye
Reviewed-by: Lionel Landwerlin # v7
Reviewed-by: Chris Wilson # v7
---
+/**
+ * struct drm_i915_engine_info
+ *
+ * Describes one engine and it's capabilities as known to the driver.
+ */
+struct drm_i915_engine_info {
+ /** Engine class and instance. */
+ struct i915_engine_class_instance engine;
+
+ /** Reserved field. */
+ __u32 rsvd0;
+
+ /** Engine flags. */
+ __u64 flags;
Do you think we could do something like
BUILD_BUG_ON(!IS_ALIGNED(offsetof(*info, flags), sizeof(info->flags));
Will that work, and worthwhile? Maybe work into a
BUILD_BUG_ON(check_user_alignment(info, flags));
Hmm.. probably manual check for no holes _and_ alignment is good enough
for uAPI since once it's in it's in. Will triple-check.
Reviewed-by: Chris Wilson
Thanks! I apparently messed up the actual branch and will resend once
IGT series finished so I can play with Test-with:
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v11] drm/i915: Engine discovery query
Quoting Tvrtko Ursulin (2019-05-01 11:52:28)
> From: Tvrtko Ursulin
>
> 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)
>
> v8:
> * Remove MBZ comments where not applicable.
> * Rename ABI flags to match engine class define naming.
> * Rename SFC ABI flag to reflect it applies to VCS and VECS.
> * SFC is wired to even _logical_ engine instances.
> * SFC applies to VCS and VECS.
> * HEVC is present on all instances on Gen11. (Tony)
> * Simplify length calculation even more. (Chris Wilson)
> * Move info_ptr assigment closer to loop for clarity. (Chris Wilson)
> * Use vdbox_sfc_access from runtime info.
> * Rebase for RUNTIME_INFO.
> * Refactor for lower indentation.
> * Rename uAPI class/instance to engine_class/instance to avoid C++
>keyword.
>
> v9:
> * Rebase for s/num_rings/num_engines/ in RUNTIME_INFO.
>
> v10:
> * Use new copy_query_item.
>
> v11:
> * Consolidate with struct i915_engine_class_instnace.
>
> Signed-off-by: Tvrtko Ursulin
> Cc: Chris Wilson
> Cc: Jon Bloomfield
> Cc: Dmitry Rogozhkin
> Cc: Lionel Landwerlin
> Cc: Joonas Lahtinen
> Cc: Tony Ye
> Reviewed-by: Lionel Landwerlin # v7
> Reviewed-by: Chris Wilson # v7
> ---
> +/**
> + * struct drm_i915_engine_info
> + *
> + * Describes one engine and it's capabilities as known to the driver.
> + */
> +struct drm_i915_engine_info {
> + /** Engine class and instance. */
> + struct i915_engine_class_instance engine;
> +
> + /** Reserved field. */
> + __u32 rsvd0;
> +
> + /** Engine flags. */
> + __u64 flags;
Do you think we could do something like
BUILD_BUG_ON(!IS_ALIGNED(offsetof(*info, flags), sizeof(info->flags));
Will that work, and worthwhile? Maybe work into a
BUILD_BUG_ON(check_user_alignment(info, flags));
Reviewed-by: Chris Wilson
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
