[PATCH i-g-t] RFC: split PM workarounds into separate lib

[PATCH i-g-t] RFC: split PM workarounds into separate lib

[David Weinehall]

Since the defaults for some external power management related settings
prevents us from testing our power management functionality properly,
we have to work around it. Currently this is done from the individual
test cases, but this is sub-optimal.  This patch moves the PM-related
workarounds into a separate library, and adds some code to restore the
previous settings for the SATA link power management while at it.

This patch should be seen as a RFC; there might be other workarounds
for external issues that should be moved into the library, and if those
workarounds aren't related to power management it might be better to
choose a different name for the library.

David Weinehall (1):
  lib/pm_workarounds: Lib for PM workarounds

 lib/Makefile.sources |   2 +
 lib/igt.h            |   1 +
 lib/igt_aux.c        |  15 +---
 lib/pm_workarounds.c | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pm_workarounds.h |  31 +++++++
 tests/pm_lpsp.c      |  25 +-----
 tests/pm_rpm.c       |  29 ++-----
 7 files changed, 279 insertions(+), 57 deletions(-)
 create mode 100644 lib/pm_workarounds.c
 create mode 100644 lib/pm_workarounds.h

-- 
2.6.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t] lib/pm_workarounds: Lib for PM workarounds

[David Weinehall]

Move workarounds for power management related issues in external
components into a separate library, and modify the users of such
workarounds accordingly.  This currently involves HD audio and
SATA link power management.  For SATA link PM there's also code to
save the previous settings, to allow for resetting the values after
we've finished testing.

Signed-off-by: David Weinehall <david.weinehall@intel.com>
---
 lib/Makefile.sources |   2 +
 lib/igt.h            |   1 +
 lib/igt_aux.c        |  15 +---
 lib/pm_workarounds.c | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pm_workarounds.h |  31 +++++++
 tests/pm_lpsp.c      |  25 +-----
 tests/pm_rpm.c       |  29 ++-----
 7 files changed, 279 insertions(+), 57 deletions(-)
 create mode 100644 lib/pm_workarounds.c
 create mode 100644 lib/pm_workarounds.h

diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index cb20f030cbec..25f67edbd19a 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -59,6 +59,8 @@ libintel_tools_la_SOURCES = 	\
 	igt_core.h		\
 	igt_draw.c		\
 	igt_draw.h		\
+	pm_workarounds.c	\
+	pm_workarounds.h	\
 	$(NULL)
 
 .PHONY: version.h.tmp
diff --git a/lib/igt.h b/lib/igt.h
index 3be25511bb77..bb1c199bbbec 100644
--- a/lib/igt.h
+++ b/lib/igt.h
@@ -44,5 +44,6 @@
 #include "media_fill.h"
 #include "media_spin.h"
 #include "rendercopy.h"
+#include "pm_workarounds.h"
 
 #endif /* IGT_H */
diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 4d08d68bb932..da9770ec88ea 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -59,6 +59,7 @@
 #include "intel_reg.h"
 #include "ioctl_wrappers.h"
 #include "igt_kms.h"
+#include "pm_workarounds.h"
 
 /**
  * SECTION:igt_aux
@@ -531,19 +532,7 @@ bool igt_setup_runtime_pm(void)
 	if (pm_status_fd >= 0)
 		return true;
 
-	/* The Audio driver can get runtime PM references, so we need to make
-	 * sure its runtime PM is enabled, so it can release the refs and
-	 * actually enable us to runtime suspend. */
-	fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert(write(fd, "1\n", 2) == 2);
-		close(fd);
-	}
-	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert(write(fd, "auto\n", 5) == 5);
-		close(fd);
-	}
+	enable_audio_runtime_pm();
 
 	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
 	 * suite goes faster and we have a higher probability of triggering race
diff --git a/lib/pm_workarounds.c b/lib/pm_workarounds.c
new file mode 100644
index 000000000000..ae9da4fad910
--- /dev/null
+++ b/lib/pm_workarounds.c
@@ -0,0 +1,233 @@
+/*
+ * Copyright © 2013, 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Paulo Zanoni <paulo.r.zanoni@intel.com>
+ *    David Weinehall <david.weinehall@intel.com>
+ *
+ */
+#include <fcntl.h>
+#include <stdio.h>
+#include <limits.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+#include "drmtest.h"
+#include "pm_workarounds.h"
+
+enum {
+	POLICY_UNKNOWN = -1,
+	POLICY_MAX_PERFORMANCE = 0,
+	POLICY_MEDIUM_POWER = 1,
+	POLICY_MIN_POWER = 2
+};
+
+#define MAX_PERFORMANCE_STR	"max_performance\n"
+#define MEDIUM_POWER_STR	"medium_power\n"
+#define MIN_POWER_STR		"min_power\n"
+/* Remember to fix this if adding longer strings */
+#define MAX_POLICY_STRLEN	strlen(MAX_PERFORMANCE_STR)
+
+/**
+ * SECTION:pm_workarounds
+ * @short_description: Workarounds for PM issues in external components
+ * @title: PM Workarounds
+ * @include: igt.h
+ *
+ * This library provides various helpers that will workaround,
+ * and in some cases subsequently allow restoring of the old behaviour of,
+ * various external components that by default are set up in a way
+ * that interferes with the testing of our power management functionality.
+ */
+/**
+ * enable_audio_runtime_pm:
+ *
+ * We know that if we don't enable audio runtime PM, snd_hda_intel will never
+ * release its power well refcount, and we'll never reach the LPSP sate.
+ * There's no guarantee that it will release the power well if we enable
+ * runtime PM, but at least we can try.
+ *
+ * We don't have any assertions on open since the user may not even have
+ * snd_hda_intel loaded, which is not a problem.
+ */
+void enable_audio_runtime_pm(void)
+{
+	int fd;
+
+	fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
+	if (fd >= 0) {
+		igt_assert_eq(write(fd, "1\n", 2), 2);
+		close(fd);
+	}
+	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
+	if (fd >= 0) {
+		igt_assert_eq(write(fd, "auto\n", 5), 5);
+		close(fd);
+	}
+	/* Give some time for it to react. */
+	sleep(1);
+}
+
+/**
+ * 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_mananagement).
+ */
+int8_t *enable_sata_link_power_management(void)
+{
+	int fd, i;
+	ssize_t len;
+	char *buf;
+	char *file_name;
+	int8_t *link_pm_policies = NULL;
+
+	file_name = malloc(PATH_MAX);
+	buf = malloc(MAX_POLICY_STRLEN + 1);
+
+	for (i = 0; ; i++) {
+		int8_t policy;
+
+		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;
+
+		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;
+
+		if (!(i % 256))
+			link_pm_policies = realloc(link_pm_policies,
+						   (i / 256 + 1) * 256 + 1);
+
+		link_pm_policies[i] = policy;
+		link_pm_policies[i + 1] = 0;
+
+		/* If the policy is something we don't know about,
+		 * don't touch it, since we might potentially break things.
+		 * And we obviously don't need to touch anything if the
+		 * setting is already correct...
+		 */
+		if (policy != POLICY_UNKNOWN &&
+		    policy != POLICY_MIN_POWER) {
+			lseek(fd, 0, SEEK_SET);
+			igt_assert_eq(write(fd, MIN_POWER_STR,
+					    strlen(MIN_POWER_STR)),
+				      strlen(MIN_POWER_STR));
+		}
+		close(fd);
+	}
+	free(buf);
+	free(file_name);
+
+	return link_pm_policies;
+}
+
+/**
+ * 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 restore_sata_link_power_management(int8_t *pm_data)
+{
+	int fd, i;
+	char *file_name;
+
+	/* Disk runtime PM policies. */
+	file_name = malloc(PATH_MAX);
+	for (i = 0; ; i++) {
+		int8_t policy;
+
+		if (!pm_data)
+			policy = POLICY_MAX_PERFORMANCE;
+		else if (pm_data[i] == POLICY_UNKNOWN)
+			continue;
+		else
+			policy = pm_data[i];
+
+		snprintf(file_name, PATH_MAX,
+			 "/sys/class/scsi_host/host%d/link_power_management_policy",
+			 i);
+
+		fd = open(file_name, O_WRONLY);
+		if (fd < 0)
+			break;
+
+		switch (policy) {
+		default:
+		case POLICY_MAX_PERFORMANCE:
+			igt_assert_eq(write(fd, MAX_PERFORMANCE_STR,
+					    strlen(MAX_PERFORMANCE_STR)),
+				      strlen(MAX_PERFORMANCE_STR));
+			break;
+
+		case POLICY_MEDIUM_POWER:
+			igt_assert_eq(write(fd, MEDIUM_POWER_STR,
+					    strlen(MEDIUM_POWER_STR)),
+				      strlen(MEDIUM_POWER_STR));
+			break;
+
+		case POLICY_MIN_POWER:
+			igt_assert_eq(write(fd, MIN_POWER_STR,
+					    strlen(MIN_POWER_STR)),
+				      strlen(MIN_POWER_STR));
+			break;
+		}
+
+		close(fd);
+	}
+	free(file_name);
+}
diff --git a/lib/pm_workarounds.h b/lib/pm_workarounds.h
new file mode 100644
index 000000000000..867e3986f91d
--- /dev/null
+++ b/lib/pm_workarounds.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef PM_WORKAROUNDS_H
+#define PM_WORKAROUNDS_H
+
+void enable_audio_runtime_pm(void);
+int8_t *enable_sata_link_power_management(void);
+void restore_sata_link_power_management(int8_t *pm_data);
+
+#endif /* PM_WORKAROUNDS_H */
diff --git a/tests/pm_lpsp.c b/tests/pm_lpsp.c
index 257ae1b8b1d9..9e3c24778e38 100644
--- a/tests/pm_lpsp.c
+++ b/tests/pm_lpsp.c
@@ -31,29 +31,6 @@
 #include <unistd.h>
 
 
