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

[Chris Wilson <chris@chris-wilson.co.uk>]

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)) ?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx