DC states igt tests patch series

From: Jyoti Yadav <[email protected]>

This patch does the following chnegs to lib/igt_pm.c

(quoted diff) +0 -3

-dmc_loaded() will be used by new test i915_pm_dc.c which will validate
 Display C States. So moving the same to igt_pm library.
 
-Introduced igt_disable_runtime_pm() in order to disable runtime suspend
 for the function which support dc9.
 
-Changed the igt_pm_enable_sata_link_power_management() and
 igt_pm_restore_sata_link_power_management() in order to save
 and restore the sata link power policy by an exit handler.

v2: Simplify the comment section.
v3: Remove . from the subject line.
v4: Rebased, resolve conflicts in pm_rpm.c
Included patch set version change log.
v5: Listing actual change in patch set changelog to make review easier.
v6: igt's lib added support for disabling runtime suspend,
change in commit log. rebased due to test name pm_rpm changed to i915_pm_rpm.
v7: Addressed review comment by saving POWER_DIR values in
igt_disable_runtime_pm(). [Imre]
v8: Addressed the review comment, igt_pm_enable_sata_link_power_management
function to restore the original SATA link power policy if things fail by using an exit handler. [Imre]
v9: IGT failure fixture in i915_pm_backlight and i915_pm_rpm. v10:Review comment fixup in sata_link_power_management
lib functions. [Imre]

Signed-off-by Jyoti Yadav <[email protected]>

Signed-off-by Anshuman Gupta <[email protected]>

---
 lib/igt_pm.c                   | 238 +++++++++++++++++++++++++--------
 lib/igt_pm.h                   |   6 +-
 tests/i915/i915_pm_backlight.c |   6 +-
 tests/i915/i915_pm_rpm.c       |  24 +---
 4 files changed, 189 insertions(+), 85 deletions(-)

lib/igt_pm.c +28 -17

index fd22273a..2a439700 100644
@@ -38,6 +38,7 @@
 #include "drmtest.h"
 #include "igt_pm.h"
 #include "igt_aux.h"
+#include "igt_sysfs.h"
 
 /**
  * SECTION:igt_pm

@@ -63,11 +64,15 @@ enum {
 #define MIN_POWER_STR		"min_power\n"
 /* Remember to fix this if adding longer strings */
 #define MAX_POLICY_STRLEN	strlen(MAX_PERFORMANCE_STR)
+int8_t *__sata_pm_policies;
 
 static char __igt_pm_audio_runtime_power_save[64];
 static char * __igt_pm_audio_runtime_control_path;
 static char __igt_pm_audio_runtime_control[64];
 
+static void __igt_pm_sata_link_pm_exit_handler(int sig);
+static void __igt_pm_restore_sata_link_power_management(void);
+
 static int __igt_pm_audio_restore_runtime_pm(void)
 {
 	int fd;

@@ -280,64 +285,72 @@ void igt_pm_enable_audio_runtime_pm(void)
 		igt_debug("Failed to enable audio runtime PM! (%d)\n", -err);
 }
 
-/**
- * igt_pm_enable_sata_link_power_management:
- *
- * Enable the min_power policy for SATA link power management.
- * Without this we cannot reach deep runtime power states.
- *
- * We don't have any assertions on open since the system might not have
- * a SATA host.
- *
- * Returns:
- * An opaque pointer to the data needed to restore the default values
- * after the test has terminated, or NULL if SATA link power management
- * is not supported. This pointer should be freed when no longer used
- * (typically after having called restore_sata_link_power_management()).
- */
-int8_t *igt_pm_enable_sata_link_power_management(void)
+static int8_t get_sata_policy_for_host(int fd, char *buf, int i)
 {
-	int fd, i;
+	int8_t policy;
 	ssize_t len;
+
+	len = read(fd, buf, MAX_POLICY_STRLEN);
+	buf[len] = '\0';
+
+	if (!strncmp(MAX_PERFORMANCE_STR, buf,
+		     strlen(MAX_PERFORMANCE_STR)))
+		policy = POLICY_MAX_PERFORMANCE;
+	else if (!strncmp(MEDIUM_POWER_STR, buf,
+			  strlen(MEDIUM_POWER_STR)))
+		policy = POLICY_MEDIUM_POWER;
+	else if (!strncmp(MIN_POWER_STR, buf,
+			  strlen(MIN_POWER_STR)))
+		policy = POLICY_MIN_POWER;
+	else
+		policy = POLICY_UNKNOWN;
+
+	return policy;
+}
+
+static void __igt_pm_enable_sata_link_power_management(void)
+{

lib/igt_pm.c +5 -17

-		int8_t policy;
-
 		snprintf(file_name, PATH_MAX,
 			 "/sys/class/scsi_host/host%d/link_power_management_policy",
 			 i);
 
+		/*
+		 * We don't have any assertions on open since the system
+		 * might not have a SATA host.
+		 */
 		fd = open(file_name, O_RDWR);
 		if (fd < 0)
 			break;
 
-		len = read(fd, buf, MAX_POLICY_STRLEN);
-		buf[len] = '\0';
-
-		if (!strncmp(MAX_PERFORMANCE_STR, buf,
-			     strlen(MAX_PERFORMANCE_STR)))
-			policy = POLICY_MAX_PERFORMANCE;
-		else if (!strncmp(MEDIUM_POWER_STR, buf,
-				  strlen(MEDIUM_POWER_STR)))
-			policy = POLICY_MEDIUM_POWER;
-		else if (!strncmp(MIN_POWER_STR, buf,
-				  strlen(MIN_POWER_STR)))
-			policy = POLICY_MIN_POWER;
-		else
-			policy = POLICY_UNKNOWN;
+		policy = get_sata_policy_for_host(fd, buf, i);
 
 		if (!(i % 256))
-			link_pm_policies = realloc(link_pm_policies,

lib/igt_pm.c +1 -16

 		/* If the policy is something we don't know about,
 		 * don't touch it, since we might potentially break things.

@@ -355,39 +368,25 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
 	}
 	free(buf);
 	free(file_name);
-
-	return link_pm_policies;
 }
 
-/**
- * igt_pm_restore_sata_link_power_management:
- * @pm_data: An opaque pointer with saved link PM policies;
- *           If NULL is passed we force enable the "max_performance" policy.
- *
- * Restore the link power management policies to the values
- * prior to enabling min_power.
- *
- * Caveat: If the system supports hotplugging and hotplugging takes
- *         place during our testing so that the hosts change numbers
- *         we might restore the settings to the wrong hosts.
- */
-void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
-
+static void __igt_pm_restore_sata_link_power_management(void)

lib/igt_pm.c +125 -4

 		int8_t policy;
 
-		if (!pm_data)
-			policy = POLICY_MAX_PERFORMANCE;
-		else if (pm_data[i] == POLICY_UNKNOWN)
+		if (__sata_pm_policies[i] == POLICY_UNKNOWN)
 			continue;
 		else
-			policy = pm_data[i];
+			policy = __sata_pm_policies[i];
 
 		snprintf(file_name, PATH_MAX,
 			 "/sys/class/scsi_host/host%d/link_power_management_policy",

@@ -421,7 +420,49 @@ void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
 		close(fd);
 	}
 	free(file_name);
+	free(__sata_pm_policies);
+	__sata_pm_policies = NULL;
+}
+
+/**
+ * igt_pm_enable_sata_link_power_management:
+ *
+ * Enable the min_power policy for SATA link power management.
+ * Without this we cannot reach deep runtime power states.
+ * Returns: Void
+ */
+void igt_pm_enable_sata_link_power_management(void)
+{
+	/* Check if has been already saved. */
+	if (__sata_pm_policies)
+		return;
+
+	 __igt_pm_enable_sata_link_power_management();
+}
+
+/**
+ * igt_pm_restore_sata_link_power_management:
+ *
+ * Restore the link power management policies to the values
+ * prior to enabling min_power.
+ *
+ * Caveat: If the system supports hotplugging and hotplugging takes
+ *         place during our testing so that the hosts change numbers
+ *         we might restore the settings to the wrong hosts.
+ */
+void igt_pm_restore_sata_link_power_management(void)
+{
+	if (!__sata_pm_policies)
+		return;
+
+	 __igt_pm_restore_sata_link_power_management();
 }
+
+static void __igt_pm_sata_link_pm_exit_handler(int sig)
+{
+	__igt_pm_restore_sata_link_power_management();
+}
+
 #define POWER_DIR "/sys/devices/pci0000:00/0000:00:02.0/power"
 /* We just leak this on exit ... */
 int pm_status_fd = -1;

@@ -585,6 +626,61 @@ bool igt_setup_runtime_pm(void)
 	return true;
 }
 