-/* We know that if we don't enable audio runtime PM, snd_hda_intel will never
- * release its power well refcount, and we'll never reach the LPSP sate. OTOH
- * there's no guarantee that it will release the power well if we enable runtime
- * PM, but at least we can try.  We don't have any assertions since the user may
- * not even have snd_hda_intel loaded, which is not a problem. */
-static void disable_audio_runtime_pm(void)
-{
-	int fd;
-
-	fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert_eq(write(fd, "1\n", 2), 2);
-		close(fd);
-	}
-	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert_eq(write(fd, "auto\n", 5), 5);
-		close(fd);
-	}
-	/* Give some time for it to react. */
-	sleep(1);
-}
-
 static bool supports_lpsp(uint32_t devid)
 {
 	return IS_HASWELL(devid) || IS_BROADWELL(devid);
@@ -234,7 +211,7 @@ igt_main
 			drm_connectors[i] = drmModeGetConnectorCurrent(drm_fd,
 							drm_res->connectors[i]);
 
-		disable_audio_runtime_pm();
+		enable_audio_runtime_pm();
 
 		igt_require(supports_lpsp(devid));
 
diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 55fdb31cb723..5bf562a99cf8 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2013 Intel Corporation
+ * Copyright © 2013, 2015 Intel Corporation
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -111,6 +111,8 @@ 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;
+
 /* If the read fails, then the machine doesn't support PC8+ residencies. */
 static bool supports_pc8_plus_residencies(void)
 {
@@ -677,25 +679,7 @@ static void setup_pc8(void)
  * anything, just try to help with the more common problems. */
 static void setup_non_graphics_runtime_pm(void)
 {
-	int fd, i;
-	char *file_name;
-
-	/* Disk runtime PM policies. */
-	file_name = malloc(PATH_MAX);
-	for (i = 0; ; i++) {
-
-		snprintf(file_name, PATH_MAX,
-			 "/sys/class/scsi_host/host%d/link_power_management_policy",
-			 i);
-
-		fd = open(file_name, O_WRONLY);
-		if (fd < 0)
-			break;
-
-		igt_assert(write(fd, "min_power\n", 10) == 10);
-		close(fd);
-	}
-	free(file_name);
+	pm_data = enable_sata_link_power_management();
 }
 
 static void setup_environment(void)
@@ -713,11 +697,16 @@ static void setup_environment(void)
 	igt_info("PC8 residency support: %d\n", has_pc8);
 
 	igt_require(has_runtime_pm);
+}
 
+static void restore_environment(void)
+{
+	restore_sata_link_power_management(pm_data);
 }
 
 static void teardown_environment(void)
 {
+	restore_environment();
 	fini_mode_set_data(&ms_data);
 	drmClose(drm_fd);
 	close(msr_fd);
-- 
2.6.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] RFC: split PM workarounds into separate lib

[Zanoni, Paulo R]

Em Ter, 2015-12-08 às 10:50 +0200, David Weinehall escreveu:
> Since the defaults for some external power management related
> settings
> prevents us from testing our power management functionality properly,
> we have to work around it. Currently this is done from the individual
> test cases, but this is sub-optimal.  This patch moves the PM-related
> workarounds into a separate library, and adds some code to restore
> the
> previous settings for the SATA link power management while at it.
> 
> This patch should be seen as a RFC; there might be other workarounds
> for external issues that should be moved into the library, and if
> those
> workarounds aren't related to power management it might be better to
> choose a different name for the library.

I didn't deeply look the implementation, but you have my Acked-by on
the idea.

You may also consider adding a function to just run "sudo powertop --
auto-tune" in addition to the other things, but you can't undo this
later.

But in the end, it all depends on your machine. A bad machine will
never reach the deepest expected PC states. That's the problem when
automating things...

Since you're interested in PM, you may also want to look at:
http://patchwork.freedesktop.org/patch/66392/
maybe there's some code there that you may want to take.

> 
> David Weinehall (1):
>   lib/pm_workarounds: Lib for PM workarounds
> 
>  lib/Makefile.sources |   2 +
>  lib/igt.h            |   1 +
>  lib/igt_aux.c        |  15 +---
>  lib/pm_workarounds.c | 233
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pm_workarounds.h |  31 +++++++
>  tests/pm_lpsp.c      |  25 +-----
>  tests/pm_rpm.c       |  29 ++-----
>  7 files changed, 279 insertions(+), 57 deletions(-)
>  create mode 100644 lib/pm_workarounds.c
>  create mode 100644 lib/pm_workarounds.h
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] RFC: split PM workarounds into separate lib

[David Weinehall]

On Tue, Dec 08, 2015 at 01:22:14PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2015-12-08 às 10:50 +0200, David Weinehall escreveu:
> > Since the defaults for some external power management related
> > settings
> > prevents us from testing our power management functionality properly,
> > we have to work around it. Currently this is done from the individual
> > test cases, but this is sub-optimal.  This patch moves the PM-related
> > workarounds into a separate library, and adds some code to restore
> > the
> > previous settings for the SATA link power management while at it.
> > 
> > This patch should be seen as a RFC; there might be other workarounds
> > for external issues that should be moved into the library, and if
> > those
> > workarounds aren't related to power management it might be better to
> > choose a different name for the library.
> 
> I didn't deeply look the implementation, but you have my Acked-by on
> the idea.
> 
> You may also consider adding a function to just run "sudo powertop --
> auto-tune" in addition to the other things, but you can't undo this
> later.
> 
> But in the end, it all depends on your machine. A bad machine will
> never reach the deepest expected PC states. That's the problem when
> automating things...
> 
> Since you're interested in PM, you may also want to look at:
> http://patchwork.freedesktop.org/patch/66392/
> maybe there's some code there that you may want to take.

Thanks! At the very least it seems like a very useful tool that I can
use as an independent test to verify my own tests against. :)


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] RFC: split PM workarounds into separate lib

[Ville Syrjälä]

On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote:
> Since the defaults for some external power management related settings
> prevents us from testing our power management functionality properly,
> we have to work around it. Currently this is done from the individual
> test cases, but this is sub-optimal.  This patch moves the PM-related
> workarounds into a separate library, and adds some code to restore the
> previous settings for the SATA link power management while at it.

