[Intel-gfx] [PATCH i-g-t v2 7/7] Make igtrc configuration common, with configurable suspend/resume delay
Paul Kocialkowski
paul.kocialkowski at linux.intel.com
Wed Jun 28 07:27:56 UTC 2017
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.kocialkowski at 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.kocialkowski at linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
More information about the Intel-gfx
mailing list