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 57ADB10E042 for ; Tue, 18 Jul 2023 18:24:57 +0000 (UTC) Date: Tue, 18 Jul 2023 11:24:55 -0700 Message-ID: <87edl5hybs.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Badal Nilawar In-Reply-To: <20230718111736.3324211-1-badal.nilawar@intel.com> References: <20230718111736.3324211-1-badal.nilawar@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 v3] 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 Tue, 18 Jul 2023 04:17:36 -0700, Badal Nilawar wrote: Hi Badal, > diff --git a/lib/igt_pm.h b/lib/igt_pm.h > index 71ec2f239..2fc7b98a1 100644 > --- a/lib/igt_pm.h > +++ b/lib/igt_pm.h > @@ -89,5 +89,6 @@ bool i915_is_slpc_enabled(int drm_fd); > int igt_pm_get_runtime_suspended_time(struct pci_device *pci_dev); > int igt_pm_get_runtime_usage(struct pci_device *pci_dev); > void igt_pm_ignore_slpc_efficient_freq(int i915, int gtfd, bool val); > +bool xe_is_gt_in_c6(int fd, int gt); > > #endif /* IGT_PM_H */ > diff --git a/tests/xe/xe_guc_pc.c b/tests/xe/xe_guc_pc.c > index c34df8d60..a04b700dd 100644 > --- a/tests/xe/xe_guc_pc.c > +++ b/tests/xe/xe_guc_pc.c > @@ -218,7 +218,7 @@ static void test_freq_basic_api(int fd, int gt_id) > * Run type: FULL > */ > > -static void test_freq_fixed(int fd, int gt_id) > +static void test_freq_fixed(int fd, int gt_id, bool gt_idle) > { > uint32_t rpn = get_freq(fd, gt_id, "rpn"); > uint32_t rpe = get_freq(fd, gt_id, "rpe"); > @@ -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,30 @@ 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) { > + /* > + * Before checking act freq usleep is added here and other places to > + * ensure GT is idle as previous get_freq call to read cur freq > + * forcewake the GT. > + */ > + usleep(ACT_FREQ_LATENCY_US); Sorry I missed this yesterday. Basically why do we have the igt_wait(xe_is_gt_in_c6()) above at the top when we are waking up the gt when reading cur freq later? Maybe instead of this usleep, we can this do here: igt_assert(igt_wait(xe_is_gt_in_c6(fd, gt_id), 1000, 1)); And get rid of the igt_require(igt_wait()) at the top? Thanks. -- Ashutosh