[PATCH i-g-t 1/7] tests/chamelium: Update connector state without reprobe when possible

[PATCH i-g-t 1/7] tests/chamelium: Update connector state without reprobe when possible

[Paul Kocialkowski]

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 b7eaa2fa..e1f21fb8 100644
--- a/tests/chamelium.c
+++ b/tests/chamelium.c
@@ -100,13 +100,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;
 
@@ -116,7 +117,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;
 
@@ -129,7 +130,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;
 		}
@@ -148,11 +149,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);
 		}
 	}
 }
@@ -172,7 +174,7 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port)
 		/* 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);
@@ -180,7 +182,7 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port)
 		/* 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);
 	}
 
@@ -201,7 +203,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,
@@ -244,13 +246,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);
 		}
 
@@ -314,7 +316,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);
 
@@ -349,7 +351,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);
@@ -587,7 +589,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

[PATCH i-g-t 2/7] tests/chamelium: Check all connectors state for basic hotplug

[Paul Kocialkowski]

Without doing a full reprobe, hpd toggles are sent without much delay
between them. With a VGA connector attached, the reset occurring before
the test will toggle its state, with a delay (inherent to hpd detection
on VGA).

It often occurs that this VGA state toggle is detected in the middle of
the current connector test, triggering a hotplug event unrelated to the
current connector and thus causing the test to fail.

Thus, the state of all connectors is checked (and waited for) before
running the basic hotplug test.

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

diff --git a/tests/chamelium.c b/tests/chamelium.c
index e1f21fb8..5075a52f 100644
--- a/tests/chamelium.c
+++ b/tests/chamelium.c
@@ -165,7 +165,7 @@ test_basic_hotplug(data_t *data, struct chamelium_port *port)
 	struct udev_monitor *mon = igt_watch_hotplug();
 	int i;
 
-	reset_state(data, port);
+	reset_state(data, NULL);
 	igt_hpd_storm_set_threshold(data->drm_fd, 0);
 
 	for (i = 0; i < 15; i++) {
-- 
2.13.1

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

[PATCH i-g-t 3/7] tests/chamelium: Use 50 ms delay to wait for connector change

[Paul Kocialkowski]

Using a 1 s delay seems too large for detecting a connector change, that
may happen faster than this. Since the constraints on execution time are
high, it is preferable to use a much smaller sleep time.

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

diff --git a/tests/chamelium.c b/tests/chamelium.c
index 5075a52f..15ed0f28 100644
--- a/tests/chamelium.c
+++ b/tests/chamelium.c
@@ -135,7 +135,7 @@ wait_for_connector(data_t *data, struct chamelium_port *port,
 			return;
 		}
 
-		sleep(1);
+		usleep(50000);
 	}
 
 	igt_assert(finished);
-- 
2.13.1

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

[PATCH i-g-t 4/7] lib/igt_chamelium: Add support for configurable DUT suspend/resume delay

[Paul Kocialkowski]

This adds support for reading a SuspendResumeDelay property (under
[DUT]) in the IGT configuration (igtrc) and exposing it through a
chamelium_get_suspend_resume_delay function.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
---
 lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
 lib/igt_chamelium.h |  1 +
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index 225f98c3..a1aaf405 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -58,7 +58,7 @@
  *	[Chamelium]
  *	URL=http://chameleon:9992 # The URL used for connecting to the Chamelium's RPC server
  *
- *	# The rest of the sections are used for defining connector mappings.
+ *	# The following sections are used for defining connector mappings.
  *	# This is required so any tests using the Chamelium know which connector
  *	# on the test machine should be connected to each Chamelium port.
  *	#
@@ -70,12 +70,19 @@
  *
  *	[Chamelium:HDMI-A-1]
  *	ChameliumPortID=3
+ *
+ *	# The following section is used for configuring the Device Under Test.
+ *	# It is not mandatory and allows overriding default values.
+ *	[DUT]
+ *	SuspendResumeDelay=10
  * ]|
  *
  * 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.
  */
 
