Re: [PATCH v5 2/7] drm/xe/eustall: Introduce API for EU stall sampling

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

On Mon, 18 Nov 2024 01:07:14 -0800, Harish Chegondi wrote:

Hi Harish,

Once again reviewing only the uapi for now.

The patch title should contain uapi in it, so drm/xe/uapi, or
drm/xe/eustall/uapi.

Also, you are not recording version log for the patches. That needs to be
added too.

> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 4a8a4a63e99c..80aaa5b50c8a 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -1397,6 +1397,8 @@ struct drm_xe_wait_user_fence {
>  enum drm_xe_observation_type {
>	/** @DRM_XE_OBSERVATION_TYPE_OA: OA observation stream type */
>	DRM_XE_OBSERVATION_TYPE_OA,
> +	/** @DRM_XE_OBSERVATION_TYPE_EU_STALL: EU stall sampling observation stream type */
> +	DRM_XE_OBSERVATION_TYPE_EU_STALL,
>  };
>
>  /**
> @@ -1713,6 +1715,43 @@ struct drm_xe_oa_stream_info {
>	__u64 reserved[3];
>  };
>
> +/**
> + * enum drm_xe_eu_stall_property_id - EU stall sampling input property ids.
> + *
> + * These properties are passed to the driver at open as a chain of
> + * @drm_xe_ext_set_property structures with @property set to these
> + * properties' enums and @value set to the corresponding values of these
> + * properties. @drm_xe_user_extension base.name should be set to
> + * @DRM_XE_EU_STALL_EXTENSION_SET_PROPERTY.
> + *
> + * With the file descriptor obtained from open, user space must enable
> + * the EU stall stream fd with @DRM_XE_OBSERVATION_IOCTL_ENABLE before
> + * calling read.

Good to add this. Maybe also add "EIO return from read() indicates buffer
overflow" since EIO return is part of uapi.

> + */
> +enum drm_xe_eu_stall_property_id {
> +#define DRM_XE_EU_STALL_EXTENSION_SET_PROPERTY		0
> +	/**
> +	 * @DRM_XE_EU_STALL_PROP_SAMPLE_RATE: Sampling rate
> +	 * in multiples of 251 cycles. Valid values are 1 to 7.
> +	 * If the value is 1, sampling interval is 251 cycles.
> +	 * If the value is 7, sampling interval is 7 x 251 cycles.
> +	 */
> +	DRM_XE_EU_STALL_PROP_SAMPLE_RATE = 1,

There is an unaddressed comment from Umesh about this (on the previous
version), we need to address it.

> +
> +	/**
> +	 * @DRM_XE_EU_STALL_PROP_EVENT_REPORT_COUNT: Minimum number of
> +	 * EU stall data rows to be present in the kernel buffer for
> +	 * poll() to set POLLIN (data present).
> +	 */
> +	DRM_XE_EU_STALL_PROP_EVENT_REPORT_COUNT,
> +
> +	/**
> +	 * @DRM_XE_EU_STALL_PROP_GT_ID: GT ID of the GT on which
> +	 * EU stall data will be captured.
> +	 */
> +	DRM_XE_EU_STALL_PROP_GT_ID,

Maybe a nit, but I would make this the first property.

> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> --
> 2.45.1
>

Thanks.
--
Ashutosh