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 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.

Thanks.
--
Ashutosh