Why is it called "workarounds"? That gives me the impression we're
working around something that's supposed to work but doesn't. That's not
the case here.

> 
> This patch should be seen as a RFC; there might be other workarounds
> for external issues that should be moved into the library, and if those
> workarounds aren't related to power management it might be better to
> choose a different name for the library.
> 
> David Weinehall (1):
>   lib/pm_workarounds: Lib for PM workarounds
> 
>  lib/Makefile.sources |   2 +
>  lib/igt.h            |   1 +
>  lib/igt_aux.c        |  15 +---
>  lib/pm_workarounds.c | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/pm_workarounds.h |  31 +++++++
>  tests/pm_lpsp.c      |  25 +-----
>  tests/pm_rpm.c       |  29 ++-----
>  7 files changed, 279 insertions(+), 57 deletions(-)
>  create mode 100644 lib/pm_workarounds.c
>  create mode 100644 lib/pm_workarounds.h
> 
> -- 
> 2.6.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] RFC: split PM workarounds into separate lib

[Paulo Zanoni]

2015-12-08 11:42 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote:
>> Since the defaults for some external power management related settings
>> prevents us from testing our power management functionality properly,
>> we have to work around it. Currently this is done from the individual
>> test cases, but this is sub-optimal.  This patch moves the PM-related
>> workarounds into a separate library, and adds some code to restore the
>> previous settings for the SATA link power management while at it.
>
> Why is it called "workarounds"? That gives me the impression we're
> working around something that's supposed to work but doesn't. That's not
> the case here.

Well, in theory they could be considered workarounds since IMHO the
machine is supposed to be saving as much power as it can on an idle
state, but it isn't. But this more of a philosophical discussion and
we can debate forever.

Anyway, if we rename the file to something like lib/igt_pm.c we'll be
able to move the residency-checking code and possibly more common
things there, so I'm not against the rename either.

>
>>
>> This patch should be seen as a RFC; there might be other workarounds
>> for external issues that should be moved into the library, and if those
>> workarounds aren't related to power management it might be better to
>> choose a different name for the library.
>>
>> David Weinehall (1):
>>   lib/pm_workarounds: Lib for PM workarounds
>>
>>  lib/Makefile.sources |   2 +
>>  lib/igt.h            |   1 +
>>  lib/igt_aux.c        |  15 +---
>>  lib/pm_workarounds.c | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/pm_workarounds.h |  31 +++++++
>>  tests/pm_lpsp.c      |  25 +-----
>>  tests/pm_rpm.c       |  29 ++-----
>>  7 files changed, 279 insertions(+), 57 deletions(-)
>>  create mode 100644 lib/pm_workarounds.c
>>  create mode 100644 lib/pm_workarounds.h
>>
>> --
>> 2.6.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] RFC: split PM workarounds into separate lib

[Daniel Vetter]

On Tue, Dec 08, 2015 at 05:05:12PM -0200, Paulo Zanoni wrote:
> 2015-12-08 11:42 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote:
> >> Since the defaults for some external power management related settings
> >> prevents us from testing our power management functionality properly,
> >> we have to work around it. Currently this is done from the individual
> >> test cases, but this is sub-optimal.  This patch moves the PM-related
> >> workarounds into a separate library, and adds some code to restore the
> >> previous settings for the SATA link power management while at it.
> >
> > Why is it called "workarounds"? That gives me the impression we're
> > working around something that's supposed to work but doesn't. That's not
> > the case here.
> 
> Well, in theory they could be considered workarounds since IMHO the
> machine is supposed to be saving as much power as it can on an idle
> state, but it isn't. But this more of a philosophical discussion and
> we can debate forever.
> 
> Anyway, if we rename the file to something like lib/igt_pm.c we'll be
> able to move the residency-checking code and possibly more common
> things there, so I'm not against the rename either.

I like the ring of lib/igt_pm.c.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] RFC: split PM workarounds into separate lib

[David Weinehall]

On Thu, Dec 10, 2015 at 11:09:42AM +0100, Daniel Vetter wrote:
> On Tue, Dec 08, 2015 at 05:05:12PM -0200, Paulo Zanoni wrote:
> > 2015-12-08 11:42 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote:
> > >> Since the defaults for some external power management related settings
> > >> prevents us from testing our power management functionality properly,
> > >> we have to work around it. Currently this is done from the individual
> > >> test cases, but this is sub-optimal.  This patch moves the PM-related
> > >> workarounds into a separate library, and adds some code to restore the
> > >> previous settings for the SATA link power management while at it.
> > >
> > > Why is it called "workarounds"? That gives me the impression we're
> > > working around something that's supposed to work but doesn't. That's not
> > > the case here.
> > 
> > Well, in theory they could be considered workarounds since IMHO the
> > machine is supposed to be saving as much power as it can on an idle
> > state, but it isn't. But this more of a philosophical discussion and
> > we can debate forever.
> > 
> > Anyway, if we rename the file to something like lib/igt_pm.c we'll be
> > able to move the residency-checking code and possibly more common
> > things there, so I'm not against the rename either.
> 
> I like the ring of lib/igt_pm.c.

No reservations from me, igt_pm it is.


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] RFC: split PM workarounds into separate lib

[David Weinehall]

On Tue, Dec 08, 2015 at 03:42:27PM +0200, Ville Syrjälä wrote:
> On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote:
> > Since the defaults for some external power management related settings
> > prevents us from testing our power management functionality properly,
> > we have to work around it. Currently this is done from the individual
> > test cases, but this is sub-optimal.  This patch moves the PM-related
> > workarounds into a separate library, and adds some code to restore the
> > previous settings for the SATA link power management while at it.
> 
> Why is it called "workarounds"? That gives me the impression we're
> working around something that's supposed to work but doesn't. That's not
> the case here.

Workarounds was because we are working around "imperfect" settings
in other components. At least to me power management should be enabled
out of the box, not something that requires admin-level workarounds.
Since we're not in control of said defaults, we have to modify the
settings when we run our tests, hence workarounds.

That said, as I've replied to a later post, igt_pm is fine by me.


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] RFC: split PM workarounds into separate lib

[Daniel Vetter]

On Thu, Dec 10, 2015 at 06:01:28PM +0200, David Weinehall wrote:
> On Tue, Dec 08, 2015 at 03:42:27PM +0200, Ville Syrjälä wrote:
> > On Tue, Dec 08, 2015 at 10:50:39AM +0200, David Weinehall wrote:
> > > Since the defaults for some external power management related settings
> > > prevents us from testing our power management functionality properly,
> > > we have to work around it. Currently this is done from the individual
> > > test cases, but this is sub-optimal.  This patch moves the PM-related
> > > workarounds into a separate library, and adds some code to restore the
> > > previous settings for the SATA link power management while at it.
> > 
> > Why is it called "workarounds"? That gives me the impression we're
> > working around something that's supposed to work but doesn't. That's not
> > the case here.
> 
> Workarounds was because we are working around "imperfect" settings
> in other components. At least to me power management should be enabled
> out of the box, not something that requires admin-level workarounds.
> Since we're not in control of said defaults, we have to modify the
> settings when we run our tests, hence workarounds.

Fully agreed that power tuning should be applied by default, but that's a
loooooong process to convince all the other kernel maintainers. And we
need to get our own house in order first too, but that's in progress.

> That said, as I've replied to a later post, igt_pm is fine by me.

One more: Please namespace all the library functions you're adding and
exporting to tests with igt_pm_. Static/internal functions can still be
named however you feel like.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2] Add a lib for power management helpers

[David Weinehall]

This patch aims to create a separate lib for power management related
helpers. Initially it only contains code that modify settings for
external components (to handle components with default settings that
prevents entering deeper sleep states), but moving i915-related
power management helpers to this lib would probably make sense too.

v2: Change name of library to igt_pm
    Namespace all exported functions with igt_pm_

