On Tue, 2017-06-27 at 17:11 -0400, Lyude Paul wrote: > > > On Tue, 2017-06-27 at 13:53 +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.kocialkow...@linux.intel.com> > > --- > > lib/Makefile.am | 2 ++ > > lib/igt_aux.c | 27 ++++++++++++++++++---- > > lib/igt_aux.h | 1 + > > lib/igt_chamelium.c | 49 +++++++++------------------------------- > > lib/igt_core.c | 64 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > lib/igt_core.h | 2 ++ > > tests/Makefile.am | 4 ++-- > > tests/chamelium.c | 11 +++++---- > > tools/Makefile.am | 4 ++-- > > 9 files changed, 112 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 225f98c3..345c44d6 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/testing/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..b3d1c608 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,20 @@ static void oom_adjust_for_doom(void) > > > > } > > > > +static void config_parse(void) > > +{ > > + GError *error = NULL; > > + int rc; > > + > > + if (!igt_key_file) > > + return; > > + > > + rc = g_key_file_get_integer(igt_key_file, "DUT", > > "SuspendResumeDelay", > > + &error); > > We should probably at least throw an error if the key value happens to > be there, but the value is invalid (such as someone specifying "foo" as > the SuspendResumeDelay value). Maybe add a function like > > static int > igt_config_get_integer(const char *section, const char *key_name, int *value, > int default) > > That calls the appropriate glib functions, and fails the current test > if any invalid non-default values are given
Fair enough, but how about simply testing error == G_KEY_FILE_ERROR_INVALID_VALUE (meaning the value was defined but could not be correctly interpreted) inline? It seems a bit overkill to add a specific helper function for that. > > + if (rc != 0) > > + igt_set_autoresume_delay(rc); > > +} > > + > > static int common_init(int *argc, char **argv, > > const char *extra_short_opts, > > const struct option *extra_long_opts, > > @@ -616,6 +650,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 +774,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 (!ret) { > > + g_key_file_free(igt_key_file); > > + igt_key_file = NULL; > > + ret = -1; > > + > > + goto out; > > + } > > + > > + config_parse(); > > + > > out: > > + if (!key_file_env && key_file_loc) > > + free(key_file_loc); > > + > > free(short_opts); > > free(combined_opts); > > > > @@ -1345,6 +1406,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 41e0e89b..88b0a94c 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.kocialkow...@linux.intel.com> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx