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

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

On Tue, 19 Nov 2024 15:59:12 -0800, Harish Chegondi wrote:
>
> > > +/**
> > > + * 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.
> > > + */
> > > +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,
> >
> > What is the rate of 251 cycles? If that can be clearly defined, then at
> > first glance, I would think it's better to define this in terms of
> > frequency. The implementation can decide how to translate that to HW
> > configuration.
> >
> Since the duration of a cycle depends on the GPU clock, it can very from
> GPU to GPU. So, if there is any translation in the driver, it will have
> to be different for each GPU. I think keeping this input as a multiplier
> of cycles may be more future proof for the uAPI. I am trying to get more
> information and feedback from the user space regarding your suggestion.
> If it is feasible, I will implement in v6.

Umesh has a point but I sort of agree with Harish because this value is
directly fed into a register. But we do need some changes:

1. This 251 value showing up here doesn't make any sense and needs to go.
2. According to Bspec 64036, HW supports "127 * N" sampling rates
   (in terms of cycles), so we should support those too.
3. Even higher sampling rates (say 10x) are being proposed for the
   future. So these should also be supported.

So my proposal is simple, but let's see if it can be made to work. The uapi
will directly input the sampling rate in number of cycles (so the value
coming in is what the GPU freq is divided by). So e.g. if "3 * 251" is
required "3 * 251" will come in through the uapi. If UMD wants "7 * 127",
they will send in "7 * 127". The driver will internally map this value into
the "closest" sampling rate supported by HW.

I am assuming that UMD's already know what sampling rates are supported by
a particular HW platform so they can send in the exact value they
need. Otherwise the driver can always map the value sent by userspace. Say
UMD sends a value 10, this will be mapped into "1 * 127" which is the
closest sampling rate supported to 10.

So this way all sampling rates can be supported. UMD just says I want a
sampling rate of "GPU_freq divided by 10" and they automatically get
whatever is the closest available. They probably do need to have an idea of
what rates are supported on a particular HW platform, I am assuming they
have this information from Bspec, so they can send in exact values if they
know and driver will be able to set the exact value UMD has specified.

Ashutosh