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

[Petri Latvala <petri.latvala@intel.com>]

On Fri, Aug 23, 2019 at 05:25:40PM +0300, Imre Deak wrote:
> 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);


Just a drive-by comment here, use igt_assert_fd(pm_status_fd) instead
of writing it out.


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev