From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id 68E0D10E064 for ; Tue, 18 Jul 2023 04:42:16 +0000 (UTC) Date: Mon, 17 Jul 2023 21:42:14 -0700 Message-ID: <87lefdj0ex.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: "Nilawar, Badal" In-Reply-To: References: <20230717105711.3277000-1-badal.nilawar@intel.com> <87o7kahruu.wl-ashutosh.dixit@intel.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [igt-dev] [PATCH i-g-t v2] tests/xe: Verify actual frequency on the basis of GT state List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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