David Weinehall (1):
  lib/igt_pm: Lib for power management

 lib/Makefile.sources |   2 +
 lib/igt.h            |   1 +
 lib/igt_aux.c        |  15 +---
 lib/igt_pm.c         | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_pm.h         |  31 +++++++
 tests/pm_lpsp.c      |  25 +-----
 tests/pm_rpm.c       |  29 ++-----
 7 files changed, 279 insertions(+), 57 deletions(-)
 create mode 100644 lib/igt_pm.c
 create mode 100644 lib/igt_pm.h

-- 
2.6.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t] lib/igt_pm: Lib for power management

[David Weinehall]

Move power management related code to a separate library.
Initially this is done only for workarounds that apply to external
components.  Modify the users of such workarounds accordingly.
This currently involves HD audio and SATA link power management.
For SATA link PM there's also code to save the previous settings,
to allow for resetting the values after we've finished testing.

Signed-off-by: David Weinehall <david.weinehall@intel.com>
---
 lib/Makefile.sources |   2 +
 lib/igt.h            |   1 +
 lib/igt_aux.c        |  15 +---
 lib/igt_pm.c         | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_pm.h         |  31 +++++++
 tests/pm_lpsp.c      |  25 +-----
 tests/pm_rpm.c       |  29 ++-----
 7 files changed, 279 insertions(+), 57 deletions(-)
 create mode 100644 lib/igt_pm.c
 create mode 100644 lib/igt_pm.h

diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index 4999868052b1..2f0eb2075e14 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -60,6 +60,8 @@ libintel_tools_la_SOURCES = 	\
 	igt_core.h		\
 	igt_draw.c		\
 	igt_draw.h		\
+	igt_pm.c	\
+	igt_pm.h	\
 	$(NULL)
 
 .PHONY: version.h.tmp
diff --git a/lib/igt.h b/lib/igt.h
index 3be25511bb77..b8792141ee3c 100644
--- a/lib/igt.h
+++ b/lib/igt.h
@@ -44,5 +44,6 @@
 #include "media_fill.h"
 #include "media_spin.h"
 #include "rendercopy.h"
+#include "igt_pm.h"
 
 #endif /* IGT_H */
diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 4d08d68bb932..cd7f14649fe2 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -59,6 +59,7 @@
 #include "intel_reg.h"
 #include "ioctl_wrappers.h"
 #include "igt_kms.h"
+#include "igt_pm.h"
 
 /**
  * SECTION:igt_aux
@@ -531,19 +532,7 @@ bool igt_setup_runtime_pm(void)
 	if (pm_status_fd >= 0)
 		return true;
 
-	/* The Audio driver can get runtime PM references, so we need to make
-	 * sure its runtime PM is enabled, so it can release the refs and
-	 * actually enable us to runtime suspend. */
-	fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert(write(fd, "1\n", 2) == 2);
-		close(fd);
-	}
-	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert(write(fd, "auto\n", 5) == 5);
-		close(fd);
-	}
+	igt_pm_enable_audio_runtime_pm();
 
 	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
 	 * suite goes faster and we have a higher probability of triggering race
diff --git a/lib/igt_pm.c b/lib/igt_pm.c
new file mode 100644
index 000000000000..f2e13ba6a44e
--- /dev/null
+++ b/lib/igt_pm.c
@@ -0,0 +1,233 @@
+/*
+ * Copyright © 2013, 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Paulo Zanoni <paulo.r.zanoni@intel.com>
+ *    David Weinehall <david.weinehall@intel.com>
+ *
+ */
+#include <fcntl.h>
+#include <stdio.h>
+#include <limits.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+#include "drmtest.h"
+#include "igt_pm.h"
+
+enum {
+	POLICY_UNKNOWN = -1,
+	POLICY_MAX_PERFORMANCE = 0,
+	POLICY_MEDIUM_POWER = 1,
+	POLICY_MIN_POWER = 2
+};
+
+#define MAX_PERFORMANCE_STR	"max_performance\n"
+#define MEDIUM_POWER_STR	"medium_power\n"
+#define MIN_POWER_STR		"min_power\n"
+/* Remember to fix this if adding longer strings */
+#define MAX_POLICY_STRLEN	strlen(MAX_PERFORMANCE_STR)
+
+/**
+ * SECTION:igt_pm
+ * @short_description: Power Management related helpers
+ * @title: IGT PM
+ * @include: igt_pm.h
+ *
+ * This library provides various helpers to enable power management,
+ * and in some cases subsequently allow restoring of the old behaviour of,
+ * various external components that by default are set up in a way
+ * that interferes with the testing of our power management functionality.
+ */
+/**
+ * igt_pm_enable_audio_runtime_pm:
+ *
+ * We know that if we don't enable audio runtime PM, snd_hda_intel will never
+ * release its power well refcount, and we'll never reach the LPSP sate.
+ * There's no guarantee that it will release the power well if we enable
+ * runtime PM, but at least we can try.
+ *
+ * We don't have any assertions on open since the user may not even have
+ * snd_hda_intel loaded, which is not a problem.
+ */
+void igt_pm_enable_audio_runtime_pm(void)
+{
+	int fd;
+
+	fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
+	if (fd >= 0) {
+		igt_assert_eq(write(fd, "1\n", 2), 2);
+		close(fd);
+	}
+	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
+	if (fd >= 0) {
+		igt_assert_eq(write(fd, "auto\n", 5), 5);
+		close(fd);
+	}
+	/* Give some time for it to react. */
+	sleep(1);
+}
+
+/**
+ * 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_mananagement).
+ */
+int8_t *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;
+
+	file_name = malloc(PATH_MAX);
+	buf = malloc(MAX_POLICY_STRLEN + 1);
+
+	for (i = 0; ; i++) {
+		int8_t policy;
+
+		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;
+
+		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;
+
+		if (!(i % 256))
+			link_pm_policies = realloc(link_pm_policies,
+						   (i / 256 + 1) * 256 + 1);
+
+		link_pm_policies[i] = policy;
+		link_pm_policies[i + 1] = 0;
+
+		/* If the policy is something we don't know about,
+		 * don't touch it, since we might potentially break things.
+		 * And we obviously don't need to touch anything if the
+		 * setting is already correct...
+		 */
+		if (policy != POLICY_UNKNOWN &&
+		    policy != POLICY_MIN_POWER) {
+			lseek(fd, 0, SEEK_SET);
+			igt_assert_eq(write(fd, MIN_POWER_STR,
+					    strlen(MIN_POWER_STR)),
+				      strlen(MIN_POWER_STR));
+		}
+		close(fd);
+	}
+	free(buf);
+	free(file_name);
+
+	return link_pm_policies;
+}
+
+/**
+ * it_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)
+{
+	int fd, i;
+	char *file_name;
+
+	/* Disk runtime PM policies. */
+	file_name = malloc(PATH_MAX);
+	for (i = 0; ; i++) {
+		int8_t policy;
+
+		if (!pm_data)
+			policy = POLICY_MAX_PERFORMANCE;
+		else if (pm_data[i] == POLICY_UNKNOWN)
+			continue;
+		else
+			policy = pm_data[i];
+
+		snprintf(file_name, PATH_MAX,
+			 "/sys/class/scsi_host/host%d/link_power_management_policy",
+			 i);
+
+		fd = open(file_name, O_WRONLY);
+		if (fd < 0)
+			break;
+
+		switch (policy) {
+		default:
+		case POLICY_MAX_PERFORMANCE:
+			igt_assert_eq(write(fd, MAX_PERFORMANCE_STR,
+					    strlen(MAX_PERFORMANCE_STR)),
+				      strlen(MAX_PERFORMANCE_STR));
+			break;
+
+		case POLICY_MEDIUM_POWER:
+			igt_assert_eq(write(fd, MEDIUM_POWER_STR,
+					    strlen(MEDIUM_POWER_STR)),
+				      strlen(MEDIUM_POWER_STR));
+			break;
+
+		case POLICY_MIN_POWER:
+			igt_assert_eq(write(fd, MIN_POWER_STR,
+					    strlen(MIN_POWER_STR)),
+				      strlen(MIN_POWER_STR));
+			break;
+		}
+
+		close(fd);
+	}
+	free(file_name);
+}
diff --git a/lib/igt_pm.h b/lib/igt_pm.h
new file mode 100644
index 000000000000..c14ff1f7a0ef
--- /dev/null
+++ b/lib/igt_pm.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef IGT_PM_H
+#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);
+
+#endif /* IGT_PM_H */
diff --git a/tests/pm_lpsp.c b/tests/pm_lpsp.c
index 257ae1b8b1d9..6cb697ed0bd5 100644
--- a/tests/pm_lpsp.c
+++ b/tests/pm_lpsp.c
@@ -31,29 +31,6 @@
 #include <unistd.h>
 
 
-/* We know that if we don't enable audio runtime PM, snd_hda_intel will never
- * release its power well refcount, and we'll never reach the LPSP sate. OTOH
- * there's no guarantee that it will release the power well if we enable runtime
- * PM, but at least we can try.  We don't have any assertions since the user may
- * not even have snd_hda_intel loaded, which is not a problem. */
-static void disable_audio_runtime_pm(void)
-{
-	int fd;
-
-	fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert_eq(write(fd, "1\n", 2), 2);
-		close(fd);
-	}
-	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert_eq(write(fd, "auto\n", 5), 5);
-		close(fd);
-	}
-	/* Give some time for it to react. */
-	sleep(1);
-}
-
 static bool supports_lpsp(uint32_t devid)
 {
 	return IS_HASWELL(devid) || IS_BROADWELL(devid);
@@ -234,7 +211,7 @@ igt_main
 			drm_connectors[i] = drmModeGetConnectorCurrent(drm_fd,
 							drm_res->connectors[i]);
 
-		disable_audio_runtime_pm();
+		igt_pm_enable_audio_runtime_pm();
 
 		igt_require(supports_lpsp(devid));
 
diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 55fdb31cb723..6d695a5e3e01 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2013 Intel Corporation
+ * Copyright © 2013, 2015 Intel Corporation
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -111,6 +111,8 @@ 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;
+
 /* If the read fails, then the machine doesn't support PC8+ residencies. */
 static bool supports_pc8_plus_residencies(void)
 {
@@ -677,25 +679,7 @@ static void setup_pc8(void)
  * anything, just try to help with the more common problems. */
 static void setup_non_graphics_runtime_pm(void)
 {
-	int fd, i;
-	char *file_name;
-
-	/* Disk runtime PM policies. */
-	file_name = malloc(PATH_MAX);
-	for (i = 0; ; i++) {
-
-		snprintf(file_name, PATH_MAX,
-			 "/sys/class/scsi_host/host%d/link_power_management_policy",
-			 i);
-
-		fd = open(file_name, O_WRONLY);
-		if (fd < 0)
-			break;
-
-		igt_assert(write(fd, "min_power\n", 10) == 10);
-		close(fd);
-	}
-	free(file_name);
+	pm_data = igt_pm_enable_sata_link_power_management();
 }
 
 static void setup_environment(void)
@@ -713,11 +697,16 @@ static void setup_environment(void)
 	igt_info("PC8 residency support: %d\n", has_pc8);
 
 	igt_require(has_runtime_pm);
+}
 
+static void restore_environment(void)
+{
+	igt_pm_restore_sata_link_power_management(pm_data);
 }
 
 static void teardown_environment(void)
 {
+	restore_environment();
 	fini_mode_set_data(&ms_data);
 	drmClose(drm_fd);
 	close(msr_fd);
-- 
2.6.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] lib/igt_pm: Lib for power management

[Thomas Wood]

On 15 December 2015 at 09:14, David Weinehall
<david.weinehall@linux.intel.com> wrote:
> Move power management related code to a separate library.
> Initially this is done only for workarounds that apply to external
> components.  Modify the users of such workarounds accordingly.
> This currently involves HD audio and SATA link power management.
> For SATA link PM there's also code to save the previous settings,
> to allow for resetting the values after we've finished testing.
>
> Signed-off-by: David Weinehall <david.weinehall@intel.com>
> ---
>  lib/Makefile.sources |   2 +
>  lib/igt.h            |   1 +
>  lib/igt_aux.c        |  15 +---
>  lib/igt_pm.c         | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_pm.h         |  31 +++++++
>  tests/pm_lpsp.c      |  25 +-----
>  tests/pm_rpm.c       |  29 ++-----

igt_pm.xml needs including in
docs/reference/intel-gpu-tools/intel-gpu-tools-docs.xml to make sure
the new documentation is included in the output.


>  7 files changed, 279 insertions(+), 57 deletions(-)
>  create mode 100644 lib/igt_pm.c
>  create mode 100644 lib/igt_pm.h
>
> diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> index 4999868052b1..2f0eb2075e14 100644
> --- a/lib/Makefile.sources
> +++ b/lib/Makefile.sources
> @@ -60,6 +60,8 @@ libintel_tools_la_SOURCES =   \
>         igt_core.h              \
>         igt_draw.c              \
>         igt_draw.h              \
> +       igt_pm.c        \
> +       igt_pm.h        \
>         $(NULL)
>
>  .PHONY: version.h.tmp
> diff --git a/lib/igt.h b/lib/igt.h
> index 3be25511bb77..b8792141ee3c 100644
> --- a/lib/igt.h
> +++ b/lib/igt.h
> @@ -44,5 +44,6 @@
>  #include "media_fill.h"
>  #include "media_spin.h"
>  #include "rendercopy.h"
> +#include "igt_pm.h"

It'd be nice to keep the list alphabetical.


>
>  #endif /* IGT_H */
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index 4d08d68bb932..cd7f14649fe2 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -59,6 +59,7 @@
>  #include "intel_reg.h"
>  #include "ioctl_wrappers.h"
>  #include "igt_kms.h"
> +#include "igt_pm.h"
>
>  /**
>   * SECTION:igt_aux
> @@ -531,19 +532,7 @@ bool igt_setup_runtime_pm(void)
>         if (pm_status_fd >= 0)
>                 return true;
>
> -       /* The Audio driver can get runtime PM references, so we need to make
> -        * sure its runtime PM is enabled, so it can release the refs and
> -        * actually enable us to runtime suspend. */
> -       fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
> -       if (fd >= 0) {
> -               igt_assert(write(fd, "1\n", 2) == 2);
> -               close(fd);
> -       }
> -       fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
> -       if (fd >= 0) {
> -               igt_assert(write(fd, "auto\n", 5) == 5);
> -               close(fd);
> -       }
> +       igt_pm_enable_audio_runtime_pm();
>
>         /* Our implementation uses autosuspend. Try to set it to 0ms so the test
>          * suite goes faster and we have a higher probability of triggering race
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> new file mode 100644
> index 000000000000..f2e13ba6a44e
> --- /dev/null
> +++ b/lib/igt_pm.c
> @@ -0,0 +1,233 @@
> +/*
> + * Copyright © 2013, 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Paulo Zanoni <paulo.r.zanoni@intel.com>
> + *    David Weinehall <david.weinehall@intel.com>
> + *
> + */
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <limits.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +
> +#include "drmtest.h"
> +#include "igt_pm.h"
> +
> +enum {
> +       POLICY_UNKNOWN = -1,
> +       POLICY_MAX_PERFORMANCE = 0,
> +       POLICY_MEDIUM_POWER = 1,
> +       POLICY_MIN_POWER = 2
> +};
> +
> +#define MAX_PERFORMANCE_STR    "max_performance\n"
> +#define MEDIUM_POWER_STR       "medium_power\n"
> +#define MIN_POWER_STR          "min_power\n"
> +/* Remember to fix this if adding longer strings */
> +#define MAX_POLICY_STRLEN      strlen(MAX_PERFORMANCE_STR)
> +
> +/**
> + * SECTION:igt_pm
> + * @short_description: Power Management related helpers
> + * @title: IGT PM

"Power Management" would be more descriptive for the title.


> + * @include: igt_pm.h

Since igt_pm.h is included in igt.h, this only needs to mention igt.h.


> + *
> + * This library provides various helpers to enable power management,
> + * and in some cases subsequently allow restoring of the old behaviour of,

Should "of," be "and" here?


> + * various external components that by default are set up in a way
> + * that interferes with the testing of our power management functionality.
> + */
> +/**
> + * igt_pm_enable_audio_runtime_pm:
> + *
> + * We know that if we don't enable audio runtime PM, snd_hda_intel will never
> + * release its power well refcount, and we'll never reach the LPSP sate.
> + * There's no guarantee that it will release the power well if we enable
> + * runtime PM, but at least we can try.
> + *
> + * We don't have any assertions on open since the user may not even have
> + * snd_hda_intel loaded, which is not a problem.
> + */
> +void igt_pm_enable_audio_runtime_pm(void)
> +{
> +       int fd;
> +
> +       fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
> +       if (fd >= 0) {
> +               igt_assert_eq(write(fd, "1\n", 2), 2);
> +               close(fd);
> +       }
> +       fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
> +       if (fd >= 0) {
> +               igt_assert_eq(write(fd, "auto\n", 5), 5);
> +               close(fd);
> +       }
> +       /* Give some time for it to react. */
> +       sleep(1);
> +}
> +
> +/**
> + * 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_mananagement).

"management" is spelt incorrectly here. Also, adding "()" to the end
of the full function name will create a link to that function.


> + */
> +int8_t *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;
> +
> +       file_name = malloc(PATH_MAX);
> +       buf = malloc(MAX_POLICY_STRLEN + 1);
> +
> +       for (i = 0; ; i++) {
> +               int8_t policy;
> +
> +               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;
> +
> +               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;
> +
> +               if (!(i % 256))
> +                       link_pm_policies = realloc(link_pm_policies,
> +                                                  (i / 256 + 1) * 256 + 1);
> +
> +               link_pm_policies[i] = policy;
> +               link_pm_policies[i + 1] = 0;
> +
> +               /* If the policy is something we don't know about,
> +                * don't touch it, since we might potentially break things.
> +                * And we obviously don't need to touch anything if the
> +                * setting is already correct...
> +                */
> +               if (policy != POLICY_UNKNOWN &&
> +                   policy != POLICY_MIN_POWER) {
> +                       lseek(fd, 0, SEEK_SET);
> +                       igt_assert_eq(write(fd, MIN_POWER_STR,
> +                                           strlen(MIN_POWER_STR)),
> +                                     strlen(MIN_POWER_STR));
> +               }
> +               close(fd);
> +       }
> +       free(buf);
> +       free(file_name);
> +
> +       return link_pm_policies;
> +}
> +
> +/**
> + * it_pm_restore_sata_link_power_management:

igt_ prefix needs fixing here.


> + * @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)
> +{
> +       int fd, i;
> +       char *file_name;
> +
> +       /* Disk runtime PM policies. */
> +       file_name = malloc(PATH_MAX);
> +       for (i = 0; ; i++) {
> +               int8_t policy;
> +
> +               if (!pm_data)
> +                       policy = POLICY_MAX_PERFORMANCE;
> +               else if (pm_data[i] == POLICY_UNKNOWN)
> +                       continue;
> +               else
> +                       policy = pm_data[i];
> +
> +               snprintf(file_name, PATH_MAX,
> +                        "/sys/class/scsi_host/host%d/link_power_management_policy",
> +                        i);
> +
> +               fd = open(file_name, O_WRONLY);
> +               if (fd < 0)
> +                       break;
> +
> +               switch (policy) {
> +               default:
> +               case POLICY_MAX_PERFORMANCE:
> +                       igt_assert_eq(write(fd, MAX_PERFORMANCE_STR,
> +                                           strlen(MAX_PERFORMANCE_STR)),
> +                                     strlen(MAX_PERFORMANCE_STR));
> +                       break;
> +
> +               case POLICY_MEDIUM_POWER:
> +                       igt_assert_eq(write(fd, MEDIUM_POWER_STR,
> +                                           strlen(MEDIUM_POWER_STR)),
> +                                     strlen(MEDIUM_POWER_STR));
> +                       break;
> +
> +               case POLICY_MIN_POWER:
> +                       igt_assert_eq(write(fd, MIN_POWER_STR,
> +                                           strlen(MIN_POWER_STR)),
> +                                     strlen(MIN_POWER_STR));
> +                       break;
> +               }
> +
> +               close(fd);
> +       }
> +       free(file_name);
> +}
> diff --git a/lib/igt_pm.h b/lib/igt_pm.h
> new file mode 100644
> index 000000000000..c14ff1f7a0ef
> --- /dev/null
> +++ b/lib/igt_pm.h
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef IGT_PM_H
> +#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);
> +
> +#endif /* IGT_PM_H */
> diff --git a/tests/pm_lpsp.c b/tests/pm_lpsp.c
> index 257ae1b8b1d9..6cb697ed0bd5 100644
> --- a/tests/pm_lpsp.c
> +++ b/tests/pm_lpsp.c
> @@ -31,29 +31,6 @@
>  #include <unistd.h>
>
>
> -/* We know that if we don't enable audio runtime PM, snd_hda_intel will never
> - * release its power well refcount, and we'll never reach the LPSP sate. OTOH
> - * there's no guarantee that it will release the power well if we enable runtime
> - * PM, but at least we can try.  We don't have any assertions since the user may
> - * not even have snd_hda_intel loaded, which is not a problem. */
> -static void disable_audio_runtime_pm(void)
> -{
> -       int fd;
> -
> -       fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
> -       if (fd >= 0) {
> -               igt_assert_eq(write(fd, "1\n", 2), 2);
> -               close(fd);
> -       }
> -       fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
> -       if (fd >= 0) {
> -               igt_assert_eq(write(fd, "auto\n", 5), 5);
> -               close(fd);
> -       }
> -       /* Give some time for it to react. */
> -       sleep(1);
> -}
> -
>  static bool supports_lpsp(uint32_t devid)
>  {
>         return IS_HASWELL(devid) || IS_BROADWELL(devid);
> @@ -234,7 +211,7 @@ igt_main
>                         drm_connectors[i] = drmModeGetConnectorCurrent(drm_fd,
>                                                         drm_res->connectors[i]);
>
> -               disable_audio_runtime_pm();
> +               igt_pm_enable_audio_runtime_pm();
>
>                 igt_require(supports_lpsp(devid));
>
> diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
> index 55fdb31cb723..6d695a5e3e01 100644
> --- a/tests/pm_rpm.c
> +++ b/tests/pm_rpm.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright © 2013 Intel Corporation
> + * Copyright © 2013, 2015 Intel Corporation
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a
>   * copy of this software and associated documentation files (the "Software"),
> @@ -111,6 +111,8 @@ 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;
> +
>  /* If the read fails, then the machine doesn't support PC8+ residencies. */
>  static bool supports_pc8_plus_residencies(void)
>  {
> @@ -677,25 +679,7 @@ static void setup_pc8(void)
>   * anything, just try to help with the more common problems. */
>  static void setup_non_graphics_runtime_pm(void)
>  {
> -       int fd, i;
> -       char *file_name;
> -
> -       /* Disk runtime PM policies. */
> -       file_name = malloc(PATH_MAX);
> -       for (i = 0; ; i++) {
> -
> -               snprintf(file_name, PATH_MAX,
> -                        "/sys/class/scsi_host/host%d/link_power_management_policy",
> -                        i);
> -
> -               fd = open(file_name, O_WRONLY);
> -               if (fd < 0)
> -                       break;
> -
> -               igt_assert(write(fd, "min_power\n", 10) == 10);
> -               close(fd);
> -       }
> -       free(file_name);
> +       pm_data = igt_pm_enable_sata_link_power_management();

Could this call be moved into setup_environment and then this function
(and comment above) be removed?


>  }
>
>  static void setup_environment(void)
> @@ -713,11 +697,16 @@ static void setup_environment(void)
>         igt_info("PC8 residency support: %d\n", has_pc8);
>
>         igt_require(has_runtime_pm);
> +}
>
> +static void restore_environment(void)
> +{
> +       igt_pm_restore_sata_link_power_management(pm_data);
>  }
>
>  static void teardown_environment(void)
>  {
> +       restore_environment();

igt_pm_restore_sata_link_power_management could be called directly here.

Also, pm_data is not freed.


>         fini_mode_set_data(&ms_data);
>         drmClose(drm_fd);
>         close(msr_fd);
> --
> 2.6.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 0/1] Add a lib for power management helpers

[David Weinehall]

This patch aims to create a separate lib for power management related
helpers. Initially it only contains code that modify settings for
external components (to handle components with default settings that
prevents entering deeper sleep states), but moving i915-related
power management helpers to this lib would probably make sense too.

v2: Change name of library to igt_pm
    Namespace all exported functions with igt_pm_

v3: Include igt_pm.xml in intel-gpu-tools-docs.xml
    Free pm_data
    Fixed a few typos

David Weinehall (1):
  lib/igt_pm: Lib for power management

 .../intel-gpu-tools/intel-gpu-tools-docs.xml       |   1 +
 lib/Makefile.sources                               |   2 +
 lib/igt.h                                          |   1 +
 lib/igt_aux.c                                      |  15 +-
 lib/igt_pm.c                                       | 233 +++++++++++++++++++++
 lib/igt_pm.h                                       |  31 +++
 tests/pm_lpsp.c                                    |  25 +--
 tests/pm_rpm.c                                     |  40 +---
 8 files changed, 281 insertions(+), 67 deletions(-)
 create mode 100644 lib/igt_pm.c
 create mode 100644 lib/igt_pm.h

-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 1/1] lib/igt_pm: Lib for power management

