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

[Cabral, Matias A]

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