[igt-dev] [PATCH i-g-t v14 1/6] lib/igt_pm: igt lib helper routines to support DC5/6 tests
Imre Deak
imre.deak at intel.com
Tue Sep 17 16:55:48 UTC 2019
On Fri, Sep 06, 2019 at 07:35:23PM +0530, Anshuman Gupta wrote:
> From: Jyoti Yadav <jyoti.r.yadav at intel.com>
>
> This patch does the following changes to lib/igt_pm.c
>
> -dmc_loaded() will be used by new test i915_pm_dc.c which will validate
> Display C States. So moving the same to igt_pm library.
>
> -Introduced igt_disable_runtime_pm() in order to disable runtime suspend
> for the function which support dc9.
>
> -Changed the igt_pm_enable_sata_link_power_management() and
> igt_pm_restore_sata_link_power_management() in order to save
> and restore the sata link power policy by an exit handler.
>
> v2: Simplify the comment section.
> v3: Remove . from the subject line.
> v4: Rebased, resolve conflicts in pm_rpm.c
> Included patch set version change log.
> v5: Listing actual change in patch set changelog to make review easier.
> v6: igt's lib added support for disabling runtime suspend,
> change in commit log. rebased due to test name pm_rpm changed
> to i915_pm_rpm.
> v7: Addressed review comment by saving POWER_DIR values in
> igt_disable_runtime_pm(). [Imre]
> v8: Addressed the review comment, igt_pm_enable_sata_link_power_management
> function to restore the original SATA link power policy if things fail
> by using an exit handler. [Imre]
> v9: IGT failure fixture in i915_pm_backlight and i915_pm_rpm.
> v10:Review comment fixup in sata_link_power_management
> lib functions. [Imre]
> v11:Add igt_assert_fd(pm_status_fd) in igt_disable_runtime_pm().
> [Imre & Petri]
> v12: Refactor is_bios_limits_pc8_plus_residencies() from
> supports_pc8_plus_residencies().
> Changed igt_disable_runtime_pm()return type. [Imre]
>
> Signed-off-by: Jyoti Yadav <jyoti.r.yadav at intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> ---
> lib/igt_pm.c | 215 ++++++++++++++++++++++++++-------
> lib/igt_pm.h | 7 +-
> tests/i915/i915_pm_backlight.c | 6 +-
> tests/i915/i915_pm_rpm.c | 39 +-----
> 4 files changed, 179 insertions(+), 88 deletions(-)
>
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index fd22273a..540a9f92 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -38,6 +38,7 @@
> #include "drmtest.h"
> #include "igt_pm.h"
> #include "igt_aux.h"
> +#include "igt_sysfs.h"
>
> /**
> * SECTION:igt_pm
> @@ -58,16 +59,30 @@ enum {
> POLICY_MIN_POWER = 2
> };
>
> +#define MSR_PKG_CST_CONFIG_CONTROL 0xE2
> +/*
> + * Below PKG CST limit mask and PC8 bits are meant for
> + * HSW,BDW SKL,ICL and Goldmont Microarch and future platforms.
> + * Refer IA S/W developers manual vol3c part3 chapter:35
> + */
> +#define PKG_CST_LIMIT_MASK 0xF
> +#define PKG_CST_LIMIT_C8 0x6
> +
> #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)
> +int8_t *__sata_pm_policies;
> +int __scsi_host_cnt;
>
> static char __igt_pm_audio_runtime_power_save[64];
> static char * __igt_pm_audio_runtime_control_path;
> static char __igt_pm_audio_runtime_control[64];
>
> +static void __igt_pm_sata_link_pm_exit_handler(int sig);
> +static void __igt_pm_restore_sata_link_power_management(void);
> +
> static int __igt_pm_audio_restore_runtime_pm(void)
> {
> int fd;
> @@ -280,39 +295,26 @@ void igt_pm_enable_audio_runtime_pm(void)
> igt_debug("Failed to enable audio runtime PM! (%d)\n", -err);
> }
>
> -/**
> - * igt_pm_enable_sata_link_power_management:
> - *
> - * Enable the min_power policy for SATA link power management.
> - * Without this we cannot reach deep runtime power states.
> - *
> - * We don't have any assertions on open since the system might not have
> - * a SATA host.
> - *
> - * Returns:
> - * An opaque pointer to the data needed to restore the default values
> - * after the test has terminated, or NULL if SATA link power management
> - * is not supported. This pointer should be freed when no longer used
> - * (typically after having called restore_sata_link_power_management()).
> - */
> -int8_t *igt_pm_enable_sata_link_power_management(void)
> +static void __igt_pm_enable_sata_link_power_management(void)
> {
> int fd, i;
> ssize_t len;
> char *buf;
> char *file_name;
> - int8_t *link_pm_policies = NULL;
> + int8_t policy;
>
> file_name = malloc(PATH_MAX);
> buf = malloc(MAX_POLICY_STRLEN + 1);
>
> - for (i = 0; ; i++) {
> - int8_t policy;
> -
> + for (__scsi_host_cnt = 0; ; __scsi_host_cnt++) {
> snprintf(file_name, PATH_MAX,
> "/sys/class/scsi_host/host%d/link_power_management_policy",
> - i);
> + __scsi_host_cnt);
>
> + /*
> + * We don't have any assertions on open since the system
> + * might not have a SATA host.
> + */
> fd = open(file_name, O_RDWR);
> if (fd < 0)
> break;
> @@ -332,12 +334,26 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
> else
> policy = POLICY_UNKNOWN;
>
> - if (!(i % 256))
> - link_pm_policies = realloc(link_pm_policies,
> - (i / 256 + 1) * 256 + 1);
> + if (!(__scsi_host_cnt % 256))
> + __sata_pm_policies = realloc(__sata_pm_policies,
> + (__scsi_host_cnt / 256 + 1)
> + * 256 + 1);
> +
> + __sata_pm_policies[__scsi_host_cnt] = policy;
> + close(fd);
> + }
> +
> + igt_install_exit_handler(__igt_pm_sata_link_pm_exit_handler);
> +
> + for (i = 0; i < __scsi_host_cnt; i++) {
> + snprintf(file_name, PATH_MAX,
> + "/sys/class/scsi_host/host%d/link_power_management_policy",
> + i);
> + fd = open(file_name, O_RDWR);
> + if (fd < 0)
> + break;
>
> - link_pm_policies[i] = policy;
> - link_pm_policies[i + 1] = 0;
> + policy = __sata_pm_policies[i];
>
> /* If the policy is something we don't know about,
> * don't touch it, since we might potentially break things.
> @@ -355,39 +371,25 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
> }
> free(buf);
> free(file_name);
> -
> - return link_pm_policies;
> }
>
> -/**
> - * igt_pm_restore_sata_link_power_management:
> - * @pm_data: An opaque pointer with saved link PM policies;
> - * If NULL is passed we force enable the "max_performance" policy.
> - *
> - * Restore the link power management policies to the values
> - * prior to enabling min_power.
> - *
> - * Caveat: If the system supports hotplugging and hotplugging takes
> - * place during our testing so that the hosts change numbers
> - * we might restore the settings to the wrong hosts.
> - */
> -void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
> -
> +static void __igt_pm_restore_sata_link_power_management(void)
> {
> int fd, i;
> char *file_name;
>
> + if (!__sata_pm_policies)
> + return;
> +
> /* Disk runtime PM policies. */
> file_name = malloc(PATH_MAX);
> - for (i = 0; ; i++) {
> + for (i = 0; i < __scsi_host_cnt; i++) {
> int8_t policy;
>
> - if (!pm_data)
> - policy = POLICY_MAX_PERFORMANCE;
> - else if (pm_data[i] == POLICY_UNKNOWN)
> + if (__sata_pm_policies[i] == POLICY_UNKNOWN)
> continue;
> else
> - policy = pm_data[i];
> + policy = __sata_pm_policies[i];
>
> snprintf(file_name, PATH_MAX,
> "/sys/class/scsi_host/host%d/link_power_management_policy",
> @@ -421,7 +423,48 @@ void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
> close(fd);
> }
> free(file_name);
> + free(__sata_pm_policies);
> + __sata_pm_policies = NULL;
> }
> +
> +/**
> + * igt_pm_enable_sata_link_power_management:
> + *
> + * Enable the min_power policy for SATA link power management.
> + * Without this we cannot reach deep runtime power states.
> + */
> +void igt_pm_enable_sata_link_power_management(void)
> +{
> + /* Check if has been already saved. */
> + if (__sata_pm_policies)
> + return;
> +
> + __igt_pm_enable_sata_link_power_management();
> +}
> +
> +/**
> + * igt_pm_restore_sata_link_power_management:
> + *
> + * Restore the link power management policies to the values
> + * prior to enabling min_power.
> + *
> + * Caveat: If the system supports hotplugging and hotplugging takes
> + * place during our testing so that the hosts change numbers
> + * we might restore the settings to the wrong hosts.
> + */
> +void igt_pm_restore_sata_link_power_management(void)
> +{
> + if (!__sata_pm_policies)
> + return;
> +
> + __igt_pm_restore_sata_link_power_management();
> +}
> +
> +static void __igt_pm_sata_link_pm_exit_handler(int sig)
> +{
> + __igt_pm_restore_sata_link_power_management();
> +}
> +
> #define POWER_DIR "/sys/devices/pci0000:00/0000:00:02.0/power"
> /* We just leak this on exit ... */
> int pm_status_fd = -1;
> @@ -585,6 +628,34 @@ bool igt_setup_runtime_pm(void)
> return true;
> }
>
> +/**
> + * igt_disable_runtime_pm:
> + *
> + * Disable the runtime pm for i915 device.
> + * igt_disable_runtime_pm assumes that igt_setup_runtime_pm has already
> + * called to save runtime autosuspend and control attributes.
> + */
> +void igt_disable_runtime_pm(void)
> +{
> + int fd;
> + ssize_t size;
> + char buf[6];
> +
> + igt_assert_fd(pm_status_fd);
> +
> + /* We know we support runtime PM, let's try to disable it now. */
> + fd = open(POWER_DIR "/control", O_RDWR);
> + igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
> +
> + size = write(fd, "on\n", 3);
> + igt_assert(size == 3);
> + lseek(fd, 0, SEEK_SET);
> + size = read(fd, buf, ARRAY_SIZE(buf));
> + igt_assert(size == 3);
> + igt_assert(strncmp(buf, "on\n", 3) == 0);
> + close(fd);
> +}
> +
> /**
> * igt_get_runtime_pm_status:
> *
> @@ -628,3 +699,53 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
> {
> return igt_wait(igt_get_runtime_pm_status() == status, 10000, 100);
> }
> +
> +/**
> + * dmc_loaded:
> + * @debugfs: fd to the debugfs dir.
> +
The DocBook header is misformatted.
> + * Check whether DMC FW is loaded or not. DMC FW is require for few Display C
> + * states like DC5 and DC6. FW does the Context Save and Restore during Display
> + * C States entry and exit.
> + *
> + * Returns:
> + * True if DMC FW is loaded otherwise false.
> + */
> +bool igt_pm_dmc_loaded(int debugfs)
> +{
> + char buf[15];
> + int len;
> +
> + len = igt_sysfs_read(debugfs, "i915_dmc_info", buf, sizeof(buf) - 1);
> + if (len < 0)
> + return true; /* no CSR support, no DMC requirement */
> +
> + buf[len] = '\0';
> +
> + igt_info("DMC: %s\n", buf);
> + return strstr(buf, "fw loaded: yes");
> +}
> +
> +/**
> + * is_bios_limits_pc8_plus_residencies:
For library functions we need a proper prefix and I think something like
igt_pm_pc8_plus_residencies_disabled
would be more descriptive.
> +
Misformatted DocBook header.
With the above fixed, on the whole series:
Reviewed-by: Imre Deak <imre.deak at intel.com>
Note that the new tests will fail consistenctly on a bunch of machines,
but those failures are most probably caused by another device blocking a
low-power state (and thus preventing DC6 entry/exit). Imo we should
still merge these patches since we want the coverage on machines w/o
such problematic devices and we could follow up trying to enable the low
power states for other devices too later (for instance by tweaking the
corresponding BIOS power setup settings).
> + * Check whether BIOS has disabled the PC8 package deeper state.
> + *
> + * Returns:
> + * True if PC8+ package deeper state enabled on machine otherwise false.
> + */
> +bool is_bios_limits_pc8_plus_residencies(int msr_fd)
> +{
> + int rc;
> + uint64_t val;
> +
> + rc = pread(msr_fd, &val, sizeof(uint64_t), MSR_PKG_CST_CONFIG_CONTROL);
> + if (rc != sizeof(val))
> + return false;
> + if ((val & PKG_CST_LIMIT_MASK) < PKG_CST_LIMIT_C8) {
> + igt_info("PKG C-states limited below PC8 by the BIOS\n");
> + return false;
> + }
> +
> + return true;
> +}
> diff --git a/lib/igt_pm.h b/lib/igt_pm.h
> index 10cc6794..f4d6c496 100644
> --- a/lib/igt_pm.h
> +++ b/lib/igt_pm.h
> @@ -25,8 +25,8 @@
> #define IGT_PM_H
>
> void igt_pm_enable_audio_runtime_pm(void);
> -int8_t *igt_pm_enable_sata_link_power_management(void);
> -void igt_pm_restore_sata_link_power_management(int8_t *pm_data);
> +void igt_pm_enable_sata_link_power_management(void);
> +void igt_pm_restore_sata_link_power_management(void);
>
> /**
> * igt_runtime_pm_status:
> @@ -47,8 +47,11 @@ enum igt_runtime_pm_status {
> };
>
> bool igt_setup_runtime_pm(void);
> +void igt_disable_runtime_pm(void);
> void igt_restore_runtime_pm(void);
> enum igt_runtime_pm_status igt_get_runtime_pm_status(void);
> bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
> +bool igt_pm_dmc_loaded(int debugfs);
> +bool is_bios_limits_pc8_plus_residencies(int msr_fd);
>
> #endif /* IGT_PM_H */
> diff --git a/tests/i915/i915_pm_backlight.c b/tests/i915/i915_pm_backlight.c
> index 4c1bff5b..9a5f4c37 100644
> --- a/tests/i915/i915_pm_backlight.c
> +++ b/tests/i915/i915_pm_backlight.c
> @@ -47,7 +47,6 @@ struct context {
> #define FADESPEED 100 /* milliseconds between steps */
>
> IGT_TEST_DESCRIPTION("Basic backlight sysfs test");
> -static int8_t *pm_data = NULL;
>
> static int backlight_read(int *result, const char *fname)
> {
> @@ -235,7 +234,7 @@ igt_main
> igt_plane_set_fb(primary, &fb);
>
> igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> - pm_data = igt_pm_enable_sata_link_power_management();
> + igt_pm_enable_sata_link_power_management();
> }
>
> igt_subtest("basic-brightness")
> @@ -255,8 +254,7 @@ igt_main
>
> igt_display_fini(&display);
> igt_remove_fb(display.drm_fd, &fb);
> - igt_pm_restore_sata_link_power_management(pm_data);
> - free(pm_data);
> + igt_pm_restore_sata_link_power_management();
> close(display.drm_fd);
> }
> }
> diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> index 2168ff72..d0a0bc90 100644
> --- a/tests/i915/i915_pm_rpm.c
> +++ b/tests/i915/i915_pm_rpm.c
> @@ -52,11 +52,6 @@
> #include "igt_device.h"
> #include "igt_edid.h"
>
> -#define MSR_PKG_CST_CONFIG_CONTROL 0xE2
> -/* HSW/BDW: */
> -#define PKG_CST_LIMIT_MASK 0xF
> -#define PKG_CST_LIMIT_C8 0x6
> -
> #define MSR_PC8_RES 0x630
> #define MSR_PC9_RES 0x631
> #define MSR_PC10_RES 0x632
> @@ -123,8 +118,6 @@ struct modeset_params lpsp_mode_params;
> struct modeset_params non_lpsp_mode_params;
> struct modeset_params *default_mode_params;
>
> -static int8_t *pm_data = NULL;
> -
> static int modprobe(const char *driver)
> {
> return igt_kmod_load(driver, NULL);
> @@ -146,15 +139,7 @@ static bool supports_pc8_plus_residencies(void)
> if (rc != sizeof(val))
> return false;
>
> - rc = pread(msr_fd, &val, sizeof(uint64_t), MSR_PKG_CST_CONFIG_CONTROL);
> - if (rc != sizeof(val))
> - return false;
> - if ((val & PKG_CST_LIMIT_MASK) < PKG_CST_LIMIT_C8) {
> - igt_info("PKG C-states limited below PC8 by the BIOS\n");
> - return false;
> - }
> -
> - return true;
> + return is_bios_limits_pc8_plus_residencies(msr_fd);
> }
>
> static uint64_t get_residency(uint32_t type)
> @@ -755,21 +740,6 @@ static void setup_pc8(void)
> has_pc8 = true;
> }
>
> -static bool dmc_loaded(void)
> -{
> - char buf[15];
> - int len;
> -
> - len = igt_sysfs_read(debugfs, "i915_dmc_info", buf, sizeof(buf) - 1);
> - if (len < 0)
> - return true; /* no CSR support, no DMC requirement */
> -
> - buf[len] = '\0';
> -
> - igt_info("DMC: %s\n", buf);
> - return strstr(buf, "fw loaded: yes");
> -}
> -
> static void dump_file(int dir, const char *filename)
> {
> char *contents;
> @@ -796,7 +766,7 @@ static bool setup_environment(void)
>
> init_mode_set_data(&ms_data);
>
> - pm_data = igt_pm_enable_sata_link_power_management();
> + igt_pm_enable_sata_link_power_management();
>
> has_runtime_pm = igt_setup_runtime_pm();
> setup_pc8();
> @@ -804,7 +774,7 @@ static bool setup_environment(void)
> igt_info("Runtime PM support: %d\n", has_runtime_pm);
> igt_info("PC8 residency support: %d\n", has_pc8);
> igt_require(has_runtime_pm);
> - igt_require(dmc_loaded());
> + igt_require(igt_pm_dmc_loaded(debugfs));
>
> out:
> disable_all_screens(&ms_data);
> @@ -821,8 +791,7 @@ static void teardown_environment(void)
>
> igt_restore_runtime_pm();
>
> - igt_pm_restore_sata_link_power_management(pm_data);
> - free(pm_data);
> + igt_pm_restore_sata_link_power_management();
>
> fini_mode_set_data(&ms_data);
>
> --
> 2.21.0
>
More information about the igt-dev
mailing list