[David Weinehall]

Move power management related code to a separate library.
Initially this is done only for workarounds that apply to external
components.  Modify the users of such workarounds accordingly.
This currently involves HD audio and SATA link power management.
For SATA link PM there's also code to save the previous settings,
to allow for resetting the values after we've finished testing.

Signed-off-by: David Weinehall <david.weinehall@intel.com>
---
 .../intel-gpu-tools/intel-gpu-tools-docs.xml       |   1 +
 lib/Makefile.sources                               |   2 +
 lib/igt.h                                          |   1 +
 lib/igt_aux.c                                      |  15 +-
 lib/igt_pm.c                                       | 233 +++++++++++++++++++++
 lib/igt_pm.h                                       |  31 +++
 tests/pm_lpsp.c                                    |  25 +--
 tests/pm_rpm.c                                     |  40 +---
 8 files changed, 281 insertions(+), 67 deletions(-)
 create mode 100644 lib/igt_pm.c
 create mode 100644 lib/igt_pm.h

diff --git a/docs/reference/intel-gpu-tools/intel-gpu-tools-docs.xml b/docs/reference/intel-gpu-tools/intel-gpu-tools-docs.xml
index 618dc5fd7076..3ea3563a029a 100644
--- a/docs/reference/intel-gpu-tools/intel-gpu-tools-docs.xml
+++ b/docs/reference/intel-gpu-tools/intel-gpu-tools-docs.xml
@@ -24,6 +24,7 @@
     <xi:include href="xml/igt_fb.xml"/>
     <xi:include href="xml/igt_aux.xml"/>
     <xi:include href="xml/igt_gt.xml"/>
