Re: [igt-dev] [PATCH i-g-t v12 1/6] lib/igt_pm: igt lib helper routines to support DC5/6 tests

[Imre Deak <imre.deak@intel.com>]

On Fri, Aug 16, 2019 at 03:57:34PM +0530, Anshuman Gupta wrote:
> From: Jyoti Yadav <jyoti.r.yadav@intel.com>
> 
> This patch does the following chnegs 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]
> 
> Signed-off-by: Jyoti Yadav <jyoti.r.yadav@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  lib/igt_pm.c                   | 210 +++++++++++++++++++++++++--------
>  lib/igt_pm.h                   |   6 +-
>  tests/i915/i915_pm_backlight.c |   6 +-
>  tests/i915/i915_pm_rpm.c       |  24 +---
>  4 files changed, 172 insertions(+), 74 deletions(-)
> 
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index fd22273a..b91a0f96 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
> @@ -63,11 +64,16 @@ enum {
>  #define MIN_POWER_STR		"min_power\n"
>  /* Remember to fix this if adding longer strings */
>  #define MAX_POLICY_STRLEN	strlen(MAX_PERFORMANCE_STR)
> +int8_t *__sata_pm_policies;
> +int __scsi_host_cnt;
>  
>  static char __igt_pm_audio_runtime_power_save[64];
>  static char * __igt_pm_audio_runtime_control_path;
>  static char __igt_pm_audio_runtime_control[64];
>  
> +static void __igt_pm_sata_link_pm_exit_handler(int sig);
> +static void __igt_pm_restore_sata_link_power_management(void);
> +
>  static int __igt_pm_audio_restore_runtime_pm(void)
>  {
>  	int fd;
> @@ -280,39 +286,26 @@ void igt_pm_enable_audio_runtime_pm(void)
>  		igt_debug("Failed to enable audio runtime PM! (%d)\n", -err);
>  }
>  
> -/**
> - * igt_pm_enable_sata_link_power_management:
> - *
> - * Enable the min_power policy for SATA link power management.
> - * Without this we cannot reach deep runtime power states.
> - *
> - * We don't have any assertions on open since the system might not have
> - * a SATA host.
> - *
> - * Returns:
> - * An opaque pointer to the data needed to restore the default values
> - * after the test has terminated, or NULL if SATA link power management
> - * is not supported. This pointer should be freed when no longer used
> - * (typically after having called restore_sata_link_power_management()).
> - */
> -int8_t *igt_pm_enable_sata_link_power_management(void)
> +static void __igt_pm_enable_sata_link_power_management(void)
>  {
>  	int fd, i;
>  	ssize_t len;
>  	char *buf;
>  	char *file_name;
> -	int8_t *link_pm_policies = NULL;
> +	int8_t policy;
>  
>  	file_name = malloc(PATH_MAX);
>  	buf = malloc(MAX_POLICY_STRLEN + 1);
>  
> -	for (i = 0; ; i++) {
> -		int8_t policy;
> -
> +	for (__scsi_host_cnt = 0; ; __scsi_host_cnt++) {
>  		snprintf(file_name, PATH_MAX,
>  			 "/sys/class/scsi_host/host%d/link_power_management_policy",
> -			 i);
> +			 __scsi_host_cnt);
>  
> +		/*
> +		 * We don't have any assertions on open since the system
> +		 * might not have a SATA host.
> +		 */
>  		fd = open(file_name, O_RDWR);
>  		if (fd < 0)
>  			break;
> @@ -332,12 +325,26 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
>  		else
>  			policy = POLICY_UNKNOWN;
>  
> -		if (!(i % 256))
> -			link_pm_policies = realloc(link_pm_policies,
> -						   (i / 256 + 1) * 256 + 1);
> +		if (!(__scsi_host_cnt % 256))
> +			__sata_pm_policies = realloc(__sata_pm_policies,
> +						     (__scsi_host_cnt / 256 + 1)
> +						     * 256 + 1);
>  
> -		link_pm_policies[i] = policy;
> -		link_pm_policies[i + 1] = 0;
> +		__sata_pm_policies[__scsi_host_cnt] = policy;
> +		close(fd);
> +	}
> +
> +	igt_install_exit_handler(__igt_pm_sata_link_pm_exit_handler);
> +
> +	for (i = 0; i < __scsi_host_cnt; i++) {
> +		snprintf(file_name, PATH_MAX,
> +			 "/sys/class/scsi_host/host%d/link_power_management_policy",
> +			 i);
> +		fd = open(file_name, O_RDWR);
> +		if (fd < 0)
> +			break;
> +
> +		policy = __sata_pm_policies[i];
>  
>  		/* If the policy is something we don't know about,
>  		 * don't touch it, since we might potentially break things.
> @@ -355,39 +362,25 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
>  	}
>  	free(buf);
>  	free(file_name);
> -
> -	return link_pm_policies;
>  }
>  
> -/**
> - * igt_pm_restore_sata_link_power_management:
> - * @pm_data: An opaque pointer with saved link PM policies;
> - *           If NULL is passed we force enable the "max_performance" policy.
> - *
> - * Restore the link power management policies to the values
> - * prior to enabling min_power.
> - *
> - * Caveat: If the system supports hotplugging and hotplugging takes
> - *         place during our testing so that the hosts change numbers
> - *         we might restore the settings to the wrong hosts.
> - */
> -void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
> -
> +static void __igt_pm_restore_sata_link_power_management(void)
>  {
>  	int fd, i;
>  	char *file_name;
>  
> +	if (!__sata_pm_policies)
> +		return;
> +
>  	/* Disk runtime PM policies. */
>  	file_name = malloc(PATH_MAX);
> -	for (i = 0; ; i++) {
> +	for (i = 0; i < __scsi_host_cnt; i++) {
>  		int8_t policy;
>  
> -		if (!pm_data)
> -			policy = POLICY_MAX_PERFORMANCE;
> -		else if (pm_data[i] == POLICY_UNKNOWN)
> +		if (__sata_pm_policies[i] == POLICY_UNKNOWN)
>  			continue;
>  		else
> -			policy = pm_data[i];
> +			policy = __sata_pm_policies[i];
>  
>  		snprintf(file_name, PATH_MAX,
>  			 "/sys/class/scsi_host/host%d/link_power_management_policy",
> @@ -421,7 +414,49 @@ void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
>  		close(fd);
>  	}
>  	free(file_name);
> +	free(__sata_pm_policies);
> +	__sata_pm_policies = NULL;
>  }
> +
> +/**
> + * igt_pm_enable_sata_link_power_management:
> + *
> + * Enable the min_power policy for SATA link power management.
> + * Without this we cannot reach deep runtime power states.
> + * Returns: Void

No need for a comment on void returns.

> + */
> +void igt_pm_enable_sata_link_power_management(void)
> +{
> +	/* Check if has been already saved. */
> +	if (__sata_pm_policies)
> +		return;
> +
> +	 __igt_pm_enable_sata_link_power_management();
> +}
> +
> +/**
> + * igt_pm_restore_sata_link_power_management:
> + *
> + * Restore the link power management policies to the values
> + * prior to enabling min_power.
> + *
> + * Caveat: If the system supports hotplugging and hotplugging takes
> + *         place during our testing so that the hosts change numbers
> + *         we might restore the settings to the wrong hosts.
> + */
> +void igt_pm_restore_sata_link_power_management(void)
> +{
> +	if (!__sata_pm_policies)
> +		return;
> +
> +	 __igt_pm_restore_sata_link_power_management();
> +}
> +
> +static void __igt_pm_sata_link_pm_exit_handler(int sig)
> +{
> +	__igt_pm_restore_sata_link_power_management();
> +}
> +
>  #define POWER_DIR "/sys/devices/pci0000:00/0000:00:02.0/power"
>  /* We just leak this on exit ... */
>  int pm_status_fd = -1;
> @@ -585,6 +620,61 @@ bool igt_setup_runtime_pm(void)
>  	return true;
>  }
>  
> +bool igt_disable_runtime_pm(void)
> +{
> +	int fd;
> +	ssize_t size;
> +	char buf[6];
> +
> +	if (pm_status_fd < 0) {
> +		fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
> +		if (fd < 0)
> +			return false;
> +
> +		size = read(fd, __igt_pm_runtime_autosuspend,
> +			    sizeof(__igt_pm_runtime_autosuspend));
> +
> +		if (size <= 0) {
> +			close(fd);
> +			return false;
> +		}
> +
> +		strchomp(__igt_pm_runtime_autosuspend);
> +		igt_install_exit_handler(__igt_pm_runtime_exit_handler);
> +		close(fd);
> +	}
> +
> +	/* We know we support runtime PM, let's try to disable it now. */
> +	fd = open(POWER_DIR "/control", O_RDWR);
> +	igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
> +
> +	if (pm_status_fd < 0) {
> +		igt_assert(read(fd, __igt_pm_runtime_control,
> +				sizeof(__igt_pm_runtime_control)) > 0);
> +		strchomp(__igt_pm_runtime_control);
> +
> +		igt_debug("Saved runtime power management as '%s' and '%s'\n",
> +			  __igt_pm_runtime_autosuspend,
> +			  __igt_pm_runtime_control);
> +	}

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

> +
> +	size = write(fd, "on\n", 3);
> +	igt_assert(size == 3);
> +	lseek(fd, 0, SEEK_SET);
> +	size = read(fd, buf, ARRAY_SIZE(buf));
> +	igt_assert(size == 3);
> +	igt_assert(strncmp(buf, "on\n", 3) == 0);
> +	close(fd);
> +
> +	if (pm_status_fd < 0) {
> +		pm_status_fd = open(POWER_DIR "/runtime_status", O_RDONLY);
> +		igt_assert_f(pm_status_fd >= 0,
> +			     "Can't open " POWER_DIR "/runtime_status\n");
> +	}
> +
> +	return true;
> +}
> +
>  /**
>   * igt_get_runtime_pm_status:
>   *
> @@ -628,3 +718,29 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
>  {
>  	return igt_wait(igt_get_runtime_pm_status() == status, 10000, 100);
>  }
> +
> +/**
> + * dmc_loaded:
> + * @debugfs: fd to the debugfs dir.
> +
> + * Check whether DMC FW is loaded or not. DMC FW is require for few Display C
> + * states like DC5 and DC6. FW does the Context Save and Restore during Display
> + * C States entry and exit.
> + *
> + * Returns:
> + * True if DMC FW is loaded otherwise false.
> + */
> +bool igt_pm_dmc_loaded(int debugfs)
> +{
> +	char buf[15];
> +	int len;
> +
> +	len = igt_sysfs_read(debugfs, "i915_dmc_info", buf, sizeof(buf) - 1);
> +	if (len < 0)
> +		return true; /* no CSR support, no DMC requirement */
> +
> +	buf[len] = '\0';
> +
> +	igt_info("DMC: %s\n", buf);
> +	return strstr(buf, "fw loaded: yes");
> +}
> diff --git a/lib/igt_pm.h b/lib/igt_pm.h
> index 10cc6794..979b3463 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,10 @@ enum igt_runtime_pm_status {
>  };
>  
>  bool igt_setup_runtime_pm(void);
> +bool igt_disable_runtime_pm(void);
>  void igt_restore_runtime_pm(void);
>  enum igt_runtime_pm_status igt_get_runtime_pm_status(void);
>  bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
> +bool igt_pm_dmc_loaded(int debugfs);
>  
>  #endif /* IGT_PM_H */
> 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..6879c513 100644
> --- a/tests/i915/i915_pm_rpm.c
> +++ b/tests/i915/i915_pm_rpm.c
> @@ -123,8 +123,6 @@ struct modeset_params lpsp_mode_params;
>  struct modeset_params non_lpsp_mode_params;
>  struct modeset_params *default_mode_params;
>  
> -static int8_t *pm_data = NULL;
> -
>  static int modprobe(const char *driver)
>  {
>  	return igt_kmod_load(driver, NULL);
> @@ -755,21 +753,6 @@ static void setup_pc8(void)
>  	has_pc8 = true;
>  }
>  
> -static bool dmc_loaded(void)
> -{
> -	char buf[15];
> -	int len;
> -
> -	len = igt_sysfs_read(debugfs, "i915_dmc_info", buf, sizeof(buf) - 1);
> -	if (len < 0)
> -	    return true; /* no CSR support, no DMC requirement */
> -
> -	buf[len] = '\0';
> -
> -	igt_info("DMC: %s\n", buf);
> -	return strstr(buf, "fw loaded: yes");
> -}
> -
>  static void dump_file(int dir, const char *filename)
>  {
>  	char *contents;
> @@ -796,7 +779,7 @@ static bool setup_environment(void)
>  
>  	init_mode_set_data(&ms_data);
>  
> -	pm_data = igt_pm_enable_sata_link_power_management();
> +	igt_pm_enable_sata_link_power_management();
>  
>  	has_runtime_pm = igt_setup_runtime_pm();
>  	setup_pc8();
> @@ -804,7 +787,7 @@ static bool setup_environment(void)
>  	igt_info("Runtime PM support: %d\n", has_runtime_pm);
>  	igt_info("PC8 residency support: %d\n", has_pc8);
>  	igt_require(has_runtime_pm);
> -	igt_require(dmc_loaded());
> +	igt_require(igt_pm_dmc_loaded(debugfs));
>  
>  out:
>  	disable_all_screens(&ms_data);
> @@ -821,8 +804,7 @@ static void teardown_environment(void)
>  
>  	igt_restore_runtime_pm();
>  
> -	igt_pm_restore_sata_link_power_management(pm_data);
> -	free(pm_data);
> +	igt_pm_restore_sata_link_power_management();
>  
>  	fini_mode_set_data(&ms_data);
>  
> -- 
> 2.21.0
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev