Re: [PATCH i-g-t v3 3/3] Make igtrc configuration common, with configurable suspend/resume delay

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2017-07-03 at 15:01 +0300, Paul Kocialkowski wrote:
> This adds support for configurable suspend/resume delay and takes the
> occasion to move igtrc configuation from igt_chamelium to igt_core.
> This way, suspend/resume delay configuration can be used for all tests.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxxxxxx>
> ---
>  lib/Makefile.am     |  2 ++
>  lib/igt_aux.c       | 27 +++++++++++++++++----
>  lib/igt_aux.h       |  1 +
>  lib/igt_chamelium.c | 49 +++++++++----------------------------
>  lib/igt_core.c      | 69
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_core.h      |  2 ++
>  tests/Makefile.am   |  4 ++--
>  tests/chamelium.c   | 11 ++++-----
>  tools/Makefile.am   |  4 ++--
>  9 files changed, 117 insertions(+), 52 deletions(-)
> 
> diff --git a/lib/Makefile.am b/lib/Makefile.am
> index 91e72c44..d4f41128 100644
> --- a/lib/Makefile.am
> +++ b/lib/Makefile.am
> @@ -41,6 +41,7 @@ AM_CFLAGS = \
>  	    $(XMLRPC_CFLAGS) \
>  	    $(LIBUDEV_CFLAGS) \
>  	    $(PIXMAN_CFLAGS) \
> +	    $(GLIB_CFLAGS) \
>  	    $(VALGRIND_CFLAGS) \
>  	    -DIGT_SRCDIR=\""$(abs_top_srcdir)/tests"\" \
>  	    -DIGT_DATADIR=\""$(pkgdatadir)"\" \
> @@ -61,5 +62,6 @@ libintel_tools_la_LIBADD = \
>  	$(XMLRPC_LIBS) \
>  	$(LIBUDEV_LIBS) \
>  	$(PIXMAN_LIBS) \
> +	$(GLIB_LIBS) \
>  	-lm
>  
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index 882dba06..86a213c2 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -748,10 +748,7 @@ static void suspend_via_rtcwake(enum igt_suspend_state
> state)
>  
>  	igt_assert(state < SUSPEND_STATE_NUM);
>  
> -	if (autoresume_delay)
> -		delay = autoresume_delay;
> -	else
> -		delay = state == SUSPEND_STATE_DISK ? 30 : 15;
> +	delay = igt_get_autoresume_delay(state);
>  
>  	/*
>  	 * Skip if rtcwake would fail for a reason not related to the
> kernel's
> @@ -899,6 +896,28 @@ void igt_set_autoresume_delay(int delay_secs)
>  }
>  
>  /**
> + * igt_get_autoresume_delay:
> + * @state: an #igt_suspend_state, the target suspend state
> + *
> + * Retrieves how long we wait to resume the system after suspending it.
> + * This can either be set through igt_set_autoresume_delay or be a default
> + * value that depends on the suspend state.
> + *
> + * Returns: The autoresume delay, in seconds.
> + */
> +int igt_get_autoresume_delay(enum igt_suspend_state state)
> +{
> +	int delay;
> +
> +	if (autoresume_delay)
> +		delay = autoresume_delay;
> +	else
> +		delay = state == SUSPEND_STATE_DISK ? 30 : 15;
> +
> +	return delay;
> +}
> +
> +/**
>   * igt_drop_root:
>   *
>   * Drop root privileges and make sure it actually worked. Useful for tests
> diff --git a/lib/igt_aux.h b/lib/igt_aux.h
> index 54b97164..499a1679 100644
> --- a/lib/igt_aux.h
> +++ b/lib/igt_aux.h
> @@ -192,6 +192,7 @@ enum igt_suspend_test {
>  void igt_system_suspend_autoresume(enum igt_suspend_state state,
>  				   enum igt_suspend_test test);
>  void igt_set_autoresume_delay(int delay_secs);
> +int igt_get_autoresume_delay(enum igt_suspend_state state);
>  
>  /* dropping priviledges */
>  void igt_drop_root(void);
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index 624f448b..bff08c0e 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -51,8 +51,8 @@
>   * [on the ChromeOS project page](https://www.chromium.org/chromium-os/testin
> g/chamelium).
>   *
>   * In order to run tests using the Chamelium, a valid configuration file must
> be
> - * present.  The configuration file is a normal Glib keyfile (similar to
> Windows
> - * INI) structured like so:
> + * present. It must contain Chamelium-specific keys as shown with the
> following
> + * example:
>   *
>   * |[<!-- language="plain" -->
>   *	[Chamelium]
> @@ -72,8 +72,6 @@
>   *	ChameliumPortID=3
>   * ]|
>   *
> - * By default, this file is expected to exist in ~/.igtrc . The directory for
> - * this can be overriden by setting the environment variable
> %IGT_CONFIG_PATH.
>   */
>  
>  struct chamelium_edid {
> @@ -1034,7 +1032,7 @@ static unsigned int chamelium_get_port_type(struct
> chamelium *chamelium,
>  }
>  
>  static bool chamelium_read_port_mappings(struct chamelium *chamelium,
> -					 int drm_fd, GKeyFile *key_file)
> +					 int drm_fd)
>  {
>  	drmModeRes *res;
>  	drmModeConnector *connector;
> @@ -1045,7 +1043,7 @@ static bool chamelium_read_port_mappings(struct
> chamelium *chamelium,
>  	int port_i, i, j;
>  	bool ret = true;
>  
> -	group_list = g_key_file_get_groups(key_file, NULL);
> +	group_list = g_key_file_get_groups(igt_key_file, NULL);
>  
>  	/* Count how many connector mappings are specified in the config */
>  	for (i = 0; group_list[i] != NULL; i++) {
> @@ -1068,7 +1066,7 @@ static bool chamelium_read_port_mappings(struct
> chamelium *chamelium,
>  
>  		port = &chamelium->ports[port_i++];
>  		port->name = strdup(map_name);
> -		port->id = g_key_file_get_integer(key_file, group,
> +		port->id = g_key_file_get_integer(igt_key_file, group,
>  						  "ChameliumPortID",
>  						  &error);
>  		if (!port->id) {
> @@ -1125,47 +1123,22 @@ out:
>  
>  static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
>  {
> -	GKeyFile *key_file = g_key_file_new();
>  	GError *error = NULL;
> -	char *key_file_loc, *key_file_env;
> -	int rc;
> -	bool ret = true;
>  
> -	key_file_env = getenv("IGT_CONFIG_PATH");
> -	if (key_file_env) {
> -		key_file_loc = key_file_env;
> -	} else {
> -		key_file_loc = malloc(100);
> -		snprintf(key_file_loc, 100, "%s/.igtrc", g_get_home_dir());
> +	if (!igt_key_file) {
> +		igt_warn("No configuration file available for chamelium\n");
> +		return false;
>  	}
>  
> -	rc = g_key_file_load_from_file(key_file, key_file_loc,
> -				       G_KEY_FILE_NONE, &error);
> -	if (!rc) {
> -		igt_warn("Failed to read chamelium configuration file: %s\n",
> -			 error->message);
> -		ret = false;
> -		goto out;
> -	}
> -
> -	chamelium->url = g_key_file_get_string(key_file, "Chamelium", "URL",
> +	chamelium->url = g_key_file_get_string(igt_key_file, "Chamelium",
> "URL",
>  					       &error);
>  	if (!chamelium->url) {
>  		igt_warn("Couldn't read chamelium URL from config file:
> %s\n",
>  			 error->message);
> -		ret = false;
> -		goto out;
> +		return false;
>  	}
>  
> -	ret = chamelium_read_port_mappings(chamelium, drm_fd, key_file);
> -
> -out:
> -	g_key_file_free(key_file);
> -
> -	if (!key_file_env)
> -		free(key_file_loc);
> -
> -	return ret;
> +	return chamelium_read_port_mappings(chamelium, drm_fd);
>  }
>  
>  /**
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 9a5ed40e..76072845 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -57,6 +57,7 @@
>  #include <limits.h>
>  #include <locale.h>
>  #include <uwildmat/uwildmat.h>
> +#include <glib.h>
>  
>  #include "drmtest.h"
>  #include "intel_chipset.h"
> @@ -225,6 +226,23 @@
>   * - 'basic*,advanced*' match any subtest starting basic or advanced
>   * - '*,!basic*' match any subtest not starting basic
>   * - 'basic*,!basic-render*' match any subtest starting basic but not
> starting basic-render
> + *
> + * # Configuration
> + *
> + * Some of IGT's behavior can be configured through a configuration file.
> + * By default, this file is expected to exist in ~/.igtrc . The directory for
> + * this can be overriden by setting the environment variable
> %IGT_CONFIG_PATH.
> + * An example configuration follows:
> + *
> + * |[<!-- language="plain" -->
> + *	# The following section is used for configuring the Device Under
> Test.
> + *	# It is not mandatory and allows overriding default values.
> + *	[DUT]
> + *	SuspendResumeDelay=10
> + * ]|
> + *
> + * Some specific configuration options may be used by specific parts of IGT,
> + * such as those related to Chamelium support.
>   */
>  
>  static unsigned int exit_handler_count;
> @@ -271,6 +289,8 @@ static struct {
>  } log_buffer;
>  static pthread_mutex_t log_buffer_mutex = PTHREAD_MUTEX_INITIALIZER;
>  
> +GKeyFile *igt_key_file;
> +
>  const char *igt_test_name(void)
>  {
>  	return command_str;
> @@ -593,6 +613,25 @@ static void oom_adjust_for_doom(void)
>  
>  }
>  
> +static int config_parse(void)
> +{
> +	GError *error = NULL;
> +	int rc;
> +
> +	if (!igt_key_file)
> +		return 0;
> +
> +	rc = g_key_file_get_integer(igt_key_file, "DUT",
> "SuspendResumeDelay",
> +				    &error);
> +	if (error == G_KEY_FILE_ERROR_INVALID_VALUE)

That felt very weird to type out and I thought that error was, for some reason,
a pointer used as integer. I double-checked that and it turns out that,
unsurprisingly, it is not.

Please hold off that patch, I will revisit it tomorrow to properly handle the
error.

> +		return -2;
> +
> +	if (rc != 0)
> +		igt_set_autoresume_delay(rc);
> +
> +	return 0;
> +}
> +
>  static int common_init(int *argc, char **argv,
>  		       const char *extra_short_opts,
>  		       const struct option *extra_long_opts,
> @@ -616,6 +655,9 @@ static int common_init(int *argc, char **argv,
>  	int extra_opt_count;
>  	int all_opt_count;
>  	int ret = 0;
> +	char *key_file_loc = NULL;
> +	char *key_file_env = NULL;
> +	GError *error = NULL;
>  	const char *env;
>  
>  	if (!isatty(STDOUT_FILENO) || getenv("IGT_PLAIN_OUTPUT"))
> @@ -737,7 +779,31 @@ static int common_init(int *argc, char **argv,
>  		}
>  	}
>  
> +	key_file_env = getenv("IGT_CONFIG_PATH");
> +	if (key_file_env) {
> +		key_file_loc = key_file_env;
> +	} else {
> +		key_file_loc = malloc(100);
> +		snprintf(key_file_loc, 100, "%s/.igtrc", g_get_home_dir());
> +	}
> +
> +	igt_key_file = g_key_file_new();
> +	ret = g_key_file_load_from_file(igt_key_file, key_file_loc,
> +					G_KEY_FILE_NONE, &error);
> +	if (error == G_KEY_FILE_ERROR) {

Same thing here.

> +		g_key_file_free(igt_key_file);
> +		igt_key_file = NULL;
> +		ret = -2;
> +
> +		goto out;
> +	}
> +
> +	ret = config_parse();
> +
>  out:
> +	if (!key_file_env && key_file_loc)
> +		free(key_file_loc);
> +
>  	free(short_opts);
>  	free(combined_opts);
>  
> @@ -1345,6 +1411,9 @@ void igt_exit(void)
>  {
>  	igt_exit_called = true;
>  
> +	if (igt_key_file)
> +		g_key_file_free(igt_key_file);
> +
>  	if (run_single_subtest && !run_single_subtest_found) {
>  		igt_warn("Unknown subtest: %s\n", run_single_subtest);
>  		exit(IGT_EXIT_INVALID);
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index a2ed9720..0739ca83 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -40,6 +40,7 @@
>  #include <stdarg.h>
>  #include <getopt.h>
>  #include <unistd.h>
> +#include <glib.h>
>  
>  #ifndef IGT_LOG_DOMAIN
>  #define IGT_LOG_DOMAIN (NULL)
> @@ -48,6 +49,7 @@
>  
>  extern const char* __igt_test_description __attribute__((weak));
>  extern bool __igt_plain_output;
> +extern GKeyFile *igt_key_file;
>  
>  
>  /**
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index b051364c..f9d11e6c 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -72,9 +72,9 @@ AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) -Wno-unused-result
> $(DEBUG_CFLAGS)\
>  	$(LIBUNWIND_CFLAGS) $(WERROR_CFLAGS) \
>  	$(NULL)
>  
> -LDADD = ../lib/libintel_tools.la $(GLIB_LIBS) $(XMLRPC_LIBS)
> +LDADD = ../lib/libintel_tools.la $(XMLRPC_LIBS)
>  
> -AM_CFLAGS += $(CAIRO_CFLAGS) $(LIBUDEV_CFLAGS) $(GLIB_CFLAGS)
> +AM_CFLAGS += $(CAIRO_CFLAGS) $(LIBUDEV_CFLAGS)
>  AM_LDFLAGS = -Wl,--as-needed
>  
>  drm_import_export_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> diff --git a/tests/chamelium.c b/tests/chamelium.c
> index 6247d537..e3067664 100644
> --- a/tests/chamelium.c
> +++ b/tests/chamelium.c
> @@ -42,7 +42,6 @@ typedef struct {
>  } data_t;
>  
>  #define HOTPLUG_TIMEOUT 20 /* seconds */
> -#define SUSPEND_RESUME_DELAY 20 /* seconds */
>  
>  #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
>  #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
> @@ -225,21 +224,21 @@ try_suspend_resume_hpd(data_t *data, struct
> chamelium_port *port,
>  		       enum igt_suspend_state state, enum igt_suspend_test
> test,
>  		       struct udev_monitor *mon, bool connected)
>  {
> +	int delay;
>  	int p;
>  
> -	igt_set_autoresume_delay(SUSPEND_RESUME_DELAY);
>  	igt_flush_hotplugs(mon);
>  
> +	delay = igt_get_autoresume_delay(state) * 1000 / 2;
> +
>  	if (port) {
> -		chamelium_schedule_hpd_toggle(data->chamelium, port,
> -					      SUSPEND_RESUME_DELAY * 1000 /
> 2,
> +		chamelium_schedule_hpd_toggle(data->chamelium, port, delay,
>  					      !connected);
>  	} else {
>  		for (p = 0; p < data->port_count; p++) {
>  			port = data->ports[p];
>  			chamelium_schedule_hpd_toggle(data->chamelium, port,
> -						      SUSPEND_RESUME_DELAY *
> 1000 / 2,
> -						      !connected);
> +						      delay, !connected);
>  		}
>  
>  		port = NULL;
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index 18a67efb..c40e75c7 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -9,8 +9,8 @@ endif
>  
>  if HAVE_UDEV
>  bin_PROGRAMS += intel_dp_compliance
> -intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
> -intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
> $(GLIB_LIBS)
> +intel_dp_compliance_CFLAGS = $(AM_CFLAGS)
> +intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
>  endif
>  
>  SUBDIRS = null_state_gen registers
-- 
Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxxxxxx>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux