Re: [Intel-xe] [PATCH 20/21] drm/xe/uapi: Use OA unit id to identify OA unit

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

On Wed, 04 Oct 2023 20:04:23 -0700, Dixit, Ashutosh wrote:
>
> On Wed, 04 Oct 2023 15:37:51 -0700, Umesh Nerlige Ramappa wrote:
> >
>
> Hi Umesh,
>
> > On Tue, Sep 19, 2023 at 09:10:48AM -0700, Ashutosh Dixit wrote:
> > > Previous uapi uses an indirect way (the engine class/instance of an engine
> > > connected to an OA unit) to identify an OA unit. Replace this by directly
> > > using the OA unit ID to identify the OA unit.
> > >
> > > With this change DRM_XE_OA_PROP_OA_ENGINE_CLASS property is not needed any
> > > more and removed. DRM_XE_OA_PROP_OA_ENGINE_INSTANCE is still used with
> > > DRM_XE_OA_PROP_EXEC_QUEUE_ID.
>
> /snip/
>
> > > +static int xe_oa_assign_hwe(struct xe_oa *oa, struct xe_oa_open_properties *props)
> > > +{
> > > +	struct xe_gt *gt;
> > > +	int i, ret = 0;
> > > +
> > > +	if (props->exec_q) {
> > > +		/* When we have an exec_q, get hwe from the exec_q */
> > > +		for_each_gt(gt, oa->xe, i) {
> > > +			props->hwe = xe_gt_hw_engine(gt, props->exec_q->class,
> > > +						     props->instance, false);
> > > +			if (props->hwe)
> > > +				break;
> > > +		}
> > > +		if (props->hwe && (xe_oa_unit_id(props->hwe) != props->oa_unit_id)) {
> > > +			drm_dbg(&oa->xe->drm, "OA unit ID mismatch for exec_q\n");
> > > +			ret = -EINVAL;
> > > +		}
> > > +	} 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 (xe_oa_unit_id(hwe) == props->oa_unit_id) {
> > > +					props->hwe = hwe;
> > > +					goto out;
> > > +				}
> > > +			}
> > > +		}
> > > +	}
> >
> > I think both the if and else blocks above will get you the same hwe object,
> > so you can just pick the else code to assign hwe.
>
> Not when there are multiple hwe's attached to a single OA unit (I am
> thinking media/compute), correct?
>
> > Are we allowing the user to pass either oa_unit_id OR exec_q/instance?
>
> No. oa_unit_id is the basic entity used to identify the OA unit. If
> userland is only using OAG they can skip exec_q. If exec_q is specified,
> the code above checks if exec_q/instance lands on the same OA unit as the
> oa_unit_id.
>
> So oa_unit_id is always provided (for both OAG/OAR) and optionally
> exec_q/instance can be provided (for OAR).
>
> i915 had basically <class, instance, exec_q> and this XE uapi has
> <oa_unit_id, exec_q, instance> so the semantics here is almost the same as
> i915.
>
> > I think based on the HW requirement we should enable OAR/OAG together, so
> > maybe enforce that the user passes both params. Thoughts?
>
> I don't think it will fly since in some cases user is only interested in
> OAG (global data for all contexts) so no point forcing them to also passing
> in exec_q/instance. The user may not even have a specific exec_q in such
> case to pass into the uapi.

There are some additional considerations for the future about this: I think
we should really not need exec_q_id in the OA uapi. We should be able to
enable OAR functionality on all exec_q/instance combos (both which are
present and which may be created while OA is enabled) which can be
scheduled on that OAR unit. This would enable an OA MI_RPC query to be
issued on any exec queue which exists while the OA stream is open. OAR
registers are part of the context image so this looks feasible to me.

The instance field is interesting since I believe compute has a limitation
that OAC can only be enabled on a single hwe at a time. So even if we don't
have an exec_q, instance can be used to identify which hwe instance we are
enabling OAC on. So the semantics of instance will change in this case: at
present instance is the context image instance on which OAR/OAC is enabled,
later it will reflect the HW limitation of the engine instance.

Though we may not be do this immediately because of other priorities. So
currently we do single exec_q but can deprecate that if we can ever get
"any exec_q" to work.

Something like that (not completely thought through yet). Let's not worry
about this now but just wanted to write out my thoughts about this in case
you had any feedback.

Thanks.
--
Ashutosh