Re: [PATCH 1/5] drm/xe/oa/uapi: Expose media OA units

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

On Thu, 05 Jun 2025 11:28:59 -0700, Dixit, Ashutosh wrote:
>
> On Wed, 04 Jun 2025 16:42:04 -0700, Umesh Nerlige Ramappa wrote:
> >

Hi Umesh,

> > On Tue, Jun 03, 2025 at 01:21:29PM -0700, Ashutosh Dixit wrote:
> > > On Xe2+ platforms, media engines are attached to "SCMI" OA media (OAM)
> > > units. One or more SCMI OAM units might be present on a platform. In
> > > addition there is another OAM unit for global events, called
> > > OAM-SAG. Performance metrics for media workloads can be obtained from these
> > > OAM units, similar to OAG.
> > >
> > > Expose these OAM units for userspace to use. OAM-SAG is exposed as an OA
> > > unit without any attached engines.
> > >
> > > Bspec: 70819, 67103, 63844, 72572, 74476, 61284
> > >
> > > v2: Fix xe_gt_WARN_ON in __hwe_oam_unit for < 12.7 platforms
> > > v3: Return XE_OA_UNIT_INVALID for < 12.7 to indicate no OAM units
> > > v4: Move xe_oa_print_oa_units() to separate patch
> > > v5: Introduce DRM_XE_OA_UNIT_TYPE_OAM_SAG
> > >
> > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_oa.c | 68 ++++++++++++++++++++++++++++----------
> > > include/uapi/drm/xe_drm.h  |  3 ++
> > > 2 files changed, 54 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > > index fb842fa0552e5..0de0d5a18df74 100644
> > > --- a/drivers/gpu/drm/xe/xe_oa.c
> > > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > > @@ -43,6 +43,12 @@
> > > #define DEFAULT_POLL_PERIOD_NS (NSEC_PER_SEC / DEFAULT_POLL_FREQUENCY_HZ)
> > > #define XE_OA_UNIT_INVALID U32_MAX
> > >
> > > +enum xe_oam_unit_type {
> > > +	XE_OAM_UNIT_SAG,
> > > +	XE_OAM_UNIT_SCMI_0,
> > > +	XE_OAM_UNIT_SCMI_1,
> > > +};
> > > +
> > > enum xe_oa_submit_deps {
> > >	XE_OA_SUBMIT_NO_DEPS,
> > >	XE_OA_SUBMIT_ADD_DEPS,
> > > @@ -1881,6 +1887,7 @@ static bool engine_supports_oa_format(const struct xe_hw_engine *hwe, int type)
> > >		return type == DRM_XE_OA_FMT_TYPE_OAG || type == DRM_XE_OA_FMT_TYPE_OAR ||
> > >			type == DRM_XE_OA_FMT_TYPE_OAC || type == DRM_XE_OA_FMT_TYPE_PEC;
> > >	case DRM_XE_OA_UNIT_TYPE_OAM:
> > > +	case DRM_XE_OA_UNIT_TYPE_OAM_SAG:
> > >		return type == DRM_XE_OA_FMT_TYPE_OAM || type == DRM_XE_OA_FMT_TYPE_OAM_MPEC;
> > >	default:
> > >		return false;
> > > @@ -2448,20 +2455,38 @@ int xe_oa_register(struct xe_device *xe)
> > >
> > > static u32 num_oa_units_per_gt(struct xe_gt *gt)
> > > {
> > > -	return 1;
> > > +	if (!xe_gt_is_media_type(gt) || GRAPHICS_VER(gt_to_xe(gt)) < 20)
> > > +		return 1;
> > > +	else if (!IS_DGFX(gt_to_xe(gt)))
> > > +		return XE_OAM_UNIT_SCMI_0 + 1; /* SAG + SCMI_0 */
> > > +	else
> > > +		return XE_OAM_UNIT_SCMI_1 + 1; /* SAG + SCMI_0 + SCMI_1 */
> > > }
> > >
> > > static u32 __hwe_oam_unit(struct xe_hw_engine *hwe)
> > > {
> > > -	if (GRAPHICS_VERx100(gt_to_xe(hwe->gt)) >= 1270) {
> > > -		/*
> > > -		 * There's 1 SAMEDIA gt and 1 OAM per SAMEDIA gt. All media slices
> > > -		 * within the gt use the same OAM. All MTL/LNL SKUs list 1 SA MEDIA
> > > -		 */
> > > -		xe_gt_WARN_ON(hwe->gt, hwe->gt->info.type != XE_GT_TYPE_MEDIA);
> > > +	if (GRAPHICS_VERx100(gt_to_xe(hwe->gt)) < 1270)
> > > +		return XE_OA_UNIT_INVALID;
> > >
> > > +	xe_gt_WARN_ON(hwe->gt, !xe_gt_is_media_type(hwe->gt));
> > > +
> > > +	if (GRAPHICS_VER(gt_to_xe(hwe->gt)) < 20)
> > >		return 0;
> > > -	}
> > > +	/*
> > > +	 * XE_OAM_UNIT_SAG has only GSCCS attached to it, but only on some platforms. Also
> > > +	 * GSCCS cannot be used to submit batches to program the OAM unit. Therefore we don't
> > > +	 * assign an OA unit to GSCCS. This means that XE_OAM_UNIT_SAG is exposed as an OA
> > > +	 * unit without attached engines. Fused off engines can also result in oa_unit's with
> > > +	 * num_engines == 0. OA streams can be opened on all OA units.
> > > +	 */
> > > +	else if (hwe->engine_id == XE_HW_ENGINE_GSCCS0)
> >
> > You could just drop the case XE_ENGINE_CLASS_OTHER in caller
> > (__hwe_oa_unit()) and then you dont need the XE_HW_ENGINE_GSCCS0 check
> > here.
>
> Yes, I was doing that initially. But then I changed to this to make it
> *explicit* that even when GSC is attached to OAM-SAG, it is not assigned to
> any OA unit. Because it is really an exception, as the comment above
> explains.
>
> If you are asking why XE_ENGINE_CLASS_OTHER calls __hwe_oam_unit(), that is
> because GSC is present on media GT and is used for HuC authentication
> etc. (also related to media).
>
> If you insist I can change it, but I think it is better to leave as
> is. This way it is part of the code, the other way it will be in a comment
> (and would be implicit and later people might wonder if it is missed). Let
> me know.
>
> >
> > > +		return XE_OA_UNIT_INVALID;
> > > +	else if (!IS_DGFX(gt_to_xe(hwe->gt)))
> > > +		return XE_OAM_UNIT_SCMI_0;
> > > +	else if (hwe->class == XE_ENGINE_CLASS_VIDEO_DECODE)
> > > +		return (hwe->instance / 2 & 0x1) + 1;
> >
> > should be same as (hwe->instance / 2) + 1;
>
> To map any possible VCS (VCS0 - VCS7) to OAM-SCMI-0 or OAM-SCMI-1 we need
> the '& 0x1'.
>
> >
> > > +	else if (hwe->class == XE_ENGINE_CLASS_VIDEO_ENHANCE)
> > > +		return (hwe->instance & 0x1) + 1;
> >
> > same here, should be same as (hwe->instance + 1)
>
> Similarly, to map any possible VECS (VECS0 - VECS3) to OAM-SCMI-0 or 1 we
> need the '& 0x1'.

So I ended up not making any changes to this patch. Anyway please take
another look and let me know what you think.

Thanks.
--
Ashutosh