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

["Souza, Jose" <jose.souza@intel.com>]

On Tue, 2024-05-21 at 07:43 -0700, José Roberto de Souza wrote:
> 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()?

Missed 'Insufficient privileges to remove xe OA config', that need to be removed too 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
>