[PATCH i-g-t] tests/xe: Add tests to validate GT coarse power gating status and toggle coarse power gating states

[Anirban, Sk]

Hi Riana,
Will fix those issues.

Thanks,
Anirban

-----Original Message-----
From: Tauro, Riana <riana.tauro at intel.com> 
Sent: Thursday, October 10, 2024 10:42 AM
To: Anirban, Sk <sk.anirban at intel.com>; igt-dev at lists.freedesktop.org; Gupta, Anshuman <anshuman.gupta at intel.com>
Cc: Nilawar, Badal <badal.nilawar at intel.com>
Subject: Re: [PATCH i-g-t] tests/xe: Add tests to validate GT coarse power gating status and toggle coarse power gating states

Hi Anirban

On 10/10/2024 12:01 AM, sk.anirban at intel.com wrote:
> From: Sk Anirban <sk.anirban at intel.com>
> 
> Implement test test-cpg-basic to validate coarse power gating status 
> using a suspend cycle.
> Add test toggle-gt-cpg to check if GT coarse power gating is up when 
> forcewake is acquired and down when released.
> 
> Signed-off-by: Sk Anirban <sk.anirban at intel.com>
> ---
>   tests/intel/xe_pm_residency.c | 104 ++++++++++++++++++++++++++++++++++
>   1 file changed, 104 insertions(+)
> 
> diff --git a/tests/intel/xe_pm_residency.c 
> b/tests/intel/xe_pm_residency.c index 772fe9b57..4245ea877 100644
> --- a/tests/intel/xe_pm_residency.c
> +++ b/tests/intel/xe_pm_residency.c
> @@ -40,6 +40,7 @@ int fw_handle = -1;
>   		     (double)(ref))
>   
>   enum test_type {
> +	TEST_S3IDLE,
%s/TEST_S3IDLE/TEST_S3
>   	TEST_S2IDLE,
>   	TEST_IDLE,
>   };
> @@ -63,6 +64,12 @@ enum test_type {
>    * SUBTEST: toggle-gt-c6
>    * Description: toggles GT C states by acquiring/releasing forcewake,
>    *		also validates power consumed by GPU in GT C6 is lesser than that of GT C0.
> + *
> + * SUBTEST: test-cpg-basic
remove test
> + * Description: Validate GT coarse power gating status with a suspend cycle.
> + *
> + * SUBTEST: toggle-gt-cpg
> + * Description: Toggle GT coarse power gating states by acquiring/releasing forcewake.
>    */
>   IGT_TEST_DESCRIPTION("Tests for gtidle properties");
>   
> @@ -317,6 +324,94 @@ static void toggle_gt_c6(int fd, int n)
>   			     "Power consumed in GT C6 should be lower than GT C0\n");
>   }
>   
> +#define RENDER_POWER_GATING "Render Power Gating Enabled: "
> +#define MEDIA_POWER_GATING "Media Power Gating Enabled: "
> +#define POWER_GATE_STATUS "Power Gate Status: "
> +
> +static void powergate_info(int fd, int gt) {
> +	int dir;
> +	char *str;
> +	char *render_substr;
> +	char *media_substr;
> +
> +	dir = igt_debugfs_gt_dir(fd, gt);
> +	igt_assert(dir >= 0);
> +
> +	str = igt_sysfs_get(dir, "powergate_info");
> +	igt_assert_f(str, "Failed to read sysfs attribute\n");
> +	render_substr = strstr(str, RENDER_POWER_GATING);
> +	if (render_substr != NULL) {
s/if (render_substr != NULL)/ if(render_substr) For all if conditions below
> +		igt_assert_f(strncmp(render_substr + strlen(RENDER_POWER_GATING), "yes", 3) == 0,
> +		"Render Power Gating should be enabled");
Indentation. Should match open parenthesis
> +	}
> +
> +	media_substr = strstr(str, MEDIA_POWER_GATING);
> +	if (media_substr != NULL) {
> +		igt_assert_f(strncmp(media_substr + strlen(MEDIA_POWER_GATING), "yes", 3) == 0,
> +		"Media Power Gating should be enabled");
Indentation. Should match open parenthesis
> +	}
> +
> +	free(str);
> +	close(dir);
> +}
> +
> +
> +static void powergate_status(int fd, int gt, const char 
> +*expected_status) {
> +	int dir;
> +	char *str;
> +	char *status_substr;
> +
> +	dir = igt_debugfs_gt_dir(fd, gt);
> +	igt_assert(dir >= 0);
> +
> +	str = igt_sysfs_get(dir, "powergate_info");
> +	igt_assert_f(str, "Failed to read sysfs attribute\n");
> +	status_substr = strstr(str, POWER_GATE_STATUS);
> +	while (status_substr != NULL) {
> +		igt_assert_f((strncmp(status_substr + strlen(POWER_GATE_STATUS), expected_status,
> +		strlen(expected_status)) == 0), "Power Gate Status Should be %s\n %s\n",
> +		expected_status, str);
> +
> +		status_substr = strstr(status_substr + strlen(POWER_GATE_STATUS),
> +		POWER_GATE_STATUS);
> +	}
> +	close(dir);
> +}
> +
> +static void test_cpg_basic(int fd, int gt, enum test_type flag) {
> +	igt_assert_f(igt_wait(xe_is_gt_in_c6(fd, gt), 1000, 1), "GT %d not 
> +in C6\n", gt);
not necessary
> +	if (flag == TEST_S3IDLE) {
you don't need a flag since there are no other conditions
> +		powergate_info(fd, gt);
> +		igt_system_suspend_autoresume(SUSPEND_STATE_FREEZE, 
> +SUSPEND_TEST_NONE);
you have mentioned S3 but using s2idle. Use SUSPEND_STATE_S3
> +		powergate_info(fd, gt);
> +	}
> +}
> +
> +static void toggle_gt_cpg(int fd, int n) {
> +	int gt;
> +
> +	xe_for_each_gt(fd, gt) {
unnecessary parenthesis
> +		powergate_info(fd, gt);
> +	}
> +
> +	do {
> +		fw_handle = igt_debugfs_open(fd, "forcewake_all", O_RDONLY);
> +		igt_assert_lte(0, fw_handle);
> +		xe_for_each_gt(fd, gt)
> +			powergate_status(fd, gt, "up");
Need an exit handler for this test. The forcewake will be held if there is an assert
> +
> +		close(fw_handle);
> +		sleep(1);
> +		xe_for_each_gt(fd, gt)
> +			powergate_status(fd, gt, "down");
> +		} while (--n);
Alignment. Remove loop. Not required
Once should be enough
> +
Extra line?
> +}
> +
>   igt_main
>   {
>   	uint32_t d3cold_allowed;
> @@ -380,6 +475,15 @@ igt_main
>   		toggle_gt_c6(fd, NUM_REPS);
>   	}
>   
> +	igt_describe("Validate Coarse power gating status during suspend cycle");
> +	igt_subtest("test-cpg-basic")
> +		xe_for_each_gt(fd, gt)
> +			test_cpg_basic(fd, gt, TEST_S3IDLE);
> +
> +	igt_describe("Toggle GT coarse power gating states by managing forcewake");
> +	igt_subtest("toggle-gt-cpg")
> +			toggle_gt_cpg(fd, NUM_REPS);Indentation

Thanks
Riana
> +
>   	igt_fixture {
>   		close(fd);
>   	}