Re: [PATCH v2 1/1] drm/xe/eustall: Add support for EU stall sampling

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

Hi

On Thu, 2024-08-22 at 15:53 -0700, Dixit, Ashutosh wrote:
> On Wed, 21 Aug 2024 12:35:51 -0700, Cabral, Matias A wrote:
> 
> Hi Matias,
> 
> Thanks for responding, the input is _very_ helpful.
> 
> Mesa folks: would it be possible for you to provide similar input too?

Felix's MR[1] is only using record_size and num_records, if the drm_xe_eu_stall_data_xe2 was the same size and the sample we would not need the header
at all, inline replies below.

[1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30142

> 
> Thanks.
> --
> Ashutosh
> 
> 
> > 
> > Hi Ashutosh,
> > 
> > Some inline questions below [MAC]
> > 
> > Thanks,
> > _MAC
> > 
> > -----Original Message-----
> > From: Dixit, Ashutosh <ashutosh.dixit@intel.com>
> > Sent: Friday, August 16, 2024 3:38 PM
> > To: intel-xe@lists.freedesktop.org
> > Cc: Chegondi, Harish <harish.chegondi@intel.com>; Nerlige Ramappa, Umesh <umesh.nerlige.ramappa@intel.com>; Degrood, Felix J <felix.j.degrood@intel.com>; Souza, Jose <jose.souza@intel.com>; Cabral, Matias A <matias.a.cabral@intel.com>
> > Subject: Re: [PATCH v2 1/1] drm/xe/eustall: Add support for EU stall sampling
> > 
> > On Sun, 07 Jul 2024 15:41:41 -0700, Ashutosh Dixit wrote:
> > 
> > Hi Harish,
> > 
> > Some comments below on just the uapi first, towards finalizing the uapi
> > with the UMD's who consume this data. And also comparing the uapi with
> > what we did in OA.
> > 
> > > 
> > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > index 19619d4952a8..343de700d10d 100644
> > 
> > /snip/
> > 
> > > +/**
> > > + * struct drm_xe_eu_stall_data_header - EU stall data header.
> > > + * Header with additional information that the driver adds
> > > + * before EU stall data of each subslice during read().
> > 
> > One question to resolve is if we really need this header and if UMD's are
> > actually using the information in this header. In OA we dropped the
> > header and are providing information in the header via different means
> > (see below).
> > 
> > Another option is to actually add a property for the header. So headers
> > are added only when user space requests headers.
> > 
> > > + */
> > > +struct drm_xe_eu_stall_data_header {
> > > +	/** @subslice: subslice number from which the following data
> > > +	 * has been captured.
> > > +	 */
> > > +	__u16 subslice;
> > 
> > Do UMD's use this subslice information? We should check with L0 and Mesa about this.
> > 
> > [MAC] L0 does not currently use this.

No usage for sublice at the moment in Mesa

> > 
> > Also about whether UMD's need or want the header itself. For OA, UMD's
> > were happy not having to parse the header.
> > 
> > > +	/** @flags: flags */
> > > +	__u16 flags;
> > > +/* EU stall data dropped by the HW due to memory buffer being full */
> > > +#define XE_EU_STALL_FLAG_OVERFLOW_DROP	(1 << 0)
> > 
> > In OA such information is returned via
> > DRM_XE_OBSERVATION_IOCTL_STATUS. For EU stall, e.g. we could return a bit
> > mask of subslices which reporting drops. So similar to OA, we could
> > return -EIO when HW reports drops and userspace optionally issues
> > DRM_XE_OBSERVATION_IOCTL_STATUS to retrieve which subslices are reporting
> > drops.
> > 
> > [MAC] having a return code to notify of reports drops would be much
> > preferable. This would allow the UMD detecting this condition during the
> > read phase without needing to process/parse each report.

But what can UMD do when that is set?
I would rather have a warn once printed on dmesg, so the issues don't go silent but it don't need to go to the uAPI.

> > 
> > > +	/** @record_size: size of each EU stall data record */
> > > +	__u16 record_size;
> > 
> > This is static information. Does it need to be in each packet header?
> > E.g. it can be returned via DRM_XE_OBSERVATION_IOCTL_INFO after a EU
> > Stall stream has been opened.
> > 
> > [MAC] since the size is constant, it seems an overhead including the info
> > in every report.

drm_xe_eu_stall_data_xe2 should be of the same size as record_size so it can also be dropped.

> > 
> > The INFO data struct could also include a capabilities field. So if new
> > features are added to EU stall in the future, they would be advertized to
> > user space using the capabilities field.
> > 
> > > +	/** @num_records: number of records following the header */
> > > +	__u16 num_records;
> > 
> > This will not be needed if just return raw EU Stall data without
> > headers. Or even otherwise it is probably not needed, it is the total
> > size of returned data minus the size of the header. Provided we return
> > all available data.

Same as above, would not be needed if drm_xe_eu_stall_data_xe2 matches samples size.

> > 
> > [MAC] the KMD will always return atomic units of reports, right? Then
> > this is not needed, having UMD the possibility to query report size when
> > opening the stream, the UMD can know how many reports are in each read.
> > 
> > > +	/** @reserved: Reserved */
> > > +	__u16 reserved[4];
> > 
> > This can be handled via 'extensions'. And if headers change they can be
> > advertized in capabilities.
> > 
> > > +};
> > > +
> > >  #if defined(__cplusplus)
> > >  }
> > >  #endif
> > > --
> > > 2.41.0
> > > 
> > 
> > Thanks.
> > --
> > Ashutosh