[igt-dev] [PATCH i-g-t 0/3] Add tests for HPD short storm detection

[igt-dev] [PATCH i-g-t 0/3] Add tests for HPD short storm detection

[Lyude]

From: Lyude Paul <lyude@redhat.com>

Adds some tests for HPD short storm detection, along with some fixes
to the chamelium helpers that are required to make this work.

Lyude Paul (3):
  lib/kms_chamelium: Use FireHpdPulse for chamelium_hpd_send_pulses()
  tests/kms_chamelium: Increase HPD storm threshold to 10 from 1
  tests/kms_chamelium: Introduce HPD short storm tests

 lib/igt_chamelium.c   | 39 ++++++++++---------
 lib/igt_chamelium.h   |  2 +-
 lib/igt_debugfs.c     | 88 +++++++++++++++++++++++++++++++++++++++++++
 lib/igt_debugfs.h     |  4 ++
 tests/kms_chamelium.c | 53 ++++++++++++++++++++++++--
 5 files changed, 164 insertions(+), 22 deletions(-)

-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Lyude]

From: Lyude Paul <lyude@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@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

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 2/3] tests/kms_chamelium: Increase HPD storm threshold to 10 from 1

[Lyude]

From: Lyude Paul <lyude@redhat.com>

This is to adjust for the changes to how long pulses are counted towards
HPD storms that were made in https://patchwork.freedesktop.org/patch/260597/

Also, update the appropriate documentation in igt to reflect this.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 lib/igt_debugfs.c     | 3 +++
 tests/kms_chamelium.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
index a3aca846..358b4cab 100644
--- a/lib/igt_debugfs.c
+++ b/lib/igt_debugfs.c
@@ -557,6 +557,9 @@ static void igt_hpd_storm_exit_handler(int sig)
  * through debugfs. Useful for hotplugging tests where HPD storm detection
  * might get in the way and slow things down.
  *
+ * Note that each long HPD pulse will count as 10 towards the threshold, and
+ * short HPD pulses (if enabled) will count as 1 towards the threshold.
+ *
  * If the system does not support HPD storm detection, this function does
  * nothing.
  *
diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index f051344d..0097dbcc 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -742,7 +742,7 @@ test_hpd_storm_detect(data_t *data, struct chamelium_port *port, int width)
 	igt_require_hpd_storm_ctl(data->drm_fd);
 	reset_state(data, port);
 
-	igt_hpd_storm_set_threshold(data->drm_fd, 1);
+	igt_hpd_storm_set_threshold(data->drm_fd, 10);
 	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10, false);
 	igt_assert(igt_hpd_storm_detected(data->drm_fd));
 
-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 3/3] tests/kms_chamelium: Introduce HPD short storm tests

[Lyude]

From: Lyude Paul <lyude@redhat.com>

This is to follow up with the changes that were recently made to HPD
storm detection in https://patchwork.freedesktop.org/patch/260597/ , as
now there are systems which will be counting short IRQs towards HPD
storms. As such, we introduce helpers to control HPD short storm
detection, along with some new kms_chamelium tests for it.

Please note that at the time of writing this: these tests currently
fail. That is because the i915_hpd_storm_ctl debugfs currently forgets
to synchronize with all IRQs and hotplugging work before returning the
HPD storm status to the user. A patch for this will be sent onto the ML
very shortly.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 lib/igt_debugfs.c     | 85 +++++++++++++++++++++++++++++++++++++++++++
 lib/igt_debugfs.h     |  4 ++
 tests/kms_chamelium.c | 45 +++++++++++++++++++++++
 3 files changed, 134 insertions(+)

diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
index 358b4cab..530b70b7 100644
--- a/lib/igt_debugfs.c
+++ b/lib/igt_debugfs.c
@@ -543,6 +543,7 @@ static void igt_hpd_storm_exit_handler(int sig)
 	int fd = drm_open_driver(DRIVER_INTEL);
 
 	/* Here we assume that only one i915 device will be ever present */
+	igt_hpd_short_storm_reset(fd);
 	igt_hpd_storm_reset(fd);
 
 	close(fd);
@@ -660,6 +661,90 @@ void igt_require_hpd_storm_ctl(int drm_fd)
 
 	igt_require_f(fd > 0, "No i915_hpd_storm_ctl found in debugfs\n");
 	close(fd);
+
+	fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_RDONLY);
+	igt_require_f(fd > 0, "No i915_hpd_short_storm_ctl found in debugfs\n");
+	close(fd);
+}
+
+/**
+ * igt_hpd_short_storm_set_enabled:
+ *
+ * Convienence helper to enable or disable HPD short storm detection. If hotplug
+ * detection was disabled on any ports due to an HPD storm, it will be
+ * immediately re-enabled.
+ *
+ * If the system does not support HPD storm detection, this function does
+ * nothing.
+ *
+ * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
+ */
+void igt_hpd_short_storm_set_enabled(int drm_fd, bool enabled)
+{
+	int fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_WRONLY);
+	const char *buf = enabled ? "y" : "n";
+
+	if (fd < 0)
+		return;
+
+	igt_debug("%sabling HPD short storm detection\n",
+		  enabled ? "En" : "Dis");
+	igt_assert_eq(write(fd, buf, strlen(buf)), strlen(buf));
+
+	close(fd);
+}
+
+/**
+ * igt_hpd_short_storm_reset:
+ *
+ * Convienence helper to reset HPD short storm detection to it's default settings.
+ * If hotplug detection was disabled on any ports due to an HPD storm, it will
+ * be immediately re-enabled. Always called on exit if the HPD storm detection
+ * threshold was modified during any tests.
+ *
+ * If the system does not support HPD storm detection, this function does
+ * nothing.
+ *
+ * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
+ */
+void igt_hpd_short_storm_reset(int drm_fd)
+{
+	int fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_WRONLY);
+	const char *buf = "reset";
+
+	if (fd < 0)
+		return;
+
+	igt_debug("Resetting HPD short storm detection\n");
+	igt_assert_eq(write(fd, buf, strlen(buf)), strlen(buf));
+
+	close(fd);
+}
+
+/**
+ * igt_require_hpd_short_storm_on_by_default:
+ *
+ * Skips the current test if the system does not have HPD short storm detection
+ * enabled by default.
+ *
+ * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
+ */
+void igt_require_hpd_short_storm_on_by_default(int drm_fd)
+{
+	int fd;
+	char buf[16] = {0};
+
+	/* First reset short storm detection to the system default */
+	igt_hpd_short_storm_reset(drm_fd);
+
+	/* Now check if the default is enabled or not */
+	fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_RDONLY);
+
+	igt_require_f(fd > 0, "No i915_hpd_short_storm_ctl found in debugfs\n");
+	igt_assert_lt(0, read(fd, buf, sizeof(buf)));
+	igt_require_f(strncmp(buf, "Enabled: yes\n", sizeof(buf)) == 0,
+		      "System keeps HPD short storm detection disabled by default\n");
+	close(fd);
 }
 
 static igt_pipe_crc_t *
diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
index 1233cd8f..21b382a3 100644
--- a/lib/igt_debugfs.h
+++ b/lib/igt_debugfs.h
@@ -143,6 +143,10 @@ void igt_hpd_storm_reset(int fd);
 bool igt_hpd_storm_detected(int fd);
 void igt_require_hpd_storm_ctl(int fd);
 
+void igt_hpd_short_storm_set_enabled(int fd, bool enabled);
+void igt_hpd_short_storm_reset(int fd);
+void igt_require_hpd_short_storm_on_by_default(int fd);
+
 /*
  * Drop caches
  */
diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index 0097dbcc..7ab33830 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -45,6 +45,7 @@ typedef struct {
 #define HOTPLUG_TIMEOUT 20 /* seconds */
 
 #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
+#define HPD_SHORT_STORM_PULSE_INTERVAL_DP 2 /* ms */
 #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
 
 #define HPD_TOGGLE_COUNT_VGA 5
@@ -775,6 +776,44 @@ test_hpd_storm_disable(data_t *data, struct chamelium_port *port, int width)
 	igt_hpd_storm_reset(data->drm_fd);
 }
 
+static void
+test_hpd_short_storm_detect(data_t *data, struct chamelium_port *port)
+{
+	igt_require_hpd_storm_ctl(data->drm_fd);
+	igt_require_hpd_short_storm_on_by_default(data->drm_fd);
+	reset_state(data, port);
+
+	/* Leave room in the threshold for the two long pulses this might
+	 * cause
+	 */
+	igt_hpd_storm_set_threshold(data->drm_fd, 21);
+	chamelium_fire_hpd_pulses(data->chamelium, port,
+				  HPD_SHORT_STORM_PULSE_INTERVAL_DP, 25, false);
+	igt_assert(igt_hpd_storm_detected(data->drm_fd));
+
+	igt_hpd_storm_reset(data->drm_fd);
+}
+
+static void
+test_hpd_short_storm_disable(data_t *data, struct chamelium_port *port)
+{
+	igt_require_hpd_storm_ctl(data->drm_fd);
+	reset_state(data, port);
+
+	igt_hpd_short_storm_set_enabled(data->drm_fd, false);
+
+	/* Leave room in the threshold for the two long pulses this might
+	 * cause
+	 */
+	igt_hpd_storm_set_threshold(data->drm_fd, 21);
+	chamelium_fire_hpd_pulses(data->chamelium, port,
+				  HPD_SHORT_STORM_PULSE_INTERVAL_DP, 25, false);
+	igt_assert(!igt_hpd_storm_detected(data->drm_fd));
+
+	igt_hpd_short_storm_reset(data->drm_fd);
+	igt_hpd_storm_reset(data->drm_fd);
+}
+
 #define for_each_port(p, port)            \
 	for (p = 0, port = data.ports[p]; \
 	     p < data.port_count;         \
@@ -856,6 +895,12 @@ igt_main
 			test_hpd_storm_disable(&data, port,
 					       HPD_STORM_PULSE_INTERVAL_DP);
 
+		connector_subtest("dp-hpd-short-storm", DisplayPort)
+			test_hpd_short_storm_detect(&data, port);
+
+		connector_subtest("dp-hpd-short-storm-disable", DisplayPort)
+			test_hpd_short_storm_disable(&data, port);
+
 		connector_subtest("dp-edid-change-during-suspend", DisplayPort)
 			test_suspend_resume_edid_change(&data, port,
 							SUSPEND_STATE_MEM,
-- 
2.19.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for Add tests for HPD short storm detection

[Patchwork]

== Series Details ==

Series: Add tests for HPD short storm detection
URL   : https://patchwork.freedesktop.org/series/52794/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4723 -> IGTPW_2082 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/52794/revisions/1/mbox/

== Known issues ==

  Here are the changes found in IGTPW_2082 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_close_race@basic-threads:
      fi-bsw-kefka:       PASS -> FAIL (fdo#108656)

    igt@gem_exec_suspend@basic-s3:
      fi-icl-u2:          PASS -> DMESG-WARN (fdo#107724)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
      fi-byt-clapper:     PASS -> FAIL (fdo#103191, fdo#107362)

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362)

    
    ==== Possible fixes ====

    igt@gem_ctx_switch@basic-default:
      fi-icl-u2:          DMESG-WARN (fdo#107724) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     FAIL (fdo#103191, fdo#107362) -> PASS

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
  fdo#108656 https://bugs.freedesktop.org/show_bug.cgi?id=108656


== Participating hosts (52 -> 46) ==

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-u3 


== Build changes ==

    * IGT: IGT_4723 -> IGTPW_2082

  CI_DRM_5174: 0bfa7192170c039a271ebc27222b4b91516e73f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2082: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2082/
  IGT_4723: 53bb24ad410b53cdd96f15ced8fd5921c8ab0eac @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@kms_chamelium@dp-hpd-short-storm
+igt@kms_chamelium@dp-hpd-short-storm-disable

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2082/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for Add tests for HPD short storm detection

[Patchwork]

== Series Details ==

Series: Add tests for HPD short storm detection
URL   : https://patchwork.freedesktop.org/series/52794/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4723_full -> IGTPW_2082_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_2082_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_2082_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/52794/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_2082_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_tiled_swapping@non-threaded:
      shard-apl:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in IGTPW_2082_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_available_modes_crc@available_mode_test_crc:
      shard-apl:          PASS -> FAIL (fdo#106641)

    igt@kms_busy@extended-modeset-hang-newfb-render-a:
      shard-hsw:          PASS -> DMESG-WARN (fdo#107956)

    igt@kms_color@pipe-b-ctm-max:
      shard-apl:          PASS -> FAIL (fdo#108147)
      shard-kbl:          PASS -> FAIL (fdo#108147)

    igt@kms_cursor_crc@cursor-128x128-random:
      shard-apl:          PASS -> FAIL (fdo#103232) +5

    igt@kms_cursor_crc@cursor-64x64-onscreen:
      shard-kbl:          PASS -> FAIL (fdo#103232) +1

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#105363)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-wc:
      shard-glk:          PASS -> FAIL (fdo#103167) +2

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
      shard-apl:          PASS -> FAIL (fdo#103167) +1
      shard-kbl:          PASS -> FAIL (fdo#103167) +1

    igt@kms_plane@plane-position-covered-pipe-a-planes:
      shard-glk:          PASS -> FAIL (fdo#103166) +3

    igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
      shard-kbl:          PASS -> FAIL (fdo#103166)

    igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
      shard-apl:          PASS -> FAIL (fdo#103166)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic:
      shard-snb:          INCOMPLETE (fdo#107469, fdo#105411) -> PASS

    igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
      shard-glk:          FAIL (fdo#108145) -> PASS

    igt@kms_cursor_crc@cursor-64x64-sliding:
      shard-glk:          FAIL (fdo#103232) -> PASS +3
      shard-kbl:          FAIL (fdo#103232) -> PASS

    igt@kms_cursor_legacy@cursora-vs-flipa-toggle:
      shard-glk:          DMESG-WARN (fdo#106538, fdo#105763) -> PASS

    igt@kms_draw_crc@draw-method-rgb565-blt-ytiled:
      shard-glk:          FAIL (fdo#103184) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-apl:          FAIL (fdo#102887, fdo#105363) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
      shard-apl:          FAIL (fdo#103167) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
      shard-kbl:          FAIL (fdo#103167) -> PASS

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
      shard-glk:          FAIL (fdo#103167) -> PASS +4

    igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
      shard-apl:          FAIL (fdo#103166) -> PASS +1

    
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#107469 https://bugs.freedesktop.org/show_bug.cgi?id=107469
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108147 https://bugs.freedesktop.org/show_bug.cgi?id=108147


== Participating hosts (7 -> 5) ==

  Missing    (2): shard-skl shard-iclb 


== Build changes ==

    * IGT: IGT_4723 -> IGTPW_2082

  CI_DRM_5174: 0bfa7192170c039a271ebc27222b4b91516e73f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2082: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2082/
  IGT_4723: 53bb24ad410b53cdd96f15ced8fd5921c8ab0eac @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2082/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Ville Syrjälä]

On Tue, Nov 20, 2018 at 07:33:36PM -0500, Lyude wrote:
> From: Lyude Paul <lyude@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@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

This terminology is a bit confusing. A hpd pulse is an
asserted->deasserted->asserted transition, so I guess by
a "high pulse" you mean we leave hpd deasserted at the end
instead of asserting it again? So kind of a half pulse.

And in which case I guess the width of the pulse just
specifies how long until a subsequent call can re-assert
hpd again?

>   *
>   * 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
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/3] tests/kms_chamelium: Increase HPD storm threshold to 10 from 1

[Ville Syrjälä]

On Tue, Nov 20, 2018 at 07:33:37PM -0500, Lyude wrote:
> From: Lyude Paul <lyude@redhat.com>
> 
> This is to adjust for the changes to how long pulses are counted towards
> HPD storms that were made in https://patchwork.freedesktop.org/patch/260597/
> 
> Also, update the appropriate documentation in igt to reflect this.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>


> ---
>  lib/igt_debugfs.c     | 3 +++
>  tests/kms_chamelium.c | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index a3aca846..358b4cab 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -557,6 +557,9 @@ static void igt_hpd_storm_exit_handler(int sig)
>   * through debugfs. Useful for hotplugging tests where HPD storm detection
>   * might get in the way and slow things down.
>   *
> + * Note that each long HPD pulse will count as 10 towards the threshold, and
> + * short HPD pulses (if enabled) will count as 1 towards the threshold.
> + *
>   * If the system does not support HPD storm detection, this function does
>   * nothing.
>   *
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index f051344d..0097dbcc 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -742,7 +742,7 @@ test_hpd_storm_detect(data_t *data, struct chamelium_port *port, int width)
>  	igt_require_hpd_storm_ctl(data->drm_fd);
>  	reset_state(data, port);
>  
> -	igt_hpd_storm_set_threshold(data->drm_fd, 1);
> +	igt_hpd_storm_set_threshold(data->drm_fd, 10);

Old threshold was one long hpd with old kernel new one is one long hpd
with new kernel. Seems consistent.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	chamelium_fire_hpd_pulses(data->chamelium, port, width, 10, false);
>  	igt_assert(igt_hpd_storm_detected(data->drm_fd));
>  
> -- 
> 2.19.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_chamelium: Introduce HPD short storm tests

[Ville Syrjälä]

On Tue, Nov 20, 2018 at 07:33:38PM -0500, Lyude wrote:
> From: Lyude Paul <lyude@redhat.com>
> 
> This is to follow up with the changes that were recently made to HPD
> storm detection in https://patchwork.freedesktop.org/patch/260597/ , as
> now there are systems which will be counting short IRQs towards HPD
> storms. As such, we introduce helpers to control HPD short storm
> detection, along with some new kms_chamelium tests for it.
> 
> Please note that at the time of writing this: these tests currently
> fail. That is because the i915_hpd_storm_ctl debugfs currently forgets
> to synchronize with all IRQs and hotplugging work before returning the
> HPD storm status to the user. A patch for this will be sent onto the ML
> very shortly.

That went in no? If yes, this paragraph seems pointless now.

> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  lib/igt_debugfs.c     | 85 +++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_debugfs.h     |  4 ++
>  tests/kms_chamelium.c | 45 +++++++++++++++++++++++
>  3 files changed, 134 insertions(+)
> 
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index 358b4cab..530b70b7 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -543,6 +543,7 @@ static void igt_hpd_storm_exit_handler(int sig)
>  	int fd = drm_open_driver(DRIVER_INTEL);
>  
>  	/* Here we assume that only one i915 device will be ever present */
> +	igt_hpd_short_storm_reset(fd);
>  	igt_hpd_storm_reset(fd);
>  
>  	close(fd);
> @@ -660,6 +661,90 @@ void igt_require_hpd_storm_ctl(int drm_fd)
>  
>  	igt_require_f(fd > 0, "No i915_hpd_storm_ctl found in debugfs\n");
>  	close(fd);
> +
> +	fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_RDONLY);
> +	igt_require_f(fd > 0, "No i915_hpd_short_storm_ctl found in debugfs\n");
> +	close(fd);
> +}
> +
> +/**
> + * igt_hpd_short_storm_set_enabled:
> + *
> + * Convienence helper to enable or disable HPD short storm detection. If hotplug
> + * detection was disabled on any ports due to an HPD storm, it will be
> + * immediately re-enabled.
> + *
> + * If the system does not support HPD storm detection, this function does
> + * nothing.
> + *
> + * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
> + */
> +void igt_hpd_short_storm_set_enabled(int drm_fd, bool enabled)
> +{
> +	int fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_WRONLY);
> +	const char *buf = enabled ? "y" : "n";
> +
> +	if (fd < 0)
> +		return;
> +
> +	igt_debug("%sabling HPD short storm detection\n",
> +		  enabled ? "En" : "Dis");
> +	igt_assert_eq(write(fd, buf, strlen(buf)), strlen(buf));
> +
> +	close(fd);
> +}
> +
> +/**
> + * igt_hpd_short_storm_reset:
> + *
> + * Convienence helper to reset HPD short storm detection to it's default settings.
> + * If hotplug detection was disabled on any ports due to an HPD storm, it will
> + * be immediately re-enabled. Always called on exit if the HPD storm detection
> + * threshold was modified during any tests.
> + *
> + * If the system does not support HPD storm detection, this function does
> + * nothing.
> + *
> + * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
> + */
> +void igt_hpd_short_storm_reset(int drm_fd)
> +{
> +	int fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_WRONLY);
> +	const char *buf = "reset";
> +
> +	if (fd < 0)
> +		return;
> +
> +	igt_debug("Resetting HPD short storm detection\n");
> +	igt_assert_eq(write(fd, buf, strlen(buf)), strlen(buf));
> +
> +	close(fd);
> +}
> +
> +/**
> + * igt_require_hpd_short_storm_on_by_default:
> + *
> + * Skips the current test if the system does not have HPD short storm detection
> + * enabled by default.
> + *
> + * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
> + */
> +void igt_require_hpd_short_storm_on_by_default(int drm_fd)
> +{
> +	int fd;
> +	char buf[16] = {0};
> +
> +	/* First reset short storm detection to the system default */
> +	igt_hpd_short_storm_reset(drm_fd);
> +
> +	/* Now check if the default is enabled or not */
> +	fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_RDONLY);
> +
> +	igt_require_f(fd > 0, "No i915_hpd_short_storm_ctl found in debugfs\n");
> +	igt_assert_lt(0, read(fd, buf, sizeof(buf)));
> +	igt_require_f(strncmp(buf, "Enabled: yes\n", sizeof(buf)) == 0,
> +		      "System keeps HPD short storm detection disabled by default\n");
> +	close(fd);
>  }
>  
>  static igt_pipe_crc_t *
> diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
> index 1233cd8f..21b382a3 100644
> --- a/lib/igt_debugfs.h
> +++ b/lib/igt_debugfs.h
> @@ -143,6 +143,10 @@ void igt_hpd_storm_reset(int fd);
>  bool igt_hpd_storm_detected(int fd);
>  void igt_require_hpd_storm_ctl(int fd);
>  
> +void igt_hpd_short_storm_set_enabled(int fd, bool enabled);
> +void igt_hpd_short_storm_reset(int fd);
> +void igt_require_hpd_short_storm_on_by_default(int fd);
> +
>  /*
>   * Drop caches
>   */
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 0097dbcc..7ab33830 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -45,6 +45,7 @@ typedef struct {
>  #define HOTPLUG_TIMEOUT 20 /* seconds */
>  
>  #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
> +#define HPD_SHORT_STORM_PULSE_INTERVAL_DP 2 /* ms */

A sink should deassert hpd for .5 to 1 ms to generate a short hpd.
2 ms is the max short pulse duration accepted by a source device.
So if if we want to guarantee that we generate a short hpd as
opposed to a long hpd we should probably not aim for the full
2 ms here.

>  #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
>  
>  #define HPD_TOGGLE_COUNT_VGA 5
> @@ -775,6 +776,44 @@ test_hpd_storm_disable(data_t *data, struct chamelium_port *port, int width)
>  	igt_hpd_storm_reset(data->drm_fd);
>  }
>  
> +static void
> +test_hpd_short_storm_detect(data_t *data, struct chamelium_port *port)
> +{
> +	igt_require_hpd_storm_ctl(data->drm_fd);
> +	igt_require_hpd_short_storm_on_by_default(data->drm_fd);
> +	reset_state(data, port);
> +
> +	/* Leave room in the threshold for the two long pulses this might
> +	 * cause
> +	 */
> +	igt_hpd_storm_set_threshold(data->drm_fd, 21);
> +	chamelium_fire_hpd_pulses(data->chamelium, port,
> +				  HPD_SHORT_STORM_PULSE_INTERVAL_DP, 25, false);
> +	igt_assert(igt_hpd_storm_detected(data->drm_fd));
> +
> +	igt_hpd_storm_reset(data->drm_fd);
> +}
> +
> +static void
> +test_hpd_short_storm_disable(data_t *data, struct chamelium_port *port)
> +{
> +	igt_require_hpd_storm_ctl(data->drm_fd);
> +	reset_state(data, port);
> +
> +	igt_hpd_short_storm_set_enabled(data->drm_fd, false);
> +
> +	/* Leave room in the threshold for the two long pulses this might
> +	 * cause
> +	 */
> +	igt_hpd_storm_set_threshold(data->drm_fd, 21);
> +	chamelium_fire_hpd_pulses(data->chamelium, port,
> +				  HPD_SHORT_STORM_PULSE_INTERVAL_DP, 25, false);
> +	igt_assert(!igt_hpd_storm_detected(data->drm_fd));
> +
> +	igt_hpd_short_storm_reset(data->drm_fd);
> +	igt_hpd_storm_reset(data->drm_fd);
> +}
> +
>  #define for_each_port(p, port)            \
>  	for (p = 0, port = data.ports[p]; \
>  	     p < data.port_count;         \
> @@ -856,6 +895,12 @@ igt_main
>  			test_hpd_storm_disable(&data, port,
>  					       HPD_STORM_PULSE_INTERVAL_DP);
>  
> +		connector_subtest("dp-hpd-short-storm", DisplayPort)
> +			test_hpd_short_storm_detect(&data, port);
> +
> +		connector_subtest("dp-hpd-short-storm-disable", DisplayPort)
> +			test_hpd_short_storm_disable(&data, port);
> +
>  		connector_subtest("dp-edid-change-during-suspend", DisplayPort)
>  			test_suspend_resume_edid_change(&data, port,
>  							SUSPEND_STATE_MEM,
> -- 
> 2.19.1
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_chamelium: Introduce HPD short storm tests

[Ville Syrjälä]

On Tue, Dec 11, 2018 at 07:09:26PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 20, 2018 at 07:33:38PM -0500, Lyude wrote:
> > From: Lyude Paul <lyude@redhat.com>
> > 
> > This is to follow up with the changes that were recently made to HPD
> > storm detection in https://patchwork.freedesktop.org/patch/260597/ , as
> > now there are systems which will be counting short IRQs towards HPD
> > storms. As such, we introduce helpers to control HPD short storm
> > detection, along with some new kms_chamelium tests for it.
> > 
> > Please note that at the time of writing this: these tests currently
> > fail. That is because the i915_hpd_storm_ctl debugfs currently forgets
> > to synchronize with all IRQs and hotplugging work before returning the
> > HPD storm status to the user. A patch for this will be sent onto the ML
> > very shortly.
> 
> That went in no? If yes, this paragraph seems pointless now.
> 
> > 
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > ---
> >  lib/igt_debugfs.c     | 85 +++++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_debugfs.h     |  4 ++
> >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++
> >  3 files changed, 134 insertions(+)
> > 
> > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > index 358b4cab..530b70b7 100644
> > --- a/lib/igt_debugfs.c
> > +++ b/lib/igt_debugfs.c
> > @@ -543,6 +543,7 @@ static void igt_hpd_storm_exit_handler(int sig)
> >  	int fd = drm_open_driver(DRIVER_INTEL);
> >  
> >  	/* Here we assume that only one i915 device will be ever present */
> > +	igt_hpd_short_storm_reset(fd);
> >  	igt_hpd_storm_reset(fd);
> >  
> >  	close(fd);
> > @@ -660,6 +661,90 @@ void igt_require_hpd_storm_ctl(int drm_fd)
> >  
> >  	igt_require_f(fd > 0, "No i915_hpd_storm_ctl found in debugfs\n");
> >  	close(fd);
> > +
> > +	fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_RDONLY);
> > +	igt_require_f(fd > 0, "No i915_hpd_short_storm_ctl found in debugfs\n");
> > +	close(fd);
> > +}
> > +
> > +/**
> > + * igt_hpd_short_storm_set_enabled:
> > + *
> > + * Convienence helper to enable or disable HPD short storm detection. If hotplug
> > + * detection was disabled on any ports due to an HPD storm, it will be
> > + * immediately re-enabled.
> > + *
> > + * If the system does not support HPD storm detection, this function does
> > + * nothing.
> > + *
> > + * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
> > + */
> > +void igt_hpd_short_storm_set_enabled(int drm_fd, bool enabled)
> > +{
> > +	int fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_WRONLY);
> > +	const char *buf = enabled ? "y" : "n";
> > +
> > +	if (fd < 0)
> > +		return;
> > +
> > +	igt_debug("%sabling HPD short storm detection\n",
> > +		  enabled ? "En" : "Dis");
> > +	igt_assert_eq(write(fd, buf, strlen(buf)), strlen(buf));
> > +
> > +	close(fd);
> > +}
> > +
> > +/**
> > + * igt_hpd_short_storm_reset:
> > + *
> > + * Convienence helper to reset HPD short storm detection to it's default settings.
> > + * If hotplug detection was disabled on any ports due to an HPD storm, it will
> > + * be immediately re-enabled. Always called on exit if the HPD storm detection
> > + * threshold was modified during any tests.
> > + *
> > + * If the system does not support HPD storm detection, this function does
> > + * nothing.
> > + *
> > + * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
> > + */
> > +void igt_hpd_short_storm_reset(int drm_fd)
> > +{
> > +	int fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_WRONLY);
> > +	const char *buf = "reset";
> > +
> > +	if (fd < 0)
> > +		return;
> > +
> > +	igt_debug("Resetting HPD short storm detection\n");
> > +	igt_assert_eq(write(fd, buf, strlen(buf)), strlen(buf));
> > +
> > +	close(fd);
> > +}
> > +
> > +/**
> > + * igt_require_hpd_short_storm_on_by_default:
> > + *
> > + * Skips the current test if the system does not have HPD short storm detection
> > + * enabled by default.
> > + *
> > + * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
> > + */
> > +void igt_require_hpd_short_storm_on_by_default(int drm_fd)
> > +{
> > +	int fd;
> > +	char buf[16] = {0};
> > +
> > +	/* First reset short storm detection to the system default */
> > +	igt_hpd_short_storm_reset(drm_fd);
> > +
> > +	/* Now check if the default is enabled or not */
> > +	fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_RDONLY);
> > +
> > +	igt_require_f(fd > 0, "No i915_hpd_short_storm_ctl found in debugfs\n");
> > +	igt_assert_lt(0, read(fd, buf, sizeof(buf)));
> > +	igt_require_f(strncmp(buf, "Enabled: yes\n", sizeof(buf)) == 0,
> > +		      "System keeps HPD short storm detection disabled by default\n");
> > +	close(fd);
> >  }
> >  
> >  static igt_pipe_crc_t *
> > diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
> > index 1233cd8f..21b382a3 100644
> > --- a/lib/igt_debugfs.h
> > +++ b/lib/igt_debugfs.h
> > @@ -143,6 +143,10 @@ void igt_hpd_storm_reset(int fd);
> >  bool igt_hpd_storm_detected(int fd);
> >  void igt_require_hpd_storm_ctl(int fd);
> >  
> > +void igt_hpd_short_storm_set_enabled(int fd, bool enabled);
> > +void igt_hpd_short_storm_reset(int fd);
> > +void igt_require_hpd_short_storm_on_by_default(int fd);
> > +
> >  /*
> >   * Drop caches
> >   */
> > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > index 0097dbcc..7ab33830 100644
> > --- a/tests/kms_chamelium.c
> > +++ b/tests/kms_chamelium.c
> > @@ -45,6 +45,7 @@ typedef struct {
> >  #define HOTPLUG_TIMEOUT 20 /* seconds */
> >  
> >  #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
> > +#define HPD_SHORT_STORM_PULSE_INTERVAL_DP 2 /* ms */
> 
> A sink should deassert hpd for .5 to 1 ms to generate a short hpd.
> 2 ms is the max short pulse duration accepted by a source device.
> So if if we want to guarantee that we generate a short hpd as
> opposed to a long hpd we should probably not aim for the full
> 2 ms here.

Ah. So we split it 1:1 for the deassert and assert phases.
But the docs do not explain how the resulting pulse will
look like.

Ie. do we get
 _________
/         \_________
<- D ms -><- A ms ->
or
          _________
_________/         \
<- D ms -><- A ms ->
or
 __________________
/                  \
<- D ms -><- A ms ->
or

_________/\_________
<- D ms -><- A ms ->

I'm guessin it's not the last one :). The third one would also be
a bit weird perhaps but could be the case I suppose, and that would
give us the full 2 ms pulse. My money would be on the first option
though, in which case the 2ms total duration seems fine.

> 
> >  #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
> >  
> >  #define HPD_TOGGLE_COUNT_VGA 5
> > @@ -775,6 +776,44 @@ test_hpd_storm_disable(data_t *data, struct chamelium_port *port, int width)
> >  	igt_hpd_storm_reset(data->drm_fd);
> >  }
> >  
> > +static void
> > +test_hpd_short_storm_detect(data_t *data, struct chamelium_port *port)
> > +{
> > +	igt_require_hpd_storm_ctl(data->drm_fd);
> > +	igt_require_hpd_short_storm_on_by_default(data->drm_fd);
> > +	reset_state(data, port);
> > +
> > +	/* Leave room in the threshold for the two long pulses this might
> > +	 * cause
> > +	 */
> > +	igt_hpd_storm_set_threshold(data->drm_fd, 21);
> > +	chamelium_fire_hpd_pulses(data->chamelium, port,
> > +				  HPD_SHORT_STORM_PULSE_INTERVAL_DP, 25, false);
> > +	igt_assert(igt_hpd_storm_detected(data->drm_fd));
> > +
> > +	igt_hpd_storm_reset(data->drm_fd);
> > +}
> > +
> > +static void
> > +test_hpd_short_storm_disable(data_t *data, struct chamelium_port *port)
> > +{
> > +	igt_require_hpd_storm_ctl(data->drm_fd);
> > +	reset_state(data, port);
> > +
> > +	igt_hpd_short_storm_set_enabled(data->drm_fd, false);
> > +
> > +	/* Leave room in the threshold for the two long pulses this might
> > +	 * cause
> > +	 */
> > +	igt_hpd_storm_set_threshold(data->drm_fd, 21);
> > +	chamelium_fire_hpd_pulses(data->chamelium, port,
> > +				  HPD_SHORT_STORM_PULSE_INTERVAL_DP, 25, false);
> > +	igt_assert(!igt_hpd_storm_detected(data->drm_fd));
> > +
> > +	igt_hpd_short_storm_reset(data->drm_fd);
> > +	igt_hpd_storm_reset(data->drm_fd);
> > +}
> > +
> >  #define for_each_port(p, port)            \
> >  	for (p = 0, port = data.ports[p]; \
> >  	     p < data.port_count;         \
> > @@ -856,6 +895,12 @@ igt_main
> >  			test_hpd_storm_disable(&data, port,
> >  					       HPD_STORM_PULSE_INTERVAL_DP);
> >  
> > +		connector_subtest("dp-hpd-short-storm", DisplayPort)
> > +			test_hpd_short_storm_detect(&data, port);
> > +
> > +		connector_subtest("dp-hpd-short-storm-disable", DisplayPort)
> > +			test_hpd_short_storm_disable(&data, port);
> > +
> >  		connector_subtest("dp-edid-change-during-suspend", DisplayPort)
> >  			test_suspend_resume_edid_change(&data, port,
> >  							SUSPEND_STATE_MEM,
> > -- 
> > 2.19.1
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

[Lyude Paul]

On Tue, 2018-12-11 at 18:52 +0200, Ville Syrjälä wrote:
> On Tue, Nov 20, 2018 at 07:33:36PM -0500, Lyude wrote:
> > From: Lyude Paul <lyude@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@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
> 
> This terminology is a bit confusing. A hpd pulse is an
> asserted->deasserted->asserted transition, so I guess by
> a "high pulse" you mean we leave hpd deasserted at the end
> instead of asserting it again? So kind of a half pulse.
> 
> And in which case I guess the width of the pulse just
> specifies how long until a subsequent call can re-assert
> hpd again?
> 

Agreed, unfortunately the end_high part is how the chamelium interface exposes
this.

So looking at the source code for chameleond, it looks like things are
actually reversed here:

  @_FlowMethod
  @_VideoMethod
  def FireHpdPulse(self, port_id, deassert_interval_usec,
                   assert_interval_usec=None, repeat_count=1,
                   end_level=1):
    """Fires one or more HPD pulse (low -> high -> low -> ...).

    Args:
      port_id: The ID of the video input port.
      deassert_interval_usec: The time in microsecond of the deassert pulse.
      assert_interval_usec: The time in microsecond of the assert pulse.
                            If None, then use the same value as
                            deassert_interval_usec.
      repeat_count: The count of HPD pulses to fire.
      end_level: HPD ends with 0 for LOW (unplugged) or 1 for HIGH (plugged).
    """
    if assert_interval_usec is None:
      # Fall back to use the same value as deassertion if not given.
      assert_interval_usec = deassert_interval_usec

    logging.info('Fire HPD pulse on port #%d, ending with %s',
                 port_id, 'high' if end_level else 'low')
    return self.flows[port_id].FireHpdPulse(
        deassert_interval_usec, assert_interval_usec, repeat_count, end_level)

I wonder if the order being reversed here explains why i915 always seems
to see a long pulse at the start of a series of pulses.

Should we add something to the documentation for this that' similar to
the "low -> high -> low" stuff mentioned in the comments here?

> >   *
> >   * 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
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
-- 
Cheers,
	Lyude Paul

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_chamelium: Introduce HPD short storm tests

[Lyude Paul]

On Tue, 2018-12-11 at 19:34 +0200, Ville Syrjälä wrote:
> On Tue, Dec 11, 2018 at 07:09:26PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 20, 2018 at 07:33:38PM -0500, Lyude wrote:
> > > From: Lyude Paul <lyude@redhat.com>
> > > 
> > > This is to follow up with the changes that were recently made to HPD
> > > storm detection in https://patchwork.freedesktop.org/patch/260597/ , as
> > > now there are systems which will be counting short IRQs towards HPD
> > > storms. As such, we introduce helpers to control HPD short storm
> > > detection, along with some new kms_chamelium tests for it.
> > > 
> > > Please note that at the time of writing this: these tests currently
> > > fail. That is because the i915_hpd_storm_ctl debugfs currently forgets
> > > to synchronize with all IRQs and hotplugging work before returning the
> > > HPD storm status to the user. A patch for this will be sent onto the ML
> > > very shortly.
> > 
> > That went in no? If yes, this paragraph seems pointless now.
> > 
> > > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > > ---
> > >  lib/igt_debugfs.c     | 85 +++++++++++++++++++++++++++++++++++++++++++
> > >  lib/igt_debugfs.h     |  4 ++
> > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++
> > >  3 files changed, 134 insertions(+)
> > > 
> > > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > > index 358b4cab..530b70b7 100644
> > > --- a/lib/igt_debugfs.c
> > > +++ b/lib/igt_debugfs.c
> > > @@ -543,6 +543,7 @@ static void igt_hpd_storm_exit_handler(int sig)
> > >  	int fd = drm_open_driver(DRIVER_INTEL);
> > >  
> > >  	/* Here we assume that only one i915 device will be ever present */
> > > +	igt_hpd_short_storm_reset(fd);
> > >  	igt_hpd_storm_reset(fd);
> > >  
> > >  	close(fd);
> > > @@ -660,6 +661,90 @@ void igt_require_hpd_storm_ctl(int drm_fd)
> > >  
> > >  	igt_require_f(fd > 0, "No i915_hpd_storm_ctl found in debugfs\n");
> > >  	close(fd);
> > > +
> > > +	fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_RDONLY);
> > > +	igt_require_f(fd > 0, "No i915_hpd_short_storm_ctl found in
> > > debugfs\n");
> > > +	close(fd);
> > > +}
> > > +
> > > +/**
> > > + * igt_hpd_short_storm_set_enabled:
> > > + *
> > > + * Convienence helper to enable or disable HPD short storm detection.
> > > If hotplug
> > > + * detection was disabled on any ports due to an HPD storm, it will be
> > > + * immediately re-enabled.
> > > + *
> > > + * If the system does not support HPD storm detection, this function
> > > does
> > > + * nothing.
> > > + *
> > > + * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
> > > + */
> > > +void igt_hpd_short_storm_set_enabled(int drm_fd, bool enabled)
> > > +{
> > > +	int fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl",
> > > O_WRONLY);
> > > +	const char *buf = enabled ? "y" : "n";
> > > +
> > > +	if (fd < 0)
> > > +		return;
> > > +
> > > +	igt_debug("%sabling HPD short storm detection\n",
> > > +		  enabled ? "En" : "Dis");
> > > +	igt_assert_eq(write(fd, buf, strlen(buf)), strlen(buf));
> > > +
> > > +	close(fd);
> > > +}
> > > +
> > > +/**
> > > + * igt_hpd_short_storm_reset:
> > > + *
> > > + * Convienence helper to reset HPD short storm detection to it's
> > > default settings.
> > > + * If hotplug detection was disabled on any ports due to an HPD storm,
> > > it will
> > > + * be immediately re-enabled. Always called on exit if the HPD storm
> > > detection
> > > + * threshold was modified during any tests.
> > > + *
> > > + * If the system does not support HPD storm detection, this function
> > > does
> > > + * nothing.
> > > + *
> > > + * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
> > > + */
> > > +void igt_hpd_short_storm_reset(int drm_fd)
> > > +{
> > > +	int fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl",
> > > O_WRONLY);
> > > +	const char *buf = "reset";
> > > +
> > > +	if (fd < 0)
> > > +		return;
> > > +
> > > +	igt_debug("Resetting HPD short storm detection\n");
> > > +	igt_assert_eq(write(fd, buf, strlen(buf)), strlen(buf));
> > > +
> > > +	close(fd);
> > > +}
> > > +
> > > +/**
> > > + * igt_require_hpd_short_storm_on_by_default:
> > > + *
> > > + * Skips the current test if the system does not have HPD short storm
> > > detection
> > > + * enabled by default.
> > > + *
> > > + * See: https://01.org/linuxgraphics/gfx-docs/drm/gpu/i915.html#hotplug
> > > + */
> > > +void igt_require_hpd_short_storm_on_by_default(int drm_fd)
> > > +{
> > > +	int fd;
> > > +	char buf[16] = {0};
> > > +
> > > +	/* First reset short storm detection to the system default */
> > > +	igt_hpd_short_storm_reset(drm_fd);
> > > +
> > > +	/* Now check if the default is enabled or not */
> > > +	fd = igt_debugfs_open(drm_fd, "i915_hpd_short_storm_ctl", O_RDONLY);
> > > +
> > > +	igt_require_f(fd > 0, "No i915_hpd_short_storm_ctl found in
> > > debugfs\n");
> > > +	igt_assert_lt(0, read(fd, buf, sizeof(buf)));
> > > +	igt_require_f(strncmp(buf, "Enabled: yes\n", sizeof(buf)) == 0,
> > > +		      "System keeps HPD short storm detection disabled by
> > > default\n");
> > > +	close(fd);
> > >  }
> > >  
> > >  static igt_pipe_crc_t *
> > > diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
> > > index 1233cd8f..21b382a3 100644
> > > --- a/lib/igt_debugfs.h
> > > +++ b/lib/igt_debugfs.h
> > > @@ -143,6 +143,10 @@ void igt_hpd_storm_reset(int fd);
> > >  bool igt_hpd_storm_detected(int fd);
> > >  void igt_require_hpd_storm_ctl(int fd);
> > >  
> > > +void igt_hpd_short_storm_set_enabled(int fd, bool enabled);
> > > +void igt_hpd_short_storm_reset(int fd);
> > > +void igt_require_hpd_short_storm_on_by_default(int fd);
> > > +
> > >  /*
> > >   * Drop caches
> > >   */
> > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > index 0097dbcc..7ab33830 100644
> > > --- a/tests/kms_chamelium.c
> > > +++ b/tests/kms_chamelium.c
> > > @@ -45,6 +45,7 @@ typedef struct {
> > >  #define HOTPLUG_TIMEOUT 20 /* seconds */
> > >  
> > >  #define HPD_STORM_PULSE_INTERVAL_DP 100 /* ms */
> > > +#define HPD_SHORT_STORM_PULSE_INTERVAL_DP 2 /* ms */
> > 
> > A sink should deassert hpd for .5 to 1 ms to generate a short hpd.
> > 2 ms is the max short pulse duration accepted by a source device.
> > So if if we want to guarantee that we generate a short hpd as
> > opposed to a long hpd we should probably not aim for the full
> > 2 ms here.
> 
> Ah. So we split it 1:1 for the deassert and assert phases.
> But the docs do not explain how the resulting pulse will
> look like.
> 
> Ie. do we get
>  _________
> /         \_________
> <- D ms -><- A ms ->
> or
>           _________
> _________/         \
> <- D ms -><- A ms ->
> or
>  __________________
> /                  \
> <- D ms -><- A ms ->
> or
> 
> _________/\_________
> <- D ms -><- A ms ->
> 
> I'm guessin it's not the last one :). The third one would also be
> a bit weird perhaps but could be the case I suppose, and that would
> give us the full 2 ms pulse. My money would be on the first option
> though, in which case the 2ms total duration seems fine.
Unfortunately I have no idea which one of these it is for sure. Judging by the
documentation for the hpd_control binary that the chameleond source has:

  plug OFFSET               - Assert HPD line to high, emulating a plug.
  unplug OFFSET             - Deassert HPD line to low, emulating an unplug.

It's probably option #2, but I could also ask the chamelium mailing list if
you'd like-they would probably know the answer to this.

> 
> > >  #define HPD_STORM_PULSE_INTERVAL_HDMI 200 /* ms */
> > >  
> > >  #define HPD_TOGGLE_COUNT_VGA 5
> > > @@ -775,6 +776,44 @@ test_hpd_storm_disable(data_t *data, struct
> > > chamelium_port *port, int width)
> > >  	igt_hpd_storm_reset(data->drm_fd);
> > >  }
> > >  
> > > +static void
> > > +test_hpd_short_storm_detect(data_t *data, struct chamelium_port *port)
> > > +{
> > > +	igt_require_hpd_storm_ctl(data->drm_fd);
> > > +	igt_require_hpd_short_storm_on_by_default(data->drm_fd);
> > > +	reset_state(data, port);
> > > +
> > > +	/* Leave room in the threshold for the two long pulses this might
> > > +	 * cause
> > > +	 */
> > > +	igt_hpd_storm_set_threshold(data->drm_fd, 21);
> > > +	chamelium_fire_hpd_pulses(data->chamelium, port,
> > > +				  HPD_SHORT_STORM_PULSE_INTERVAL_DP, 25,
> > > false);
> > > +	igt_assert(igt_hpd_storm_detected(data->drm_fd));
> > > +
> > > +	igt_hpd_storm_reset(data->drm_fd);
> > > +}
> > > +
> > > +static void
> > > +test_hpd_short_storm_disable(data_t *data, struct chamelium_port *port)
> > > +{
> > > +	igt_require_hpd_storm_ctl(data->drm_fd);
> > > +	reset_state(data, port);
> > > +
> > > +	igt_hpd_short_storm_set_enabled(data->drm_fd, false);
> > > +
> > > +	/* Leave room in the threshold for the two long pulses this might
> > > +	 * cause
> > > +	 */
> > > +	igt_hpd_storm_set_threshold(data->drm_fd, 21);
> > > +	chamelium_fire_hpd_pulses(data->chamelium, port,
> > > +				  HPD_SHORT_STORM_PULSE_INTERVAL_DP, 25,
> > > false);
> > > +	igt_assert(!igt_hpd_storm_detected(data->drm_fd));
> > > +
> > > +	igt_hpd_short_storm_reset(data->drm_fd);
> > > +	igt_hpd_storm_reset(data->drm_fd);
> > > +}
> > > +
> > >  #define for_each_port(p, port)            \
> > >  	for (p = 0, port = data.ports[p]; \
> > >  	     p < data.port_count;         \
> > > @@ -856,6 +895,12 @@ igt_main
> > >  			test_hpd_storm_disable(&data, port,
> > >  					       HPD_STORM_PULSE_INTERVAL_DP);
> > >  
> > > +		connector_subtest("dp-hpd-short-storm", DisplayPort)
> > > +			test_hpd_short_storm_detect(&data, port);
> > > +
> > > +		connector_subtest("dp-hpd-short-storm-disable", DisplayPort)
> > > +			test_hpd_short_storm_disable(&data, port);
> > > +
> > >  		connector_subtest("dp-edid-change-during-suspend",
> > > DisplayPort)
> > >  			test_suspend_resume_edid_change(&data, port,
> > >  							SUSPEND_STATE_MEM,
> > > -- 
> > > 2.19.1
> > > 
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > 
> > -- 
> > Ville Syrjälä
> > Intel
-- 
Cheers,
	Lyude Paul

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev