[PATCH 00/17] Add OA functionality to Xe

[Souza, Jose]

On Fri, 2024-05-17 at 18:42 -0700, Dixit, Ashutosh wrote:
> 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

Other ask, can you remove this 'Failed to remove unknown OA config' debug message from xe_oa_remove_config_ioctl()?
Mesa will be using DRM_XE_PERF_OP_REMOVE_CONFIG with config id set to UINT64_MAX to detect if Xe KMD supports OA counters and if application has
enough permissions to use it.
So it causes dmesg to be flooded with 'xe 0000:00:02.0: [drm:xe_oa_remove_config_ioctl [xe]] Failed to remove unknown OA config' messages when running
tests suites.

Or do you have other suggestion of uAPI that I can use.

> 
> Thanks for the review and feedback :)
> --
> Ashutosh