Re: [i-g-t,v2,1/2] tests/xe: Add tests to validate GT coarse power gating status and toggle coarse power gating states

[Riana Tauro <riana.tauro@intel.com>]

On 10/10/2024 12:23 PM, sk.anirban@intel.com wrote:
> From: Sk Anirban <sk.anirban@intel.com>
> 
> Implement test test-cpg-basic to validate coarse power gating status
> using a suspend cycle.
name has been changed

after S3 cycle

> Add test toggle-gt-cpg to check if GT coarse power gating is up when
> forcewake is acquired and down when released.
> 
> v2: Address cosmetic review comments (Riana)
>      Fix suspend state (Riana)
>      Add exit handler for this test toggle_gt_cpg (Riana)
> 
> Signed-off-by: Sk Anirban <sk.anirban@intel.com>
> ---
>   tests/intel/xe_pm_residency.c | 98 +++++++++++++++++++++++++++++++++++
>   1 file changed, 98 insertions(+)
> 
> diff --git a/tests/intel/xe_pm_residency.c b/tests/intel/xe_pm_residency.c
> index 772fe9b57..54a8c62f7 100644
> --- a/tests/intel/xe_pm_residency.c
> +++ b/tests/intel/xe_pm_residency.c
> @@ -63,6 +63,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: cpg-basic
> + * 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 +323,87 @@ 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: "
defines are not necessary. you are using the string only in one 
function. Keep it local
> +
> +static void powergate_info(int fd, int gt)
> +{
> +	int dir;
> +	char *str;
> +	char *render_substr;
> +	char *media_substr;
use inverted xmas tree order. Define all char* in one line
> +
> +	dir = igt_debugfs_gt_dir(fd, gt);
> +	igt_assert(dir >= 0);
> +
> +	str = igt_sysfs_get(dir, "powergate_info");
use igt_debugfs_read or __igt_debugfs_read

> +	igt_assert_f(str, "Failed to read sysfs attribute\n"
> +	render_substr = strstr(str, RENDER_POWER_GATING);
> +	if (render_substr) {
unnecesaary parenthesis
> +		igt_assert_f(strncmp(render_substr + strlen(RENDER_POWER_GATING), "yes", 3) == 0,
> +			     "Render Power Gating should be enabled");
> +	}
> +
> +	media_substr = strstr(str, MEDIA_POWER_GATING);
> +	if (media_substr) {
same as above
> +		igt_assert_f(strncmp(media_substr + strlen(MEDIA_POWER_GATING), "yes", 3) == 0,
> +			     "Media Power Gating should be enabled");
> +	}
> +
assert at last to avoid any open dirs.

render_substr = strstr(str, RENDER_POWER_GATING);
media_substr = strstr(str, MEDIA_POWER_GATING);

free(str);
close(dir);

as assert doesn't require any directories to be open for this test

> +	free(str);
> +	close(dir);
> +}
> +
> +
> +static void powergate_status(int fd, int gt, const char *expected_status)
> +{
> +	int dir;
> +	char *str;
> +	char *status_substr;
> +
use inverted xmas tree order. Define all char* in one line
> +	dir = igt_debugfs_gt_dir(fd, gt);
> +	igt_assert(dir >= 0);
> +
> +	str = igt_sysfs_get(dir, "powergate_info");
same as above
> +	igt_assert_f(str, "Failed to read sysfs attribute\n");
> +	status_substr = strstr(str, POWER_GATE_STATUS);
> +	while (status_substr) {
> +		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);
match the parenthesis
> +
> +		status_substr = strstr(status_substr + strlen(POWER_GATE_STATUS),
> +		POWER_GATE_STATUS);
> +	}
> +	close(dir);
> +}
> +
> +static void cpg_basic(int fd, int gt)
> +{
> +	powergate_info(fd, gt);
powergate_info doesn't seem right.
check_powergate_status / cpg_enabled?
> +	igt_system_suspend_autoresume(SUSPEND_STATE_S3, SUSPEND_TEST_NONE);
> +	powergate_info(fd, gt);
> +}
> +
> +static void toggle_gt_cpg(int fd)
> +{
> +	int gt;
> +
> +	xe_for_each_gt(fd, gt)
> +		powergate_info(fd, gt);
> +
> +	fw_handle = igt_debugfs_open(fd, "forcewake_all", O_RDONLY);
> +	igt_assert_lte(0, fw_handle);
reorder powergate info below under this for_each_gt

Thanks,
Riana
> +	xe_for_each_gt(fd, gt)
> +		powergate_status(fd, gt, "up");
> +
> +	close(fw_handle);
> +	sleep(1);
> +	xe_for_each_gt(fd, gt)
> +		powergate_status(fd, gt, "down");
> +}
> +
>   igt_main
>   {
>   	uint32_t d3cold_allowed;
> @@ -380,6 +467,17 @@ igt_main
>   		toggle_gt_c6(fd, NUM_REPS);
>   	}
>   
> +	igt_describe("Validate Coarse power gating status during suspend cycle");
> +	igt_subtest("cpg-basic")
> +		xe_for_each_gt(fd, gt)
> +			cpg_basic(fd, gt);
> +
> +	igt_describe("Toggle GT coarse power gating states by managing forcewake");
> +	igt_subtest("toggle-gt-cpg") {
> +		igt_install_exit_handler(close_fw_handle);
> +		toggle_gt_cpg(fd);
> +	}
> +
>   	igt_fixture {
>   		close(fd);
>   	}