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

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

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?

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.
>
> 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