+#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
+
 struct chamelium_edid {
 	int id;
 	struct igt_list link;
@@ -100,6 +107,7 @@ struct chamelium {
 	xmlrpc_env env;
 	xmlrpc_client *client;
 	char *url;
+	int suspend_resume_delay;
 
 	/* Indicates the last port to have been used for capturing video */
 	struct chamelium_port *capturing_port;
@@ -114,6 +122,20 @@ struct chamelium {
 static struct chamelium *cleanup_instance;
 
 /**
+ * chamelium_get_suspend_resume_delay:
+ * @chamelium: The Chamelium instance to use
+ *
+ * Retrieves the suspend/resume delay as specified in the configuration or
+ * its default value.
+ *
+ * Returns: the suspend/resume delay in seconds
+ */
+int chamelium_get_suspend_resume_delay(struct chamelium *chamelium)
+{
+	return chamelium->suspend_resume_delay;
+}
+
+/**
  * chamelium_get_ports:
  * @chamelium: The Chamelium instance to use
  * @count: Where to store the number of ports
@@ -1157,6 +1179,13 @@ static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
 		goto out;
 	}
 
+	rc = g_key_file_get_integer(key_file, "DUT", "SuspendResumeDelay",
+				    &error);
+	if (rc == 0)
+		chamelium->suspend_resume_delay = SUSPEND_RESUME_DELAY_DEFAULT;
+	else
+		chamelium->suspend_resume_delay = rc;
+
 	ret = chamelium_read_port_mappings(chamelium, drm_fd, key_file);
 
 out:
diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
index 81322ad2..380f9b36 100644
--- a/lib/igt_chamelium.h
+++ b/lib/igt_chamelium.h
@@ -41,6 +41,7 @@ struct chamelium *chamelium_init(int drm_fd);
 void chamelium_deinit(struct chamelium *chamelium);
 void chamelium_reset(struct chamelium *chamelium);
 
+int chamelium_get_suspend_resume_delay(struct chamelium *chamelium);
 struct chamelium_port **chamelium_get_ports(struct chamelium *chamelium,
 					    int *count);
 unsigned int chamelium_port_get_type(const struct chamelium_port *port);
-- 
2.13.1

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

[PATCH i-g-t 5/7] tests/chamelium: Use configurable DUT suspend/resume delay

[Paul Kocialkowski]

Now that the suspend/resume delay can be configured in the IGT
configuration (igtrc), use it in the chamelium suspend/resume tests.

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

diff --git a/tests/chamelium.c b/tests/chamelium.c
index 15ed0f28..3ee57af6 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 */
@@ -222,21 +221,22 @@ 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 = chamelium_get_suspend_resume_delay(data->chamelium);
 	int p;
 
-	igt_set_autoresume_delay(SUSPEND_RESUME_DELAY);
+	igt_set_autoresume_delay(delay);
 	igt_flush_hotplugs(mon);
 
+	delay = delay * 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;
@@ -310,9 +310,12 @@ test_suspend_resume_edid_change(data_t *data, struct chamelium_port *port,
 				int alt_edid_id)
 {
 	struct udev_monitor *mon = igt_watch_hotplug();
+	int delay = chamelium_get_suspend_resume_delay(data->chamelium);
 
 	reset_state(data, port);
 
+	igt_set_autoresume_delay(delay);
+
 	/* First plug in the port */
 	chamelium_port_set_edid(data->chamelium, port, edid_id);
 	chamelium_plug(data->chamelium, port);
-- 
2.13.1

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

[PATCH i-g-t 6/7] tests/chamelium: Reduce the simple hotplug test toggle count for VGA

[Paul Kocialkowski]

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 3ee57af6..01ae4cd7 100644
--- a/tests/chamelium.c
+++ b/tests/chamelium.c
@@ -46,6 +46,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
  */
@@ -159,7 +162,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;
@@ -167,7 +170,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 */
@@ -685,7 +688,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,
@@ -741,7 +745,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,
@@ -797,7 +802,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

[PATCH i-g-t 7/7] tests/chamelium: Disconnect connectors without extra reset

[Paul Kocialkowski]

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 01ae4cd7..7d6893da 100644
--- a/tests/chamelium.c
+++ b/tests/chamelium.c
@@ -144,17 +144,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);
 		}
@@ -167,7 +167,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++) {
@@ -201,7 +201,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);
@@ -315,7 +315,7 @@ test_suspend_resume_edid_change(data_t *data, struct chamelium_port *port,
 	struct udev_monitor *mon = igt_watch_hotplug();
 	int delay = chamelium_get_suspend_resume_delay(data->chamelium);
 
-	reset_state(data, port);
+	disconnect_connector(data, port);
 
 	igt_set_autoresume_delay(delay);
 
@@ -585,7 +585,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
@@ -607,7 +607,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);
@@ -632,7 +632,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

Re: [PATCH i-g-t 4/7] lib/igt_chamelium: Add support for configurable DUT suspend/resume delay

[Martin Peres]

On 26/06/17 16:59, Paul Kocialkowski wrote:
> This adds support for reading a SuspendResumeDelay property (under
> [DUT]) in the IGT configuration (igtrc) and exposing it through a
> chamelium_get_suspend_resume_delay function.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> ---
>   lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
>   lib/igt_chamelium.h |  1 +
>   2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index 225f98c3..a1aaf405 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -58,7 +58,7 @@
>    *	[Chamelium]
>    *	URL=http://chameleon:9992 # The URL used for connecting to the Chamelium's RPC server
>    *
> - *	# The rest of the sections are used for defining connector mappings.
> + *	# The following sections are used for defining connector mappings.
>    *	# This is required so any tests using the Chamelium know which connector
>    *	# on the test machine should be connected to each Chamelium port.
>    *	#
> @@ -70,12 +70,19 @@
>    *
>    *	[Chamelium:HDMI-A-1]
>    *	ChameliumPortID=3
> + *
> + *	# The following section is used for configuring the Device Under Test.
> + *	# It is not mandatory and allows overriding default values.
> + *	[DUT]
> + *	SuspendResumeDelay=10
>    * ]|
>    *
>    * 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.
>    */
>   
> +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
> +
>   struct chamelium_edid {
>   	int id;
>   	struct igt_list link;
> @@ -100,6 +107,7 @@ struct chamelium {
>   	xmlrpc_env env;
>   	xmlrpc_client *client;
>   	char *url;
> +	int suspend_resume_delay;
>   
>   	/* Indicates the last port to have been used for capturing video */
>   	struct chamelium_port *capturing_port;
> @@ -114,6 +122,20 @@ struct chamelium {
>   static struct chamelium *cleanup_instance;

Why do you make this part of the chamelium configuration? This should be 
common to all the suspend tests.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 4/7] lib/igt_chamelium: Add support for configurable DUT suspend/resume delay

[Paul Kocialkowski]

On Mon, 2017-06-26 at 17:25 +0300, Martin Peres wrote:
> On 26/06/17 16:59, Paul Kocialkowski wrote:
> > This adds support for reading a SuspendResumeDelay property (under
> > [DUT]) in the IGT configuration (igtrc) and exposing it through a
> > chamelium_get_suspend_resume_delay function.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> > ---
> >   lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
> >   lib/igt_chamelium.h |  1 +
> >   2 files changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > index 225f98c3..a1aaf405 100644
> > --- a/lib/igt_chamelium.c
> > +++ b/lib/igt_chamelium.c
> > @@ -58,7 +58,7 @@
> >    *	[Chamelium]
> >    *	URL=http://chameleon:9992 # The URL used for connecting to the
> > Chamelium's RPC server
> >    *
> > - *	# The rest of the sections are used for defining connector
> > mappings.
> > + *	# The following sections are used for defining connector
> > mappings.
> >    *	# This is required so any tests using the Chamelium know which
> > connector
> >    *	# on the test machine should be connected to each Chamelium
> > port.
> >    *	#
> > @@ -70,12 +70,19 @@
> >    *
> >    *	[Chamelium:HDMI-A-1]
> >    *	ChameliumPortID=3
> > + *
> > + *	# The following section is used for configuring the Device Under
> > Test.
> > + *	# It is not mandatory and allows overriding default values.
> > + *	[DUT]
> > + *	SuspendResumeDelay=10
> >    * ]|
> >    *
> >    * 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.
> >    */
> >   
> > +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
> > +
> >   struct chamelium_edid {
> >   	int id;
> >   	struct igt_list link;
> > @@ -100,6 +107,7 @@ struct chamelium {
> >   	xmlrpc_env env;
> >   	xmlrpc_client *client;
> >   	char *url;
> > +	int suspend_resume_delay;
> >   
> >   	/* Indicates the last port to have been used for capturing video
> > */
> >   	struct chamelium_port *capturing_port;
> > @@ -114,6 +122,20 @@ struct chamelium {
> >   static struct chamelium *cleanup_instance;
> 
> Why do you make this part of the chamelium configuration? This should be 
> common to all the suspend tests.

Fair enough, but at this point, only lib/igt_chamelium.c is using igtrc and all
the parsing is implemented there.

So one course of action here would be to move igtrc parsing outside of
igt_chamelium (maybe to igt_core?) and use some global variable for storing this
value. What do you think?

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

Re: [PATCH i-g-t 4/7] lib/igt_chamelium: Add support for configurable DUT suspend/resume delay

[Martin Peres]

On 26/06/17 17:33, Paul Kocialkowski wrote:
> On Mon, 2017-06-26 at 17:25 +0300, Martin Peres wrote:
>> On 26/06/17 16:59, Paul Kocialkowski wrote:
>>> This adds support for reading a SuspendResumeDelay property (under
>>> [DUT]) in the IGT configuration (igtrc) and exposing it through a
>>> chamelium_get_suspend_resume_delay function.
>>>
>>> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
>>> ---
>>>    lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
>>>    lib/igt_chamelium.h |  1 +
>>>    2 files changed, 31 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
>>> index 225f98c3..a1aaf405 100644
>>> --- a/lib/igt_chamelium.c
>>> +++ b/lib/igt_chamelium.c
>>> @@ -58,7 +58,7 @@
>>>     *	[Chamelium]
>>>     *	URL=http://chameleon:9992 # The URL used for connecting to the
>>> Chamelium's RPC server
>>>     *
>>> - *	# The rest of the sections are used for defining connector
>>> mappings.
>>> + *	# The following sections are used for defining connector
>>> mappings.
>>>     *	# This is required so any tests using the Chamelium know which
>>> connector
>>>     *	# on the test machine should be connected to each Chamelium
>>> port.
>>>     *	#
>>> @@ -70,12 +70,19 @@
>>>     *
>>>     *	[Chamelium:HDMI-A-1]
>>>     *	ChameliumPortID=3
>>> + *
>>> + *	# The following section is used for configuring the Device Under
>>> Test.
>>> + *	# It is not mandatory and allows overriding default values.
>>> + *	[DUT]
>>> + *	SuspendResumeDelay=10
>>>     * ]|
>>>     *
>>>     * 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.
>>>     */
>>>    
>>> +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
>>> +
>>>    struct chamelium_edid {
>>>    	int id;
>>>    	struct igt_list link;
>>> @@ -100,6 +107,7 @@ struct chamelium {
>>>    	xmlrpc_env env;
>>>    	xmlrpc_client *client;
>>>    	char *url;
>>> +	int suspend_resume_delay;
>>>    
>>>    	/* Indicates the last port to have been used for capturing video
>>> */
>>>    	struct chamelium_port *capturing_port;
>>> @@ -114,6 +122,20 @@ struct chamelium {
>>>    static struct chamelium *cleanup_instance;
>>
>> Why do you make this part of the chamelium configuration? This should be
>> common to all the suspend tests.
> 
> Fair enough, but at this point, only lib/igt_chamelium.c is using igtrc and all
> the parsing is implemented there.
> 
> So one course of action here would be to move igtrc parsing outside of
> igt_chamelium (maybe to igt_core?) and use some global variable for storing this
> value. What do you think?
> 

That seems like a plan, except you can just call a function that will do 
the parsing for you and return a structure with all the parameters. If 
it is called a second time, just return the cached value.

What do you think, Lyude?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 4/7] lib/igt_chamelium: Add support for configurable DUT suspend/resume delay

[Lyude Paul]

> > 
> > Fair enough, but at this point, only lib/igt_chamelium.c is using
> > igtrc and all
> > the parsing is implemented there.
> > 
> > So one course of action here would be to move igtrc parsing outside
> > of
> > igt_chamelium (maybe to igt_core?) and use some global variable for
> > storing this
> > value. What do you think?
> > 
> 
> That seems like a plan, except you can just call a function that will
> do 
> the parsing for you and return a structure with all the parameters.
> If 
> it is called a second time, just return the cached value.
> 
> What do you think, Lyude?

Yep, the reason it's called .igtrc is mainly because we were eventually
expecting someone else to start using it, and suspend/resume delay is
definitely something that will be useful for the rest of the tests
(I've got quite a few machines myself that i had to increase the delay
on just because the firmware was too slow when suspending/resuming). So
I'm all for it.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/7] tests/chamelium: Check all connectors state for basic hotplug

[Lyude Paul]

rb'd and pushed, thanks!

On Mon, 2017-06-26 at 16:59 +0300, Paul Kocialkowski wrote:
> Without doing a full reprobe, hpd toggles are sent without much delay
> between them. With a VGA connector attached, the reset occurring
> before
> the test will toggle its state, with a delay (inherent to hpd
> detection
> on VGA).
> 
> It often occurs that this VGA state toggle is detected in the middle
> of
> the current connector test, triggering a hotplug event unrelated to
> the
> current connector and thus causing the test to fail.
> 
> Thus, the state of all connectors is checked (and waited for) before
> running the basic hotplug test.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> ---
>  tests/chamelium.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/chamelium.c b/tests/chamelium.c
> index e1f21fb8..5075a52f 100644
> --- a/tests/chamelium.c
> +++ b/tests/chamelium.c
> @@ -165,7 +165,7 @@ test_basic_hotplug(data_t *data, struct
> chamelium_port *port)
>  	struct udev_monitor *mon = igt_watch_hotplug();
>  	int i;
>  
> -	reset_state(data, port);
> +	reset_state(data, NULL);
>  	igt_hpd_storm_set_threshold(data->drm_fd, 0);
>  
>  	for (i = 0; i < 15; i++) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 3/7] tests/chamelium: Use 50 ms delay to wait for connector change

[Lyude Paul]

rb'd and pushed, thanks!

On Mon, 2017-06-26 at 16:59 +0300, Paul Kocialkowski wrote:
> Using a 1 s delay seems too large for detecting a connector change,
> that
> may happen faster than this. Since the constraints on execution time
> are
> high, it is preferable to use a much smaller sleep time.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> ---
>  tests/chamelium.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/chamelium.c b/tests/chamelium.c
> index 5075a52f..15ed0f28 100644
> --- a/tests/chamelium.c
> +++ b/tests/chamelium.c
> @@ -135,7 +135,7 @@ wait_for_connector(data_t *data, struct
> chamelium_port *port,
>  			return;
>  		}
>  
> -		sleep(1);
> +		usleep(50000);
>  	}
>  
>  	igt_assert(finished);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 7/7] tests/chamelium: Disconnect connectors without extra reset

[Lyude Paul]

On Mon, 2017-06-26 at 16:59 +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>

NAK
It's annoying that we have to reset the connectors so much, but
unfortunately there is a reason in this case. The problem is that
chamelium_init() does not get called once per test. At first glance
when running through ./scripts/run_tests.sh this might appear to be the
case, however if you run the test by manually calling the built
chamelium binary (generally useful for debugging the tests themselves
with gdb) it won't actually get called in-between tests.

This being said, I'm all for getting rid of a couple extraneous hotplug
resets if possible, but I'd prefer to avoid breaking the ability to run
the chamelium test binary manually.

> ---
>  tests/chamelium.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/chamelium.c b/tests/chamelium.c
> index 01ae4cd7..7d6893da 100644
> --- a/tests/chamelium.c
> +++ b/tests/chamelium.c
> @@ -144,17 +144,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);
>  		}
> @@ -167,7 +167,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++) {
> @@ -201,7 +201,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);
> @@ -315,7 +315,7 @@ test_suspend_resume_edid_change(data_t *data,
> struct chamelium_port *port,
>  	struct udev_monitor *mon = igt_watch_hotplug();
>  	int delay = chamelium_get_suspend_resume_delay(data-
> >chamelium);
>  
> -	reset_state(data, port);
> +	disconnect_connector(data, port);
>  
>  	igt_set_autoresume_delay(delay);
>  
> @@ -585,7 +585,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
> @@ -607,7 +607,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);
> @@ -632,7 +632,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

