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

[Umesh Nerlige Ramappa]

On Wed, Nov 20, 2024 at 11:04:28AM -0800, Dixit, Ashutosh wrote:
>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.

I am okay with whatever makes sense from a UMD perspective and whatever 
can be extended easily in future. Just the cycles as you are suggesting 
should be good as well.

Just curious, since the gpu frequency is may vary, any idea how the UMDs 
map this data to their timeline?

Thanks,
Umesh

>
>Ashutosh