Re: [igt-dev] [PATCH i-g-t v2] tests/xe: Verify actual frequency on the basis of GT state

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

On Mon, 17 Jul 2023 21:27:02 -0700, Nilawar, Badal wrote:
>
> >> @@ -226,6 +226,9 @@ static void test_freq_fixed(int fd, int gt_id)
> >>
> >>	igt_debug("Starting testing fixed request\n");
> >>
> >> +	if (gt_idle)
> >> +		igt_require(igt_wait(xe_is_gt_in_c6(fd, gt_id), 1000, 1));
> >> +
> >>	/*
> >>	 * For Fixed freq we need to set both min and max to the desired value
> >>	 * Then we check if hardware is actually operating at the desired freq
> >> @@ -235,13 +238,25 @@ static void test_freq_fixed(int fd, int gt_id)
> >>	igt_assert(set_freq(fd, gt_id, "max", rpn) > 0);
> >>	usleep(ACT_FREQ_LATENCY_US);
> >>	igt_assert(get_freq(fd, gt_id, "cur") == rpn);
> >> -	igt_assert(get_freq(fd, gt_id, "act") == rpn);
> >> +
> >> +	if (gt_idle) {
> >> +		usleep(ACT_FREQ_LATENCY_US);
> >
> > Looks like this usleep should in the else? Since we've already igt_wait'ed
> > above for the idle case? In all places in this patch.
>
> As get_freq(fd, gt_id, "cur") will forcewake the gt so added usleep to gt
> to settle down.

Ah good point. I missed that reading requested freq takes forcewake (unlike
act freq).

> Else case is covering exec case so it is expected that act
> freq is already reached to set freq with the delay added after set_freq.

Missed this too, so this is ok too.

> >
> >> +		igt_assert(get_freq(fd, gt_id, "act") == 0);
> >> +	} else {
> >> +		igt_assert(get_freq(fd, gt_id, "act") == rpn);
> >> +	}
> >>
> >>	igt_assert(set_freq(fd, gt_id, "min", rpe) > 0);
> >>	igt_assert(set_freq(fd, gt_id, "max", rpe) > 0);
> >>	usleep(ACT_FREQ_LATENCY_US);
> >>	igt_assert(get_freq(fd, gt_id, "cur") == rpe);
> >> -	igt_assert(get_freq(fd, gt_id, "act") == rpe);
> >> +
> >> +	if (gt_idle) {
> >> +		usleep(ACT_FREQ_LATENCY_US);
> >> +		igt_assert(get_freq(fd, gt_id, "act") == 0);
> >> +	} else {
> >> +		igt_assert(get_freq(fd, gt_id, "act") == rpe);
> >> +	}
> >>
> >>	igt_assert(set_freq(fd, gt_id, "min", rp0) > 0);
> >>	igt_assert(set_freq(fd, gt_id, "max", rp0) > 0);
> >
> > We can't check for 0 act freq in this rp0 case?
> Yes we can check. I will add this check.
> >
> >> diff --git a/tests/xe/xe_pm_residency.c b/tests/xe/xe_pm_residency.c
> >> index a20c4449c..5c4516d03 100644
> >> --- a/tests/xe/xe_pm_residency.c
> >> +++ b/tests/xe/xe_pm_residency.c
> >> @@ -28,6 +28,16 @@ const double tolerance = 0.1;
> >>		     (tol) * 100.0, (tol) * 100.0, \
> >>		     (double)(ref))
> >>
> >> +/**
> >> + * SUBTEST: gt-c6-on-idle
> >> + * Description: Validate GT C6 state on idle
> >> + * Run type: BAT
> >> + *
> >> + * SUBTEST: idle-residency
> >> + * Description: basic residency test to validate idle residency
> >> + *		measured over a time interval is within the tolerance
> >> + * Run type: FULL
> >> + */
> >
> > Why combine these two doc sections? idle-residency description should be
> > above test_idle_residency?
> Since gt-c6-on-idle moved to lib I moved its doc here. To maintain
> uniformity I combined all the docs. tests/xe/xe_ccs.c is following this
> practice.

OK, this is fine too.

Thanks.
--
Ashutosh


> >
> >>   IGT_TEST_DESCRIPTION("Tests for gtidle properties");
> >>
> >>   static unsigned int measured_usleep(unsigned int usec)
> >> @@ -45,24 +55,6 @@ static unsigned int measured_usleep(unsigned int usec)
> >>	return igt_nsec_elapsed(&ts) / 1000;
> >>   }
> >>
> >> -/**
> >> - * SUBTEST: gt-c6-on-idle
> >> - * Description: Validate GT C6 state on idle
> >> - * Run type: BAT
> >> - */
> >> -static bool is_gt_in_c6(int fd, int gt)
> >> -{
> >> -	char gt_c_state[16];
> >> -	int gt_fd;
> >> -
> >> -	gt_fd = xe_sysfs_gt_open(fd, gt);
> >> -	igt_assert(gt_fd >= 0);
> >> -	igt_assert(igt_sysfs_scanf(gt_fd, "gtidle/idle_status", "%s", gt_c_state) == 1);
> >> -	close(gt_fd);
> >> -
> >> -	return strcmp(gt_c_state, "gt-c6") == 0;
> >> -}
> >> -
> >>   static unsigned long read_idle_residency(int fd, int gt)
> >>   {
> >>	unsigned long residency = 0;
> >> @@ -76,17 +68,11 @@ static unsigned long read_idle_residency(int fd, int gt)
> >>	return residency;
> >>   }
> >>
> >> -/**
> >> - * SUBTEST: idle-residency
> >> - * Description: basic residency test to validate idle residency
> >> - *		measured over a time interval is within the tolerance
> >> - * Run type: FULL
> >> - */
> >>   static void test_idle_residency(int fd, int gt)
> >>   {
> >>	unsigned long elapsed_ms, residency_start, residency_end;
> >>
> >> -	igt_assert_f(igt_wait(is_gt_in_c6(fd, gt), 1000, 1), "GT not in C6\n");
> >> +	igt_assert_f(igt_wait(xe_is_gt_in_c6(fd, gt), 1000, 1), "GT not in C6\n");
> >>
> >>	residency_start = read_idle_residency(fd, gt);
> >>	elapsed_ms = measured_usleep(SLEEP_DURATION * 1000) / 1000;
> >> @@ -110,7 +96,7 @@ igt_main
> >>	igt_describe("Validate GT C6 on idle");
> >>	igt_subtest("gt-c6-on-idle")
> >>		xe_for_each_gt(fd, gt)
> >> -			igt_assert_f(igt_wait(is_gt_in_c6(fd, gt), 1000, 1), "GT not in C6\n");
> >> +			igt_assert_f(igt_wait(xe_is_gt_in_c6(fd, gt), 1000, 1), "GT not in C6\n");
> >>
> >>	igt_describe("Validate idle residency measured over a time interval is within the tolerance");
> >>	igt_subtest("idle-residency")
> >> --
> >> 2.25.1
> >
> > Thanks.
> > --
> > Ashutosh