Re: [Intel-gfx] [PATCH] drm/i915/uapi: Add DRM_I915_QUERY_GEOMETRY_SUBSLICES

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

Quoting Tvrtko Ursulin (2022-03-14 17:35:17)
> 
> On 12/03/2022 04:16, Matt Atwood wrote:
> > On Thu, Mar 10, 2022 at 12:26:12PM +0000, Tvrtko Ursulin wrote:
> >>
> >> On 10/03/2022 05:18, Matt Atwood wrote:
> >>> Newer platforms have DSS that aren't necessarily available for both
> >>> geometry and compute, two queries will need to exist. This introduces
> >>> the first, when passing a valid engine class and engine instance in the
> >>> flags returns a topology describing geometry.
> >>>
> >>> v2: fix white space errors
> >>>
> >>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >>> Cc: Matt Roper <matthew.d.roper@intel.com>
> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >>> UMD (mesa): https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14143
> >>> Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>

<SNIP>

> >>> @@ -2714,6 +2715,9 @@ struct drm_i915_query_item {
> >>>      *      - DRM_I915_QUERY_PERF_CONFIG_LIST
> >>>      *      - DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_UUID
> >>>      *      - DRM_I915_QUERY_PERF_CONFIG_FOR_UUID
> >>> +    *
> >>> +    * When query_id == DRM_I915_QUERY_GEOMETRY_SUBSLICES must have bits 0:7 set
> >>> +    * as a valid engine class, and bits 8:15 must have a valid engine instance.
> >>
> >> Alternatively, all other uapi uses struct i915_engine_class_instance to
> >> address engines which uses u16:u16.
> >>
> >> How ugly it is to stuff a struct into u32 flags is the question... But you
> >> could at least use u16:u16 for consistency. Unless you wanted to leave some
> >> bits free for the future?
> > Originally when I wrote this I was wanting to leave space in case it was
> > ever needed. I'm not particularly for or against keeping the space now.
> 
> Yes, shrug... Neither I can't guess if we are ever likely to hit a 
> problem by having fewer bits for class:instance here compared to other 
> uapi, or if stuffing struct i915_engine_class_instance into flags would 
> just be too ugly. I mean there is option to define a new struct and not 
> use flags at all but that's probably to complicated for what it is.
> 
> Anyone else with an opinion? Consistency or should be fine even like it is?

Stuffing a full i915_engine_class_instance was definitely intended when
putting it into the flags was suggested.

If that is hit with a complication, the next proposed alternative was a
new struct. That's why the query interface was made easily extensible,
after all...

Regards, Joonas