Re: [Intel-xe] [PATCH 01/10] drm/xe/oa: Introduce OA uapi

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

On Wed, 16 Aug 2023 12:58:52 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> On Tue, Aug 15, 2023 at 08:43:55PM -0700, Dixit, Ashutosh wrote:
> > On Tue, 15 Aug 2023 19:21:23 -0700, Umesh Nerlige Ramappa wrote:
> >>
> >> On Tue, Aug 15, 2023 at 06:30:12PM -0700, Dixit, Ashutosh wrote:
> >> > On Tue, 15 Aug 2023 18:11:55 -0700, Umesh Nerlige Ramappa wrote:
> >> >>
> >> >> >> +	/**
> >> >> >> +	 * Multiple engines may be mapped to the same OA unit. The OA unit is
> >> >> >> +	 * identified by class:instance of any engine mapped to it.
> >> >> >> +	 *
> >> >> >> +	 * This parameter specifies the engine class and must be passed along
> >> >> >> +	 * with DRM_XE_OA_PROP_OA_ENGINE_INSTANCE.
> >> >> >> +	 */
> >> >> >> +	DRM_XE_OA_PROP_OA_ENGINE_CLASS,
> >> >> >> +
> >> >> >> +	/**
> >> >> >> +	 * This parameter specifies the engine instance and must be passed along
> >> >> >> +	 * with DRM_XE_OA_PROP_OA_ENGINE_CLASS.
> >> >> >> +	 */
> >> >> >> +	DRM_XE_OA_PROP_OA_ENGINE_INSTANCE,
> >> >> >
> >> >> > Because oa_unit_id has been introduced above in
> >> >> > drm_xe_engine_class_instance, should we remove these engine class/instance
> >> >> > properties? Since it seems OA streams can be opened directly against the oa
> >> >> > unit identified by oa_unit_id. And the driver can itself figure out the
> >> >> > engine class/instance from the oa_unit_id?
> >> >>
> >> >> Agree for the OA buffer use case.
> >> >>
> >> >> For query, we have used the class:instance to enable query support for
> >> >> specific engine instance. If we can work around that somehow, then we can
> >> >> do away with these 2 params. More like enable query for all engines that
> >> >> belong to the OAG unit. For render, it is straight forward since we have
> >> >> one render per OAG unit. For compute, multiple compute instances can map to
> >> >> one OAG unit and only one instance can support the query. So user may want
> >> >> to choose which instance that is. Although, I would just simplify it and
> >> >> support only instance 0 of compute. If UMD is okay with it, then we should
> >> >> be good.
> >> >
> >> > Can we enable the query (I guess "query" is MI_RPC) on all attached compute
> >> > engines and have the UMD execute the query on any of the attached engines?
> >> > Or is this mode of operation never been tried out before so risky?
> >>
> >> We cannot, since that is gated by a global configuration in HW that selects
> >> a specific instance to enable the query for compute (ccs_select to be
> >> specific). I think the limitation is that HW has only one OAC unit and only
> >> one ccs can use it.
> >
> > OK. But iiuc there is also a context (or, in Xe, an exec_queue). So we
> > should be able to turn on the query on the context/exec_queue hw_engine, no?
> > Assuming more than one hw_engine in the context/exec_queue is an error?
>
> The global config (ccs_select) needs to be applied at the context switch
> granularity which the KMD is unaware of. If the ccs_select configuration
> were a part of the context image, it would have been simpler. If we were to
> do this, it would have to be in the WA batch buffer in the context.  The WA
> batch would run every context switch and would do an LRI on the global
> register. Possible, but hacky and architecturally incorrect since we would
> be modifying a global state from the context.

I think you misunderstood. I was only saying (e.g. with the current uapi)
that 'drm_xe_oa_property_id' has a 'DRM_XE_OA_PROP_EXEC_QUEUE_ID' field in
it. So we know which exec_queue (XE exec_queue is equivalent to i915
gem_context) the query needs to run on. And we know the exec_queue engine
class as well as which hardware engines are backing that exec_queue. So now
say instead of having engine ci in 'drm_xe_oa_property_id' we can say get
the engine ci from exec_queue. This is just exec_q->hwe[0].

So if we have multiple hwe's backing backing the exec_q (say a compute
virtual engine) we can restrict that the query runs only on the 0'th
hwe. If we include engine ci in 'drm_xe_oa_property_id', the query can run
on any hwe backing the exec_queue. In this case we need engine ci in
'drm_xe_oa_property_id', if we can impose the restriction that the query
only run on exec_q->hwe[0], then we don't.

(The correction I have from my previous mail that having only a single hwe
in an exec_queue will probably be unacceptable).

Though I am now thinking just leaving engine ci in 'drm_xe_oa_property_id'
since there is a valid use case for it.

Thanks.
--
Ashutosh

> >> >>
> >> >> Media engines do not support query, so we should be good for OAM.
> >> >>
> >> >> >
> >> >> > Separately, there is also the question of whether we want to share the Xe
> >> >> > OA IGT code with i915 as was done for this series. Since duplicating the
> >> >> > IGT code as well as all the perf tools seems a bit much.
> >> >> >
> >> >> > Thanks.
> >> >> > --
> >> >> > Ashutosh