Re: [PATCH v11] drm/i915: Engine discovery query

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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 <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)
>>>>
>>>> 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 <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>
>>>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> # v7
>>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # 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