Re: [Intel-gfx] [igt-dev] [PATCH v2 i-g-t] i915_pm_freq_api: Add some debug to tests

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

On Mon, 17 Jul 2023 21:19:13 -0700, Belgaumkar, Vinay wrote:
>
>
> On 7/17/2023 6:50 PM, Dixit, Ashutosh wrote:
> > On Mon, 17 Jul 2023 11:42:13 -0700, Vinay Belgaumkar wrote:
> >> Some subtests seem to be failing in CI, use igt_assert_(lt/eq) which
> >> print the values being compared and some additional debug as well.
> >>
> >> v2: Print GT as well (Ashutosh)
> >>
> >> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> >> ---
> >>   tests/i915/i915_pm_freq_api.c | 18 ++++++++----------
> >>   1 file changed, 8 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/tests/i915/i915_pm_freq_api.c b/tests/i915/i915_pm_freq_api.c
> >> index 522abee35..a7bbd4896 100644
> >> --- a/tests/i915/i915_pm_freq_api.c
> >> +++ b/tests/i915/i915_pm_freq_api.c
> >> @@ -55,6 +55,7 @@ static void test_freq_basic_api(int dirfd, int gt)
> >>	rpn = get_freq(dirfd, RPS_RPn_FREQ_MHZ);
> >>	rp0 = get_freq(dirfd, RPS_RP0_FREQ_MHZ);
> >>	rpe = get_freq(dirfd, RPS_RP1_FREQ_MHZ);
> >> +	igt_debug("GT: %d, RPn: %d, RPe: %d, RP0: %d", gt, rpn, rpe, rp0);
> >>
> >>	/*
> >>	 * Negative bound tests
> >> @@ -90,21 +91,18 @@ static void test_reset(int i915, int dirfd, int gt, int count)
> >>	int fd;
> >>
> >>	for (int i = 0; i < count; i++) {
> >> -		igt_assert_f(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rpn) > 0,
> >> -			     "Failed after %d good cycles\n", i);
> >> -		igt_assert_f(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rpn) > 0,
> >> -			     "Failed after %d good cycles\n", i);
> >> +		igt_debug("Running cycle: %d", i);
> >> +		igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rpn) > 0);
> >> +		igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rpn) > 0);
> > I am R-b'ing this but stuff like this should be using igt_assert_lt()
> > according to the commit message?
> >
> > This _lt stuff has to be fixed all over the file, not just this patch, if
> > it brings any value (again according to the commit message).
> >
> > Let me know if you want to fix this now or in a later patch. I'll wait
> > before merging.
>
> Yup, I will send out another version with the corrected commit message.

Hmm, I thought the code needs to be fixed not the commit message :)

>
> Thanks,
>
> Vinay.
>
> >
> > Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >
> >>		usleep(ACT_FREQ_LATENCY_US);
> >> -		igt_assert_f(get_freq(dirfd, RPS_CUR_FREQ_MHZ) == rpn,
> >> -			     "Failed after %d good cycles\n", i);
> >> +		igt_assert_eq(get_freq(dirfd, RPS_CUR_FREQ_MHZ), rpn);
> >>
> >>		/* Manually trigger a GT reset */
> >>		fd = igt_debugfs_gt_open(i915, gt, "reset", O_WRONLY);
> >>		igt_require(fd >= 0);
> >>		igt_ignore_warn(write(fd, "1\n", 2));
> >>
> >> -		igt_assert_f(get_freq(dirfd, RPS_CUR_FREQ_MHZ) == rpn,
> >> -			     "Failed after %d good cycles\n", i);
> >> +		igt_assert_eq(get_freq(dirfd, RPS_CUR_FREQ_MHZ), rpn);
> >>	}
> >>	close(fd);
> >>   }
> >> @@ -116,13 +114,13 @@ static void test_suspend(int i915, int dirfd, int gt)
> >>	igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rpn) > 0);
> >>	igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rpn) > 0);
> >>	usleep(ACT_FREQ_LATENCY_US);
> >> -	igt_assert(get_freq(dirfd, RPS_CUR_FREQ_MHZ) == rpn);
> >> +	igt_assert_eq(get_freq(dirfd, RPS_CUR_FREQ_MHZ), rpn);
> >>
> >>	/* Manually trigger a suspend */
> >>	igt_system_suspend_autoresume(SUSPEND_STATE_S3,
> >>				      SUSPEND_TEST_NONE);
> >>
> >> -	igt_assert(get_freq(dirfd, RPS_CUR_FREQ_MHZ) == rpn);
> >> +	igt_assert_eq(get_freq(dirfd, RPS_CUR_FREQ_MHZ), rpn);
> >>   }
> >>
> >>   int i915 = -1;
> >> --
> >> 2.38.1
> >>