On 15 December 2015 at 09:14, David Weinehall
<david.weinehall@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> + * David Weinehall <david.weinehall@xxxxxxxxx>
> + *
> + */
> +#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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