+bool igt_disable_runtime_pm(void)
+{
+	int fd;
+	ssize_t size;
+	char buf[6];
+
+	if (pm_status_fd < 0) {
+		fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
+		if (fd < 0)
+			return false;
+
+		size = read(fd, __igt_pm_runtime_autosuspend,
+			    sizeof(__igt_pm_runtime_autosuspend));
+
+		if (size <= 0) {
+			close(fd);
+			return false;
+		}
+
+		strchomp(__igt_pm_runtime_autosuspend);
+		igt_install_exit_handler(__igt_pm_runtime_exit_handler);
+		close(fd);
+	}
+
+	/* We know we support runtime PM, let's try to disable it now. */
+	fd = open(POWER_DIR "/control", O_RDWR);
+	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
+
+	if (pm_status_fd < 0) {
+		igt_assert(read(fd, __igt_pm_runtime_control,
+				sizeof(__igt_pm_runtime_control)) > 0);
+		strchomp(__igt_pm_runtime_control);
+
+		igt_debug("Saved runtime power management as '%s' and '%s'\n",
+			  __igt_pm_runtime_autosuspend,
+			  __igt_pm_runtime_control);
+	}
+
+	size = write(fd, "on\n", 3);
+	igt_assert(size == 3);
+	lseek(fd, 0, SEEK_SET);
+	size = read(fd, buf, ARRAY_SIZE(buf));
+	igt_assert(size == 3);
+	igt_assert(strncmp(buf, "on\n", 3) == 0);
+	close(fd);
+
+	if (pm_status_fd < 0) {
+		pm_status_fd = open(POWER_DIR "/runtime_status", O_RDONLY);
+		igt_assert_f(pm_status_fd >= 0,
+			     "Can't open " POWER_DIR "/runtime_status\n");
+	}
+
+	return true;
+}
+
 /**
  * igt_get_runtime_pm_status:
  *

@@ -628,3 +724,29 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
 {
 	return igt_wait(igt_get_runtime_pm_status() == status, 10000, 100);
 }
+
+/**
+ * dmc_loaded:
+ * @debugfs: fd to the debugfs dir.
+
+ * Check whether DMC FW is loaded or not. DMC FW is require for few Display C
+ * states like DC5 and DC6. FW does the Context Save and Restore during Display
+ * C States entry and exit.
+ *
+ * Returns:
+ * True if DMC FW is loaded otherwise false.
+ */
+bool igt_pm_dmc_loaded(int debugfs)
+{
+	char buf[15];
+	int len;
+
+	len = igt_sysfs_read(debugfs, "i915_dmc_info", buf, sizeof(buf) - 1);
+	if (len < 0)
+		return true; /* no CSR support, no DMC requirement */
+
+	buf[len] = '\0';
+
+	igt_info("DMC: %s\n", buf);
+	return strstr(buf, "fw loaded: yes");
+}

lib/igt_pm.h +4 -2

index 10cc6794..979b3463 100644
@@ -25,8 +25,8 @@
 #define IGT_PM_H
 
 void igt_pm_enable_audio_runtime_pm(void);
-int8_t *igt_pm_enable_sata_link_power_management(void);
-void igt_pm_restore_sata_link_power_management(int8_t *pm_data);
+void igt_pm_enable_sata_link_power_management(void);
+void igt_pm_restore_sata_link_power_management(void);
 
 /**
  * igt_runtime_pm_status:

@@ -47,8 +47,10 @@ enum igt_runtime_pm_status {
 };
 
 bool igt_setup_runtime_pm(void);
+bool igt_disable_runtime_pm(void);
 void igt_restore_runtime_pm(void);
 enum igt_runtime_pm_status igt_get_runtime_pm_status(void);
 bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
+bool igt_pm_dmc_loaded(int debugfs);
 
 #endif /* IGT_PM_H */

tests/i915/i915_pm_backlight.c +2 -4

index 4c1bff5b..9a5f4c37 100644
@@ -47,7 +47,6 @@ struct context {
 #define FADESPEED 100 /* milliseconds between steps */
 
 IGT_TEST_DESCRIPTION("Basic backlight sysfs test");
-static int8_t *pm_data = NULL;
 
 static int backlight_read(int *result, const char *fname)
 {

@@ -235,7 +234,7 @@ igt_main
 		igt_plane_set_fb(primary, &fb);
 
 		igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
-		pm_data = igt_pm_enable_sata_link_power_management();
+		igt_pm_enable_sata_link_power_management();
 	}
 
 	igt_subtest("basic-brightness")

@@ -255,8 +254,7 @@ igt_main
 
 		igt_display_fini(&display);
 		igt_remove_fb(display.drm_fd, &fb);
-		igt_pm_restore_sata_link_power_management(pm_data);
-		free(pm_data);
+		igt_pm_restore_sata_link_power_management();
 		close(display.drm_fd);
 	}
 }

tests/i915/i915_pm_rpm.c +3 -21

index a2c9d0ed..2d2f77be 100644
@@ -122,8 +122,6 @@ struct modeset_params lpsp_mode_params;
 struct modeset_params non_lpsp_mode_params;
 struct modeset_params *default_mode_params;
 
-static int8_t *pm_data = NULL;
-
 static int modprobe(const char *driver)
 {
 	return igt_kmod_load(driver, NULL);

@@ -710,21 +708,6 @@ static void setup_pc8(void)
 	has_pc8 = true;
 }
 