+    <xi:include href="xml/igt_pm.xml"/>
     <xi:include href="xml/ioctl_wrappers.xml"/>
     <xi:include href="xml/intel_batchbuffer.xml"/>
     <xi:include href="xml/intel_chipset.xml"/>
diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index 4999868052b1..2f0eb2075e14 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -60,6 +60,8 @@ libintel_tools_la_SOURCES = 	\
 	igt_core.h		\
 	igt_draw.c		\
 	igt_draw.h		\
+	igt_pm.c	\
+	igt_pm.h	\
 	$(NULL)
 
 .PHONY: version.h.tmp
diff --git a/lib/igt.h b/lib/igt.h
index 3be25511bb77..d751f2439de2 100644
--- a/lib/igt.h
+++ b/lib/igt.h
@@ -35,6 +35,7 @@
 #include "igt_fb.h"
 #include "igt_gt.h"
 #include "igt_kms.h"
+#include "igt_pm.h"
 #include "igt_stats.h"
 #include "instdone.h"
 #include "intel_batchbuffer.h"
diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index ebee119c411d..7d35666eb7f3 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -59,6 +59,7 @@
 #include "intel_reg.h"
 #include "ioctl_wrappers.h"
 #include "igt_kms.h"
+#include "igt_pm.h"
 
 /**
  * SECTION:igt_aux
@@ -544,19 +545,7 @@ bool igt_setup_runtime_pm(void)
 	if (pm_status_fd >= 0)
 		return true;
 
-	/* The Audio driver can get runtime PM references, so we need to make
-	 * sure its runtime PM is enabled, so it can release the refs and
-	 * actually enable us to runtime suspend. */
-	fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert(write(fd, "1\n", 2) == 2);
-		close(fd);
-	}
-	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert(write(fd, "auto\n", 5) == 5);
-		close(fd);
-	}
+	igt_pm_enable_audio_runtime_pm();
 
 	/* Our implementation uses autosuspend. Try to set it to 0ms so the test
 	 * suite goes faster and we have a higher probability of triggering race
diff --git a/lib/igt_pm.c b/lib/igt_pm.c
new file mode 100644
index 000000000000..2f574961d179
--- /dev/null
+++ b/lib/igt_pm.c
@@ -0,0 +1,233 @@
+/*
+ * Copyright © 2013, 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Paulo Zanoni <paulo.r.zanoni@intel.com>
+ *    David Weinehall <david.weinehall@intel.com>
+ *
+ */
+#include <fcntl.h>
+#include <stdio.h>
+#include <limits.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+#include "drmtest.h"
+#include "igt_pm.h"
+
+enum {
+	POLICY_UNKNOWN = -1,
+	POLICY_MAX_PERFORMANCE = 0,
+	POLICY_MEDIUM_POWER = 1,
+	POLICY_MIN_POWER = 2
+};
+
+#define MAX_PERFORMANCE_STR	"max_performance\n"
+#define MEDIUM_POWER_STR	"medium_power\n"
+#define MIN_POWER_STR		"min_power\n"
+/* Remember to fix this if adding longer strings */
+#define MAX_POLICY_STRLEN	strlen(MAX_PERFORMANCE_STR)
+
+/**
+ * SECTION:igt_pm
+ * @short_description: Power Management related helpers
+ * @title: Power Management
+ * @include: igt.h
+ *
+ * This library provides various helpers to enable power management for,
+ * and in some cases subsequently allow restoring the old behaviour of,
+ * various external components that by default are set up in a way
+ * that interferes with the testing of our power management functionality.
+ */
+/**
+ * igt_pm_enable_audio_runtime_pm:
+ *
+ * We know that if we don't enable audio runtime PM, snd_hda_intel will never
+ * release its power well refcount, and we'll never reach the LPSP sate.
+ * There's no guarantee that it will release the power well if we enable
+ * runtime PM, but at least we can try.
+ *
+ * We don't have any assertions on open since the user may not even have
+ * snd_hda_intel loaded, which is not a problem.
+ */
+void igt_pm_enable_audio_runtime_pm(void)
+{
+	int fd;
+
+	fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
+	if (fd >= 0) {
+		igt_assert_eq(write(fd, "1\n", 2), 2);
+		close(fd);
+	}
+	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
+	if (fd >= 0) {
+		igt_assert_eq(write(fd, "auto\n", 5), 5);
+		close(fd);
+	}
+	/* Give some time for it to react. */
+	sleep(1);
+}
+
+/**
+ * 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)
+{
+	int fd, i;
+	ssize_t len;
+	char *buf;
+	char *file_name;
+	int8_t *link_pm_policies = NULL;
+
+	file_name = malloc(PATH_MAX);
+	buf = malloc(MAX_POLICY_STRLEN + 1);
+
+	for (i = 0; ; i++) {
+		int8_t policy;
+
+		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;
+
+		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;
+
+		if (!(i % 256))
+			link_pm_policies = realloc(link_pm_policies,
+						   (i / 256 + 1) * 256 + 1);
+
+		link_pm_policies[i] = policy;
+		link_pm_policies[i + 1] = 0;
+
+		/* If the policy is something we don't know about,
+		 * don't touch it, since we might potentially break things.
+		 * And we obviously don't need to touch anything if the
+		 * setting is already correct...
+		 */
+		if (policy != POLICY_UNKNOWN &&
+		    policy != POLICY_MIN_POWER) {
+			lseek(fd, 0, SEEK_SET);
+			igt_assert_eq(write(fd, MIN_POWER_STR,
+					    strlen(MIN_POWER_STR)),
+				      strlen(MIN_POWER_STR));
+		}
+		close(fd);
+	}
+	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)
+{
+	int fd, i;
+	char *file_name;
+
+	/* Disk runtime PM policies. */
+	file_name = malloc(PATH_MAX);
+	for (i = 0; ; i++) {
+		int8_t policy;
+
+		if (!pm_data)
+			policy = POLICY_MAX_PERFORMANCE;
+		else if (pm_data[i] == POLICY_UNKNOWN)
+			continue;
+		else
+			policy = pm_data[i];
+
+		snprintf(file_name, PATH_MAX,
+			 "/sys/class/scsi_host/host%d/link_power_management_policy",
+			 i);
+
+		fd = open(file_name, O_WRONLY);
+		if (fd < 0)
+			break;
+
+		switch (policy) {
+		default:
+		case POLICY_MAX_PERFORMANCE:
+			igt_assert_eq(write(fd, MAX_PERFORMANCE_STR,
+					    strlen(MAX_PERFORMANCE_STR)),
+				      strlen(MAX_PERFORMANCE_STR));
+			break;
+
+		case POLICY_MEDIUM_POWER:
+			igt_assert_eq(write(fd, MEDIUM_POWER_STR,
+					    strlen(MEDIUM_POWER_STR)),
+				      strlen(MEDIUM_POWER_STR));
+			break;
+
+		case POLICY_MIN_POWER:
+			igt_assert_eq(write(fd, MIN_POWER_STR,
+					    strlen(MIN_POWER_STR)),
+				      strlen(MIN_POWER_STR));
+			break;
+		}
+
+		close(fd);
+	}
+	free(file_name);
+}
diff --git a/lib/igt_pm.h b/lib/igt_pm.h
new file mode 100644
index 000000000000..c14ff1f7a0ef
--- /dev/null
+++ b/lib/igt_pm.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef IGT_PM_H
+#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);
+
+#endif /* IGT_PM_H */
diff --git a/tests/pm_lpsp.c b/tests/pm_lpsp.c
index a82420bf06de..4cedefffb545 100644
--- a/tests/pm_lpsp.c
+++ b/tests/pm_lpsp.c
@@ -31,29 +31,6 @@
 #include <unistd.h>
 
 