Re: [PATCH i-g-t 7/7] tests/chamelium: Disconnect connectors without extra reset

[Paul Kocialkowski]

Hey,

On Mon, 2017-06-26 at 18:17 -0400, Lyude Paul wrote:
> On Mon, 2017-06-26 at 16:59 +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>
> 
> NAK
> It's annoying that we have to reset the connectors so much, but
> unfortunately there is a reason in this case. The problem is that
> chamelium_init() does not get called once per test. At first glance
> when running through ./scripts/run_tests.sh this might appear to be the
> case, however if you run the test by manually calling the built
> chamelium binary (generally useful for debugging the tests themselves
> with gdb) it won't actually get called in-between tests.

I am well aware of that situation and made sure that it works correctly in that
case too. Disconnecting the connector is really all that should be needed, and
resetting the whole chamelium seems overkill.

There was one occurence where not fully resetting caused an issue when running
the whole test binary on i915 (when switching from poll-based detection to irq
after hpd storm testing), which was definitely a kernel bug. I submitted a patch
to fix it already.

So this can even help discover new kernel bugs!

> This being said, I'm all for getting rid of a couple extraneous hotplug
> resets if possible, but I'd prefer to avoid breaking the ability to run
> the chamelium test binary manually.

Yeah, the real problem was that when running tests standalone, two resets are
done in a row. So worst case, we could ditch the reset at init and make sure
each test calls it.

Thanks for the review!

> > ---
> >  tests/chamelium.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tests/chamelium.c b/tests/chamelium.c
> > index 01ae4cd7..7d6893da 100644
> > --- a/tests/chamelium.c
> > +++ b/tests/chamelium.c
> > @@ -144,17 +144,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);
> >  		}
> > @@ -167,7 +167,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++) {
> > @@ -201,7 +201,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);
> > @@ -315,7 +315,7 @@ test_suspend_resume_edid_change(data_t *data,
> > struct chamelium_port *port,
> >  	struct udev_monitor *mon = igt_watch_hotplug();
> >  	int delay = chamelium_get_suspend_resume_delay(data-
> > > chamelium);
> > 
> >  
> > -	reset_state(data, port);
> > +	disconnect_connector(data, port);
> >  
> >  	igt_set_autoresume_delay(delay);
> >  
> > @@ -585,7 +585,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
> > @@ -607,7 +607,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);
> > @@ -632,7 +632,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

Re: [PATCH i-g-t 4/7] lib/igt_chamelium: Add support for configurable DUT suspend/resume delay

[Paul Kocialkowski]

On Mon, 2017-06-26 at 18:15 +0300, Martin Peres wrote:
> On 26/06/17 17:33, Paul Kocialkowski wrote:
> > On Mon, 2017-06-26 at 17:25 +0300, Martin Peres wrote:
> > > On 26/06/17 16:59, Paul Kocialkowski wrote:
> > > > This adds support for reading a SuspendResumeDelay property (under
> > > > [DUT]) in the IGT configuration (igtrc) and exposing it through a
> > > > chamelium_get_suspend_resume_delay function.
> > > > 
> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> > > > ---
> > > >    lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
> > > >    lib/igt_chamelium.h |  1 +
> > > >    2 files changed, 31 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > > > index 225f98c3..a1aaf405 100644
> > > > --- a/lib/igt_chamelium.c
> > > > +++ b/lib/igt_chamelium.c
> > > > @@ -58,7 +58,7 @@
> > > >     *	[Chamelium]
> > > >     *	URL=http://chameleon:9992 # The URL used for connecting to
> > > > the
> > > > Chamelium's RPC server
> > > >     *
> > > > - *	# The rest of the sections are used for defining connector
> > > > mappings.
> > > > + *	# The following sections are used for defining connector
> > > > mappings.
> > > >     *	# This is required so any tests using the Chamelium know
> > > > which
> > > > connector
> > > >     *	# on the test machine should be connected to each Chamelium
> > > > port.
> > > >     *	#
> > > > @@ -70,12 +70,19 @@
> > > >     *
> > > >     *	[Chamelium:HDMI-A-1]
> > > >     *	ChameliumPortID=3
> > > > + *
> > > > + *	# The following section is used for configuring the Device
> > > > Under
> > > > Test.
> > > > + *	# It is not mandatory and allows overriding default values.
> > > > + *	[DUT]
> > > > + *	SuspendResumeDelay=10
> > > >     * ]|
> > > >     *
> > > >     * 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.
> > > >     */
> > > >    
> > > > +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
> > > > +
> > > >    struct chamelium_edid {
> > > >    	int id;
> > > >    	struct igt_list link;
> > > > @@ -100,6 +107,7 @@ struct chamelium {
> > > >    	xmlrpc_env env;
> > > >    	xmlrpc_client *client;
> > > >    	char *url;
> > > > +	int suspend_resume_delay;
> > > >    
> > > >    	/* Indicates the last port to have been used for capturing
> > > > video
> > > > */
> > > >    	struct chamelium_port *capturing_port;
> > > > @@ -114,6 +122,20 @@ struct chamelium {
> > > >    static struct chamelium *cleanup_instance;
> > > 
> > > Why do you make this part of the chamelium configuration? This should be
> > > common to all the suspend tests.
> > 
> > Fair enough, but at this point, only lib/igt_chamelium.c is using igtrc and
> > all
> > the parsing is implemented there.
> > 
> > So one course of action here would be to move igtrc parsing outside of
> > igt_chamelium (maybe to igt_core?) and use some global variable for storing
> > this
> > value. What do you think?
> > 
> 
> That seems like a plan, except you can just call a function that will do 
> the parsing for you and return a structure with all the parameters. If 
> it is called a second time, just return the cached value.

Thinking about it twice, I think it would make a lot more sense to delegate (at
least some of) the parsing to specific parts of the code. For instance for
chamelium, the data structures are really only defined in igt_chamelium and I
don't think it would make a lot of sense to make those common just for the sake
of returning a common configuration structure. We could also have some
intermediate representation of that data for that structure, which isn't very
straightforward either.

So I would suggest making GKeyFile *igt_key_file; a global (instead of the
parsed parameters) so that specific parts of IGT can use it to get the keys they
want.

Then, having a common parsing function for common things (suspend/resume delay
would fit well there). Also, we already have igt_set_autoresume_delay so no
extra global variable would be needed for that (or rather, it already exists).

Does that seem agreeable?

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

Re: [PATCH i-g-t 4/7] lib/igt_chamelium: Add support for configurable DUT suspend/resume delay

[Paul Kocialkowski]

On Tue, 2017-06-27 at 10:33 +0300, Paul Kocialkowski wrote:
> On Mon, 2017-06-26 at 18:15 +0300, Martin Peres wrote:
> > On 26/06/17 17:33, Paul Kocialkowski wrote:
> > > On Mon, 2017-06-26 at 17:25 +0300, Martin Peres wrote:
> > > > On 26/06/17 16:59, Paul Kocialkowski wrote:
> > > > > This adds support for reading a SuspendResumeDelay property (under
> > > > > [DUT]) in the IGT configuration (igtrc) and exposing it through a
> > > > > chamelium_get_suspend_resume_delay function.
> > > > > 
> > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> > > > > ---
> > > > >    lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
> > > > >    lib/igt_chamelium.h |  1 +
> > > > >    2 files changed, 31 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > > > > index 225f98c3..a1aaf405 100644
> > > > > --- a/lib/igt_chamelium.c
> > > > > +++ b/lib/igt_chamelium.c
> > > > > @@ -58,7 +58,7 @@
> > > > >     *	[Chamelium]
> > > > >     *	URL=http://chameleon:9992 # The URL used for connecting
> > > > > to
> > > > > the
> > > > > Chamelium's RPC server
> > > > >     *
> > > > > - *	# The rest of the sections are used for defining connector
> > > > > mappings.
> > > > > + *	# The following sections are used for defining connector
> > > > > mappings.
> > > > >     *	# This is required so any tests using the Chamelium know
> > > > > which
> > > > > connector
> > > > >     *	# on the test machine should be connected to each
> > > > > Chamelium
> > > > > port.
> > > > >     *	#
> > > > > @@ -70,12 +70,19 @@
> > > > >     *
> > > > >     *	[Chamelium:HDMI-A-1]
> > > > >     *	ChameliumPortID=3
> > > > > + *
> > > > > + *	# The following section is used for configuring the Device
> > > > > Under
> > > > > Test.
> > > > > + *	# It is not mandatory and allows overriding default values.
> > > > > + *	[DUT]
> > > > > + *	SuspendResumeDelay=10
> > > > >     * ]|
> > > > >     *
> > > > >     * 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.
> > > > >     */
> > > > >    
> > > > > +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
> > > > > +
> > > > >    struct chamelium_edid {
> > > > >    	int id;
> > > > >    	struct igt_list link;
> > > > > @@ -100,6 +107,7 @@ struct chamelium {
> > > > >    	xmlrpc_env env;
> > > > >    	xmlrpc_client *client;
> > > > >    	char *url;
> > > > > +	int suspend_resume_delay;
> > > > >    
> > > > >    	/* Indicates the last port to have been used for capturing
> > > > > video
> > > > > */
> > > > >    	struct chamelium_port *capturing_port;
> > > > > @@ -114,6 +122,20 @@ struct chamelium {
> > > > >    static struct chamelium *cleanup_instance;
> > > > 
> > > > Why do you make this part of the chamelium configuration? This should be
> > > > common to all the suspend tests.
> > > 
> > > Fair enough, but at this point, only lib/igt_chamelium.c is using igtrc
> > > and
> > > all
> > > the parsing is implemented there.
> > > 
> > > So one course of action here would be to move igtrc parsing outside of
> > > igt_chamelium (maybe to igt_core?) and use some global variable for
> > > storing
> > > this
> > > value. What do you think?
> > > 
> > 
> > That seems like a plan, except you can just call a function that will do 
> > the parsing for you and return a structure with all the parameters. If 
> > it is called a second time, just return the cached value.
> 
> Thinking about it twice, I think it would make a lot more sense to delegate
> (at
> least some of) the parsing to specific parts of the code. For instance for
> chamelium, the data structures are really only defined in igt_chamelium and I
> don't think it would make a lot of sense to make those common just for the
> sake
> of returning a common configuration structure. We could also have some
> intermediate representation of that data for that structure, which isn't very
> straightforward either.
> 
> So I would suggest making GKeyFile *igt_key_file; a global (instead of the
> parsed parameters) so that specific parts of IGT can use it to get the keys
> they
> want.
> 
> Then, having a common parsing function for common things (suspend/resume delay
> would fit well there). Also, we already have igt_set_autoresume_delay so no
> extra global variable would be needed for that (or rather, it already exists).
> 
> Does that seem agreeable?

By the way, I suggest sending this as a follow-up patch, since moving the config
out of IGT is a separate topic in itself. Would that make sense?

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

Re: [PATCH i-g-t 4/7] lib/igt_chamelium: Add support for configurable DUT suspend/resume delay

[Paul Kocialkowski]

On Tue, 2017-06-27 at 10:45 +0300, Paul Kocialkowski wrote:
> On Tue, 2017-06-27 at 10:33 +0300, Paul Kocialkowski wrote:
> > On Mon, 2017-06-26 at 18:15 +0300, Martin Peres wrote:
> > > On 26/06/17 17:33, Paul Kocialkowski wrote:
> > > > On Mon, 2017-06-26 at 17:25 +0300, Martin Peres wrote:
> > > > > On 26/06/17 16:59, Paul Kocialkowski wrote:
> > > > > > This adds support for reading a SuspendResumeDelay property (under
> > > > > > [DUT]) in the IGT configuration (igtrc) and exposing it through a
> > > > > > chamelium_get_suspend_resume_delay function.
> > > > > > 
> > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
> > > > > > ---
> > > > > >    lib/igt_chamelium.c | 31 ++++++++++++++++++++++++++++++-
> > > > > >    lib/igt_chamelium.h |  1 +
> > > > > >    2 files changed, 31 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > > > > > index 225f98c3..a1aaf405 100644
> > > > > > --- a/lib/igt_chamelium.c
> > > > > > +++ b/lib/igt_chamelium.c
> > > > > > @@ -58,7 +58,7 @@
> > > > > >     *	[Chamelium]
> > > > > >     *	URL=http://chameleon:9992 # The URL used for connecting
> > > > > > to
> > > > > > the
> > > > > > Chamelium's RPC server
> > > > > >     *
> > > > > > - *	# The rest of the sections are used for defining
> > > > > > connector
> > > > > > mappings.
> > > > > > + *	# The following sections are used for defining connector
> > > > > > mappings.
> > > > > >     *	# This is required so any tests using the Chamelium
> > > > > > know
> > > > > > which
> > > > > > connector
> > > > > >     *	# on the test machine should be connected to each
> > > > > > Chamelium
> > > > > > port.
> > > > > >     *	#
> > > > > > @@ -70,12 +70,19 @@
> > > > > >     *
> > > > > >     *	[Chamelium:HDMI-A-1]
> > > > > >     *	ChameliumPortID=3
> > > > > > + *
> > > > > > + *	# The following section is used for configuring the
> > > > > > Device
> > > > > > Under
> > > > > > Test.
> > > > > > + *	# It is not mandatory and allows overriding default
> > > > > > values.
> > > > > > + *	[DUT]
> > > > > > + *	SuspendResumeDelay=10
> > > > > >     * ]|
> > > > > >     *
> > > > > >     * 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.
> > > > > >     */
> > > > > >    
> > > > > > +#define SUSPEND_RESUME_DELAY_DEFAULT 20 /* seconds */
> > > > > > +
> > > > > >    struct chamelium_edid {
> > > > > >    	int id;
> > > > > >    	struct igt_list link;
> > > > > > @@ -100,6 +107,7 @@ struct chamelium {
> > > > > >    	xmlrpc_env env;
> > > > > >    	xmlrpc_client *client;
> > > > > >    	char *url;
> > > > > > +	int suspend_resume_delay;
> > > > > >    
> > > > > >    	/* Indicates the last port to have been used for
> > > > > > capturing
> > > > > > video
> > > > > > */
> > > > > >    	struct chamelium_port *capturing_port;
> > > > > > @@ -114,6 +122,20 @@ struct chamelium {
> > > > > >    static struct chamelium *cleanup_instance;
> > > > > 
> > > > > Why do you make this part of the chamelium configuration? This should
> > > > > be
> > > > > common to all the suspend tests.
> > > > 
> > > > Fair enough, but at this point, only lib/igt_chamelium.c is using igtrc
> > > > and
> > > > all
> > > > the parsing is implemented there.
> > > > 
> > > > So one course of action here would be to move igtrc parsing outside of
> > > > igt_chamelium (maybe to igt_core?) and use some global variable for
> > > > storing
> > > > this
> > > > value. What do you think?
> > > > 
> > > 
> > > That seems like a plan, except you can just call a function that will do 
> > > the parsing for you and return a structure with all the parameters. If 
> > > it is called a second time, just return the cached value.
> > 
> > Thinking about it twice, I think it would make a lot more sense to delegate
> > (at
> > least some of) the parsing to specific parts of the code. For instance for
> > chamelium, the data structures are really only defined in igt_chamelium and
> > I
> > don't think it would make a lot of sense to make those common just for the
> > sake
> > of returning a common configuration structure. We could also have some
> > intermediate representation of that data for that structure, which isn't
> > very
> > straightforward either.
> > 
> > So I would suggest making GKeyFile *igt_key_file; a global (instead of the
> > parsed parameters) so that specific parts of IGT can use it to get the keys
> > they
> > want.
> > 
> > Then, having a common parsing function for common things (suspend/resume
> > delay
> > would fit well there). Also, we already have igt_set_autoresume_delay so no
> > extra global variable would be needed for that (or rather, it already
> > exists).
> > 
> > Does that seem agreeable?
> 
> By the way, I suggest sending this as a follow-up patch, since moving the
> config out of IGT is a separate topic in itself. Would that make sense?

Or maybe not, it's way too unreadable to do this in many patches, where things
get added and removed later. I'm crafting v2 as a single patch with the proposal
I have in mind. Let's follow-up the discussing there.

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