-static bool dmc_loaded(void)
-{
-	char buf[15];
-	int len;
-
-	len = igt_sysfs_read(debugfs, "i915_dmc_info", buf, sizeof(buf) - 1);
-	if (len < 0)
-	    return true; /* no CSR support, no DMC requirement */
-
-	buf[len] = '\0';
-
-	igt_info("DMC: %s\n", buf);
-	return strstr(buf, "fw loaded: yes");
-}
-
 static void dump_file(int dir, const char *filename)
 {
 	char *contents;

@@ -751,7 +734,7 @@ static bool setup_environment(void)
 
 	init_mode_set_data(&ms_data);
 
-	pm_data = igt_pm_enable_sata_link_power_management();
+	igt_pm_enable_sata_link_power_management();
 
 	has_runtime_pm = igt_setup_runtime_pm();
 	setup_pc8();

@@ -759,7 +742,7 @@ static bool setup_environment(void)
 	igt_info("Runtime PM support: %d\n", has_runtime_pm);
 	igt_info("PC8 residency support: %d\n", has_pc8);
 	igt_require(has_runtime_pm);
-	igt_require(dmc_loaded());
+	igt_require(igt_pm_dmc_loaded(debugfs));
 
 out:
 	disable_all_screens(&ms_data);

@@ -776,8 +759,7 @@ static void teardown_environment(void)
 
 	igt_restore_runtime_pm();
 
-	igt_pm_restore_sata_link_power_management(pm_data);
-	free(pm_data);
+	igt_pm_restore_sata_link_power_management();
 
 	fini_mode_set_data(&ms_data);
 

Signature · 1 line

2.21.0

From: Jyoti Yadav <[email protected]>

This patch does the following chnegs to lib/igt_pm.c

(quoted diff) +0 -3

-dmc_loaded() will be used by new test i915_pm_dc.c which will validate
 Display C States. So moving the same to igt_pm library.
 
-Introduced igt_disable_runtime_pm() in order to disable runtime suspend
 for the function which support dc9.
 
-Changed the igt_pm_enable_sata_link_power_management() and
 igt_pm_restore_sata_link_power_management() in order to save
 and restore the sata link power policy by an exit handler.

v2: Simplify the comment section.
v3: Remove . from the subject line.
v4: Rebased, resolve conflicts in pm_rpm.c
Included patch set version change log.
v5: Listing actual change in patch set changelog to make review easier.
v6: igt's lib added support for disabling runtime suspend,
change in commit log. rebased due to test name pm_rpm changed to i915_pm_rpm.
v7: Addressed review comment by saving POWER_DIR values in
igt_disable_runtime_pm(). [Imre]
v8: Addressed the review comment, igt_pm_enable_sata_link_power_management
function to restore the original SATA link power policy if things fail by using an exit handler. [Imre]
v9: IGT failure fixture in i915_pm_backlight and i915_pm_rpm. v10:Review comment fixup in sata_link_power_management
lib functions. [Imre]

Signed-off-by Jyoti Yadav <[email protected]>

Signed-off-by Anshuman Gupta <[email protected]>

---
 lib/igt_pm.c                   | 210 +++++++++++++++++++++++++--------
 lib/igt_pm.h                   |   6 +-
 tests/i915/i915_pm_backlight.c |   6 +-
 tests/i915/i915_pm_rpm.c       |  24 +---
 4 files changed, 172 insertions(+), 74 deletions(-)

lib/igt_pm.c +40 -47

index fd22273a..b91a0f96 100644
@@ -38,6 +38,7 @@
 #include "drmtest.h"
 #include "igt_pm.h"
 #include "igt_aux.h"
+#include "igt_sysfs.h"
 
 /**
  * SECTION:igt_pm

@@ -63,11 +64,16 @@ enum {
 #define MIN_POWER_STR		"min_power\n"
 /* Remember to fix this if adding longer strings */
 #define MAX_POLICY_STRLEN	strlen(MAX_PERFORMANCE_STR)
+int8_t *__sata_pm_policies;
+int __scsi_host_cnt;
 
 static char __igt_pm_audio_runtime_power_save[64];
 static char * __igt_pm_audio_runtime_control_path;
 static char __igt_pm_audio_runtime_control[64];
 
+static void __igt_pm_sata_link_pm_exit_handler(int sig);
+static void __igt_pm_restore_sata_link_power_management(void);
+
 static int __igt_pm_audio_restore_runtime_pm(void)
 {
 	int fd;

@@ -280,39 +286,26 @@ void igt_pm_enable_audio_runtime_pm(void)
 		igt_debug("Failed to enable audio runtime PM! (%d)\n", -err);
 }
 
-/**
- * igt_pm_enable_sata_link_power_management:
- *
- * Enable the min_power policy for SATA link power management.
- * Without this we cannot reach deep runtime power states.
- *
- * We don't have any assertions on open since the system might not have
- * a SATA host.
- *
- * Returns:
- * An opaque pointer to the data needed to restore the default values
- * after the test has terminated, or NULL if SATA link power management
- * is not supported. This pointer should be freed when no longer used
- * (typically after having called restore_sata_link_power_management()).
- */
-int8_t *igt_pm_enable_sata_link_power_management(void)
+static void __igt_pm_enable_sata_link_power_management(void)
 {
 	int fd, i;
 	ssize_t len;
 	char *buf;
 	char *file_name;
-	int8_t *link_pm_policies = NULL;
+	int8_t policy;
 
 	file_name = malloc(PATH_MAX);
 	buf = malloc(MAX_POLICY_STRLEN + 1);
 
-	for (i = 0; ; i++) {
-		int8_t policy;
-
+	for (__scsi_host_cnt = 0; ; __scsi_host_cnt++) {
 		snprintf(file_name, PATH_MAX,
 			 "/sys/class/scsi_host/host%d/link_power_management_policy",
-			 i);
+			 __scsi_host_cnt);
 
+		/*
+		 * We don't have any assertions on open since the system
+		 * might not have a SATA host.
+		 */
 		fd = open(file_name, O_RDWR);
 		if (fd < 0)
 			break;

@@ -332,12 +325,26 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
 		else
 			policy = POLICY_UNKNOWN;
 
-		if (!(i % 256))
-			link_pm_policies = realloc(link_pm_policies,
-						   (i / 256 + 1) * 256 + 1);
+		if (!(__scsi_host_cnt % 256))
+			__sata_pm_policies = realloc(__sata_pm_policies,
+						     (__scsi_host_cnt / 256 + 1)
+						     * 256 + 1);
 
-		link_pm_policies[i] = policy;
-		link_pm_policies[i + 1] = 0;
+		__sata_pm_policies[__scsi_host_cnt] = policy;
+		close(fd);
+	}
+
+	igt_install_exit_handler(__igt_pm_sata_link_pm_exit_handler);
+
+	for (i = 0; i < __scsi_host_cnt; i++) {
+		snprintf(file_name, PATH_MAX,
+			 "/sys/class/scsi_host/host%d/link_power_management_policy",
+			 i);
+		fd = open(file_name, O_RDWR);
+		if (fd < 0)
+			break;
+
+		policy = __sata_pm_policies[i];
 
 		/* If the policy is something we don't know about,
 		 * don't touch it, since we might potentially break things.

@@ -355,39 +362,25 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
 	}
 	free(buf);
 	free(file_name);
-
-	return link_pm_policies;
 }
 
-/**
- * igt_pm_restore_sata_link_power_management:
- * @pm_data: An opaque pointer with saved link PM policies;
- *           If NULL is passed we force enable the "max_performance" policy.
- *
- * Restore the link power management policies to the values
- * prior to enabling min_power.
- *
- * Caveat: If the system supports hotplugging and hotplugging takes
- *         place during our testing so that the hosts change numbers
- *         we might restore the settings to the wrong hosts.
- */
-void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
-
+static void __igt_pm_restore_sata_link_power_management(void)
 {
 	int fd, i;
 	char *file_name;
 
+	if (!__sata_pm_policies)
+		return;
+
 	/* Disk runtime PM policies. */
 	file_name = malloc(PATH_MAX);
-	for (i = 0; ; i++) {
+	for (i = 0; i < __scsi_host_cnt; i++) {
 		int8_t policy;
 
-		if (!pm_data)
-			policy = POLICY_MAX_PERFORMANCE;
-		else if (pm_data[i] == POLICY_UNKNOWN)
+		if (__sata_pm_policies[i] == POLICY_UNKNOWN)
 			continue;
 		else
-			policy = pm_data[i];
+			policy = __sata_pm_policies[i];
 
 		snprintf(file_name, PATH_MAX,
 			 "/sys/class/scsi_host/host%d/link_power_management_policy",

@@ -421,7 +414,49 @@ void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
 		close(fd);
 	}
 	free(file_name);

lib/igt_pm.c +60 -0

+ */
+void igt_pm_enable_sata_link_power_management(void)
+{
+	/* Check if has been already saved. */
+	if (__sata_pm_policies)
+		return;
+
+	 __igt_pm_enable_sata_link_power_management();
+}
+
+/**
+ * igt_pm_restore_sata_link_power_management:
+ *
+ * Restore the link power management policies to the values
+ * prior to enabling min_power.
+ *
+ * Caveat: If the system supports hotplugging and hotplugging takes
+ *         place during our testing so that the hosts change numbers
+ *         we might restore the settings to the wrong hosts.
+ */
+void igt_pm_restore_sata_link_power_management(void)
+{
+	if (!__sata_pm_policies)
+		return;
+
+	 __igt_pm_restore_sata_link_power_management();
+}
+
+static void __igt_pm_sata_link_pm_exit_handler(int sig)
+{
+	__igt_pm_restore_sata_link_power_management();
+}
+
 #define POWER_DIR "/sys/devices/pci0000:00/0000:00:02.0/power"
 /* We just leak this on exit ... */
 int pm_status_fd = -1;

@@ -585,6 +620,61 @@ bool igt_setup_runtime_pm(void)
 	return true;
 }
 
+bool igt_disable_runtime_pm(void)
+{
+	int fd;
+	ssize_t size;
+	char buf[6];
+
+	if (pm_status_fd < 0) {
+		fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
+		if (fd < 0)
+			return false;
+
+		size = read(fd, __igt_pm_runtime_autosuspend,
+			    sizeof(__igt_pm_runtime_autosuspend));
+
+		if (size <= 0) {
+			close(fd);
+			return false;
+		}
+
+		strchomp(__igt_pm_runtime_autosuspend);
+		igt_install_exit_handler(__igt_pm_runtime_exit_handler);
+		close(fd);
+	}
+
+	/* We know we support runtime PM, let's try to disable it now. */
+	fd = open(POWER_DIR "/control", O_RDWR);
+	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");

lib/igt_pm.c +44 -0

+
+	size = write(fd, "on\n", 3);
+	igt_assert(size == 3);
+	lseek(fd, 0, SEEK_SET);
+	size = read(fd, buf, ARRAY_SIZE(buf));
+	igt_assert(size == 3);
+	igt_assert(strncmp(buf, "on\n", 3) == 0);
+	close(fd);
+
+	if (pm_status_fd < 0) {
+		pm_status_fd = open(POWER_DIR "/runtime_status", O_RDONLY);
+		igt_assert_f(pm_status_fd >= 0,
+			     "Can't open " POWER_DIR "/runtime_status\n");
+	}
+
+	return true;
+}
+
 /**
  * igt_get_runtime_pm_status:
  *

@@ -628,3 +718,29 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
 {
 	return igt_wait(igt_get_runtime_pm_status() == status, 10000, 100);
 }
+
+/**
+ * dmc_loaded:
+ * @debugfs: fd to the debugfs dir.
+
+ * Check whether DMC FW is loaded or not. DMC FW is require for few Display C
+ * states like DC5 and DC6. FW does the Context Save and Restore during Display
+ * C States entry and exit.
+ *
+ * Returns:
+ * True if DMC FW is loaded otherwise false.
+ */
+bool igt_pm_dmc_loaded(int debugfs)
+{
+	char buf[15];
+	int len;
+
+	len = igt_sysfs_read(debugfs, "i915_dmc_info", buf, sizeof(buf) - 1);
+	if (len < 0)
+		return true; /* no CSR support, no DMC requirement */
+
+	buf[len] = '\0';
+
+	igt_info("DMC: %s\n", buf);
+	return strstr(buf, "fw loaded: yes");
+}

lib/igt_pm.h +4 -2

index 10cc6794..979b3463 100644
@@ -25,8 +25,8 @@
 #define IGT_PM_H
 
 void igt_pm_enable_audio_runtime_pm(void);
-int8_t *igt_pm_enable_sata_link_power_management(void);
-void igt_pm_restore_sata_link_power_management(int8_t *pm_data);
+void igt_pm_enable_sata_link_power_management(void);
+void igt_pm_restore_sata_link_power_management(void);
 
 /**
  * igt_runtime_pm_status:

@@ -47,8 +47,10 @@ enum igt_runtime_pm_status {
 };
 
 bool igt_setup_runtime_pm(void);
+bool igt_disable_runtime_pm(void);
 void igt_restore_runtime_pm(void);
 enum igt_runtime_pm_status igt_get_runtime_pm_status(void);
 bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
+bool igt_pm_dmc_loaded(int debugfs);
 
 #endif /* IGT_PM_H */

tests/i915/i915_pm_backlight.c +2 -4

index 4c1bff5b..9a5f4c37 100644
@@ -47,7 +47,6 @@ struct context {
 #define FADESPEED 100 /* milliseconds between steps */
 
 IGT_TEST_DESCRIPTION("Basic backlight sysfs test");
-static int8_t *pm_data = NULL;
 
 static int backlight_read(int *result, const char *fname)
 {

@@ -235,7 +234,7 @@ igt_main
 		igt_plane_set_fb(primary, &fb);
 
 		igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
-		pm_data = igt_pm_enable_sata_link_power_management();
+		igt_pm_enable_sata_link_power_management();
 	}
 
 	igt_subtest("basic-brightness")

@@ -255,8 +254,7 @@ igt_main
 
 		igt_display_fini(&display);
 		igt_remove_fb(display.drm_fd, &fb);
-		igt_pm_restore_sata_link_power_management(pm_data);
-		free(pm_data);
+		igt_pm_restore_sata_link_power_management();
 		close(display.drm_fd);
 	}
 }

tests/i915/i915_pm_rpm.c +3 -21

index 2168ff72..6879c513 100644
@@ -123,8 +123,6 @@ struct modeset_params lpsp_mode_params;
 struct modeset_params non_lpsp_mode_params;
 struct modeset_params *default_mode_params;
 
-static int8_t *pm_data = NULL;
-
 static int modprobe(const char *driver)
 {
 	return igt_kmod_load(driver, NULL);

@@ -755,21 +753,6 @@ static void setup_pc8(void)
 	has_pc8 = true;
 }
 
-static bool dmc_loaded(void)
-{
-	char buf[15];
-	int len;
-
-	len = igt_sysfs_read(debugfs, "i915_dmc_info", buf, sizeof(buf) - 1);
-	if (len < 0)
-	    return true; /* no CSR support, no DMC requirement */
-
-	buf[len] = '\0';
-
-	igt_info("DMC: %s\n", buf);
-	return strstr(buf, "fw loaded: yes");
-}
-
 static void dump_file(int dir, const char *filename)
 {
 	char *contents;

@@ -796,7 +779,7 @@ static bool setup_environment(void)
 
 	init_mode_set_data(&ms_data);
 
-	pm_data = igt_pm_enable_sata_link_power_management();
+	igt_pm_enable_sata_link_power_management();
 
 	has_runtime_pm = igt_setup_runtime_pm();
 	setup_pc8();

@@ -804,7 +787,7 @@ static bool setup_environment(void)
 	igt_info("Runtime PM support: %d\n", has_runtime_pm);
 	igt_info("PC8 residency support: %d\n", has_pc8);
 	igt_require(has_runtime_pm);
-	igt_require(dmc_loaded());
+	igt_require(igt_pm_dmc_loaded(debugfs));
 
 out:
 	disable_all_screens(&ms_data);

@@ -821,8 +804,7 @@ static void teardown_environment(void)
 
 	igt_restore_runtime_pm();
 
-	igt_pm_restore_sata_link_power_management(pm_data);
-	free(pm_data);
+	igt_pm_restore_sata_link_power_management();
 
 	fini_mode_set_data(&ms_data);
 

Signature · 1 line

2.21.0

Quoted 200 lines · Anshuman Gupta

From: Jyoti Yadav <[email protected]>

This patch does the following chnegs to lib/igt_pm.c

(quoted diff) +0 -3

-dmc_loaded() will be used by new test i915_pm_dc.c which will validate
 Display C States. So moving the same to igt_pm library.
 
-Introduced igt_disable_runtime_pm() in order to disable runtime suspend
 for the function which support dc9.
 
-Changed the igt_pm_enable_sata_link_power_management() and
 igt_pm_restore_sata_link_power_management() in order to save
 and restore the sata link power policy by an exit handler.

v2: Simplify the comment section.
v3: Remove . from the subject line.
v4: Rebased, resolve conflicts in pm_rpm.c
Included patch set version change log.
v5: Listing actual change in patch set changelog to make review easier.
v6: igt's lib added support for disabling runtime suspend,
change in commit log. rebased due to test name pm_rpm changed to i915_pm_rpm.
v7: Addressed review comment by saving POWER_DIR values in
igt_disable_runtime_pm(). [Imre]
v8: Addressed the review comment, igt_pm_enable_sata_link_power_management
function to restore the original SATA link power policy if things fail by using an exit handler. [Imre]
v9: IGT failure fixture in i915_pm_backlight and i915_pm_rpm. v10:Review comment fixup in sata_link_power_management
lib functions. [Imre]

Signed-off-by Jyoti Yadav <[email protected]>

Signed-off-by Anshuman Gupta <[email protected]>

---
 lib/igt_pm.c                   | 210 +++++++++++++++++++++++++--------
 lib/igt_pm.h                   |   6 +-
 tests/i915/i915_pm_backlight.c |   6 +-
 tests/i915/i915_pm_rpm.c       |  24 +---
 4 files changed, 172 insertions(+), 74 deletions(-)

lib/igt_pm.c +40 -47

index fd22273a..b91a0f96 100644
@@ -38,6 +38,7 @@
 #include "drmtest.h"
 #include "igt_pm.h"
 #include "igt_aux.h"
+#include "igt_sysfs.h"
 
 /**
  * SECTION:igt_pm

@@ -63,11 +64,16 @@ enum {
 #define MIN_POWER_STR		"min_power\n"
 /* Remember to fix this if adding longer strings */
 #define MAX_POLICY_STRLEN	strlen(MAX_PERFORMANCE_STR)
+int8_t *__sata_pm_policies;
+int __scsi_host_cnt;
 
 static char __igt_pm_audio_runtime_power_save[64];
 static char * __igt_pm_audio_runtime_control_path;
 static char __igt_pm_audio_runtime_control[64];
 
+static void __igt_pm_sata_link_pm_exit_handler(int sig);
+static void __igt_pm_restore_sata_link_power_management(void);
+
 static int __igt_pm_audio_restore_runtime_pm(void)
 {
 	int fd;

@@ -280,39 +286,26 @@ void igt_pm_enable_audio_runtime_pm(void)
 		igt_debug("Failed to enable audio runtime PM! (%d)\n", -err);
 }
 
-/**
- * igt_pm_enable_sata_link_power_management:
- *
- * Enable the min_power policy for SATA link power management.
- * Without this we cannot reach deep runtime power states.
- *
- * We don't have any assertions on open since the system might not have
- * a SATA host.
- *
- * Returns:
- * An opaque pointer to the data needed to restore the default values
- * after the test has terminated, or NULL if SATA link power management
- * is not supported. This pointer should be freed when no longer used
- * (typically after having called restore_sata_link_power_management()).
- */
-int8_t *igt_pm_enable_sata_link_power_management(void)
+static void __igt_pm_enable_sata_link_power_management(void)
 {
 	int fd, i;
 	ssize_t len;
 	char *buf;
 	char *file_name;
-	int8_t *link_pm_policies = NULL;
+	int8_t policy;
 
 	file_name = malloc(PATH_MAX);
 	buf = malloc(MAX_POLICY_STRLEN + 1);
 
-	for (i = 0; ; i++) {
-		int8_t policy;
-
+	for (__scsi_host_cnt = 0; ; __scsi_host_cnt++) {
 		snprintf(file_name, PATH_MAX,
 			 "/sys/class/scsi_host/host%d/link_power_management_policy",
-			 i);
+			 __scsi_host_cnt);
 
+		/*
+		 * We don't have any assertions on open since the system
+		 * might not have a SATA host.
+		 */
 		fd = open(file_name, O_RDWR);
 		if (fd < 0)
 			break;

@@ -332,12 +325,26 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
 		else
 			policy = POLICY_UNKNOWN;
 
-		if (!(i % 256))
-			link_pm_policies = realloc(link_pm_policies,
-						   (i / 256 + 1) * 256 + 1);
+		if (!(__scsi_host_cnt % 256))
+			__sata_pm_policies = realloc(__sata_pm_policies,
+						     (__scsi_host_cnt / 256 + 1)
+						     * 256 + 1);
 
-		link_pm_policies[i] = policy;
-		link_pm_policies[i + 1] = 0;
+		__sata_pm_policies[__scsi_host_cnt] = policy;
+		close(fd);
+	}
+
+	igt_install_exit_handler(__igt_pm_sata_link_pm_exit_handler);
+
+	for (i = 0; i < __scsi_host_cnt; i++) {
+		snprintf(file_name, PATH_MAX,
+			 "/sys/class/scsi_host/host%d/link_power_management_policy",
+			 i);
+		fd = open(file_name, O_RDWR);
+		if (fd < 0)
+			break;
+
+		policy = __sata_pm_policies[i];
 
 		/* If the policy is something we don't know about,
 		 * don't touch it, since we might potentially break things.

@@ -355,39 +362,25 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
 	}
 	free(buf);
 	free(file_name);
-
-	return link_pm_policies;
 }
 
-/**
- * igt_pm_restore_sata_link_power_management:
- * @pm_data: An opaque pointer with saved link PM policies;
- *           If NULL is passed we force enable the "max_performance" policy.
- *
- * Restore the link power management policies to the values
- * prior to enabling min_power.
- *
- * Caveat: If the system supports hotplugging and hotplugging takes
- *         place during our testing so that the hosts change numbers
- *         we might restore the settings to the wrong hosts.
- */
-void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
-
+static void __igt_pm_restore_sata_link_power_management(void)
 {
 	int fd, i;
 	char *file_name;
 
+	if (!__sata_pm_policies)
+		return;
+
 	/* Disk runtime PM policies. */
 	file_name = malloc(PATH_MAX);
-	for (i = 0; ; i++) {
+	for (i = 0; i < __scsi_host_cnt; i++) {
 		int8_t policy;
 
-		if (!pm_data)
-			policy = POLICY_MAX_PERFORMANCE;
-		else if (pm_data[i] == POLICY_UNKNOWN)
+		if (__sata_pm_policies[i] == POLICY_UNKNOWN)
 			continue;
 		else
-			policy = pm_data[i];
+			policy = __sata_pm_policies[i];
 
 		snprintf(file_name, PATH_MAX,
 			 "/sys/class/scsi_host/host%d/link_power_management_policy",

@@ -421,7 +414,49 @@ void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
 		close(fd);
 	}
 	free(file_name);

lib/igt_pm.c void igt_pm_restore_sata_link_power_management(int8_t *pm_data) · line 426

Quoted 10 lines

lib/igt_pm.c +9 -0

+	free(__sata_pm_policies);
+	__sata_pm_policies = NULL;
 }
+
+/**
+ * igt_pm_enable_sata_link_power_management:
+ *
+ * Enable the min_power policy for SATA link power management.
+ * Without this we cannot reach deep runtime power states.
+ * Returns: Void

No need for a comment on void returns.

Quoted 74 lines

lib/igt_pm.c +67 -0

+ */
+void igt_pm_enable_sata_link_power_management(void)
+{
+	/* Check if has been already saved. */
+	if (__sata_pm_policies)
+		return;
+
+	 __igt_pm_enable_sata_link_power_management();
+}
+
+/**
+ * igt_pm_restore_sata_link_power_management:
+ *
+ * Restore the link power management policies to the values
+ * prior to enabling min_power.
+ *
+ * Caveat: If the system supports hotplugging and hotplugging takes
+ *         place during our testing so that the hosts change numbers
+ *         we might restore the settings to the wrong hosts.
+ */
+void igt_pm_restore_sata_link_power_management(void)
+{
+	if (!__sata_pm_policies)
+		return;
+
+	 __igt_pm_restore_sata_link_power_management();
+}
+
+static void __igt_pm_sata_link_pm_exit_handler(int sig)
+{
+	__igt_pm_restore_sata_link_power_management();
+}
+
 #define POWER_DIR "/sys/devices/pci0000:00/0000:00:02.0/power"
 /* We just leak this on exit ... */
 int pm_status_fd = -1;

@@ -585,6 +620,61 @@ bool igt_setup_runtime_pm(void)
 	return true;
 }
 
+bool igt_disable_runtime_pm(void)
+{
+	int fd;
+	ssize_t size;
+	char buf[6];
+
+	if (pm_status_fd < 0) {
+		fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
+		if (fd < 0)
+			return false;
+
+		size = read(fd, __igt_pm_runtime_autosuspend,
+			    sizeof(__igt_pm_runtime_autosuspend));
+
+		if (size <= 0) {
+			close(fd);
+			return false;
+		}
+
+		strchomp(__igt_pm_runtime_autosuspend);
+		igt_install_exit_handler(__igt_pm_runtime_exit_handler);
+		close(fd);
+	}
+
+	/* We know we support runtime PM, let's try to disable it now. */
+	fd = open(POWER_DIR "/control", O_RDWR);
+	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
+
+	if (pm_status_fd < 0) {
+		igt_assert(read(fd, __igt_pm_runtime_control,
+				sizeof(__igt_pm_runtime_control)) > 0);
+		strchomp(__igt_pm_runtime_control);
+
+		igt_debug("Saved runtime power management as '%s' and '%s'\n",

lib/igt_pm.c bool igt_setup_runtime_pm(void) · line 659

Quoted 3 lines

lib/igt_pm.c +3 -0

+			  __igt_pm_runtime_autosuspend,
+			  __igt_pm_runtime_control);
+	}

We require to call igt_setup_runtime_pm() before igt_disable_runtime_pm(), so the original state will be saved away already, so instead of the above duplication of saving the state we should just igt_assert(pm_status_fd >= 0);

From: Jyoti Yadav <[email protected]>

Added new subtest for DC5 entry during DPMS on/off cycle. During DPMS on/off cycle DC5 counter is incremented.

v2: Rename the subtest with meaningful name.
v3: Rebased.
v4: Addressed review comments by removing leftover code

    cleanup().

v5: Addressed the review comment by removing redundant
read_dc_counter() suggested by Imre.
Listing actual change in patch set changelog to make review easier.
v6: Three way patch applied, no functional change.
v7: Disabling runtime suspend for the platform which support, DC9.
rebased due to test name pm_dc changed to i915_pm_dc, aligning to other PM tests.
v8: Introduced setup_dc_dpms() in order to disable runtime pm, restoring
POWER_DIR values to its original and enabling runtime pm for other followed sub-tests.
v9: Check DC5 counter value after DPMS off, broke the dpms_on_off
function to dpms_on and dpms_off. [Imre]
v10:Added AT_LEAST_Gen11 condition instead of IS_ICELAKE in order to
disable runtime suspend. [Imre]

Signed-off-by Jyoti Yadav <[email protected]>

Signed-off-by Anshuman Gupta <[email protected]>

---
 tests/i915/i915_pm_dc.c | 62 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

tests/i915/i915_pm_dc.c +45 -0

index f261ecbf..f03d30a8 100644
@@ -46,6 +46,7 @@ typedef struct {
 	enum psr_mode op_psr_mode;
 	drmModeModeInfo *mode;
 	igt_output_t *output;
+	bool runtime_suspend_disabled;
 } data_t;
 
 bool dc_state_wait_entry(int drm_fd, int dc_flag, int prev_dc_count);

@@ -173,6 +174,62 @@ static void test_dc_state_psr(data_t *data, int dc_flag)
 	cleanup(data);
 }
 
+static void setup_dc_dpms(data_t *data)
+{
+	if (IS_BROXTON(data->devid) || IS_GEMINILAKE(data->devid) ||
+	    AT_LEAST_GEN(data->devid, 11)) {
+		data->runtime_suspend_disabled = igt_disable_runtime_pm();
+		igt_require_f(data->runtime_suspend_disabled,
+			      "unable to disable runtime pm for i915\n");
+	} else {
+		data->runtime_suspend_disabled = false;
+	}
+}
+
+static void dpms_off(data_t *data)
+{
+	for (int i = 0; i < data->display.n_outputs; i++) {
+		kmstest_set_connector_dpms(data->drm_fd,
+					   data->display.outputs[i].config.connector,
+					   DRM_MODE_DPMS_OFF);
+	}
+
+	if (!data->runtime_suspend_disabled)
+		igt_assert(igt_wait_for_pm_status
+			   (IGT_RUNTIME_PM_STATUS_SUSPENDED));
+}
+
+static void dpms_on(data_t *data)
+{
+	for (int i = 0; i < data->display.n_outputs; i++) {
+		kmstest_set_connector_dpms(data->drm_fd,
+					   data->display.outputs[i].config.connector,
+					   DRM_MODE_DPMS_ON);
+	}
+
+	if (!data->runtime_suspend_disabled)
+		igt_assert(igt_wait_for_pm_status
+			   (IGT_RUNTIME_PM_STATUS_ACTIVE));
+}
+
+static void test_dc_state_dpms(data_t *data, int dc_flag)
+{
+	uint32_t dc_counter;
+
+	dc_counter = read_dc_counter(data->drm_fd, dc_flag);
+	dpms_off(data);

tests/i915/i915_pm_dc.c +3 -0

+		test_dc_state_dpms(&data, CHECK_DC5);
+	}
+
 	igt_fixture {
 		close(data.debugfs_fd);
 		display_fini(&data);

Signature · 1 line

2.21.0

Quoted 90 lines · Anshuman Gupta

From: Jyoti Yadav <[email protected]>

Added new subtest for DC5 entry during DPMS on/off cycle. During DPMS on/off cycle DC5 counter is incremented.

v2: Rename the subtest with meaningful name.
v3: Rebased.
v4: Addressed review comments by removing leftover code

     cleanup().

v5: Addressed the review comment by removing redundant
read_dc_counter() suggested by Imre.
Listing actual change in patch set changelog to make review easier.
v6: Three way patch applied, no functional change.
v7: Disabling runtime suspend for the platform which support, DC9.
rebased due to test name pm_dc changed to i915_pm_dc, aligning to other PM tests.
v8: Introduced setup_dc_dpms() in order to disable runtime pm, restoring
POWER_DIR values to its original and enabling runtime pm for other followed sub-tests.
v9: Check DC5 counter value after DPMS off, broke the dpms_on_off
function to dpms_on and dpms_off. [Imre]
v10:Added AT_LEAST_Gen11 condition instead of IS_ICELAKE in order to
disable runtime suspend. [Imre]

Signed-off-by Jyoti Yadav <[email protected]>

Signed-off-by Anshuman Gupta <[email protected]>

---
  tests/i915/i915_pm_dc.c | 62 +++++++++++++++++++++++++++++++++++++++++
  1 file changed, 62 insertions(+)

tests/i915/i915_pm_dc.c +45 -0

index f261ecbf..f03d30a8 100644
@@ -46,6 +46,7 @@ typedef struct {
  	enum psr_mode op_psr_mode;
  	drmModeModeInfo *mode;
  	igt_output_t *output;
+	bool runtime_suspend_disabled;
  } data_t;
 
  bool dc_state_wait_entry(int drm_fd, int dc_flag, int prev_dc_count);

@@ -173,6 +174,62 @@ static void test_dc_state_psr(data_t *data, int dc_flag)
  	cleanup(data);
  }
 
+static void setup_dc_dpms(data_t *data)
+{
+	if (IS_BROXTON(data->devid) || IS_GEMINILAKE(data->devid) ||
+	    AT_LEAST_GEN(data->devid, 11)) {
+		data->runtime_suspend_disabled = igt_disable_runtime_pm();
+		igt_require_f(data->runtime_suspend_disabled,
+			      "unable to disable runtime pm for i915\n");
+	} else {
+		data->runtime_suspend_disabled = false;
+	}
+}
+
+static void dpms_off(data_t *data)
+{
+	for (int i = 0; i < data->display.n_outputs; i++) {
+		kmstest_set_connector_dpms(data->drm_fd,
+					   data->display.outputs[i].config.connector,
+					   DRM_MODE_DPMS_OFF);
+	}
+
+	if (!data->runtime_suspend_disabled)
+		igt_assert(igt_wait_for_pm_status
+			   (IGT_RUNTIME_PM_STATUS_SUSPENDED));
+}
+
+static void dpms_on(data_t *data)
+{
+	for (int i = 0; i < data->display.n_outputs; i++) {
+		kmstest_set_connector_dpms(data->drm_fd,
+					   data->display.outputs[i].config.connector,
+					   DRM_MODE_DPMS_ON);
+	}
+
+	if (!data->runtime_suspend_disabled)
+		igt_assert(igt_wait_for_pm_status
+			   (IGT_RUNTIME_PM_STATUS_ACTIVE));
+}
+
+static void test_dc_state_dpms(data_t *data, int dc_flag)
+{
+	uint32_t dc_counter;
+
+	dc_counter = read_dc_counter(data->drm_fd, dc_flag);
+	dpms_off(data);

tests/i915/i915_pm_dc.c static void test_dc_state_psr(data_t *data, int dc_flag) · line 230

Quoted 10 lines

tests/i915/i915_pm_dc.c +10 -0

+	check_dc_counter(data->drm_fd, dc_flag, dc_counter);
+	dpms_on(data);
+
+	/* if runtime PM is disabled for i915 restore it,
+	 * so any other sub-test can use runtime-PM.
+	 */
+	if (data->runtime_suspend_disabled) {
+		igt_restore_runtime_pm();
+		igt_setup_runtime_pm();
+	}

The above restores what setup_dc_dpms() did so could you move it a cleanup_dc_dpms() function for clarity?

Quoted 9 lines

tests/i915/i915_pm_dc.c +2 -0

+}
+
  int main(int argc, char *argv[])
  {
  	bool has_runtime_pm;

@@ -210,6 +267,11 @@ int main(int argc, char *argv[])
  		test_dc_state_psr(&data, CHECK_DC6);
  	}
 

tests/i915/i915_pm_dc.c int main(int argc, char *argv[]) · line 271

Quoted 2 lines

tests/i915/i915_pm_dc.c +2 -0

+	igt_subtest("dc5-dpms") {
+		setup_dc_dpms(&data);

Could you move the above call to test_dc_state_dpms() and check for

CHECK_DC5 withing setup_dc_dpms()?

I couldn't spot any other issues so with these changes on the patchset:

Reviewed-by Imre Deak <[email protected]>

Could you please resend the whole patchset?

Quoted 9 lines

tests/i915/i915_pm_dc.c +3 -0

+		test_dc_state_dpms(&data, CHECK_DC5);
+	}
+
  	igt_fixture {
  		close(data.debugfs_fd);
  		display_fini(&data);

Signature · 1 line

2.21.0

Quoted 103 lines · Imre Deak

Quoted 88 lines · Anshuman Gupta

From: Jyoti Yadav <[email protected]>

Added new subtest for DC5 entry during DPMS on/off cycle. During DPMS on/off cycle DC5 counter is incremented.

v2: Rename the subtest with meaningful name.
v3: Rebased.
v4: Addressed review comments by removing leftover code

     cleanup().

v5: Addressed the review comment by removing redundant
read_dc_counter() suggested by Imre.
Listing actual change in patch set changelog to make review easier.
v6: Three way patch applied, no functional change.
v7: Disabling runtime suspend for the platform which support, DC9.
rebased due to test name pm_dc changed to i915_pm_dc, aligning to other PM tests.
v8: Introduced setup_dc_dpms() in order to disable runtime pm, restoring
POWER_DIR values to its original and enabling runtime pm for other followed sub-tests.
v9: Check DC5 counter value after DPMS off, broke the dpms_on_off
function to dpms_on and dpms_off. [Imre]
v10:Added AT_LEAST_Gen11 condition instead of IS_ICELAKE in order to
disable runtime suspend. [Imre]

Signed-off-by Jyoti Yadav <[email protected]>

Signed-off-by Anshuman Gupta <[email protected]>

---
  tests/i915/i915_pm_dc.c | 62 +++++++++++++++++++++++++++++++++++++++++
  1 file changed, 62 insertions(+)

tests/i915/i915_pm_dc.c +45 -0

index f261ecbf..f03d30a8 100644
@@ -46,6 +46,7 @@ typedef struct {
  	enum psr_mode op_psr_mode;
  	drmModeModeInfo *mode;
  	igt_output_t *output;
+	bool runtime_suspend_disabled;
  } data_t;
  bool dc_state_wait_entry(int drm_fd, int dc_flag, int prev_dc_count);

@@ -173,6 +174,62 @@ static void test_dc_state_psr(data_t *data, int dc_flag)
  	cleanup(data);
  }
+static void setup_dc_dpms(data_t *data)
+{
+	if (IS_BROXTON(data->devid) || IS_GEMINILAKE(data->devid) ||
+	    AT_LEAST_GEN(data->devid, 11)) {
+		data->runtime_suspend_disabled = igt_disable_runtime_pm();
+		igt_require_f(data->runtime_suspend_disabled,
+			      "unable to disable runtime pm for i915\n");
+	} else {
+		data->runtime_suspend_disabled = false;
+	}
+}
+
+static void dpms_off(data_t *data)
+{
+	for (int i = 0; i < data->display.n_outputs; i++) {
+		kmstest_set_connector_dpms(data->drm_fd,
+					   data->display.outputs[i].config.connector,
+					   DRM_MODE_DPMS_OFF);
+	}
+
+	if (!data->runtime_suspend_disabled)
+		igt_assert(igt_wait_for_pm_status
+			   (IGT_RUNTIME_PM_STATUS_SUSPENDED));
+}
+
+static void dpms_on(data_t *data)
+{
+	for (int i = 0; i < data->display.n_outputs; i++) {
+		kmstest_set_connector_dpms(data->drm_fd,
+					   data->display.outputs[i].config.connector,
+					   DRM_MODE_DPMS_ON);
+	}
+
+	if (!data->runtime_suspend_disabled)
+		igt_assert(igt_wait_for_pm_status
+			   (IGT_RUNTIME_PM_STATUS_ACTIVE));
+}
+
+static void test_dc_state_dpms(data_t *data, int dc_flag)
+{
+	uint32_t dc_counter;
+
+	dc_counter = read_dc_counter(data->drm_fd, dc_flag);
+	dpms_off(data);

tests/i915/i915_pm_dc.c static void test_dc_state_psr(data_t *data, int dc_flag) · line 229

Quoted 10 lines

tests/i915/i915_pm_dc.c +10 -0

+	check_dc_counter(data->drm_fd, dc_flag, dc_counter);
+	dpms_on(data);
+
+	/* if runtime PM is disabled for i915 restore it,
+	 * so any other sub-test can use runtime-PM.
+	 */
+	if (data->runtime_suspend_disabled) {
+		igt_restore_runtime_pm();
+		igt_setup_runtime_pm();
+	}

The above restores what setup_dc_dpms() did so could you move it a cleanup_dc_dpms() function for clarity?

Quoted 12 lines

tests/i915/i915_pm_dc.c int main(int argc, char *argv[]) · line 270

Quoted 10 lines

tests/i915/i915_pm_dc.c +4 -0

+}
+
  int main(int argc, char *argv[])
  {
  	bool has_runtime_pm;

@@ -210,6 +267,11 @@ int main(int argc, char *argv[])
  		test_dc_state_psr(&data, CHECK_DC6);
  	}
+	igt_subtest("dc5-dpms") {
+		setup_dc_dpms(&data);

Could you move the above call to test_dc_state_dpms() and check for

I will do this and cleanup_dc_dpms() changes.

Quoted 1 line

CHECK_DC5 withing setup_dc_dpms()?

but i did not understand why do we need to check for CHECK_DC5 (dc flag) in setup_dc_dpms(), it is agnostic to DC5 and DC6. dc flag will be require in read_dc_counter() and check_dc_counter() function.
May be it will be clear when i will send entire patch set.

Quoted 6 lines

I couldn't spot any other issues so with these changes on the patchset:

Reviewed-by Imre Deak <[email protected]>

Could you please resend the whole patchset?

Quoted 9 lines

Quoted 9 lines

tests/i915/i915_pm_dc.c +3 -0

+		test_dc_state_dpms(&data, CHECK_DC5);
+	}
+
  	igt_fixture {
  		close(data.debugfs_fd);
  		display_fini(&data);

Signature · 1 line

2.21.0