Re: [PATCH 00/17] Add OA functionality to Xe

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

On Fri, 17 May 2024 11:42:39 -0700, Souza, Jose wrote:
>

Hi Jose,

> Hi
>
> I hope I'm replying to the latest version...
>
> Just a few comments I have in the uAPI:
>
> - Rename DRM_XE_OA_PROPERTY_OA_EXPONENT to DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT, for better clarity.

Sure, will do.

> - Some Perf uAPIs added here don't have usage in Mesa, see those below. Not sure if MDAPI or gviz will make use of it, if not the approach is to
> remove uAPIs until there is UMD using it:
>	- DRM_XE_PERF_IOCTL_INFO + drm_xe_oa_stream_info

MDAPI uses this to implement mmap + mmio trigger.

>	- DRM_XE_DEVICE_QUERY_OA_UNITS + drm_xe_query_oa_units + drm_xe_oa_unit

Yes Gpuvis as well as I believe MDAPI use this. Actually I am surprised
Mesa is not using this. Basically the query provides the following
information:

* Which engines are attached to which OA units. This maybe Mesa can skip
  since render engine is almost certainly attached to OA unit 0
  (DRM_XE_OA_PROPERTY_OA_UNIT_ID to be provided when opening OA stream).

* But the query also provides oa_timestamp_freq which needs to be used in
  interpreting OA timestamps. oa_timestamp_freq can differ from
  reference_clock included in 'struct drm_xe_gt' for certain platforms
  (currently DG2/PVC/MTL). So if Mesa needs to work for these platforms,
  you need to use drm_xe_query_oa_units.

> - DRM_XE_OASTATUS_* values are based on HW definition, I think it should
> not follow HW at least for DRM_XE_OASTATUS_MMIO_TRG_Q_FULL and avoid have
> a blank space of bits not used in the uAPI

Sure, will change this too. Umesh also mentioned this.

> - Perf stream read() should give us a hint that drm_xe_oa_stream_status
> - needs to be read, that discussion is already in progress thank you for
> - that

Yes, we are going ahead with that, read() will return EIO errno when KMD
encounters errors (or oastatus) signalled by HW, and when UMD sees that,
UMD can call the stream status ioctl to get the status.

>
> Other than that the uAPI LGTM.
>
> There is the hold_preemption feature that is missing but I think that can
> be added later...

Yes, the plan is to implement it soon. Also another feature to synchronize
OA unit programming with workload submission is missing, and the plan is to
implement that soon too. Hopefully, we can get what we currently have
merged first, based on Mesa and GpuVis pull requests and do these missing
pieces as follow on's.

>
> thank you

Thanks for the review and feedback :)
--
Ashutosh