[i-g-t,3/3] tests/kms_chamelium: Introduce HPD short storm tests

Submitted by Lyude Paul on Nov. 21, 2018, 12:33 a.m.

Details

Message ID 20181121003338.16956-4-lyude@redhat.com
State New
Series "Add tests for HPD short storm detection"
Headers show

Commit Message

Lyude Paul Nov. 21, 2018, 12:33 a.m.
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(+)

Patch hide | download patch | download mbox

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,

Comments

Ville Syrjälä Dec. 11, 2018, 5:09 p.m.
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ä Dec. 11, 2018, 5:34 p.m.
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
Lyude Paul Dec. 11, 2018, 9:39 p.m.
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