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

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

On Fri, 23 Aug 2024 14:22:19 -0700, Souza, Jose wrote:
>
> Hi

Thanks Jose. One question for Matias/L0 below.

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

Matias: could you please explain what L0 does with this dropped flag?

Harish: do we know what is the reason HW sets this dropped flag? Is it
because userland is not reading fast enough so HW is forced to drop data?

>
> But what can UMD do when that is set?

Mesa can ignore this if they don't need it.

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

dmesg warn is likely not an option because it will trigger bugs in our
CI.

>
> > >
> > > > +	/** @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
>