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

[Dixit, Ashutosh]

On Fri, 23 Aug 2024 06:09:58 -0700, Souza, Jose wrote:
>

Hi Jose,

> 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?
>
> https://patchwork.freedesktop.org/patch/602553/?series=135559&rev=2#comment_1098100
> https://patchwork.freedesktop.org/patch/602553/?series=135559&rev=2#comment_1099803

This is not what I meant, these comments are more in the nature of code
reviews. What I meant was review of the uapi itself. E.g., following the
discussion below, what we are interested in is things like:

a. Whether Mesa is actually consuming fields in the EU Stall header which
   KMD appends to EU stall packets sent to user space.
b. Whether Mesa prefers to parse the header or get status from a return
   code

So things like that. It would be great if someone from Mesa could respond
to my original email and address the points raised there.

Thanks.
--
Ashutosh



>
> >
> > Thanks.
> > --
> > Ashutosh
> >
> >
> > >
> > > Hi Ashutosh,
> > >
> > > Some inline questions below [MAC]
> > >
> > > Thanks,
> > > _MAC
> > >
> > > -----Original Message-----
> > > From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> > > Sent: Friday, August 16, 2024 3:38 PM
> > > To: intel-xe at lists.freedesktop.org
> > > Cc: Chegondi, Harish <harish.chegondi at intel.com>; Nerlige Ramappa, Umesh <umesh.nerlige.ramappa at intel.com>; Degrood, Felix J <felix.j.degrood at intel.com>; Souza, Jose <jose.souza at intel.com>; Cabral, Matias A <matias.a.cabral at 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.
> > >
> > > 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.
> > >
> > > > +	/** @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.
> > >
> > > 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.
> > >
> > > [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
>