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

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

On Wed, 16 Aug 2023 13:38:25 -0700, Dixit, Ashutosh wrote:
>
> 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.

Sorry for that rambling mail. But I think, to be fully accurate, we only
need the hw engine instance in 'drm_xe_oa_property_id'. We can get the
class from exec_q->class.

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