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

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

On Sun, 13 Oct 2024 23:00:33 -0700, Harish Chegondi wrote:
>

Hi Harish,

Once again only reviewing the uapi (changes to xe_drm.h) for now.

> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index c4182e95a619..50ad6b2e1450 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,
>  };
>
>  /**
> @@ -1696,6 +1698,46 @@ 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 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.

We need to add a comment here saying that EU stall stream has to be enabled
using @DRM_XE_OBSERVATION_IOCTL_ENABLE before reading data. Also, another
comment that EIO return from read() indicates buffer overflow.

We are not using DRM_XE_OBSERVATION_IOCTL_STATUS here the way OA does to
return the reason for EIO return. I think that is OK for now. If later it
turns out we have other reasons (apart from overflow) for returning EIO, we
can handle it via capabilities.

> + */
> +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,
> +
> +	/**
> +	 * @DRM_XE_EU_STALL_PROP_POLL_PERIOD: EU stall data
> +	 * poll period in nanoseconds at which the driver polls
> +	 * for EU stall data in the buffer. Should be at least 100000 ns.
> +	 */
> +	DRM_XE_EU_STALL_PROP_POLL_PERIOD,
> +
> +	/**
> +	 * @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,

Note that we need to show UMD usage of /each/ of these properties in order
to merge them. So are we sure at least one UMD is using each of these
properties? If not we need to drop them from xe_drm.h.

I don't like DRM_XE_EU_STALL_PROP_POLL_PERIOD in particular because it is
exposing a parameter related to internal implemenation in the kernel. OA
had a similar property but turned out it was never used. Not sure what the
situation here is.

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

Thanks.
--
Ashutosh