[PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake
@ 2017-06-27 10:53 Paul Kocialkowski
  2017-06-27 10:53 ` [PATCH i-g-t v2 2/7] chamelium: Add support for HPD toggle scheduling instead of async pulses Paul Kocialkowski
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Paul Kocialkowski @ 2017-06-27 10:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: Lyude

This stores the autoresume delay provided via igt_set_autoresume_delay
for use during suspend via rtc wake scheduling. This delay was
previously only used for pm_test suspendm while the function suggests
it should be applied to all autoresume cases.

There is also definitely a use case for configuring the rtc wake delay,
so this implements it.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
---
 lib/igt_aux.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index eb563f72..882dba06 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -680,6 +680,8 @@ void igt_cleanup_aperture_trashers(void)
 	free(trash_bos);
 }
 
+static int autoresume_delay;
+
 static const char *suspend_state_name[] = {
 	[SUSPEND_STATE_FREEZE] = "freeze",
 	[SUSPEND_STATE_STANDBY] = "standby",
@@ -746,7 +748,10 @@ static void suspend_via_rtcwake(enum igt_suspend_state state)
 
 	igt_assert(state < SUSPEND_STATE_NUM);
 
-	delay = state == SUSPEND_STATE_DISK ? 30 : 15;
+	if (autoresume_delay)
+		delay = autoresume_delay;
+	else
+		delay = state == SUSPEND_STATE_DISK ? 30 : 15;
 
 	/*
 	 * Skip if rtcwake would fail for a reason not related to the kernel's
@@ -889,6 +894,8 @@ void igt_set_autoresume_delay(int delay_secs)
 	igt_require(write(delay_fd, delay_str, strlen(delay_str)));
 
 	close(delay_fd);
+
+	autoresume_delay = delay_secs;
 }
 
 /**
-- 
2.13.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH i-g-t v2 2/7] chamelium: Add support for HPD toggle scheduling instead of async pulses
  2017-06-27 10:53 [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake Paul Kocialkowski
@ 2017-06-27 10:53 ` Paul Kocialkowski
  2017-06-27 10:53 ` [PATCH i-g-t v2 3/7] tests/chamelium: Add VGA HPD toggle tests after suspend and hibernate Paul Kocialkowski
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Paul Kocialkowski @ 2017-06-27 10:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: Lyude

This adds support for the newly-introduced ScheduleHpdToggle XMLRPC
method of the Chamelium's interface and makes use of it instead of
starting pulses with an asynchronous call, suspending and dealing with
the result at resume.

The XMLRPC library does not guarantee that the call will be made before
caring for its outcome and this is in fact what was happening:
the call was being delayed until resume time, as can be seen from the
Chamelium's logs. The quite generous timeout for HPD event detection
would then catch the toggle, that was sent after resume.

This is especially useful for testing HPD during suspend/resume.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
---
 lib/igt_chamelium.c | 78 ++++++++++-------------------------------------------
 lib/igt_chamelium.h |  7 +++--
 tests/chamelium.c   | 13 +++++----
 3 files changed, 23 insertions(+), 75 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index 4a2af796..225f98c3 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -447,76 +447,26 @@ void chamelium_fire_mixed_hpd_pulses(struct chamelium *chamelium,
 	xmlrpc_DECREF(pulse_widths);
 }
 
-static void async_rpc_handler(const char *server_url, const char *method_name,
-			      xmlrpc_value *param_array, void *user_data,
-			      xmlrpc_env *fault, xmlrpc_value *result)
-{
-	/* We don't care about the responses */
-}
-
 /**
- * chamelium_async_hpd_pulse_start:
+ * chamelium_schedule_hpd_toggle:
  * @chamelium: The Chamelium instance to use
  * @port: The port to fire the HPD pulses on
- * @high: Whether to fire a high pulse (e.g. simulate a connect), or a low
- * pulse (e.g. simulate a disconnect)
- * @delay_secs: How long to wait before sending the HPD pulse.
- *
- * Instructs the chamelium to send an hpd pulse after @delay_secs seconds have
- * passed, without waiting for the chamelium to finish. This is useful for
- * testing things such as hpd after a suspend/resume cycle, since we can't tell
- * the chamelium to send a hotplug at the same time that our system is
- * suspended.
- *
- * It is required that the user eventually call
- * #chamelium_async_hpd_pulse_finish, to clean up the leftover XML-RPC
- * responses from the chamelium.
- */
-void chamelium_async_hpd_pulse_start(struct chamelium *chamelium,
-				     struct chamelium_port *port,
-				     bool high, int delay_secs)
-{
-	xmlrpc_value *pulse_widths = xmlrpc_array_new(&chamelium->env), *width;
-
-	/* TODO: Actually implement something in the chameleon server to allow
-	 * for delayed actions such as hotplugs. This would work a bit better
-	 * and allow us to test suspend/resume on ports without hpd like VGA
-	 */
-
-	igt_debug("Sending HPD pulse (%s) on %s with %d second delay\n",
-		  high ? "high->low" : "low->high", port->name, delay_secs);
-
-	/* If we're starting at high, make the first pulse width 0 so we keep
-	 * the port connected */
-	if (high) {
-		width = xmlrpc_int_new(&chamelium->env, 0);
-		xmlrpc_array_append_item(&chamelium->env, pulse_widths, width);
-		xmlrpc_DECREF(width);
-	}
-
-	width = xmlrpc_int_new(&chamelium->env, delay_secs * 1000);
-	xmlrpc_array_append_item(&chamelium->env, pulse_widths, width);
-	xmlrpc_DECREF(width);
-
-	xmlrpc_client_start_rpcf(&chamelium->env, chamelium->client,
-				 chamelium->url,
-				 "FireMixedHpdPulses", async_rpc_handler, NULL,
-				 "(iA)", port->id, pulse_widths);
-	xmlrpc_DECREF(pulse_widths);
-}
-
-/**
- * chamelium_async_hpd_pulse_finish:
- * @chamelium: The Chamelium instance to use
+ * @delay_ms: Delay in milli-second before the toggle takes place
+ * @rising_edge: Whether the toggle should be a rising edge or a falling edge
  *
- * Waits for any asynchronous RPC started by #chamelium_async_hpd_pulse_start
- * to complete, and then cleans up any leftover responses from the chamelium.
- * If all of the RPC calls have already completed, this function returns
- * immediately.
+ * Instructs the chamelium to schedule an hpd toggle (either a rising edge or
+ * a falling edge, depending on @rising_edg) after @delay_ms have passed.
+ * This is useful for testing things such as hpd after a suspend/resume cycle.
  */
-void chamelium_async_hpd_pulse_finish(struct chamelium *chamelium)
+void chamelium_schedule_hpd_toggle(struct chamelium *chamelium,
+				   struct chamelium_port *port, int delay_ms,
+				   bool rising_edge)
 {
-	xmlrpc_client_event_loop_finish(chamelium->client);
+	igt_debug("Scheduling HPD toggle on %s in %d ms\n", port->name,
+		  delay_ms);
+
+	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "ScheduleHpdToggle",
+				    "(iii)", port->id, delay_ms, rising_edge));
 }
 
 /**
diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
index 15f60246..81322ad2 100644
--- a/lib/igt_chamelium.h
+++ b/lib/igt_chamelium.h
@@ -61,10 +61,9 @@ void chamelium_fire_mixed_hpd_pulses(struct chamelium *chamelium,
 void chamelium_fire_hpd_pulses(struct chamelium *chamelium,
 			       struct chamelium_port *port,
 			       int width_msec, int count);
-void chamelium_async_hpd_pulse_start(struct chamelium *chamelium,
-				     struct chamelium_port *port,
-				     bool high, int delay_secs);
-void chamelium_async_hpd_pulse_finish(struct chamelium *chamelium);
+void chamelium_schedule_hpd_toggle(struct chamelium *chamelium,
+				   struct chamelium_port *port, int delay_ms,
+				   bool rising_edge);
 int chamelium_new_edid(struct chamelium *chamelium, const unsigned char *edid);
 void chamelium_port_set_edid(struct chamelium *chamelium,
 			     struct chamelium_port *port, int edid_id);
diff --git a/tests/chamelium.c b/tests/chamelium.c
index f1addb0f..04e7e379 100644
--- a/tests/chamelium.c
+++ b/tests/chamelium.c
@@ -226,25 +226,24 @@ try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
 	igt_flush_hotplugs(mon);
 
 	if (port) {
-		chamelium_async_hpd_pulse_start(data->chamelium, port,
-						connected,
-						SUSPEND_RESUME_DELAY / 2);
+		chamelium_schedule_hpd_toggle(data->chamelium, port,
+					      SUSPEND_RESUME_DELAY * 1000 / 2,
+					      !connected);
 	} else {
 		for (p = 0; p < data->port_count; p++) {
 			port = data->ports[p];
 			if (chamelium_port_get_type(port) == DRM_MODE_CONNECTOR_VGA)
 				continue;
 
-			chamelium_async_hpd_pulse_start(data->chamelium, port,
-							connected,
-							SUSPEND_RESUME_DELAY / 2);
+			chamelium_schedule_hpd_toggle(data->chamelium, port,
+						      SUSPEND_RESUME_DELAY * 1000 / 2,
+						      !connected);
 		}
 
 		port = NULL;
 	}
 
 	igt_system_suspend_autoresume(state, test);
-	chamelium_async_hpd_pulse_finish(data->chamelium);
 
 	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
 	if (port) {
-- 
2.13.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH i-g-t v2 3/7] tests/chamelium: Add VGA HPD toggle tests after suspend and hibernate
  2017-06-27 10:53 [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake Paul Kocialkowski
  2017-06-27 10:53 ` [PATCH i-g-t v2 2/7] chamelium: Add support for HPD toggle scheduling instead of async pulses Paul Kocialkowski
@ 2017-06-27 10:53 ` Paul Kocialkowski
  2017-06-27 10:53 ` [PATCH i-g-t v2 4/7] tests/chamelium: Reduce the simple hotplug test toggle count for VGA Paul Kocialkowski
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Paul Kocialkowski @ 2017-06-27 10:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: Lyude

This adds HPD toggle after suspend and hibernate testing for VGA, in the
same fashion as currently done for DP and HDMI and includes VGA in the
common suspend and hibernate test.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
---
 tests/chamelium.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/tests/chamelium.c b/tests/chamelium.c
index 04e7e379..835c3282 100644
--- a/tests/chamelium.c
+++ b/tests/chamelium.c
@@ -232,9 +232,6 @@ try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
 	} else {
 		for (p = 0; p < data->port_count; p++) {
 			port = data->ports[p];
-			if (chamelium_port_get_type(port) == DRM_MODE_CONNECTOR_VGA)
-				continue;
-
 			chamelium_schedule_hpd_toggle(data->chamelium, port,
 						      SUSPEND_RESUME_DELAY * 1000 / 2,
 						      !connected);
@@ -252,9 +249,6 @@ try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
 	} else {
 		for (p = 0; p < data->port_count; p++) {
 			port = data->ports[p];
-			if (chamelium_port_get_type(port) == DRM_MODE_CONNECTOR_VGA)
-				continue;
-
 			igt_assert_eq(reprobe_connector(data, port), connected ?
 				      DRM_MODE_DISCONNECTED :
 				      DRM_MODE_CONNECTED);
@@ -292,9 +286,6 @@ test_suspend_resume_hpd_common(data_t *data, enum igt_suspend_state state,
 
 	for (p = 0; p < data->port_count; p++) {
 		port = data->ports[p];
-		if (chamelium_port_get_type(port) == DRM_MODE_CONNECTOR_VGA)
-			continue;
-
 		igt_debug("Testing port %s\n", chamelium_port_get_name(port));
 	}
 
@@ -810,10 +801,15 @@ igt_main
 				       igt_kms_get_alt_edid());
 		}
 
-		/* FIXME: Right now there isn't a way to do any sort of delayed
-		 * psuedo-hotplug with VGA, so testing detection after a
-		 * suspend/resume cycle isn't possible yet
-		 */
+		connector_subtest("vga-hpd-after-suspend", VGA)
+			test_suspend_resume_hpd(&data, port,
+						SUSPEND_STATE_MEM,
+						SUSPEND_TEST_NONE);
+
+		connector_subtest("vga-hpd-after-hibernate", VGA)
+			test_suspend_resume_hpd(&data, port,
+						SUSPEND_STATE_DISK,
+						SUSPEND_TEST_DEVICES);
 
 		connector_subtest("vga-hpd-without-ddc", VGA)
 			test_hpd_without_ddc(&data, port);
-- 
2.13.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH i-g-t v2 4/7] tests/chamelium: Reduce the simple hotplug test toggle count for VGA
  2017-06-27 10:53 [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake Paul Kocialkowski
  2017-06-27 10:53 ` [PATCH i-g-t v2 2/7] chamelium: Add support for HPD toggle scheduling instead of async pulses Paul Kocialkowski
  2017-06-27 10:53 ` [PATCH i-g-t v2 3/7] tests/chamelium: Add VGA HPD toggle tests after suspend and hibernate Paul Kocialkowski
@ 2017-06-27 10:53 ` Paul Kocialkowski
  2017-06-27 10:53 ` [PATCH i-g-t v2 5/7] tests/chamelium: Update connector state without reprobe when possible Paul Kocialkowski
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Paul Kocialkowski @ 2017-06-27 10:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: Lyude

Since VGA hpd detection is done through RGB lines load detection and
nowadays often goes through a bridge to some digital interface, HPD
detection usually takes a while (up to a couple seconds).

Thus, it is not as relevant to stress it as many as 15 times.
This brings the toggle count down to 5 times, which makes the test run
a lot faster without really changing the outcome much.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
---
 tests/chamelium.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tests/chamelium.c b/tests/chamelium.c
index 835c3282..b412c6a7 100644
--- a/tests/chamelium.c
+++ b/tests/chamelium.c
@@ -47,6 +47,9 @@ typedef struct {
 #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
 #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
 
+#define HPD_TOGGLE_COUNT_VGA 5
+#define HPD_TOGGLE_COUNT_DP_HDMI 15
+
 /* Pre-calculated CRCs for the pattern fb, for all the modes in the default
  * chamelium edid
  */
@@ -158,7 +161,7 @@ reset_state(data_t *data, struct chamelium_port *port)
 }
 
 static void
-test_basic_hotplug(data_t *data, struct chamelium_port *port)
+test_basic_hotplug(data_t *data, struct chamelium_port *port, int toggle_count)
 {
 	struct udev_monitor *mon = igt_watch_hotplug();
 	int i;
@@ -166,7 +169,7 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port)
 	reset_state(data, NULL);
 	igt_hpd_storm_set_threshold(data->drm_fd, 0);
 
-	for (i = 0; i < 15; i++) {
+	for (i = 0; i < toggle_count; i++) {
 		igt_flush_hotplugs(mon);
 
 		/* Check if we get a sysfs hotplug event */
@@ -680,7 +683,8 @@ igt_main
 		}
 
 		connector_subtest("dp-hpd", DisplayPort)
-			test_basic_hotplug(&data, port);
+			test_basic_hotplug(&data, port,
+					   HPD_TOGGLE_COUNT_DP_HDMI);
 
 		connector_subtest("dp-edid-read", DisplayPort) {
 			test_edid_read(&data, port, edid_id,
@@ -736,7 +740,8 @@ igt_main
 		}
 
 		connector_subtest("hdmi-hpd", HDMIA)
-			test_basic_hotplug(&data, port);
+			test_basic_hotplug(&data, port,
+					   HPD_TOGGLE_COUNT_DP_HDMI);
 
 		connector_subtest("hdmi-edid-read", HDMIA) {
 			test_edid_read(&data, port, edid_id,
@@ -792,7 +797,7 @@ igt_main
 		}
 
 		connector_subtest("vga-hpd", VGA)
-			test_basic_hotplug(&data, port);
+			test_basic_hotplug(&data, port, HPD_TOGGLE_COUNT_VGA);
 
 		connector_subtest("vga-edid-read", VGA) {
 			test_edid_read(&data, port, edid_id,
-- 
2.13.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH i-g-t v2 5/7] tests/chamelium: Update connector state without reprobe when possible
  2017-06-27 10:53 [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake Paul Kocialkowski
                   ` (2 preceding siblings ...)
  2017-06-27 10:53 ` [PATCH i-g-t v2 4/7] tests/chamelium: Reduce the simple hotplug test toggle count for VGA Paul Kocialkowski
@ 2017-06-27 10:53 ` Paul Kocialkowski
  2017-06-27 21:26   ` Lyude Paul
  2017-06-27 10:53 ` [PATCH i-g-t v2 6/7] tests/chamelium: Disconnect connectors without extra reset Paul Kocialkowski
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Paul Kocialkowski @ 2017-06-27 10:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: Lyude

This renames the reprobe_connector function to update_connector and
ensures that full reprobe of the connector is only done when really
necessary (e.g. when changing the EDID).

A full reprobe takes time and is not required for updating the connector
state. Thus, this allows executing tests faster.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
---
 tests/chamelium.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/tests/chamelium.c b/tests/chamelium.c
index b412c6a7..346018a5 100644
--- a/tests/chamelium.c
+++ b/tests/chamelium.c
@@ -103,13 +103,14 @@ require_connector_present(data_t *data, unsigned int type)
 }
 
 static drmModeConnection
-reprobe_connector(data_t *data, struct chamelium_port *port)
+update_connector(data_t *data, struct chamelium_port *port, bool reprobe)
 {
 	drmModeConnector *connector;
 	drmModeConnection status;
 
-	igt_debug("Reprobing %s...\n", chamelium_port_get_name(port));
-	connector = chamelium_port_get_connector(data->chamelium, port, true);
+	igt_debug("Updating %s...\n", chamelium_port_get_name(port));
+	connector = chamelium_port_get_connector(data->chamelium, port,
+						 reprobe);
 	igt_assert(connector);
 	status = connector->connection;
 
@@ -119,7 +120,7 @@ reprobe_connector(data_t *data, struct chamelium_port *port)
 
 static void
 wait_for_connector(data_t *data, struct chamelium_port *port,
-		   drmModeConnection status)
+		   drmModeConnection status, bool reprobe)
 {
 	bool finished = false;
 
@@ -132,7 +133,7 @@ wait_for_connector(data_t *data, struct chamelium_port *port,
 	 * that hpd events work in the event that hpd doesn't work on the system
 	 */
 	igt_until_timeout(HOTPLUG_TIMEOUT) {
-		if (reprobe_connector(data, port) == status) {
+		if (update_connector(data, port, reprobe) == status) {
 			finished = true;
 			return;
 		}
@@ -151,11 +152,12 @@ reset_state(data_t *data, struct chamelium_port *port)
 	chamelium_reset(data->chamelium);
 
 	if (port) {
-		wait_for_connector(data, port, DRM_MODE_DISCONNECTED);
+		wait_for_connector(data, port, DRM_MODE_DISCONNECTED, false);
 	} else {
 		for (p = 0; p < data->port_count; p++) {
 			port = data->ports[p];
-			wait_for_connector(data, port, DRM_MODE_DISCONNECTED);
+			wait_for_connector(data, port, DRM_MODE_DISCONNECTED,
+					   false);
 		}
 	}
 }
@@ -175,7 +177,7 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port, int toggle_count)
 		/* Check if we get a sysfs hotplug event */
 		chamelium_plug(data->chamelium, port);
 		igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
-		igt_assert_eq(reprobe_connector(data, port),
+		igt_assert_eq(update_connector(data, port, false),
 			      DRM_MODE_CONNECTED);
 
 		igt_flush_hotplugs(mon);
@@ -183,7 +185,7 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port, int toggle_count)
 		/* Now check if we get a hotplug from disconnection */
 		chamelium_unplug(data->chamelium, port);
 		igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
-		igt_assert_eq(reprobe_connector(data, port),
+		igt_assert_eq(update_connector(data, port, false),
 			      DRM_MODE_DISCONNECTED);
 	}
 
@@ -204,7 +206,7 @@ test_edid_read(data_t *data, struct chamelium_port *port,
 
 	chamelium_port_set_edid(data->chamelium, port, edid_id);
 	chamelium_plug(data->chamelium, port);
-	wait_for_connector(data, port, DRM_MODE_CONNECTED);
+	wait_for_connector(data, port, DRM_MODE_CONNECTED, true);
 
 	igt_assert(kmstest_get_property(data->drm_fd, connector->connector_id,
 					DRM_MODE_OBJECT_CONNECTOR, "EDID", NULL,
@@ -247,13 +249,13 @@ try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
 
 	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
 	if (port) {
-		igt_assert_eq(reprobe_connector(data, port), connected ?
+		igt_assert_eq(update_connector(data, port, false), connected ?
 			      DRM_MODE_DISCONNECTED : DRM_MODE_CONNECTED);
 	} else {
 		for (p = 0; p < data->port_count; p++) {
 			port = data->ports[p];
-			igt_assert_eq(reprobe_connector(data, port), connected ?
-				      DRM_MODE_DISCONNECTED :
+			igt_assert_eq(update_connector(data, port, false),
+				      connected ? DRM_MODE_DISCONNECTED :
 				      DRM_MODE_CONNECTED);
 		}
 
@@ -317,7 +319,7 @@ test_suspend_resume_edid_change(data_t *data, struct chamelium_port *port,
 	/* First plug in the port */
 	chamelium_port_set_edid(data->chamelium, port, edid_id);
 	chamelium_plug(data->chamelium, port);
-	wait_for_connector(data, port, DRM_MODE_CONNECTED);
+	wait_for_connector(data, port, DRM_MODE_CONNECTED, true);
 
 	igt_flush_hotplugs(mon);
 
@@ -352,7 +354,7 @@ prepare_output(data_t *data,
 	chamelium_port_set_edid(data->chamelium, port, data->edid_id);
 
 	chamelium_plug(data->chamelium, port);
-	wait_for_connector(data, port, DRM_MODE_CONNECTED);
+	wait_for_connector(data, port, DRM_MODE_CONNECTED, true);
 
 	igt_display_init(display, data->drm_fd);
 	output = igt_output_from_connector(display, connector);
@@ -590,7 +592,7 @@ test_hpd_without_ddc(data_t *data, struct chamelium_port *port)
 	chamelium_plug(data->chamelium, port);
 
 	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
-	igt_assert_eq(reprobe_connector(data, port), DRM_MODE_CONNECTED);
+	igt_assert_eq(update_connector(data, port, false), DRM_MODE_CONNECTED);
 
 	igt_cleanup_hotplug(mon);
 }
-- 
2.13.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v2 5/7] tests/chamelium: Update connector state without reprobe when possible
  2017-06-27 10:53 ` [PATCH i-g-t v2 5/7] tests/chamelium: Update connector state without reprobe when possible Paul Kocialkowski
@ 2017-06-27 21:26   ` Lyude Paul
  2017-06-28  7:47     ` Paul Kocialkowski
  0 siblings, 1 reply; 14+ messages in thread
From: Lyude Paul @ 2017-06-27 21:26 UTC (permalink / raw)
  To: Paul Kocialkowski, intel-gfx

I -think- this one might be okay but I just got reminded of a rather
not-obvious part of the DRM spec for hotplugging, and I need to check
that this doesn't actually break that. will get back to you in a bit
with an actual review :)

On Tue, 2017-06-27 at 13:53 +0300, Paul Kocialkowski wrote:
> This renames the reprobe_connector function to update_connector and
> ensures that full reprobe of the connector is only done when really
> necessary (e.g. when changing the EDID).
> 
> A full reprobe takes time and is not required for updating the
> connector
> state. Thus, this allows executing tests faster.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> ---
>  tests/chamelium.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/tests/chamelium.c b/tests/chamelium.c
> index b412c6a7..346018a5 100644
> --- a/tests/chamelium.c
> +++ b/tests/chamelium.c
> @@ -103,13 +103,14 @@ require_connector_present(data_t *data,
> unsigned int type)
>  }
>  
>  static drmModeConnection
> -reprobe_connector(data_t *data, struct chamelium_port *port)
> +update_connector(data_t *data, struct chamelium_port *port, bool
> reprobe)
>  {
>  	drmModeConnector *connector;
>  	drmModeConnection status;
>  
> -	igt_debug("Reprobing %s...\n",
> chamelium_port_get_name(port));
> -	connector = chamelium_port_get_connector(data->chamelium,
> port, true);
> +	igt_debug("Updating %s...\n",
> chamelium_port_get_name(port));
> +	connector = chamelium_port_get_connector(data->chamelium,
> port,
> +						 reprobe);
>  	igt_assert(connector);
>  	status = connector->connection;
>  
> @@ -119,7 +120,7 @@ reprobe_connector(data_t *data, struct
> chamelium_port *port)
>  
>  static void
>  wait_for_connector(data_t *data, struct chamelium_port *port,
> -		   drmModeConnection status)
> +		   drmModeConnection status, bool reprobe)
>  {
>  	bool finished = false;
>  
> @@ -132,7 +133,7 @@ wait_for_connector(data_t *data, struct
> chamelium_port *port,
>  	 * that hpd events work in the event that hpd doesn't work
> on the system
>  	 */
>  	igt_until_timeout(HOTPLUG_TIMEOUT) {
> -		if (reprobe_connector(data, port) == status) {
> +		if (update_connector(data, port, reprobe) == status)
> {
>  			finished = true;
>  			return;
>  		}
> @@ -151,11 +152,12 @@ reset_state(data_t *data, struct chamelium_port
> *port)
>  	chamelium_reset(data->chamelium);
>  
>  	if (port) {
> -		wait_for_connector(data, port,
> DRM_MODE_DISCONNECTED);
> +		wait_for_connector(data, port,
> DRM_MODE_DISCONNECTED, false);
>  	} else {
>  		for (p = 0; p < data->port_count; p++) {
>  			port = data->ports[p];
> -			wait_for_connector(data, port,
> DRM_MODE_DISCONNECTED);
> +			wait_for_connector(data, port,
> DRM_MODE_DISCONNECTED,
> +					   false);
>  		}
>  	}
>  }
> @@ -175,7 +177,7 @@ test_basic_hotplug(data_t *data, struct
> chamelium_port *port, int toggle_count)
>  		/* Check if we get a sysfs hotplug event */
>  		chamelium_plug(data->chamelium, port);
>  		igt_assert(igt_hotplug_detected(mon,
> HOTPLUG_TIMEOUT));
> -		igt_assert_eq(reprobe_connector(data, port),
> +		igt_assert_eq(update_connector(data, port, false),
>  			      DRM_MODE_CONNECTED);
>  
>  		igt_flush_hotplugs(mon);
> @@ -183,7 +185,7 @@ test_basic_hotplug(data_t *data, struct
> chamelium_port *port, int toggle_count)
>  		/* Now check if we get a hotplug from disconnection
> */
>  		chamelium_unplug(data->chamelium, port);
>  		igt_assert(igt_hotplug_detected(mon,
> HOTPLUG_TIMEOUT));
> -		igt_assert_eq(reprobe_connector(data, port),
> +		igt_assert_eq(update_connector(data, port, false),
>  			      DRM_MODE_DISCONNECTED);
>  	}
>  
> @@ -204,7 +206,7 @@ test_edid_read(data_t *data, struct
> chamelium_port *port,
>  
>  	chamelium_port_set_edid(data->chamelium, port, edid_id);
>  	chamelium_plug(data->chamelium, port);
> -	wait_for_connector(data, port, DRM_MODE_CONNECTED);
> +	wait_for_connector(data, port, DRM_MODE_CONNECTED, true);
>  
>  	igt_assert(kmstest_get_property(data->drm_fd, connector-
> >connector_id,
>  					DRM_MODE_OBJECT_CONNECTOR,
> "EDID", NULL,
> @@ -247,13 +249,13 @@ try_suspend_resume_hpd(data_t *data, struct
> chamelium_port *port,
>  
>  	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
>  	if (port) {
> -		igt_assert_eq(reprobe_connector(data, port),
> connected ?
> +		igt_assert_eq(update_connector(data, port, false),
> connected ?
>  			      DRM_MODE_DISCONNECTED :
> DRM_MODE_CONNECTED);
>  	} else {
>  		for (p = 0; p < data->port_count; p++) {
>  			port = data->ports[p];
> -			igt_assert_eq(reprobe_connector(data, port),
> connected ?
> -				      DRM_MODE_DISCONNECTED :
> +			igt_assert_eq(update_connector(data, port,
> false),
> +				      connected ?
> DRM_MODE_DISCONNECTED :
>  				      DRM_MODE_CONNECTED);
>  		}
>  
> @@ -317,7 +319,7 @@ test_suspend_resume_edid_change(data_t *data,
> struct chamelium_port *port,
>  	/* First plug in the port */
>  	chamelium_port_set_edid(data->chamelium, port, edid_id);
>  	chamelium_plug(data->chamelium, port);
> -	wait_for_connector(data, port, DRM_MODE_CONNECTED);
> +	wait_for_connector(data, port, DRM_MODE_CONNECTED, true);
>  
>  	igt_flush_hotplugs(mon);
>  
> @@ -352,7 +354,7 @@ prepare_output(data_t *data,
>  	chamelium_port_set_edid(data->chamelium, port, data-
> >edid_id);
>  
>  	chamelium_plug(data->chamelium, port);
> -	wait_for_connector(data, port, DRM_MODE_CONNECTED);
> +	wait_for_connector(data, port, DRM_MODE_CONNECTED, true);
>  
>  	igt_display_init(display, data->drm_fd);
>  	output = igt_output_from_connector(display, connector);
> @@ -590,7 +592,7 @@ test_hpd_without_ddc(data_t *data, struct
> chamelium_port *port)
>  	chamelium_plug(data->chamelium, port);
>  
>  	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> -	igt_assert_eq(reprobe_connector(data, port),
> DRM_MODE_CONNECTED);
> +	igt_assert_eq(update_connector(data, port, false),
> DRM_MODE_CONNECTED);
>  
>  	igt_cleanup_hotplug(mon);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v2 5/7] tests/chamelium: Update connector state without reprobe when possible
  2017-06-27 21:26   ` Lyude Paul
@ 2017-06-28  7:47     ` Paul Kocialkowski
  0 siblings, 0 replies; 14+ messages in thread
From: Paul Kocialkowski @ 2017-06-28  7:47 UTC (permalink / raw)
  To: Lyude Paul, intel-gfx

On Tue, 2017-06-27 at 17:26 -0400, Lyude Paul wrote:
> I -think- this one might be okay but I just got reminded of a rather
> not-obvious part of the DRM spec for hotplugging, and I need to check
> that this doesn't actually break that. will get back to you in a bit
> with an actual review :)

Martin was really expecting this to come around, so I hope it is in accordance
with the DRM spec. At least, it works properly with i915 on SKL.

Thanks for double-checking!

> On Tue, 2017-06-27 at 13:53 +0300, Paul Kocialkowski wrote:
> > This renames the reprobe_connector function to update_connector and
> > ensures that full reprobe of the connector is only done when really
> > necessary (e.g. when changing the EDID).
> > 
> > A full reprobe takes time and is not required for updating the
> > connector
> > state. Thus, this allows executing tests faster.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> > ---
> >  tests/chamelium.c | 34 ++++++++++++++++++----------------
> >  1 file changed, 18 insertions(+), 16 deletions(-)
> > 
> > diff --git a/tests/chamelium.c b/tests/chamelium.c
> > index b412c6a7..346018a5 100644
> > --- a/tests/chamelium.c
> > +++ b/tests/chamelium.c
> > @@ -103,13 +103,14 @@ require_connector_present(data_t *data,
> > unsigned int type)
> >  }
> >  
> >  static drmModeConnection
> > -reprobe_connector(data_t *data, struct chamelium_port *port)
> > +update_connector(data_t *data, struct chamelium_port *port, bool
> > reprobe)
> >  {
> >  	drmModeConnector *connector;
> >  	drmModeConnection status;
> >  
> > -	igt_debug("Reprobing %s...\n",
> > chamelium_port_get_name(port));
> > -	connector = chamelium_port_get_connector(data->chamelium,
> > port, true);
> > +	igt_debug("Updating %s...\n",
> > chamelium_port_get_name(port));
> > +	connector = chamelium_port_get_connector(data->chamelium,
> > port,
> > +						 reprobe);
> >  	igt_assert(connector);
> >  	status = connector->connection;
> >  
> > @@ -119,7 +120,7 @@ reprobe_connector(data_t *data, struct
> > chamelium_port *port)
> >  
> >  static void
> >  wait_for_connector(data_t *data, struct chamelium_port *port,
> > -		   drmModeConnection status)
> > +		   drmModeConnection status, bool reprobe)
> >  {
> >  	bool finished = false;
> >  
> > @@ -132,7 +133,7 @@ wait_for_connector(data_t *data, struct
> > chamelium_port *port,
> >  	 * that hpd events work in the event that hpd doesn't work
> > on the system
> >  	 */
> >  	igt_until_timeout(HOTPLUG_TIMEOUT) {
> > -		if (reprobe_connector(data, port) == status) {
> > +		if (update_connector(data, port, reprobe) == status)
> > {
> >  			finished = true;
> >  			return;
> >  		}
> > @@ -151,11 +152,12 @@ reset_state(data_t *data, struct chamelium_port
> > *port)
> >  	chamelium_reset(data->chamelium);
> >  
> >  	if (port) {
> > -		wait_for_connector(data, port,
> > DRM_MODE_DISCONNECTED);
> > +		wait_for_connector(data, port,
> > DRM_MODE_DISCONNECTED, false);
> >  	} else {
> >  		for (p = 0; p < data->port_count; p++) {
> >  			port = data->ports[p];
> > -			wait_for_connector(data, port,
> > DRM_MODE_DISCONNECTED);
> > +			wait_for_connector(data, port,
> > DRM_MODE_DISCONNECTED,
> > +					   false);
> >  		}
> >  	}
> >  }
> > @@ -175,7 +177,7 @@ test_basic_hotplug(data_t *data, struct
> > chamelium_port *port, int toggle_count)
> >  		/* Check if we get a sysfs hotplug event */
> >  		chamelium_plug(data->chamelium, port);
> >  		igt_assert(igt_hotplug_detected(mon,
> > HOTPLUG_TIMEOUT));
> > -		igt_assert_eq(reprobe_connector(data, port),
> > +		igt_assert_eq(update_connector(data, port, false),
> >  			      DRM_MODE_CONNECTED);
> >  
> >  		igt_flush_hotplugs(mon);
> > @@ -183,7 +185,7 @@ test_basic_hotplug(data_t *data, struct
> > chamelium_port *port, int toggle_count)
> >  		/* Now check if we get a hotplug from disconnection
> > */
> >  		chamelium_unplug(data->chamelium, port);
> >  		igt_assert(igt_hotplug_detected(mon,
> > HOTPLUG_TIMEOUT));
> > -		igt_assert_eq(reprobe_connector(data, port),
> > +		igt_assert_eq(update_connector(data, port, false),
> >  			      DRM_MODE_DISCONNECTED);
> >  	}
> >  
> > @@ -204,7 +206,7 @@ test_edid_read(data_t *data, struct
> > chamelium_port *port,
> >  
> >  	chamelium_port_set_edid(data->chamelium, port, edid_id);
> >  	chamelium_plug(data->chamelium, port);
> > -	wait_for_connector(data, port, DRM_MODE_CONNECTED);
> > +	wait_for_connector(data, port, DRM_MODE_CONNECTED, true);
> >  
> >  	igt_assert(kmstest_get_property(data->drm_fd, connector-
> > > connector_id,
> > 
> >  					DRM_MODE_OBJECT_CONNECTOR,
> > "EDID", NULL,
> > @@ -247,13 +249,13 @@ try_suspend_resume_hpd(data_t *data, struct
> > chamelium_port *port,
> >  
> >  	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> >  	if (port) {
> > -		igt_assert_eq(reprobe_connector(data, port),
> > connected ?
> > +		igt_assert_eq(update_connector(data, port, false),
> > connected ?
> >  			      DRM_MODE_DISCONNECTED :
> > DRM_MODE_CONNECTED);
> >  	} else {
> >  		for (p = 0; p < data->port_count; p++) {
> >  			port = data->ports[p];
> > -			igt_assert_eq(reprobe_connector(data, port),
> > connected ?
> > -				      DRM_MODE_DISCONNECTED :
> > +			igt_assert_eq(update_connector(data, port,
> > false),
> > +				      connected ?
> > DRM_MODE_DISCONNECTED :
> >  				      DRM_MODE_CONNECTED);
> >  		}
> >  
> > @@ -317,7 +319,7 @@ test_suspend_resume_edid_change(data_t *data,
> > struct chamelium_port *port,
> >  	/* First plug in the port */
> >  	chamelium_port_set_edid(data->chamelium, port, edid_id);
> >  	chamelium_plug(data->chamelium, port);
> > -	wait_for_connector(data, port, DRM_MODE_CONNECTED);
> > +	wait_for_connector(data, port, DRM_MODE_CONNECTED, true);
> >  
> >  	igt_flush_hotplugs(mon);
> >  
> > @@ -352,7 +354,7 @@ prepare_output(data_t *data,
> >  	chamelium_port_set_edid(data->chamelium, port, data-
> > > edid_id);
> > 
> >  
> >  	chamelium_plug(data->chamelium, port);
> > -	wait_for_connector(data, port, DRM_MODE_CONNECTED);
> > +	wait_for_connector(data, port, DRM_MODE_CONNECTED, true);
> >  
> >  	igt_display_init(display, data->drm_fd);
> >  	output = igt_output_from_connector(display, connector);
> > @@ -590,7 +592,7 @@ test_hpd_without_ddc(data_t *data, struct
> > chamelium_port *port)
> >  	chamelium_plug(data->chamelium, port);
> >  
> >  	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > -	igt_assert_eq(reprobe_connector(data, port),
> > DRM_MODE_CONNECTED);
> > +	igt_assert_eq(update_connector(data, port, false),
> > DRM_MODE_CONNECTED);
> >  
> >  	igt_cleanup_hotplug(mon);
> >  }
-- 
Paul Kocialkowski <paul.kocialkowski@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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH i-g-t v2 6/7] tests/chamelium: Disconnect connectors without extra reset
  2017-06-27 10:53 [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake Paul Kocialkowski
                   ` (3 preceding siblings ...)
  2017-06-27 10:53 ` [PATCH i-g-t v2 5/7] tests/chamelium: Update connector state without reprobe when possible Paul Kocialkowski
@ 2017-06-27 10:53 ` Paul Kocialkowski
  2017-06-27 21:21   ` Lyude Paul
  2017-06-27 10:53 ` [PATCH i-g-t v2 7/7] Make igtrc configuration common, with configurable suspend/resume delay Paul Kocialkowski
  2017-06-27 21:25 ` [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake Lyude Paul
  6 siblings, 1 reply; 14+ messages in thread
From: Paul Kocialkowski @ 2017-06-27 10:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: Lyude

This removes the reset call from the reset_state function and renames it
to disconnect_connector. Since a call to reset is already done in
chamelium_init, there is no need to do an extra reset in each test.

This allows reducing the execution time a bit.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
---
 tests/chamelium.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/tests/chamelium.c b/tests/chamelium.c
index 346018a5..41e0e89b 100644
--- a/tests/chamelium.c
+++ b/tests/chamelium.c
@@ -145,17 +145,17 @@ wait_for_connector(data_t *data, struct chamelium_port *port,
 }
 
 static void
-reset_state(data_t *data, struct chamelium_port *port)
+disconnect_connector(data_t *data, struct chamelium_port *port)
 {
 	int p;
 
-	chamelium_reset(data->chamelium);
-
 	if (port) {
+		chamelium_unplug(data->chamelium, port);
 		wait_for_connector(data, port, DRM_MODE_DISCONNECTED, false);
 	} else {
 		for (p = 0; p < data->port_count; p++) {
 			port = data->ports[p];
+			chamelium_unplug(data->chamelium, port);
 			wait_for_connector(data, port, DRM_MODE_DISCONNECTED,
 					   false);
 		}
@@ -168,7 +168,7 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port, int toggle_count)
 	struct udev_monitor *mon = igt_watch_hotplug();
 	int i;
 
-	reset_state(data, NULL);
+	disconnect_connector(data, NULL);
 	igt_hpd_storm_set_threshold(data->drm_fd, 0);
 
 	for (i = 0; i < toggle_count; i++) {
@@ -202,7 +202,7 @@ test_edid_read(data_t *data, struct chamelium_port *port,
 	    data->chamelium, port, false);
 	uint64_t edid_blob_id;
 
-	reset_state(data, port);
+	disconnect_connector(data, port);
 
 	chamelium_port_set_edid(data->chamelium, port, edid_id);
 	chamelium_plug(data->chamelium, port);
@@ -270,7 +270,7 @@ test_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
 {
 	struct udev_monitor *mon = igt_watch_hotplug();
 
-	reset_state(data, port);
+	disconnect_connector(data, port);
 
 	/* Make sure we notice new connectors after resuming */
 	try_suspend_resume_hpd(data, port, state, test, mon, false);
@@ -294,7 +294,7 @@ test_suspend_resume_hpd_common(data_t *data, enum igt_suspend_state state,
 		igt_debug("Testing port %s\n", chamelium_port_get_name(port));
 	}
 
-	reset_state(data, NULL);
+	disconnect_connector(data, NULL);
 
 	/* Make sure we notice new connectors after resuming */
 	try_suspend_resume_hpd(data, NULL, state, test, mon, false);
@@ -314,7 +314,7 @@ test_suspend_resume_edid_change(data_t *data, struct chamelium_port *port,
 {
 	struct udev_monitor *mon = igt_watch_hotplug();
 
-	reset_state(data, port);
+	disconnect_connector(data, port);
 
 	/* First plug in the port */
 	chamelium_port_set_edid(data->chamelium, port, edid_id);
@@ -582,7 +582,7 @@ test_hpd_without_ddc(data_t *data, struct chamelium_port *port)
 {
 	struct udev_monitor *mon = igt_watch_hotplug();
 
-	reset_state(data, port);
+	disconnect_connector(data, port);
 	igt_flush_hotplugs(mon);
 
 	/* Disable the DDC on the connector and make sure we still get a
@@ -604,7 +604,7 @@ test_hpd_storm_detect(data_t *data, struct chamelium_port *port, int width)
 	int count = 0;
 
 	igt_require_hpd_storm_ctl(data->drm_fd);
-	reset_state(data, port);
+	disconnect_connector(data, port);
 
 	igt_hpd_storm_set_threshold(data->drm_fd, 1);
 	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10);
@@ -629,7 +629,7 @@ static void
 test_hpd_storm_disable(data_t *data, struct chamelium_port *port, int width)
 {
 	igt_require_hpd_storm_ctl(data->drm_fd);
-	reset_state(data, port);
+	disconnect_connector(data, port);
 
 	igt_hpd_storm_set_threshold(data->drm_fd, 0);
 	chamelium_fire_hpd_pulses(data->chamelium, port,
-- 
2.13.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v2 6/7] tests/chamelium: Disconnect connectors without extra reset
  2017-06-27 10:53 ` [PATCH i-g-t v2 6/7] tests/chamelium: Disconnect connectors without extra reset Paul Kocialkowski
@ 2017-06-27 21:21   ` Lyude Paul
  2017-06-28  7:49     ` Paul Kocialkowski
  0 siblings, 1 reply; 14+ messages in thread
From: Lyude Paul @ 2017-06-27 21:21 UTC (permalink / raw)
  To: Paul Kocialkowski, intel-gfx

NAK, I'd rather just go with the idea you had with removing the
connector reset in chamelium_init()

On Tue, 2017-06-27 at 13:53 +0300, Paul Kocialkowski wrote:
> This removes the reset call from the reset_state function and renames
> it
> to disconnect_connector. Since a call to reset is already done in
> chamelium_init, there is no need to do an extra reset in each test.
> 
> This allows reducing the execution time a bit.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> ---
>  tests/chamelium.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/chamelium.c b/tests/chamelium.c
> index 346018a5..41e0e89b 100644
> --- a/tests/chamelium.c
> +++ b/tests/chamelium.c
> @@ -145,17 +145,17 @@ wait_for_connector(data_t *data, struct
> chamelium_port *port,
>  }
>  
>  static void
> -reset_state(data_t *data, struct chamelium_port *port)
> +disconnect_connector(data_t *data, struct chamelium_port *port)
>  {
>  	int p;
>  
> -	chamelium_reset(data->chamelium);
> -
>  	if (port) {
> +		chamelium_unplug(data->chamelium, port);
>  		wait_for_connector(data, port,
> DRM_MODE_DISCONNECTED, false);
>  	} else {
>  		for (p = 0; p < data->port_count; p++) {
>  			port = data->ports[p];
> +			chamelium_unplug(data->chamelium, port);
>  			wait_for_connector(data, port,
> DRM_MODE_DISCONNECTED,
>  					   false);
>  		}
> @@ -168,7 +168,7 @@ test_basic_hotplug(data_t *data, struct
> chamelium_port *port, int toggle_count)
>  	struct udev_monitor *mon = igt_watch_hotplug();
>  	int i;
>  
> -	reset_state(data, NULL);
> +	disconnect_connector(data, NULL);
>  	igt_hpd_storm_set_threshold(data->drm_fd, 0);
>  
>  	for (i = 0; i < toggle_count; i++) {
> @@ -202,7 +202,7 @@ test_edid_read(data_t *data, struct
> chamelium_port *port,
>  	    data->chamelium, port, false);
>  	uint64_t edid_blob_id;
>  
> -	reset_state(data, port);
> +	disconnect_connector(data, port);
>  
>  	chamelium_port_set_edid(data->chamelium, port, edid_id);
>  	chamelium_plug(data->chamelium, port);
> @@ -270,7 +270,7 @@ test_suspend_resume_hpd(data_t *data, struct
> chamelium_port *port,
>  {
>  	struct udev_monitor *mon = igt_watch_hotplug();
>  
> -	reset_state(data, port);
> +	disconnect_connector(data, port);
>  
>  	/* Make sure we notice new connectors after resuming */
>  	try_suspend_resume_hpd(data, port, state, test, mon, false);
> @@ -294,7 +294,7 @@ test_suspend_resume_hpd_common(data_t *data, enum
> igt_suspend_state state,
>  		igt_debug("Testing port %s\n",
> chamelium_port_get_name(port));
>  	}
>  
> -	reset_state(data, NULL);
> +	disconnect_connector(data, NULL);
>  
>  	/* Make sure we notice new connectors after resuming */
>  	try_suspend_resume_hpd(data, NULL, state, test, mon, false);
> @@ -314,7 +314,7 @@ test_suspend_resume_edid_change(data_t *data,
> struct chamelium_port *port,
>  {
>  	struct udev_monitor *mon = igt_watch_hotplug();
>  
> -	reset_state(data, port);
> +	disconnect_connector(data, port);
>  
>  	/* First plug in the port */
>  	chamelium_port_set_edid(data->chamelium, port, edid_id);
> @@ -582,7 +582,7 @@ test_hpd_without_ddc(data_t *data, struct
> chamelium_port *port)
>  {
>  	struct udev_monitor *mon = igt_watch_hotplug();
>  
> -	reset_state(data, port);
> +	disconnect_connector(data, port);
>  	igt_flush_hotplugs(mon);
>  
>  	/* Disable the DDC on the connector and make sure we still
> get a
> @@ -604,7 +604,7 @@ test_hpd_storm_detect(data_t *data, struct
> chamelium_port *port, int width)
>  	int count = 0;
>  
>  	igt_require_hpd_storm_ctl(data->drm_fd);
> -	reset_state(data, port);
> +	disconnect_connector(data, port);
>  
>  	igt_hpd_storm_set_threshold(data->drm_fd, 1);
>  	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10);
> @@ -629,7 +629,7 @@ static void
>  test_hpd_storm_disable(data_t *data, struct chamelium_port *port,
> int width)
>  {
>  	igt_require_hpd_storm_ctl(data->drm_fd);
> -	reset_state(data, port);
> +	disconnect_connector(data, port);
>  
>  	igt_hpd_storm_set_threshold(data->drm_fd, 0);
>  	chamelium_fire_hpd_pulses(data->chamelium, port,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v2 6/7] tests/chamelium: Disconnect connectors without extra reset
  2017-06-27 21:21   ` Lyude Paul
@ 2017-06-28  7:49     ` Paul Kocialkowski
  0 siblings, 0 replies; 14+ messages in thread
From: Paul Kocialkowski @ 2017-06-28  7:49 UTC (permalink / raw)
  To: Lyude Paul, intel-gfx

On Tue, 2017-06-27 at 17:21 -0400, Lyude Paul wrote:
> NAK, I'd rather just go with the idea you had with removing the
> connector reset in chamelium_init()

Okay then, I'll craft v3 in that directon. This is not a big overhead anyways,
and it does not change anything when running subtests separately (which is what
most test infrastructures using igt seem to be doing).

> On Tue, 2017-06-27 at 13:53 +0300, Paul Kocialkowski wrote:
> > This removes the reset call from the reset_state function and renames
> > it
> > to disconnect_connector. Since a call to reset is already done in
> > chamelium_init, there is no need to do an extra reset in each test.
> > 
> > This allows reducing the execution time a bit.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> > ---
> >  tests/chamelium.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tests/chamelium.c b/tests/chamelium.c
> > index 346018a5..41e0e89b 100644
> > --- a/tests/chamelium.c
> > +++ b/tests/chamelium.c
> > @@ -145,17 +145,17 @@ wait_for_connector(data_t *data, struct
> > chamelium_port *port,
> >  }
> >  
> >  static void
> > -reset_state(data_t *data, struct chamelium_port *port)
> > +disconnect_connector(data_t *data, struct chamelium_port *port)
> >  {
> >  	int p;
> >  
> > -	chamelium_reset(data->chamelium);
> > -
> >  	if (port) {
> > +		chamelium_unplug(data->chamelium, port);
> >  		wait_for_connector(data, port,
> > DRM_MODE_DISCONNECTED, false);
> >  	} else {
> >  		for (p = 0; p < data->port_count; p++) {
> >  			port = data->ports[p];
> > +			chamelium_unplug(data->chamelium, port);
> >  			wait_for_connector(data, port,
> > DRM_MODE_DISCONNECTED,
> >  					   false);
> >  		}
> > @@ -168,7 +168,7 @@ test_basic_hotplug(data_t *data, struct
> > chamelium_port *port, int toggle_count)
> >  	struct udev_monitor *mon = igt_watch_hotplug();
> >  	int i;
> >  
> > -	reset_state(data, NULL);
> > +	disconnect_connector(data, NULL);
> >  	igt_hpd_storm_set_threshold(data->drm_fd, 0);
> >  
> >  	for (i = 0; i < toggle_count; i++) {
> > @@ -202,7 +202,7 @@ test_edid_read(data_t *data, struct
> > chamelium_port *port,
> >  	    data->chamelium, port, false);
> >  	uint64_t edid_blob_id;
> >  
> > -	reset_state(data, port);
> > +	disconnect_connector(data, port);
> >  
> >  	chamelium_port_set_edid(data->chamelium, port, edid_id);
> >  	chamelium_plug(data->chamelium, port);
> > @@ -270,7 +270,7 @@ test_suspend_resume_hpd(data_t *data, struct
> > chamelium_port *port,
> >  {
> >  	struct udev_monitor *mon = igt_watch_hotplug();
> >  
> > -	reset_state(data, port);
> > +	disconnect_connector(data, port);
> >  
> >  	/* Make sure we notice new connectors after resuming */
> >  	try_suspend_resume_hpd(data, port, state, test, mon, false);
> > @@ -294,7 +294,7 @@ test_suspend_resume_hpd_common(data_t *data, enum
> > igt_suspend_state state,
> >  		igt_debug("Testing port %s\n",
> > chamelium_port_get_name(port));
> >  	}
> >  
> > -	reset_state(data, NULL);
> > +	disconnect_connector(data, NULL);
> >  
> >  	/* Make sure we notice new connectors after resuming */
> >  	try_suspend_resume_hpd(data, NULL, state, test, mon, false);
> > @@ -314,7 +314,7 @@ test_suspend_resume_edid_change(data_t *data,
> > struct chamelium_port *port,
> >  {
> >  	struct udev_monitor *mon = igt_watch_hotplug();
> >  
> > -	reset_state(data, port);
> > +	disconnect_connector(data, port);
> >  
> >  	/* First plug in the port */
> >  	chamelium_port_set_edid(data->chamelium, port, edid_id);
> > @@ -582,7 +582,7 @@ test_hpd_without_ddc(data_t *data, struct
> > chamelium_port *port)
> >  {
> >  	struct udev_monitor *mon = igt_watch_hotplug();
> >  
> > -	reset_state(data, port);
> > +	disconnect_connector(data, port);
> >  	igt_flush_hotplugs(mon);
> >  
> >  	/* Disable the DDC on the connector and make sure we still
> > get a
> > @@ -604,7 +604,7 @@ test_hpd_storm_detect(data_t *data, struct
> > chamelium_port *port, int width)
> >  	int count = 0;
> >  
> >  	igt_require_hpd_storm_ctl(data->drm_fd);
> > -	reset_state(data, port);
> > +	disconnect_connector(data, port);
> >  
> >  	igt_hpd_storm_set_threshold(data->drm_fd, 1);
> >  	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10);
> > @@ -629,7 +629,7 @@ static void
> >  test_hpd_storm_disable(data_t *data, struct chamelium_port *port,
> > int width)
> >  {
> >  	igt_require_hpd_storm_ctl(data->drm_fd);
> > -	reset_state(data, port);
> > +	disconnect_connector(data, port);
> >  
> >  	igt_hpd_storm_set_threshold(data->drm_fd, 0);
> >  	chamelium_fire_hpd_pulses(data->chamelium, port,
-- 
Paul Kocialkowski <paul.kocialkowski@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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH i-g-t v2 7/7] Make igtrc configuration common, with configurable suspend/resume delay
  2017-06-27 10:53 [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake Paul Kocialkowski
                   ` (4 preceding siblings ...)
  2017-06-27 10:53 ` [PATCH i-g-t v2 6/7] tests/chamelium: Disconnect connectors without extra reset Paul Kocialkowski
@ 2017-06-27 10:53 ` Paul Kocialkowski
  2017-06-27 21:11   ` Lyude Paul
  2017-06-27 21:25 ` [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake Lyude Paul
  6 siblings, 1 reply; 14+ messages in thread
From: Paul Kocialkowski @ 2017-06-27 10:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: Lyude

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@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);
+	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
-- 
2.13.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v2 7/7] Make igtrc configuration common, with configurable suspend/resume delay
  2017-06-27 10:53 ` [PATCH i-g-t v2 7/7] Make igtrc configuration common, with configurable suspend/resume delay Paul Kocialkowski
@ 2017-06-27 21:11   ` Lyude Paul
  2017-06-28  7:27     ` Paul Kocialkowski
  0 siblings, 1 reply; 14+ messages in thread
From: Lyude Paul @ 2017-06-27 21:11 UTC (permalink / raw)
  To: Paul Kocialkowski, intel-gfx




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@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

> +	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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v2 7/7] Make igtrc configuration common, with configurable suspend/resume delay
  2017-06-27 21:11   ` Lyude Paul
@ 2017-06-28  7:27     ` Paul Kocialkowski
  0 siblings, 0 replies; 14+ messages in thread
From: Paul Kocialkowski @ 2017-06-28  7:27 UTC (permalink / raw)
  To: Lyude Paul, intel-gfx

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@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@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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake
  2017-06-27 10:53 [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake Paul Kocialkowski
                   ` (5 preceding siblings ...)
  2017-06-27 10:53 ` [PATCH i-g-t v2 7/7] Make igtrc configuration common, with configurable suspend/resume delay Paul Kocialkowski
@ 2017-06-27 21:25 ` Lyude Paul
  6 siblings, 0 replies; 14+ messages in thread
From: Lyude Paul @ 2017-06-27 21:25 UTC (permalink / raw)
  To: Paul Kocialkowski, intel-gfx

FYI: I reviewed and pushed patches 1-4 of this series, thanks!

On Tue, 2017-06-27 at 13:53 +0300, Paul Kocialkowski wrote:
> This stores the autoresume delay provided via
> igt_set_autoresume_delay
> for use during suspend via rtc wake scheduling. This delay was
> previously only used for pm_test suspendm while the function suggests
> it should be applied to all autoresume cases.
> 
> There is also definitely a use case for configuring the rtc wake
> delay,
> so this implements it.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> ---
>  lib/igt_aux.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index eb563f72..882dba06 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -680,6 +680,8 @@ void igt_cleanup_aperture_trashers(void)
>  	free(trash_bos);
>  }
>  
> +static int autoresume_delay;
> +
>  static const char *suspend_state_name[] = {
>  	[SUSPEND_STATE_FREEZE] = "freeze",
>  	[SUSPEND_STATE_STANDBY] = "standby",
> @@ -746,7 +748,10 @@ static void suspend_via_rtcwake(enum
> igt_suspend_state state)
>  
>  	igt_assert(state < SUSPEND_STATE_NUM);
>  
> -	delay = state == SUSPEND_STATE_DISK ? 30 : 15;
> +	if (autoresume_delay)
> +		delay = autoresume_delay;
> +	else
> +		delay = state == SUSPEND_STATE_DISK ? 30 : 15;
>  
>  	/*
>  	 * Skip if rtcwake would fail for a reason not related to
> the kernel's
> @@ -889,6 +894,8 @@ void igt_set_autoresume_delay(int delay_secs)
>  	igt_require(write(delay_fd, delay_str, strlen(delay_str)));
>  
>  	close(delay_fd);
> +
> +	autoresume_delay = delay_secs;
>  }
>  
>  /**
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-06-28  7:49 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-27 10:53 [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake Paul Kocialkowski
2017-06-27 10:53 ` [PATCH i-g-t v2 2/7] chamelium: Add support for HPD toggle scheduling instead of async pulses Paul Kocialkowski
2017-06-27 10:53 ` [PATCH i-g-t v2 3/7] tests/chamelium: Add VGA HPD toggle tests after suspend and hibernate Paul Kocialkowski
2017-06-27 10:53 ` [PATCH i-g-t v2 4/7] tests/chamelium: Reduce the simple hotplug test toggle count for VGA Paul Kocialkowski
2017-06-27 10:53 ` [PATCH i-g-t v2 5/7] tests/chamelium: Update connector state without reprobe when possible Paul Kocialkowski
2017-06-27 21:26   ` Lyude Paul
2017-06-28  7:47     ` Paul Kocialkowski
2017-06-27 10:53 ` [PATCH i-g-t v2 6/7] tests/chamelium: Disconnect connectors without extra reset Paul Kocialkowski
2017-06-27 21:21   ` Lyude Paul
2017-06-28  7:49     ` Paul Kocialkowski
2017-06-27 10:53 ` [PATCH i-g-t v2 7/7] Make igtrc configuration common, with configurable suspend/resume delay Paul Kocialkowski
2017-06-27 21:11   ` Lyude Paul
2017-06-28  7:27     ` Paul Kocialkowski
2017-06-27 21:25 ` [PATCH i-g-t v2 1/7] lib/igt_aux: Use provided autoresume delay for rtc wake Lyude Paul

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.