Re: [PATCH 4/5] drm/xe/oa: Assign hwe for OAM_SAG

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

On Wed, 04 Jun 2025 17:14:40 -0700, Umesh Nerlige Ramappa wrote:
>
> On Tue, Jun 03, 2025 at 01:21:32PM -0700, Ashutosh Dixit wrote:
> > Because OAM_SAG doesn't have an attached hwe, assign another hwe belonging
> > to the same gt (and different OAM unit) to OAM_SAG. A hwe is needed for
> > batch submissions to program OA HW.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_oa.c       | 54 +++++++++++++++++++-------------
> > drivers/gpu/drm/xe/xe_oa_types.h |  3 ++
> > 2 files changed, 35 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index 4d6d9f0189a83..35157424010bb 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -1923,37 +1923,45 @@ u16 xe_oa_unit_id(struct xe_hw_engine *hwe)
> >		hwe->oa_unit->oa_unit_id : U16_MAX;
> > }
> >
> > +/* A hwe must be assigned to stream/oa_unit for batch submissions */
> > static int xe_oa_assign_hwe(struct xe_oa *oa, struct xe_oa_open_param *param)
> > {
> > -	struct xe_gt *gt;
> > -	int i, ret = 0;
> > +	struct xe_hw_engine *hwe;
> > +	enum xe_hw_engine_id id;
> > +	int ret = 0;
> > +
> > +	/* If not provided, OA unit defaults to OA unit 0 as per uapi */
> > +	if (!param->oa_unit)
> > +		param->oa_unit = &xe_device_get_gt(oa->xe, 0)->oa.oa_unit[0];
> >
> > +	/* When we have an exec_q, get hwe from the exec_q */
> >	if (param->exec_q) {
> > -		/* When we have an exec_q, get hwe from the exec_q */
> >		param->hwe = xe_gt_hw_engine(param->exec_q->gt, param->exec_q->class,
> >					     param->engine_instance, true);
> > -	} else {
> > -		struct xe_hw_engine *hwe;
> > -		enum xe_hw_engine_id id;
> > -
> > -		/* Else just get the first hwe attached to the oa unit */
> > -		for_each_gt(gt, oa->xe, i) {
> > -			for_each_hw_engine(hwe, gt, id) {
> > -				if (hwe->oa_unit == param->oa_unit) {
> > -					param->hwe = hwe;
> > -					goto out;
> > -				}
> > -			}
> > -		}
> > +		if (!param->hwe || param->hwe->oa_unit != param->oa_unit)
> > +			goto err;
> > +		goto out;
> >	}
> > -out:
> > -	if (!param->hwe || param->hwe->oa_unit != param->oa_unit) {
> > -		drm_dbg(&oa->xe->drm, "Unable to find hwe (%d, %d) for OA unit ID %d\n",
> > -			param->exec_q ? param->exec_q->class : -1,
> > -			param->engine_instance, param->oa_unit->oa_unit_id);
> > -		ret = -EINVAL;
> > +
> > +	/* Else just get the first hwe attached to the oa unit */
> > +	for_each_hw_engine(hwe, param->oa_unit->gt, id) {
> > +		if (hwe->oa_unit == param->oa_unit) {
> > +			param->hwe = hwe;
> > +			goto out;
> > +		}
> >	}
> >
> > +	/* If we still didn't find a hwe, just get one from the same gt */
> > +	for_each_hw_engine(hwe, param->oa_unit->gt, id) {
>
> Not sure if for_each_hw_engine omits reserved engines.

It does not, they have to be omitted separately.

> If we should not use reserved engines, we should check for that here
> using xe_hw_engine_is_reserved().

Reserved means those engines are not available for user space. I think it
is ok for the kernel to use them, at least in some cases reserved means
reserved for the kernel. Reserved engines are different from fused off
engines and for_each_hw_engine() skips fused off engines (via
xe_hw_engine_is_valid()).

I was looking at OA code and looks like in OA we have assumed (mostly
without realizing it :/) that reserved engines can be used by OA internally
and reserved engines are attached to OA units. But we don't expose reserved
engines to userspace in the OA query.

So along those lines, I have skipped checking for reserved engines here. If
we really need to skip reserved engines, that should be done globally in OA
code (don't attach reserved engines to OA units itself), otherwise it will
unnecessarily mess up the code. So for now I am assuming OA code reserved
engine usage is ok.

> Also, maybe we should only try to submit the batch on engine classes
> supported by OA. Submitting the batch from other engines (like bcs) is
> not really tested.

I have added this check now, good catch:

	if (!hwe->oa_unit)
		continue;

Thanks.
--
Ashutosh

>
> > +		param->hwe = hwe;
> > +		goto out;
> > +	}
> > +err:
> > +	drm_dbg(&oa->xe->drm, "Unable to find hwe (%d, %d) for OA unit ID %d\n",
> > +		param->exec_q ? param->exec_q->class : -1,
> > +		param->engine_instance, param->oa_unit->oa_unit_id);
> > +	ret = -EINVAL;
> > +out:
> >	return ret;
> > }
> >
> > @@ -2578,6 +2586,8 @@ static void __xe_oa_init_oa_units(struct xe_gt *gt)
> >				DRM_XE_OA_UNIT_TYPE_OAM_SAG : DRM_XE_OA_UNIT_TYPE_OAM;
> >		}
> >
> > +		u->gt = gt;
> > +
> >		xe_mmio_write32(&gt->mmio, u->regs.oa_ctrl, 0);
> >
> >		/* Ensure MMIO trigger remains disabled till there is a stream */
> > diff --git a/drivers/gpu/drm/xe/xe_oa_types.h b/drivers/gpu/drm/xe/xe_oa_types.h
> > index a01c85931e2a5..2628f78c4e8dc 100644
> > --- a/drivers/gpu/drm/xe/xe_oa_types.h
> > +++ b/drivers/gpu/drm/xe/xe_oa_types.h
> > @@ -95,6 +95,9 @@ struct xe_oa_unit {
> >	/** @oa_unit_id: identifier for the OA unit */
> >	u16 oa_unit_id;
> >
> > +	/** @gt: gt associated with the OA unit */
> > +	struct xe_gt *gt;
> > +
> >	/** @type: Type of OA unit - OAM, OAG etc. */
> >	enum drm_xe_oa_unit_type type;
> >
> > --
> > 2.48.1
> >