Re: [PATCH 13/16] drm/xe/oa/uapi: Query OA unit properties

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 12 Feb 2024 11:59:30 -0800, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> On Wed, Feb 07, 2024 at 09:49:13PM -0800, Ashutosh Dixit wrote:
> > @@ -1411,6 +1412,68 @@ enum drm_xe_oa_unit_type {
> >	DRM_XE_OA_UNIT_TYPE_OAM,
> > };
> >
> > +/**
> > + * struct drm_xe_oa_unit - describe OA unit
> > + */
> > +struct drm_xe_oa_unit {
> > +	/** @oa_unit_id: OA unit ID */
> > +	__u32 oa_unit_id;
> > +
> > +	/** @oa_unit_type: OA unit type of @drm_xe_oa_unit_type */
> > +	__u32 oa_unit_type;
> > +
> > +	/**
> > +	 * @capabilities: OA capabilities bit-mask: this is a bit-mask of
> > +	 * property id's in enum @drm_xe_oa_property_id
> > +	 */
> > +	__u64 capabilities;
>
> IMO, this should be a specific set of flags that are defined as and when we
> add new features so that it helps UMDs detect them. Some features or
> behaviors may be independent of the properties, so a 1:1 mapping to
> properties may not be the best solution. In the past we have had i915 perf
> revision bumps even when we haven't added a new property (For ex: when we
> supported media engines in i915 - class:instance properties were already
> present and we just added support for OAM in that case. For such cases
> capabilities would define a new bit, but not properties).
>
> The first set of properties defined with this series should not need a
> capabilities entry. If the uApi changes in future or adds new features, we
> need to use these bits.

OK, I went ahead and implemented this. Though after this, this seems more
like a version to me, rather than capabilities. The only difference being,
instead of incrementing the version, we add bits to the capabilities. Which
seems fine.

I did pick a DRM_XE_OA_CAPS_BASE value of 1:

https://patchwork.freedesktop.org/patch/578014/?series=121084&rev=10

Thanks.
--
Ashutosh