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

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

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.

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.

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

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.

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