-/* We know that if we don't enable audio runtime PM, snd_hda_intel will never
- * release its power well refcount, and we'll never reach the LPSP sate. OTOH
- * there's no guarantee that it will release the power well if we enable runtime
- * PM, but at least we can try.  We don't have any assertions since the user may
- * not even have snd_hda_intel loaded, which is not a problem. */
-static void disable_audio_runtime_pm(void)
-{
-	int fd;
-
-	fd = open("/sys/module/snd_hda_intel/parameters/power_save", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert_eq(write(fd, "1\n", 2), 2);
-		close(fd);
-	}
-	fd = open("/sys/bus/pci/devices/0000:00:03.0/power/control", O_WRONLY);
-	if (fd >= 0) {
-		igt_assert_eq(write(fd, "auto\n", 5), 5);
-		close(fd);
-	}
-	/* Give some time for it to react. */
-	sleep(1);
-}
-
 static bool supports_lpsp(uint32_t devid)
 {
 	return IS_HASWELL(devid) || IS_BROADWELL(devid);
@@ -227,7 +204,7 @@ igt_main
 			drm_connectors[i] = drmModeGetConnectorCurrent(drm_fd,
 							drm_res->connectors[i]);
 
-		disable_audio_runtime_pm();
+		igt_pm_enable_audio_runtime_pm();
 
 		igt_require(supports_lpsp(devid));
 
diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 22dc2b4d6de4..2aa6c1018aa2 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2013 Intel Corporation
+ * Copyright © 2013, 2015 Intel Corporation
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
@@ -109,6 +109,8 @@ 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;
+
 /* If the read fails, then the machine doesn't support PC8+ residencies. */
 static bool supports_pc8_plus_residencies(void)
 {
@@ -685,41 +687,13 @@ static void setup_pc8(void)
 	has_pc8 = true;
 }
 
-/* If we want to actually reach PC8+ states, we need to properly configure all
- * the devices on the system to allow this. This function will try to setup the
- * things we know we need, but won't scream in case anything fails: we don't
- * know which devices are present on your machine, so we can't really expect
- * anything, just try to help with the more common problems. */
-static void setup_non_graphics_runtime_pm(void)
-{
-	int fd, i;
-	char *file_name;
-
-	/* Disk runtime PM policies. */
-	file_name = malloc(PATH_MAX);
-	for (i = 0; ; i++) {
-
-		snprintf(file_name, PATH_MAX,
-			 "/sys/class/scsi_host/host%d/link_power_management_policy",
-			 i);
-
-		fd = open(file_name, O_WRONLY);
-		if (fd < 0)
-			break;
-
-		igt_assert(write(fd, "min_power\n", 10) == 10);
-		close(fd);
-	}
-	free(file_name);
-}
-
 static void setup_environment(void)
 {
 	drm_fd = drm_open_driver_master(DRIVER_INTEL);
 
 	init_mode_set_data(&ms_data);
 
-	setup_non_graphics_runtime_pm();
+	pm_data = igt_pm_enable_sata_link_power_management();
 
 	has_runtime_pm = igt_setup_runtime_pm();
 	setup_pc8();
@@ -728,11 +702,17 @@ static void setup_environment(void)
 	igt_info("PC8 residency support: %d\n", has_pc8);
 
 	igt_require(has_runtime_pm);
+}
 
+static void restore_environment(void)
+{
+	igt_pm_restore_sata_link_power_management(pm_data);
+	free(pm_data);
 }
 
 static void teardown_environment(void)
 {
+	restore_environment();
 	fini_mode_set_data(&ms_data);
 	drmClose(drm_fd);
 	close(msr_fd);
-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/1] Add a lib for power management helpers

[Daniel Vetter]

On Thu, Feb 11, 2016 at 09:25:35AM +0200, David Weinehall wrote:
> This patch aims to create a separate lib for power management related
> helpers. Initially it only contains code that modify settings for
> external components (to handle components with default settings that
> prevents entering deeper sleep states), but moving i915-related
> power management helpers to this lib would probably make sense too.
> 
> v2: Change name of library to igt_pm
>     Namespace all exported functions with igt_pm_
> 
> v3: Include igt_pm.xml in intel-gpu-tools-docs.xml
>     Free pm_data
>     Fixed a few typos
> 
> David Weinehall (1):
>   lib/igt_pm: Lib for power management

When resending anew complete please don't in-reply-to but start a new
thread. In-reply-to is just for new versions of single patches to the
existing series.

Wrt the patch: If Paulo is ok, just push. lgmt.
-Daniel

> 
>  .../intel-gpu-tools/intel-gpu-tools-docs.xml       |   1 +
>  lib/Makefile.sources                               |   2 +
>  lib/igt.h                                          |   1 +
>  lib/igt_aux.c                                      |  15 +-
>  lib/igt_pm.c                                       | 233 +++++++++++++++++++++
>  lib/igt_pm.h                                       |  31 +++
>  tests/pm_lpsp.c                                    |  25 +--
>  tests/pm_rpm.c                                     |  40 +---
>  8 files changed, 281 insertions(+), 67 deletions(-)
>  create mode 100644 lib/igt_pm.c
>  create mode 100644 lib/igt_pm.h
> 
> -- 
> 2.7.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx