[igt-dev] [PATCH i-g-t 1/3] lib/kms_chamelium: Use FireHpdPulse for chamelium_hpd_send_pulses()

[Lyude]

From: Lyude Paul <lyude at redhat.com>

While trying to implement a short HPD storm detection test, I noticed
that I couldn't actually manage to get the Chamelium to properly fire
short HPD signals, instead all of the signals it was sending would be
interpreted as long. However-when using my chameleond client, I could
get it to send short pulses just fine.

It turns out the difference between the two is the RPC call that they
were making to send pulses. Currently the igt library uses
FireMixedHpdPulses() on the chamelium for any kind of hotplug pulses,
but my chameleond client was using FireHpdPulse().

So I did a benchmark of the two RPC calls, sending 21 pulses each with a
width of 2ms per pulse. The problem then became very obvious:

FireMixedHpdPulses: 0.168565ms total, ~8ms per-pulse
FireHpdPulse: 0.052269ms total, ~2.4ms per-pulse

FireMixedHpdPulses() is literally too slow to fire a proper short HPD
pulse. This makes sense: internally FireMixedHpdPulses() sends each
pulse by writing to the appropriate registers using only python2, and
FireHpdPulse() just calls down to a C helper which handles sending the
repeated pulses from there.

While this is definitely a bug that needs to be fixed in chameleond, we
can at least workaround it for the time being by switching to using
FireHpdPulse() in chamelium_hpd_send_pulses() instead of
FireMixedHpdPulses(). This is probably a better idea in the long run
anyway, since being able to specify whether we end in a low or high
pulse is a lot less awkward for users then having to change that
behavior by ensuring the count of pulses is even or odd, something which
FireMixedHpdPulses() required. Additionally-it seems the C helper that
chameleond is supposed to be (but isn't) using for mixed pulses only
accepts up to 20 pulses anyway.

Signed-off-by: Lyude Paul <lyude at redhat.com>
---
 lib/igt_chamelium.c   | 39 ++++++++++++++++++++++-----------------
 lib/igt_chamelium.h   |  2 +-
 tests/kms_chamelium.c |  6 +++---
 3 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index a80ead16..fd17c1a0 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -394,6 +394,7 @@ bool chamelium_port_wait_video_input_stable(struct chamelium *chamelium,
  * @port: The port to fire the HPD pulses on
  * @width_msec: How long each pulse should last
  * @count: The number of pulses to send
+ * @end_high: Whether or not to end with a high pulse
  *
  * A convienence function for sending multiple hotplug pulses to the system.
  * The pulses start at low (e.g. connector is disconnected), and then alternate
@@ -401,29 +402,33 @@ bool chamelium_port_wait_video_input_stable(struct chamelium *chamelium,
  * repeatedly calling #chamelium_plug and #chamelium_unplug, waiting
  * @width_msec between each call.
  *
- * If @count is even, the last pulse sent will be high, and if it's odd then it
- * will be low. Resetting the HPD line back to it's previous state, if desired,
- * is the responsibility of the caller.
+ * If @end_high is set, then the last pulse that is sent will be high, otherwise
+ * it will be low. Keep in mind that enabling this option will potentially
+ * result in the system registering one additional hotplug signal.
+ *
+ * It should be noted that in many cases, sending HPD pulses with very short
+ * durations (like 2ms for a DP short pulse) will likely have the first and last
+ * pulse have longer durations then specified, while the pulses inbetween will
+ * be equivalent to @width_msec. Callers should compensate for this in their
+ * tests.
+ *
  */
 void chamelium_fire_hpd_pulses(struct chamelium *chamelium,
 			       struct chamelium_port *port,
-			       int width_msec, int count)
+			       int width_msec, int count, bool end_high)
 {
-	xmlrpc_value *pulse_widths = xmlrpc_array_new(&chamelium->env);
-	xmlrpc_value *width = xmlrpc_int_new(&chamelium->env, width_msec);
-	int i;
-
-	igt_debug("Firing %d HPD pulses with width of %d msec on %s\n",
-		  count, width_msec, port->name);
-
-	for (i = 0; i < count; i++)
-		xmlrpc_array_append_item(&chamelium->env, pulse_widths, width);
+	/* We half the width here and split it between the assert and deassert
+	 * interval, since the sum of the two will be the real-world pulse
+	 * length
+	 */
+	int width_usec = (width_msec * 1000) / 2;
 
-	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "FireMixedHpdPulses",
-				    "(iA)", port->id, pulse_widths));
+	igt_debug("Firing %d HPD pulses with width of %d msec on %s, ending %s\n",
+		  count, width_msec, port->name, end_high ? "high" : "low");
 
-	xmlrpc_DECREF(width);
-	xmlrpc_DECREF(pulse_widths);
+	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "FireHpdPulse", "(iiiii)",
+				    port->id, width_usec, width_usec, count,
+				    end_high));
 }
 
 /**
diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
index af9655a0..46fca1b1 100644
--- a/lib/igt_chamelium.h
+++ b/lib/igt_chamelium.h
@@ -63,7 +63,7 @@ void chamelium_fire_mixed_hpd_pulses(struct chamelium *chamelium,
 				     struct chamelium_port *port, ...);
 void chamelium_fire_hpd_pulses(struct chamelium *chamelium,
 			       struct chamelium_port *port,
-			       int width_msec, int count);
+			       int width_msec, int count, bool end_high);
 void chamelium_schedule_hpd_toggle(struct chamelium *chamelium,
 				   struct chamelium_port *port, int delay_ms,
 				   bool rising_edge);
diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index e0e3e3f1..f051344d 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -743,11 +743,11 @@ test_hpd_storm_detect(data_t *data, struct chamelium_port *port, int width)
 	reset_state(data, port);
 
 	igt_hpd_storm_set_threshold(data->drm_fd, 1);
-	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10);
+	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10, false);
 	igt_assert(igt_hpd_storm_detected(data->drm_fd));
 
 	mon = igt_watch_hotplug();
-	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10);
+	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10, false);
 
 	/*
 	 * Polling should have been enabled by the HPD storm at this point,
@@ -769,7 +769,7 @@ test_hpd_storm_disable(data_t *data, struct chamelium_port *port, int width)
 
 	igt_hpd_storm_set_threshold(data->drm_fd, 0);
 	chamelium_fire_hpd_pulses(data->chamelium, port,
-				  width, 10);
+				  width, 10, false);
 	igt_assert(!igt_hpd_storm_detected(data->drm_fd));
 
 	igt_hpd_storm_reset(data->drm_fd);
-- 
2.19.1