Re: [PATCH i-g-t v2 2/2] tests/intel/xe_oa: Remove hardcoded time heuristics

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

On Sun, 05 Jan 2025 19:46:38 -0800, Sai Teja Pottumuttu wrote:
>

Hi Sai Teja,

Thanks for the patch, mostly looks good but I have some usual nits.

> Some tests in xe_oa tests have hardcoded timing heuristics. Refactor it to
> make it more robust and reliable. The patch extends the wait time logically
> but usually it would take a single iteration for the required reports to be
> available so wait time doesn't change much.
>
> v2:
>  - Extend commit message [Lucas]
>  - Make wait function more generic [Lucas]
>
> Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com>
> ---
>  tests/intel/xe_oa.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> index ad3526406..b271278d6 100644
> --- a/tests/intel/xe_oa.c
> +++ b/tests/intel/xe_oa.c
> @@ -4367,6 +4367,29 @@ static void map_oa_buffer_forked_access(const struct drm_xe_engine_class_instanc
>	munmap(vaddr, size);
>  }
>
> +static void wait_for_periodic_reports(void *oa_vaddr,

Let's change this name to 'mmap_wait_for_periodic_reports', to highlight it
only applies to the mmap'd OA buffer case.


> +				      uint32_t n,

optional nit: move this to the previous line, I generally try to to optimize
vertical real estate.

> +				      const struct drm_xe_engine_class_instance *hwe)
> +{
> +	uint32_t period_us = oa_exponent_to_ns(oa_exp_1_millisec) / 1000;
> +	struct intel_xe_perf_metric_set *test_set = metric_set(hwe);
> +	uint64_t fmt = test_set->perf_oa_format;
> +	struct oa_format format = get_oa_format(fmt);

optional nit again, but delete the two temporary variables above, see
below.

> +	uint32_t num_periodic_reports = 0;
> +	uint32_t *reports;
> +
> +	while (num_periodic_reports < n) {
> +		usleep(100 * period_us);

I think this should be something like:

		usleep(2 * n * period_us);

So the wait time should be a function of n (not constant like 100). Here
I'm assuming if we wait for '2 * n' periods, we should probably have n
periodic reports.

> +		num_periodic_reports = 0;
> +		for (reports = (uint32_t *)oa_vaddr;
> +		     reports[0] && oa_timestamp(reports, fmt);
> +		     reports += format.size) {
> +			if (oa_report_is_periodic(reports))
> +				num_periodic_reports++;
> +		}

optional nit: I think this entire loop can just be:

		num_periodic_reports = 0;
		for (reports = (uint32_t *)oa_vaddr;
		     reports[0] && oa_timestamp(reports, fmt) && oa_report_is_periodic(reports);
		     reports += get_oa_format(test_set->perf_oa_format).size)
			num_periodic_reports++;

Just a couple of general comments below, no need to change anything:

* The loop is a little 'funky' in that it goes over the mapped OA buffer
  multiple times, even over previoulsy found reports. I think that is ok
  for now.

* The other thing is that OA periodic reports should be generated as long
  as DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT has been used in OA
  properties. Both places where this function is called from has that, so
  that should be ok.

* If HW somehow doesn't generate periodic reports we'll hang here. But that
  is not expected, so leave as is and we'll deal with it later if we ever
  hit that.

> +	}
> +}
> +
>  static void check_reports(void *oa_vaddr, uint32_t oa_size,
>			  const struct drm_xe_engine_class_instance *hwe)
>  {
> @@ -4396,12 +4419,10 @@ static void check_reports_from_mapped_buffer(const struct drm_xe_engine_class_in
>  {
>	void *vaddr;
>	uint32_t size;
> -	uint32_t period_us = oa_exponent_to_ns(oa_exp_1_millisec) / 1000;
>
>	vaddr = map_oa_buffer(&size);
>
> -	/* wait for approx 100 reports */
> -	usleep(100 * period_us);
> +	wait_for_periodic_reports(vaddr, 20, hwe);

I am wondering if we should make this 10 instead of 20, and also change 20
to 10 in check_reports().

>	check_reports(vaddr, size, hwe);
>
>	munmap(vaddr, size);
> @@ -4426,12 +4447,11 @@ static void closed_fd_and_unmapped_access(const struct drm_xe_engine_class_insta
>	};
>	void *vaddr;
>	uint32_t size;
> -	uint32_t period_us = oa_exponent_to_ns(oa_exp_1_millisec) / 1000;
>
>	stream_fd = __perf_open(drm_fd, &param, false);
>	vaddr = map_oa_buffer(&size);
>
> -	usleep(100 * period_us);
> +	wait_for_periodic_reports(vaddr, 20, hwe);

Here too.

>	check_reports(vaddr, size, hwe);
>
>	munmap(vaddr, size);
> --
> 2.34.1
>

Thanks.
--
Ashutosh