Re: [PATCH v9 2/8] drm/xe/uapi: Introduce API for EU stall sampling

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

On Mon, 10 Feb 2025 15:07:51 -0800, Dixit, Ashutosh wrote:
>
> > +static int set_prop_eu_stall_sampling_rate(struct xe_device *xe, u64 value,
> > +					   struct eu_stall_open_properties *props)
> > +{
> > +	value = div_u64(value, 251);
> > +	if (value == 0 || value > 7) {
> > +		drm_dbg(&xe->drm, "Invalid EU stall sampling rate %llu\n", value);
> > +		return -EINVAL;
> > +	}
> > +	props->sampling_rate_mult = value;
> > +	return 0;
> > +}
> > +
> > +static int set_prop_eu_stall_wait_num_reports(struct xe_device *xe, u64 value,
> > +					      struct eu_stall_open_properties *props)
> > +{
> > +	unsigned int max_wait_num_reports;
> > +
> > +	max_wait_num_reports = num_data_rows(per_xecore_buf_size * XE_MAX_DSS_FUSE_BITS);
>
> This seems wrong. Instead of XE_MAX_DSS_FUSE_BITS, shouldn't we use the
> value returned by xe_gt_topology_mask_last_dss()?
>
> Note that a large value can result in the poll/read never getting
> unblocked!
>
> To solve this issue I think num_xecore should be maintained in struct
> xe_eu_stall_gt. Though let's see what happens to 'struct xe_device *' arg
> to these functions if we do this.

Also, even with num_xecore (instead of XE_MAX_DSS_FUSE_BITS), I think

      max_wait_num_reports = num_data_rows(per_xecore_buf_size * num_xecore)

is a very large value. If user specifies this value, we will almost
certainly start dropping data. So maybe max_wait_num_reports should be half
of this value?

Or do we just leave this max and let the user deal with -EIO returns?

Anyway something to think about.