[igt-dev] [PATCH i-g-t] tests/kms_chamelium: Make sure we wait for each connectors' hotplug event

[igt-dev] [PATCH i-g-t] tests/kms_chamelium: Make sure we wait for each connectors' hotplug event

[Imre Deak]

After scheduling an HPD toggle event, make sure that we wait for the
hotplug event for each connector that may be sent by the driver.

Depending on the scheduling there could be 1 event or as many events as
connectors we scheduled an HPD toggle event on, depending on the timing.
So if we don't yet see the expected connector state on a given connector
try to wait for an additional hotplug event and reprobe/recheck the
state.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index 714e5e06..39da9e47 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct chamelium_port *port,
 	drmModeFreeConnector(connector);
 }
 
+/* Wait for hotplug and return the remaining time left from timeout */
+static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
+{
+	struct timespec start, end;
+	int elapsed;
+	bool detected;
+
+	igt_assert_eq(igt_gettime(&start), 0);
+	detected = igt_hotplug_detected(mon, *timeout);
+	igt_assert_eq(igt_gettime(&end), 0);
+
+	elapsed = igt_time_elapsed(&start, &end);
+	igt_assert(elapsed >= 0);
+	*timeout = max(0, *timeout - elapsed);
+
+	return detected;
+}
+
 static void
 try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
 		       enum igt_suspend_state state, enum igt_suspend_test test,
 		       struct udev_monitor *mon, bool connected)
 {
+	drmModeConnection target_state = connected ? DRM_MODE_DISCONNECTED :
+						     DRM_MODE_CONNECTED;
+	int timeout = HOTPLUG_TIMEOUT;
 	int delay;
 	int p;
 
@@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
 	}
 
 	igt_system_suspend_autoresume(state, test);
+	igt_assert(wait_for_hotplug(mon, &timeout));
 
-	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
 	if (port) {
-		igt_assert_eq(reprobe_connector(data, port), connected ?
-			      DRM_MODE_DISCONNECTED : DRM_MODE_CONNECTED);
+		igt_assert_eq(reprobe_connector(data, port), target_state);
 	} else {
 		for (p = 0; p < data->port_count; p++) {
+			drmModeConnection current_state;
+
 			port = data->ports[p];
-			igt_assert_eq(reprobe_connector(data, port), connected ?
-				      DRM_MODE_DISCONNECTED :
-				      DRM_MODE_CONNECTED);
+			/*
+			 * There could be as many hotplug events sent by
+			 * driver as connectors we scheduled an HPD toggle on
+			 * above, depending on timing. So if we're not seeing
+			 * the expected connector state try to wait for an HPD
+			 * event for each connector/port.
+			 */
+			current_state = reprobe_connector(data, port);
+			if (p > 0 && current_state != target_state) {
+				igt_assert(wait_for_hotplug(mon, &timeout));
+				current_state = reprobe_connector(data, port);
+			}
+
+			igt_assert_eq(current_state, target_state);
 		}
 
 		port = NULL;
-- 
2.17.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 tests/kms_chamelium: Make sure we wait for each connectors' hotplug event

[Patchwork]

== Series Details ==

Series: tests/kms_chamelium: Make sure we wait for each connectors' hotplug event
URL   : https://patchwork.freedesktop.org/series/60334/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6051 -> IGTPW_2946
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      [PASS][1] -> [FAIL][2] ([fdo#108511])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html

  
#### Possible fixes ####

  * igt@i915_selftest@live_guc:
    - fi-apl-guc:         [INCOMPLETE][3] ([fdo#103927]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/fi-apl-guc/igt@i915_selftest@live_guc.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/fi-apl-guc/igt@i915_selftest@live_guc.html

  * igt@i915_selftest@live_hangcheck:
    - fi-apl-guc:         [DMESG-FAIL][5] ([fdo#110620]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/fi-apl-guc/igt@i915_selftest@live_hangcheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/fi-apl-guc/igt@i915_selftest@live_hangcheck.html

  
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
  [fdo#110620]: https://bugs.freedesktop.org/show_bug.cgi?id=110620


Participating hosts (53 -> 46)
------------------------------

  Additional (1): fi-pnv-d510 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * IGT: IGT_4972 -> IGTPW_2946

  CI_DRM_6051: fac89f79a454771f6595bcd11d9a119d5acc42d0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2946: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/
  IGT_4972: f052e49a43cc9704ea5f240df15dd9d3dfed68ab @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

[igt-dev] ✓ Fi.CI.IGT: success for tests/kms_chamelium: Make sure we wait for each connectors' hotplug event

[Patchwork]

== Series Details ==

Series: tests/kms_chamelium: Make sure we wait for each connectors' hotplug event
URL   : https://patchwork.freedesktop.org/series/60334/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6051_full -> IGTPW_2946_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-hsw:          [PASS][1] -> [FAIL][2] ([fdo#108686])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-hsw5/igt@gem_tiled_swapping@non-threaded.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-hsw4/igt@gem_tiled_swapping@non-threaded.html

  * igt@i915_suspend@debugfs-reader:
    - shard-apl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#108566]) +4 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-apl8/igt@i915_suspend@debugfs-reader.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-apl4/igt@i915_suspend@debugfs-reader.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-iclb:         [PASS][5] -> [FAIL][6] ([fdo#103167]) +9 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move.html

  * igt@kms_lease@page_flip_implicit_plane:
    - shard-snb:          [PASS][7] -> [SKIP][8] ([fdo#109271])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-snb5/igt@kms_lease@page_flip_implicit_plane.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-snb2/igt@kms_lease@page_flip_implicit_plane.html

  * igt@kms_plane@pixel-format-pipe-b-planes:
    - shard-glk:          [PASS][9] -> [SKIP][10] ([fdo#109271])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-glk9/igt@kms_plane@pixel-format-pipe-b-planes.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-glk7/igt@kms_plane@pixel-format-pipe-b-planes.html

  * igt@kms_plane_scaling@pipe-c-scaler-with-pixel-format:
    - shard-glk:          [PASS][11] -> [SKIP][12] ([fdo#109271] / [fdo#109278])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-glk9/igt@kms_plane_scaling@pipe-c-scaler-with-pixel-format.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-glk1/igt@kms_plane_scaling@pipe-c-scaler-with-pixel-format.html

  * igt@kms_psr@no_drrs:
    - shard-iclb:         [PASS][13] -> [FAIL][14] ([fdo#108341])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-iclb7/igt@kms_psr@no_drrs.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-iclb1/igt@kms_psr@no_drrs.html

  * igt@kms_psr@psr2_cursor_plane_onoff:
    - shard-iclb:         [PASS][15] -> [SKIP][16] ([fdo#109441]) +2 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-iclb1/igt@kms_psr@psr2_cursor_plane_onoff.html

  
#### Possible fixes ####

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-snb:          [SKIP][17] ([fdo#109271]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-snb7/igt@i915_pm_rc6_residency@rc6-accuracy.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-snb6/igt@i915_pm_rc6_residency@rc6-accuracy.html

  * igt@i915_pm_rpm@i2c:
    - shard-iclb:         [FAIL][19] ([fdo#104097]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-iclb4/igt@i915_pm_rpm@i2c.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-iclb6/igt@i915_pm_rpm@i2c.html

  * igt@i915_suspend@sysfs-reader:
    - shard-apl:          [DMESG-WARN][21] ([fdo#108566]) -> [PASS][22] +3 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-apl5/igt@i915_suspend@sysfs-reader.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-apl7/igt@i915_suspend@sysfs-reader.html

  * igt@kms_cursor_legacy@pipe-c-forked-move:
    - shard-hsw:          [INCOMPLETE][23] ([fdo#103540]) -> [PASS][24] +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-hsw4/igt@kms_cursor_legacy@pipe-c-forked-move.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-hsw2/igt@kms_cursor_legacy@pipe-c-forked-move.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [SKIP][25] ([fdo#109349]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-iclb7/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-glk:          [FAIL][27] ([fdo#105363]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-glk9/igt@kms_flip@flip-vs-expired-vblank.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-glk1/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_flip@modeset-vs-vblank-race-interruptible:
    - shard-glk:          [FAIL][29] ([fdo#103060]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-glk5/igt@kms_flip@modeset-vs-vblank-race-interruptible.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-glk7/igt@kms_flip@modeset-vs-vblank-race-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt:
    - shard-iclb:         [FAIL][31] ([fdo#103167]) -> [PASS][32] +2 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt.html

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-move:
    - shard-glk:          [FAIL][33] ([fdo#103167]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-glk4/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-move.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-glk1/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-move.html

  * igt@kms_plane@pixel-format-pipe-a-planes:
    - shard-glk:          [SKIP][35] ([fdo#109271]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-glk5/igt@kms_plane@pixel-format-pipe-a-planes.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-glk9/igt@kms_plane@pixel-format-pipe-a-planes.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [SKIP][37] ([fdo#109642]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-iclb8/igt@kms_psr2_su@page_flip.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-iclb2/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_cursor_mmap_cpu:
    - shard-iclb:         [SKIP][39] ([fdo#109441]) -> [PASS][40] +1 similar issue
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-iclb4/igt@kms_psr@psr2_cursor_mmap_cpu.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-iclb2/igt@kms_psr@psr2_cursor_mmap_cpu.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][41] ([fdo#99912]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6051/shard-kbl4/igt@kms_setmode@basic.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/shard-kbl1/igt@kms_setmode@basic.html

  
  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#104097]: https://bugs.freedesktop.org/show_bug.cgi?id=104097
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 6)
------------------------------

  Missing    (4): pig-skl-6260u shard-skl pig-hsw-4770r pig-glk-j5005 


Build changes
-------------

  * IGT: IGT_4972 -> IGTPW_2946
  * Piglit: piglit_4509 -> None

  CI_DRM_6051: fac89f79a454771f6595bcd11d9a119d5acc42d0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2946: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2946/
  IGT_4972: f052e49a43cc9704ea5f240df15dd9d3dfed68ab @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [igt-dev] [PATCH i-g-t] tests/kms_chamelium: Make sure we wait for each connectors' hotplug event

[Lyude Paul]

One small nitpick

On Mon, 2019-05-06 at 20:51 +0300, Imre Deak wrote:
> After scheduling an HPD toggle event, make sure that we wait for the
> hotplug event for each connector that may be sent by the driver.
> 
> Depending on the scheduling there could be 1 event or as many events as
> connectors we scheduled an HPD toggle event on, depending on the timing.
> So if we don't yet see the expected connector state on a given connector
> try to wait for an additional hotplug event and reprobe/recheck the
> state.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 714e5e06..39da9e47 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct chamelium_port
> *port,
>  	drmModeFreeConnector(connector);
>  }
>  
> +/* Wait for hotplug and return the remaining time left from timeout */
> +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> +{
> +	struct timespec start, end;
> +	int elapsed;
> +	bool detected;
> +
> +	igt_assert_eq(igt_gettime(&start), 0);
> +	detected = igt_hotplug_detected(mon, *timeout);
> +	igt_assert_eq(igt_gettime(&end), 0);
> +
> +	elapsed = igt_time_elapsed(&start, &end);
> +	igt_assert(elapsed >= 0);

igt_assert_lte(0, elapsed);

With that one small change:

Reviewed-by: Lyude Paul <lyude@redhat.com>

> +	*timeout = max(0, *timeout - elapsed);
> +
> +	return detected;
> +}
> +
>  static void
>  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
>  		       enum igt_suspend_state state, enum igt_suspend_test
> test,
>  		       struct udev_monitor *mon, bool connected)
>  {
> +	drmModeConnection target_state = connected ? DRM_MODE_DISCONNECTED :
> +						     DRM_MODE_CONNECTED;
> +	int timeout = HOTPLUG_TIMEOUT;
>  	int delay;
>  	int p;
>  
> @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> chamelium_port *port,
>  	}
>  
>  	igt_system_suspend_autoresume(state, test);
> +	igt_assert(wait_for_hotplug(mon, &timeout));
>  
> -	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
>  	if (port) {
> -		igt_assert_eq(reprobe_connector(data, port), connected ?
> -			      DRM_MODE_DISCONNECTED : DRM_MODE_CONNECTED);
> +		igt_assert_eq(reprobe_connector(data, port), target_state);
>  	} else {
>  		for (p = 0; p < data->port_count; p++) {
> +			drmModeConnection current_state;
> +
>  			port = data->ports[p];
> -			igt_assert_eq(reprobe_connector(data, port), connected
> ?
> -				      DRM_MODE_DISCONNECTED :
> -				      DRM_MODE_CONNECTED);
> +			/*
> +			 * There could be as many hotplug events sent by
> +			 * driver as connectors we scheduled an HPD toggle on
> +			 * above, depending on timing. So if we're not seeing
> +			 * the expected connector state try to wait for an HPD
> +			 * event for each connector/port.
> +			 */
> +			current_state = reprobe_connector(data, port);
> +			if (p > 0 && current_state != target_state) {
> +				igt_assert(wait_for_hotplug(mon, &timeout));
> +				current_state = reprobe_connector(data, port);
> +			}
> +
> +			igt_assert_eq(current_state, target_state);
>  		}
>  
>  		port = NULL;
-- 
Cheers,
	Lyude Paul

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

[igt-dev] [PATCH i-g-t v2] tests/kms_chamelium: Make sure we wait for each connectors' hotplug event

[Imre Deak]

After scheduling an HPD toggle event, make sure that we wait for the
hotplug event for each connector that may be sent by the driver.

Depending on the scheduling there could be 1 event or as many events as
connectors we scheduled an HPD toggle event on, depending on the timing.
So if we don't yet see the expected connector state on a given connector
try to wait for an additional hotplug event and reprobe/recheck the
state.

v2:
- s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits in
  the debugging output. (Lyude)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Lyude Paul <lyude@redhat.com>
---
 tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index 714e5e06..502f1efa 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct chamelium_port *port,
 	drmModeFreeConnector(connector);
 }
 
+/* Wait for hotplug and return the remaining time left from timeout */
+static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
+{
+	struct timespec start, end;
+	int elapsed;
+	bool detected;
+
+	igt_assert_eq(igt_gettime(&start), 0);
+	detected = igt_hotplug_detected(mon, *timeout);
+	igt_assert_eq(igt_gettime(&end), 0);
+
+	elapsed = igt_time_elapsed(&start, &end);
+	igt_assert_lte(0, elapsed);
+	*timeout = max(0, *timeout - elapsed);
+
+	return detected;
+}
+
 static void
 try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
 		       enum igt_suspend_state state, enum igt_suspend_test test,
 		       struct udev_monitor *mon, bool connected)
 {
+	drmModeConnection target_state = connected ? DRM_MODE_DISCONNECTED :
+						     DRM_MODE_CONNECTED;
+	int timeout = HOTPLUG_TIMEOUT;
 	int delay;
 	int p;
 
@@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
 	}
 
 	igt_system_suspend_autoresume(state, test);
+	igt_assert(wait_for_hotplug(mon, &timeout));
 
-	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
 	if (port) {
-		igt_assert_eq(reprobe_connector(data, port), connected ?
-			      DRM_MODE_DISCONNECTED : DRM_MODE_CONNECTED);
+		igt_assert_eq(reprobe_connector(data, port), target_state);
 	} else {
 		for (p = 0; p < data->port_count; p++) {
+			drmModeConnection current_state;
+
 			port = data->ports[p];
-			igt_assert_eq(reprobe_connector(data, port), connected ?
-				      DRM_MODE_DISCONNECTED :
-				      DRM_MODE_CONNECTED);
+			/*
+			 * There could be as many hotplug events sent by
+			 * driver as connectors we scheduled an HPD toggle on
+			 * above, depending on timing. So if we're not seeing
+			 * the expected connector state try to wait for an HPD
+			 * event for each connector/port.
+			 */
+			current_state = reprobe_connector(data, port);
+			if (p > 0 && current_state != target_state) {
+				igt_assert(wait_for_hotplug(mon, &timeout));
+				current_state = reprobe_connector(data, port);
+			}
+
+			igt_assert_eq(current_state, target_state);
 		}
 
 		port = NULL;
-- 
2.17.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 tests/kms_chamelium: Make sure we wait for each connectors' hotplug event (rev2)

[Patchwork]

== Series Details ==

Series: tests/kms_chamelium: Make sure we wait for each connectors' hotplug event (rev2)
URL   : https://patchwork.freedesktop.org/series/60334/
State : success

== Summary ==

CI Bug Log - changes from IGT_4973 -> IGTPW_2950
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/60334/revisions/2/mbox/

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       [PASS][1] -> [DMESG-WARN][2] ([fdo#108965])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/fi-kbl-8809g/igt@amdgpu/amd_basic@userptr.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/fi-kbl-8809g/igt@amdgpu/amd_basic@userptr.html

  * igt@i915_selftest@live_hangcheck:
    - fi-skl-iommu:       [PASS][3] -> [INCOMPLETE][4] ([fdo#108602] / [fdo#108744])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][5] -> [FAIL][6] ([fdo#109485])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485


Participating hosts (51 -> 44)
------------------------------

  Additional (2): fi-icl-u2 fi-apl-guc 
  Missing    (9): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-icl-dsi fi-bdw-samus 


Build changes
-------------

  * IGT: IGT_4973 -> IGTPW_2950

  CI_DRM_6063: 44ae4003d35743cbc7883825c5fe777d136b5247 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2950: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/
  IGT_4973: 3e3ff0e48989abd25fce4916e85e8fef20a3c63a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

[igt-dev] ✓ Fi.CI.IGT: success for tests/kms_chamelium: Make sure we wait for each connectors' hotplug event (rev2)

[Patchwork]

== Series Details ==

Series: tests/kms_chamelium: Make sure we wait for each connectors' hotplug event (rev2)
URL   : https://patchwork.freedesktop.org/series/60334/
State : success

== Summary ==

CI Bug Log - changes from IGT_4973_full -> IGTPW_2950_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/60334/revisions/2/mbox/

Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-hsw:          [PASS][1] -> [FAIL][2] ([fdo#108686])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-hsw8/igt@gem_tiled_swapping@non-threaded.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-hsw5/igt@gem_tiled_swapping@non-threaded.html
    - shard-apl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#108686])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl5/igt@gem_tiled_swapping@non-threaded.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl1/igt@gem_tiled_swapping@non-threaded.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-kbl:          [PASS][5] -> [INCOMPLETE][6] ([fdo#103665])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-kbl1/igt@i915_suspend@fence-restore-tiled2untiled.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-kbl2/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_cursor_crc@cursor-128x42-random:
    - shard-apl:          [PASS][7] -> [FAIL][8] ([fdo#103232])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl5/igt@kms_cursor_crc@cursor-128x42-random.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl3/igt@kms_cursor_crc@cursor-128x42-random.html
    - shard-kbl:          [PASS][9] -> [FAIL][10] ([fdo#103232])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-kbl4/igt@kms_cursor_crc@cursor-128x42-random.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-kbl5/igt@kms_cursor_crc@cursor-128x42-random.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [PASS][11] -> [SKIP][12] ([fdo#109349])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb6/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * igt@kms_flip@plain-flip-ts-check:
    - shard-hsw:          [PASS][13] -> [INCOMPLETE][14] ([fdo#103540])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-hsw5/igt@kms_flip@plain-flip-ts-check.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-hsw5/igt@kms_flip@plain-flip-ts-check.html
    - shard-glk:          [PASS][15] -> [FAIL][16] ([fdo#100368])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-glk2/igt@kms_flip@plain-flip-ts-check.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-glk8/igt@kms_flip@plain-flip-ts-check.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
    - shard-iclb:         [PASS][17] -> [FAIL][18] ([fdo#103167]) +8 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-apl:          [PASS][19] -> [DMESG-WARN][20] ([fdo#108566]) +4 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html

  * igt@kms_plane_scaling@pipe-a-scaler-with-clipping-clamping:
    - shard-glk:          [PASS][21] -> [SKIP][22] ([fdo#109271] / [fdo#109278])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-glk9/igt@kms_plane_scaling@pipe-a-scaler-with-clipping-clamping.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-glk7/igt@kms_plane_scaling@pipe-a-scaler-with-clipping-clamping.html

  * igt@kms_psr@psr2_sprite_render:
    - shard-iclb:         [PASS][23] -> [SKIP][24] ([fdo#109441])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb2/igt@kms_psr@psr2_sprite_render.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb7/igt@kms_psr@psr2_sprite_render.html

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         [PASS][25] -> [FAIL][26] ([fdo#100047])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb4/igt@kms_sysfs_edid_timing.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb2/igt@kms_sysfs_edid_timing.html

  
#### Possible fixes ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-glk:          [DMESG-WARN][27] ([fdo#108686]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-glk7/igt@gem_tiled_swapping@non-threaded.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-glk8/igt@gem_tiled_swapping@non-threaded.html

  * igt@i915_suspend@debugfs-reader:
    - shard-apl:          [DMESG-WARN][29] ([fdo#108566]) -> [PASS][30] +3 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl4/igt@i915_suspend@debugfs-reader.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl4/igt@i915_suspend@debugfs-reader.html

  * igt@kms_cursor_crc@cursor-64x21-sliding:
    - shard-apl:          [FAIL][31] ([fdo#103232]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl3/igt@kms_cursor_crc@cursor-64x21-sliding.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl5/igt@kms_cursor_crc@cursor-64x21-sliding.html
    - shard-kbl:          [FAIL][33] ([fdo#103232]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-kbl3/igt@kms_cursor_crc@cursor-64x21-sliding.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-kbl5/igt@kms_cursor_crc@cursor-64x21-sliding.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          [FAIL][35] ([fdo#105363]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-glk4/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-glk5/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@2x-flip-vs-suspend:
    - shard-hsw:          [INCOMPLETE][37] ([fdo#103540]) -> [PASS][38] +1 similar issue
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-hsw7/igt@kms_flip@2x-flip-vs-suspend.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-hsw5/igt@kms_flip@2x-flip-vs-suspend.html

  * igt@kms_flip@plain-flip-fb-recreate:
    - shard-kbl:          [FAIL][39] ([fdo#100368]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-kbl4/igt@kms_flip@plain-flip-fb-recreate.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-kbl6/igt@kms_flip@plain-flip-fb-recreate.html

  * igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
    - shard-iclb:         [FAIL][41] ([fdo#103167]) -> [PASS][42] +5 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb5/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html

  * igt@kms_plane@pixel-format-pipe-b-planes:
    - shard-glk:          [SKIP][43] ([fdo#109271]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-glk1/igt@kms_plane@pixel-format-pipe-b-planes.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-glk9/igt@kms_plane@pixel-format-pipe-b-planes.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [FAIL][45] ([fdo#103166]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb2/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_sprite_plane_move:
    - shard-iclb:         [SKIP][47] ([fdo#109441]) -> [PASS][48] +2 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb5/igt@kms_psr@psr2_sprite_plane_move.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html

  * igt@kms_setmode@basic:
    - shard-apl:          [FAIL][49] ([fdo#99912]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl8/igt@kms_setmode@basic.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl4/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-kbl:          [INCOMPLETE][51] ([fdo#103665]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-kbl1/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-kbl4/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  
  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 6)
------------------------------

  Missing    (1): shard-skl 


Build changes
-------------

  * IGT: IGT_4973 -> IGTPW_2950

  CI_DRM_6063: 44ae4003d35743cbc7883825c5fe777d136b5247 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2950: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/
  IGT_4973: 3e3ff0e48989abd25fce4916e85e8fef20a3c63a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

Re: [igt-dev] [PATCH i-g-t v2] tests/kms_chamelium: Make sure we wait for each connectors' hotplug event

[Paul Kocialkowski]

On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> After scheduling an HPD toggle event, make sure that we wait for the
> hotplug event for each connector that may be sent by the driver.
> 
> Depending on the scheduling there could be 1 event or as many events as
> connectors we scheduled an HPD toggle event on, depending on the timing.
> So if we don't yet see the expected connector state on a given connector
> try to wait for an additional hotplug event and reprobe/recheck the
> state.

This changes makes good sense to me, so:

Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

And it brings back hard feelings about the hotplug interface we have
today...

Cheers,

Paul

> v2:
> - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits in
>   the debugging output. (Lyude)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> ---
>  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 714e5e06..502f1efa 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct chamelium_port *port,
>  	drmModeFreeConnector(connector);
>  }
>  
> +/* Wait for hotplug and return the remaining time left from timeout */
> +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> +{
> +	struct timespec start, end;
> +	int elapsed;
> +	bool detected;
> +
> +	igt_assert_eq(igt_gettime(&start), 0);
> +	detected = igt_hotplug_detected(mon, *timeout);
> +	igt_assert_eq(igt_gettime(&end), 0);
> +
> +	elapsed = igt_time_elapsed(&start, &end);
> +	igt_assert_lte(0, elapsed);
> +	*timeout = max(0, *timeout - elapsed);
> +
> +	return detected;
> +}
> +
>  static void
>  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
>  		       enum igt_suspend_state state, enum igt_suspend_test test,
>  		       struct udev_monitor *mon, bool connected)
>  {
> +	drmModeConnection target_state = connected ? DRM_MODE_DISCONNECTED :
> +						     DRM_MODE_CONNECTED;
> +	int timeout = HOTPLUG_TIMEOUT;
>  	int delay;
>  	int p;
>  
> @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
>  	}
>  
>  	igt_system_suspend_autoresume(state, test);
> +	igt_assert(wait_for_hotplug(mon, &timeout));
>  
> -	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
>  	if (port) {
> -		igt_assert_eq(reprobe_connector(data, port), connected ?
> -			      DRM_MODE_DISCONNECTED : DRM_MODE_CONNECTED);
> +		igt_assert_eq(reprobe_connector(data, port), target_state);
>  	} else {
>  		for (p = 0; p < data->port_count; p++) {
> +			drmModeConnection current_state;
> +
>  			port = data->ports[p];
> -			igt_assert_eq(reprobe_connector(data, port), connected ?
> -				      DRM_MODE_DISCONNECTED :
> -				      DRM_MODE_CONNECTED);
> +			/*
> +			 * There could be as many hotplug events sent by
> +			 * driver as connectors we scheduled an HPD toggle on
> +			 * above, depending on timing. So if we're not seeing
> +			 * the expected connector state try to wait for an HPD
> +			 * event for each connector/port.
> +			 */
> +			current_state = reprobe_connector(data, port);
> +			if (p > 0 && current_state != target_state) {
> +				igt_assert(wait_for_hotplug(mon, &timeout));
> +				current_state = reprobe_connector(data, port);
> +			}
> +
> +			igt_assert_eq(current_state, target_state);
>  		}
>  
>  		port = NULL;
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: [igt-dev] [PATCH i-g-t v2] tests/kms_chamelium: Make sure we wait for each connectors' hotplug event

[Imre Deak]

On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > After scheduling an HPD toggle event, make sure that we wait for the
> > hotplug event for each connector that may be sent by the driver.
> > 
> > Depending on the scheduling there could be 1 event or as many events as
> > connectors we scheduled an HPD toggle event on, depending on the timing.
> > So if we don't yet see the expected connector state on a given connector
> > try to wait for an additional hotplug event and reprobe/recheck the
> > state.
> 
> This changes makes good sense to me, so:
> 
> Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> 
> And it brings back hard feelings about the hotplug interface we have
> today...

Yes, I was surprised too not find any way to identify which connector(s)
the hotplug event is associated with. We discussed with Ville if it'd
make sense to add a connector ID map to the uevent, but not sure if it'd
be useful outside of this test, or how that would align with Chris'
connector probe state generation idea.

> 
> Cheers,
> 
> Paul
> 
> > v2:
> > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits in
> >   the debugging output. (Lyude)
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > ---
> >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 39 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > index 714e5e06..502f1efa 100644
> > --- a/tests/kms_chamelium.c
> > +++ b/tests/kms_chamelium.c
> > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct chamelium_port *port,
> >  	drmModeFreeConnector(connector);
> >  }
> >  
> > +/* Wait for hotplug and return the remaining time left from timeout */
> > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > +{
> > +	struct timespec start, end;
> > +	int elapsed;
> > +	bool detected;
> > +
> > +	igt_assert_eq(igt_gettime(&start), 0);
> > +	detected = igt_hotplug_detected(mon, *timeout);
> > +	igt_assert_eq(igt_gettime(&end), 0);
> > +
> > +	elapsed = igt_time_elapsed(&start, &end);
> > +	igt_assert_lte(0, elapsed);
> > +	*timeout = max(0, *timeout - elapsed);
> > +
> > +	return detected;
> > +}
> > +
> >  static void
> >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> >  		       enum igt_suspend_state state, enum igt_suspend_test test,
> >  		       struct udev_monitor *mon, bool connected)
> >  {
> > +	drmModeConnection target_state = connected ? DRM_MODE_DISCONNECTED :
> > +						     DRM_MODE_CONNECTED;
> > +	int timeout = HOTPLUG_TIMEOUT;
> >  	int delay;
> >  	int p;
> >  
> > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> >  	}
> >  
> >  	igt_system_suspend_autoresume(state, test);
> > +	igt_assert(wait_for_hotplug(mon, &timeout));
> >  
> > -	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> >  	if (port) {
> > -		igt_assert_eq(reprobe_connector(data, port), connected ?
> > -			      DRM_MODE_DISCONNECTED : DRM_MODE_CONNECTED);
> > +		igt_assert_eq(reprobe_connector(data, port), target_state);
> >  	} else {
> >  		for (p = 0; p < data->port_count; p++) {
> > +			drmModeConnection current_state;
> > +
> >  			port = data->ports[p];
> > -			igt_assert_eq(reprobe_connector(data, port), connected ?
> > -				      DRM_MODE_DISCONNECTED :
> > -				      DRM_MODE_CONNECTED);
> > +			/*
> > +			 * There could be as many hotplug events sent by
> > +			 * driver as connectors we scheduled an HPD toggle on
> > +			 * above, depending on timing. So if we're not seeing
> > +			 * the expected connector state try to wait for an HPD
> > +			 * event for each connector/port.
> > +			 */
> > +			current_state = reprobe_connector(data, port);
> > +			if (p > 0 && current_state != target_state) {
> > +				igt_assert(wait_for_hotplug(mon, &timeout));
> > +				current_state = reprobe_connector(data, port);
> > +			}
> > +
> > +			igt_assert_eq(current_state, target_state);
> >  		}
> >  
> >  		port = NULL;
> -- 
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2] tests/kms_chamelium: Make sure we wait for each connectors' hotplug event

[Paul Kocialkowski]

Hi,

On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > After scheduling an HPD toggle event, make sure that we wait for the
> > > hotplug event for each connector that may be sent by the driver.
> > > 
> > > Depending on the scheduling there could be 1 event or as many events as
> > > connectors we scheduled an HPD toggle event on, depending on the timing.
> > > So if we don't yet see the expected connector state on a given connector
> > > try to wait for an additional hotplug event and reprobe/recheck the
> > > state.
> > 
> > This changes makes good sense to me, so:
> > 
> > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > 
> > And it brings back hard feelings about the hotplug interface we have
> > today...
> 
> Yes, I was surprised too not find any way to identify which connector(s)
> the hotplug event is associated with. We discussed with Ville if it'd
> make sense to add a connector ID map to the uevent, but not sure if it'd
> be useful outside of this test, or how that would align with Chris'
> connector probe state generation idea.

Either way, I'm definitely in favor of having something more reliable
to expose to userspace. Having something to correlate each event to one
(or more) connector(s) sounds good to me.

And I think there is a general need for this, it's not just IGT.

Currently, every userspace application using DRM has to do a full
reprobe once a hotplug uevent is received, which is really not optimal.
If an entry identifying the connector was also provided, userspace
could only reprobe that connector. The step after that would be having
the indication of whether the connector was connected or disconnected
directly in uevent too, so that no probing needs to be done at all when
a given connector is disconnected.

Anyway, if you're interested in the idea and that Ville and Chris are
in favor, I really think it would be worth fixing that. And we'd only
be adding new elements to uevent, so that would stay backward-
compatible too.

Cheers,

Paul

> > > v2:
> > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits in
> > >   the debugging output. (Lyude)
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > ---
> > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > index 714e5e06..502f1efa 100644
> > > --- a/tests/kms_chamelium.c
> > > +++ b/tests/kms_chamelium.c
> > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct chamelium_port *port,
> > >  	drmModeFreeConnector(connector);
> > >  }
> > >  
> > > +/* Wait for hotplug and return the remaining time left from timeout */
> > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > +{
> > > +	struct timespec start, end;
> > > +	int elapsed;
> > > +	bool detected;
> > > +
> > > +	igt_assert_eq(igt_gettime(&start), 0);
> > > +	detected = igt_hotplug_detected(mon, *timeout);
> > > +	igt_assert_eq(igt_gettime(&end), 0);
> > > +
> > > +	elapsed = igt_time_elapsed(&start, &end);
> > > +	igt_assert_lte(0, elapsed);
> > > +	*timeout = max(0, *timeout - elapsed);
> > > +
> > > +	return detected;
> > > +}
> > > +
> > >  static void
> > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > >  		       enum igt_suspend_state state, enum igt_suspend_test test,
> > >  		       struct udev_monitor *mon, bool connected)
> > >  {
> > > +	drmModeConnection target_state = connected ? DRM_MODE_DISCONNECTED :
> > > +						     DRM_MODE_CONNECTED;
> > > +	int timeout = HOTPLUG_TIMEOUT;
> > >  	int delay;
> > >  	int p;
> > >  
> > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > >  	}
> > >  
> > >  	igt_system_suspend_autoresume(state, test);
> > > +	igt_assert(wait_for_hotplug(mon, &timeout));
> > >  
> > > -	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > >  	if (port) {
> > > -		igt_assert_eq(reprobe_connector(data, port), connected ?
> > > -			      DRM_MODE_DISCONNECTED : DRM_MODE_CONNECTED);
> > > +		igt_assert_eq(reprobe_connector(data, port), target_state);
> > >  	} else {
> > >  		for (p = 0; p < data->port_count; p++) {
> > > +			drmModeConnection current_state;
> > > +
> > >  			port = data->ports[p];
> > > -			igt_assert_eq(reprobe_connector(data, port), connected ?
> > > -				      DRM_MODE_DISCONNECTED :
> > > -				      DRM_MODE_CONNECTED);
> > > +			/*
> > > +			 * There could be as many hotplug events sent by
> > > +			 * driver as connectors we scheduled an HPD toggle on
> > > +			 * above, depending on timing. So if we're not seeing
> > > +			 * the expected connector state try to wait for an HPD
> > > +			 * event for each connector/port.
> > > +			 */
> > > +			current_state = reprobe_connector(data, port);
> > > +			if (p > 0 && current_state != target_state) {
> > > +				igt_assert(wait_for_hotplug(mon, &timeout));
> > > +				current_state = reprobe_connector(data, port);
> > > +			}
> > > +
> > > +			igt_assert_eq(current_state, target_state);
> > >  		}
> > >  
> > >  		port = NULL;
> > -- 
> > Paul Kocialkowski, Bootlin
> > Embedded Linux and kernel engineering
> > https://bootlin.com
> > 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: [igt-dev] ✓ Fi.CI.IGT: success for tests/kms_chamelium: Make sure we wait for each connectors' hotplug event (rev2)

[Imre Deak]

On Wed, May 08, 2019 at 11:06:47AM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: tests/kms_chamelium: Make sure we wait for each connectors' hotplug event (rev2)
> URL   : https://patchwork.freedesktop.org/series/60334/
> State : success

Pushed, thanks for the reviews.

> 
> == Summary ==
> 
> CI Bug Log - changes from IGT_4973_full -> IGTPW_2950_full
> ====================================================
> 
> Summary
> -------
> 
>   **SUCCESS**
> 
>   No regressions found.
> 
>   External URL: https://patchwork.freedesktop.org/api/1.0/series/60334/revisions/2/mbox/
> 
> Known issues
> ------------
> 
>   Here are the changes found in IGTPW_2950_full that come from known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>   * igt@gem_tiled_swapping@non-threaded:
>     - shard-hsw:          [PASS][1] -> [FAIL][2] ([fdo#108686])
>    [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-hsw8/igt@gem_tiled_swapping@non-threaded.html
>    [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-hsw5/igt@gem_tiled_swapping@non-threaded.html
>     - shard-apl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#108686])
>    [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl5/igt@gem_tiled_swapping@non-threaded.html
>    [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl1/igt@gem_tiled_swapping@non-threaded.html
> 
>   * igt@i915_suspend@fence-restore-tiled2untiled:
>     - shard-kbl:          [PASS][5] -> [INCOMPLETE][6] ([fdo#103665])
>    [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-kbl1/igt@i915_suspend@fence-restore-tiled2untiled.html
>    [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-kbl2/igt@i915_suspend@fence-restore-tiled2untiled.html
> 
>   * igt@kms_cursor_crc@cursor-128x42-random:
>     - shard-apl:          [PASS][7] -> [FAIL][8] ([fdo#103232])
>    [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl5/igt@kms_cursor_crc@cursor-128x42-random.html
>    [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl3/igt@kms_cursor_crc@cursor-128x42-random.html
>     - shard-kbl:          [PASS][9] -> [FAIL][10] ([fdo#103232])
>    [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-kbl4/igt@kms_cursor_crc@cursor-128x42-random.html
>    [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-kbl5/igt@kms_cursor_crc@cursor-128x42-random.html
> 
>   * igt@kms_dp_dsc@basic-dsc-enable-edp:
>     - shard-iclb:         [PASS][11] -> [SKIP][12] ([fdo#109349])
>    [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
>    [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb6/igt@kms_dp_dsc@basic-dsc-enable-edp.html
> 
>   * igt@kms_flip@plain-flip-ts-check:
>     - shard-hsw:          [PASS][13] -> [INCOMPLETE][14] ([fdo#103540])
>    [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-hsw5/igt@kms_flip@plain-flip-ts-check.html
>    [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-hsw5/igt@kms_flip@plain-flip-ts-check.html
>     - shard-glk:          [PASS][15] -> [FAIL][16] ([fdo#100368])
>    [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-glk2/igt@kms_flip@plain-flip-ts-check.html
>    [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-glk8/igt@kms_flip@plain-flip-ts-check.html
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
>     - shard-iclb:         [PASS][17] -> [FAIL][18] ([fdo#103167]) +8 similar issues
>    [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
>    [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
> 
>   * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
>     - shard-apl:          [PASS][19] -> [DMESG-WARN][20] ([fdo#108566]) +4 similar issues
>    [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
>    [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
> 
>   * igt@kms_plane_scaling@pipe-a-scaler-with-clipping-clamping:
>     - shard-glk:          [PASS][21] -> [SKIP][22] ([fdo#109271] / [fdo#109278])
>    [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-glk9/igt@kms_plane_scaling@pipe-a-scaler-with-clipping-clamping.html
>    [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-glk7/igt@kms_plane_scaling@pipe-a-scaler-with-clipping-clamping.html
> 
>   * igt@kms_psr@psr2_sprite_render:
>     - shard-iclb:         [PASS][23] -> [SKIP][24] ([fdo#109441])
>    [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb2/igt@kms_psr@psr2_sprite_render.html
>    [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb7/igt@kms_psr@psr2_sprite_render.html
> 
>   * igt@kms_sysfs_edid_timing:
>     - shard-iclb:         [PASS][25] -> [FAIL][26] ([fdo#100047])
>    [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb4/igt@kms_sysfs_edid_timing.html
>    [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb2/igt@kms_sysfs_edid_timing.html
> 
>   
> #### Possible fixes ####
> 
>   * igt@gem_tiled_swapping@non-threaded:
>     - shard-glk:          [DMESG-WARN][27] ([fdo#108686]) -> [PASS][28]
>    [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-glk7/igt@gem_tiled_swapping@non-threaded.html
>    [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-glk8/igt@gem_tiled_swapping@non-threaded.html
> 
>   * igt@i915_suspend@debugfs-reader:
>     - shard-apl:          [DMESG-WARN][29] ([fdo#108566]) -> [PASS][30] +3 similar issues
>    [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl4/igt@i915_suspend@debugfs-reader.html
>    [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl4/igt@i915_suspend@debugfs-reader.html
> 
>   * igt@kms_cursor_crc@cursor-64x21-sliding:
>     - shard-apl:          [FAIL][31] ([fdo#103232]) -> [PASS][32]
>    [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl3/igt@kms_cursor_crc@cursor-64x21-sliding.html
>    [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl5/igt@kms_cursor_crc@cursor-64x21-sliding.html
>     - shard-kbl:          [FAIL][33] ([fdo#103232]) -> [PASS][34]
>    [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-kbl3/igt@kms_cursor_crc@cursor-64x21-sliding.html
>    [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-kbl5/igt@kms_cursor_crc@cursor-64x21-sliding.html
> 
>   * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
>     - shard-glk:          [FAIL][35] ([fdo#105363]) -> [PASS][36]
>    [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-glk4/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
>    [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-glk5/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
> 
>   * igt@kms_flip@2x-flip-vs-suspend:
>     - shard-hsw:          [INCOMPLETE][37] ([fdo#103540]) -> [PASS][38] +1 similar issue
>    [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-hsw7/igt@kms_flip@2x-flip-vs-suspend.html
>    [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-hsw5/igt@kms_flip@2x-flip-vs-suspend.html
> 
>   * igt@kms_flip@plain-flip-fb-recreate:
>     - shard-kbl:          [FAIL][39] ([fdo#100368]) -> [PASS][40]
>    [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-kbl4/igt@kms_flip@plain-flip-fb-recreate.html
>    [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-kbl6/igt@kms_flip@plain-flip-fb-recreate.html
> 
>   * igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
>     - shard-iclb:         [FAIL][41] ([fdo#103167]) -> [PASS][42] +5 similar issues
>    [41]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb5/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
>    [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
> 
>   * igt@kms_plane@pixel-format-pipe-b-planes:
>     - shard-glk:          [SKIP][43] ([fdo#109271]) -> [PASS][44]
>    [43]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-glk1/igt@kms_plane@pixel-format-pipe-b-planes.html
>    [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-glk9/igt@kms_plane@pixel-format-pipe-b-planes.html
> 
>   * igt@kms_plane_lowres@pipe-a-tiling-x:
>     - shard-iclb:         [FAIL][45] ([fdo#103166]) -> [PASS][46]
>    [45]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-x.html
>    [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb2/igt@kms_plane_lowres@pipe-a-tiling-x.html
> 
>   * igt@kms_psr@psr2_sprite_plane_move:
>     - shard-iclb:         [SKIP][47] ([fdo#109441]) -> [PASS][48] +2 similar issues
>    [47]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-iclb5/igt@kms_psr@psr2_sprite_plane_move.html
>    [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html
> 
>   * igt@kms_setmode@basic:
>     - shard-apl:          [FAIL][49] ([fdo#99912]) -> [PASS][50]
>    [49]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-apl8/igt@kms_setmode@basic.html
>    [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-apl4/igt@kms_setmode@basic.html
> 
>   * igt@kms_vblank@pipe-a-ts-continuation-suspend:
>     - shard-kbl:          [INCOMPLETE][51] ([fdo#103665]) -> [PASS][52]
>    [51]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4973/shard-kbl1/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
>    [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/shard-kbl4/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
> 
>   
>   [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
>   [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
>   [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
>   [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
>   [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
>   [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
>   [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
>   [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
>   [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
>   [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
>   [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
>   [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
>   [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
>   [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
>   [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
> 
> 
> Participating hosts (7 -> 6)
> ------------------------------
> 
>   Missing    (1): shard-skl 
> 
> 
> Build changes
> -------------
> 
>   * IGT: IGT_4973 -> IGTPW_2950
> 
>   CI_DRM_6063: 44ae4003d35743cbc7883825c5fe777d136b5247 @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGTPW_2950: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/
>   IGT_4973: 3e3ff0e48989abd25fce4916e85e8fef20a3c63a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2950/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2] tests/kms_chamelium: Make sure we wait for each connectors' hotplug event

[Lyude Paul]

I'm in support of this as well, we really could use better hotplug events.
Feel free to cc patches to me for review :)

On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> Hi,
> 
> On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > hotplug event for each connector that may be sent by the driver.
> > > > 
> > > > Depending on the scheduling there could be 1 event or as many events
> > > > as
> > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > timing.
> > > > So if we don't yet see the expected connector state on a given
> > > > connector
> > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > state.
> > > 
> > > This changes makes good sense to me, so:
> > > 
> > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > 
> > > And it brings back hard feelings about the hotplug interface we have
> > > today...
> > 
> > Yes, I was surprised too not find any way to identify which connector(s)
> > the hotplug event is associated with. We discussed with Ville if it'd
> > make sense to add a connector ID map to the uevent, but not sure if it'd
> > be useful outside of this test, or how that would align with Chris'
> > connector probe state generation idea.
> 
> Either way, I'm definitely in favor of having something more reliable
> to expose to userspace. Having something to correlate each event to one
> (or more) connector(s) sounds good to me.
> 
> And I think there is a general need for this, it's not just IGT.
> 
> Currently, every userspace application using DRM has to do a full
> reprobe once a hotplug uevent is received, which is really not optimal.
> If an entry identifying the connector was also provided, userspace
> could only reprobe that connector. The step after that would be having
> the indication of whether the connector was connected or disconnected
> directly in uevent too, so that no probing needs to be done at all when
> a given connector is disconnected.
> 
> Anyway, if you're interested in the idea and that Ville and Chris are
> in favor, I really think it would be worth fixing that. And we'd only
> be adding new elements to uevent, so that would stay backward-
> compatible too.
> 
> Cheers,
> 
> Paul
> 
> > > > v2:
> > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > in
> > > >   the debugging output. (Lyude)
> > > > 
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > ---
> > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > -
> > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > index 714e5e06..502f1efa 100644
> > > > --- a/tests/kms_chamelium.c
> > > > +++ b/tests/kms_chamelium.c
> > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > chamelium_port *port,
> > > >  	drmModeFreeConnector(connector);
> > > >  }
> > > >  
> > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > */
> > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > +{
> > > > +	struct timespec start, end;
> > > > +	int elapsed;
> > > > +	bool detected;
> > > > +
> > > > +	igt_assert_eq(igt_gettime(&start), 0);
> > > > +	detected = igt_hotplug_detected(mon, *timeout);
> > > > +	igt_assert_eq(igt_gettime(&end), 0);
> > > > +
> > > > +	elapsed = igt_time_elapsed(&start, &end);
> > > > +	igt_assert_lte(0, elapsed);
> > > > +	*timeout = max(0, *timeout - elapsed);
> > > > +
> > > > +	return detected;
> > > > +}
> > > > +
> > > >  static void
> > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > >  		       enum igt_suspend_state state, enum
> > > > igt_suspend_test test,
> > > >  		       struct udev_monitor *mon, bool connected)
> > > >  {
> > > > +	drmModeConnection target_state = connected ?
> > > > DRM_MODE_DISCONNECTED :
> > > > +						     DRM_MODE_CONNECTE
> > > > D;
> > > > +	int timeout = HOTPLUG_TIMEOUT;
> > > >  	int delay;
> > > >  	int p;
> > > >  
> > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > chamelium_port *port,
> > > >  	}
> > > >  
> > > >  	igt_system_suspend_autoresume(state, test);
> > > > +	igt_assert(wait_for_hotplug(mon, &timeout));
> > > >  
> > > > -	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > >  	if (port) {
> > > > -		igt_assert_eq(reprobe_connector(data, port), connected
> > > > ?
> > > > -			      DRM_MODE_DISCONNECTED :
> > > > DRM_MODE_CONNECTED);
> > > > +		igt_assert_eq(reprobe_connector(data, port),
> > > > target_state);
> > > >  	} else {
> > > >  		for (p = 0; p < data->port_count; p++) {
> > > > +			drmModeConnection current_state;
> > > > +
> > > >  			port = data->ports[p];
> > > > -			igt_assert_eq(reprobe_connector(data, port),
> > > > connected ?
> > > > -				      DRM_MODE_DISCONNECTED :
> > > > -				      DRM_MODE_CONNECTED);
> > > > +			/*
> > > > +			 * There could be as many hotplug events sent
> > > > by
> > > > +			 * driver as connectors we scheduled an HPD
> > > > toggle on
> > > > +			 * above, depending on timing. So if we're not
> > > > seeing
> > > > +			 * the expected connector state try to wait
> > > > for an HPD
> > > > +			 * event for each connector/port.
> > > > +			 */
> > > > +			current_state = reprobe_connector(data, port);
> > > > +			if (p > 0 && current_state != target_state) {
> > > > +				igt_assert(wait_for_hotplug(mon,
> > > > &timeout));
> > > > +				current_state =
> > > > reprobe_connector(data, port);
> > > > +			}
> > > > +
> > > > +			igt_assert_eq(current_state, target_state);
> > > >  		}
> > > >  
> > > >  		port = NULL;
> > > -- 
> > > Paul Kocialkowski, Bootlin
> > > Embedded Linux and kernel engineering
> > > https://bootlin.com
> > > 
-- 
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 v2] tests/kms_chamelium: Make sure we wait for each connectors' hotplug event

[Ser, Simon]

On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> I'm in support of this as well, we really could use better hotplug events.
> Feel free to cc patches to me for review :)

Any idea(s) how the API would look like?

> On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > Hi,
> > 
> > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > hotplug event for each connector that may be sent by the driver.
> > > > > 
> > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > as
> > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > timing.
> > > > > So if we don't yet see the expected connector state on a given
> > > > > connector
> > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > state.
> > > > 
> > > > This changes makes good sense to me, so:
> > > > 
> > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > 
> > > > And it brings back hard feelings about the hotplug interface we have
> > > > today...
> > > 
> > > Yes, I was surprised too not find any way to identify which connector(s)
> > > the hotplug event is associated with. We discussed with Ville if it'd
> > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > be useful outside of this test, or how that would align with Chris'
> > > connector probe state generation idea.
> > 
> > Either way, I'm definitely in favor of having something more reliable
> > to expose to userspace. Having something to correlate each event to one
> > (or more) connector(s) sounds good to me.
> > 
> > And I think there is a general need for this, it's not just IGT.
> > 
> > Currently, every userspace application using DRM has to do a full
> > reprobe once a hotplug uevent is received, which is really not optimal.
> > If an entry identifying the connector was also provided, userspace
> > could only reprobe that connector. The step after that would be having
> > the indication of whether the connector was connected or disconnected
> > directly in uevent too, so that no probing needs to be done at all when
> > a given connector is disconnected.
> > 
> > Anyway, if you're interested in the idea and that Ville and Chris are
> > in favor, I really think it would be worth fixing that. And we'd only
> > be adding new elements to uevent, so that would stay backward-
> > compatible too.
> > 
> > Cheers,
> > 
> > Paul
> > 
> > > > > v2:
> > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > in
> > > > >   the debugging output. (Lyude)
> > > > > 
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > ---
> > > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > -
> > > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > index 714e5e06..502f1efa 100644
> > > > > --- a/tests/kms_chamelium.c
> > > > > +++ b/tests/kms_chamelium.c
> > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > chamelium_port *port,
> > > > >  	drmModeFreeConnector(connector);
> > > > >  }
> > > > >  
> > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > */
> > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > +{
> > > > > +	struct timespec start, end;
> > > > > +	int elapsed;
> > > > > +	bool detected;
> > > > > +
> > > > > +	igt_assert_eq(igt_gettime(&start), 0);
> > > > > +	detected = igt_hotplug_detected(mon, *timeout);
> > > > > +	igt_assert_eq(igt_gettime(&end), 0);
> > > > > +
> > > > > +	elapsed = igt_time_elapsed(&start, &end);
> > > > > +	igt_assert_lte(0, elapsed);
> > > > > +	*timeout = max(0, *timeout - elapsed);
> > > > > +
> > > > > +	return detected;
> > > > > +}
> > > > > +
> > > > >  static void
> > > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > >  		       enum igt_suspend_state state, enum
> > > > > igt_suspend_test test,
> > > > >  		       struct udev_monitor *mon, bool connected)
> > > > >  {
> > > > > +	drmModeConnection target_state = connected ?
> > > > > DRM_MODE_DISCONNECTED :
> > > > > +						     DRM_MODE_CONNECTE
> > > > > D;
> > > > > +	int timeout = HOTPLUG_TIMEOUT;
> > > > >  	int delay;
> > > > >  	int p;
> > > > >  
> > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > chamelium_port *port,
> > > > >  	}
> > > > >  
> > > > >  	igt_system_suspend_autoresume(state, test);
> > > > > +	igt_assert(wait_for_hotplug(mon, &timeout));
> > > > >  
> > > > > -	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > >  	if (port) {
> > > > > -		igt_assert_eq(reprobe_connector(data, port), connected
> > > > > ?
> > > > > -			      DRM_MODE_DISCONNECTED :
> > > > > DRM_MODE_CONNECTED);
> > > > > +		igt_assert_eq(reprobe_connector(data, port),
> > > > > target_state);
> > > > >  	} else {
> > > > >  		for (p = 0; p < data->port_count; p++) {
> > > > > +			drmModeConnection current_state;
> > > > > +
> > > > >  			port = data->ports[p];
> > > > > -			igt_assert_eq(reprobe_connector(data, port),
> > > > > connected ?
> > > > > -				      DRM_MODE_DISCONNECTED :
> > > > > -				      DRM_MODE_CONNECTED);
> > > > > +			/*
> > > > > +			 * There could be as many hotplug events sent
> > > > > by
> > > > > +			 * driver as connectors we scheduled an HPD
> > > > > toggle on
> > > > > +			 * above, depending on timing. So if we're not
> > > > > seeing
> > > > > +			 * the expected connector state try to wait
> > > > > for an HPD
> > > > > +			 * event for each connector/port.
> > > > > +			 */
> > > > > +			current_state = reprobe_connector(data, port);
> > > > > +			if (p > 0 && current_state != target_state) {
> > > > > +				igt_assert(wait_for_hotplug(mon,
> > > > > &timeout));
> > > > > +				current_state =
> > > > > reprobe_connector(data, port);
> > > > > +			}
> > > > > +
> > > > > +			igt_assert_eq(current_state, target_state);
> > > > >  		}
> > > > >  
> > > > >  		port = NULL;
> > > > -- 
> > > > Paul Kocialkowski, Bootlin
> > > > Embedded Linux and kernel engineering
> > > > https://bootlin.com
> > > > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Improving DRM connector hotplug uevents

[Paul Kocialkowski]

Hi,

So this thread has drifted off from IGT to a general DRM improvement,
so renaming the thread and adding some CCs.

It's about avoiding full reprobes (which may include slow EDID reads,
etc) when a connector change was detected, by providing information
about the connector and the new state to userspace. This way, userspace
can reprobe the changed connector alone (or not reprobe at all in case
of a disconnect).

It feels like there is growing temptation to have per-driver hacks to
avoid full reprobes when the driver knows that nothing changed, but I
think it makes more sense to fixup the uevent we send to make sure that
userspace knows what to probe exactly.

On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > I'm in support of this as well, we really could use better hotplug events.
> > Feel free to cc patches to me for review :)
> 
> Any idea(s) how the API would look like?

From a quick chat on IRC yesterday, it seems that just adding entries
to the uevent would do and would be backwards-compatible (well,
provided that the userspace uevent parsers will not fail if something
extra to "HOTPLUG=1" is provided).

Currently, the notification is sent globally at each round of detected
connector change (which may concern multiple connectors). So I think we
should just list the connectors that changed in the uevent and their
new state. So my proposal here is something like:

HOTPLUG=1
CONNECTOR_ID=47
STATUS=Connected
CONNECTOR_ID=48
STATUS=Disconnected

Where each STATUS entry would refer to the previous CONNECTOR_ID entry.

What do you think?

Cheers,

Paul

> > On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > > Hi,
> > > 
> > > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > > hotplug event for each connector that may be sent by the driver.
> > > > > > 
> > > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > > as
> > > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > > timing.
> > > > > > So if we don't yet see the expected connector state on a given
> > > > > > connector
> > > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > > state.
> > > > > 
> > > > > This changes makes good sense to me, so:
> > > > > 
> > > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > 
> > > > > And it brings back hard feelings about the hotplug interface we have
> > > > > today...
> > > > 
> > > > Yes, I was surprised too not find any way to identify which connector(s)
> > > > the hotplug event is associated with. We discussed with Ville if it'd
> > > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > > be useful outside of this test, or how that would align with Chris'
> > > > connector probe state generation idea.
> > > 
> > > Either way, I'm definitely in favor of having something more reliable
> > > to expose to userspace. Having something to correlate each event to one
> > > (or more) connector(s) sounds good to me.
> > > 
> > > And I think there is a general need for this, it's not just IGT.
> > > 
> > > Currently, every userspace application using DRM has to do a full
> > > reprobe once a hotplug uevent is received, which is really not optimal.
> > > If an entry identifying the connector was also provided, userspace
> > > could only reprobe that connector. The step after that would be having
> > > the indication of whether the connector was connected or disconnected
> > > directly in uevent too, so that no probing needs to be done at all when
> > > a given connector is disconnected.
> > > 
> > > Anyway, if you're interested in the idea and that Ville and Chris are
> > > in favor, I really think it would be worth fixing that. And we'd only
> > > be adding new elements to uevent, so that would stay backward-
> > > compatible too.
> > > 
> > > Cheers,
> > > 
> > > Paul
> > > 
> > > > > > v2:
> > > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > > in
> > > > > >   the debugging output. (Lyude)
> > > > > > 
> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > > ---
> > > > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > > -
> > > > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > > index 714e5e06..502f1efa 100644
> > > > > > --- a/tests/kms_chamelium.c
> > > > > > +++ b/tests/kms_chamelium.c
> > > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > > chamelium_port *port,
> > > > > >  	drmModeFreeConnector(connector);
> > > > > >  }
> > > > > >  
> > > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > > */
> > > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > > +{
> > > > > > +	struct timespec start, end;
> > > > > > +	int elapsed;
> > > > > > +	bool detected;
> > > > > > +
> > > > > > +	igt_assert_eq(igt_gettime(&start), 0);
> > > > > > +	detected = igt_hotplug_detected(mon, *timeout);
> > > > > > +	igt_assert_eq(igt_gettime(&end), 0);
> > > > > > +
> > > > > > +	elapsed = igt_time_elapsed(&start, &end);
> > > > > > +	igt_assert_lte(0, elapsed);
> > > > > > +	*timeout = max(0, *timeout - elapsed);
> > > > > > +
> > > > > > +	return detected;
> > > > > > +}
> > > > > > +
> > > > > >  static void
> > > > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > > >  		       enum igt_suspend_state state, enum
> > > > > > igt_suspend_test test,
> > > > > >  		       struct udev_monitor *mon, bool connected)
> > > > > >  {
> > > > > > +	drmModeConnection target_state = connected ?
> > > > > > DRM_MODE_DISCONNECTED :
> > > > > > +						     DRM_MODE_CONNECTE
> > > > > > D;
> > > > > > +	int timeout = HOTPLUG_TIMEOUT;
> > > > > >  	int delay;
> > > > > >  	int p;
> > > > > >  
> > > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > > chamelium_port *port,
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_system_suspend_autoresume(state, test);
> > > > > > +	igt_assert(wait_for_hotplug(mon, &timeout));
> > > > > >  
> > > > > > -	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > > >  	if (port) {
> > > > > > -		igt_assert_eq(reprobe_connector(data, port), connected
> > > > > > ?
> > > > > > -			      DRM_MODE_DISCONNECTED :
> > > > > > DRM_MODE_CONNECTED);
> > > > > > +		igt_assert_eq(reprobe_connector(data, port),
> > > > > > target_state);
> > > > > >  	} else {
> > > > > >  		for (p = 0; p < data->port_count; p++) {
> > > > > > +			drmModeConnection current_state;
> > > > > > +
> > > > > >  			port = data->ports[p];
> > > > > > -			igt_assert_eq(reprobe_connector(data, port),
> > > > > > connected ?
> > > > > > -				      DRM_MODE_DISCONNECTED :
> > > > > > -				      DRM_MODE_CONNECTED);
> > > > > > +			/*
> > > > > > +			 * There could be as many hotplug events sent
> > > > > > by
> > > > > > +			 * driver as connectors we scheduled an HPD
> > > > > > toggle on
> > > > > > +			 * above, depending on timing. So if we're not
> > > > > > seeing
> > > > > > +			 * the expected connector state try to wait
> > > > > > for an HPD
> > > > > > +			 * event for each connector/port.
> > > > > > +			 */
> > > > > > +			current_state = reprobe_connector(data, port);
> > > > > > +			if (p > 0 && current_state != target_state) {
> > > > > > +				igt_assert(wait_for_hotplug(mon,
> > > > > > &timeout));
> > > > > > +				current_state =
> > > > > > reprobe_connector(data, port);
> > > > > > +			}
> > > > > > +
> > > > > > +			igt_assert_eq(current_state, target_state);
> > > > > >  		}
> > > > > >  
> > > > > >  		port = NULL;
> > > > > -- 
> > > > > Paul Kocialkowski, Bootlin
> > > > > Embedded Linux and kernel engineering
> > > > > https://bootlin.com
> > > > > 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[igt-dev] Improving DRM connector hotplug uevents

[Paul Kocialkowski]

Hi,

So this thread has drifted off from IGT to a general DRM improvement,
so renaming the thread and adding some CCs.

It's about avoiding full reprobes (which may include slow EDID reads,
etc) when a connector change was detected, by providing information
about the connector and the new state to userspace. This way, userspace
can reprobe the changed connector alone (or not reprobe at all in case
of a disconnect).

It feels like there is growing temptation to have per-driver hacks to
avoid full reprobes when the driver knows that nothing changed, but I
think it makes more sense to fixup the uevent we send to make sure that
userspace knows what to probe exactly.

On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > I'm in support of this as well, we really could use better hotplug events.
> > Feel free to cc patches to me for review :)
> 
> Any idea(s) how the API would look like?

From a quick chat on IRC yesterday, it seems that just adding entries
to the uevent would do and would be backwards-compatible (well,
provided that the userspace uevent parsers will not fail if something
extra to "HOTPLUG=1" is provided).

Currently, the notification is sent globally at each round of detected
connector change (which may concern multiple connectors). So I think we
should just list the connectors that changed in the uevent and their
new state. So my proposal here is something like:

HOTPLUG=1
CONNECTOR_ID=47
STATUS=Connected
CONNECTOR_ID=48
STATUS=Disconnected

Where each STATUS entry would refer to the previous CONNECTOR_ID entry.

What do you think?

Cheers,

Paul

> > On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > > Hi,
> > > 
> > > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > > hotplug event for each connector that may be sent by the driver.
> > > > > > 
> > > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > > as
> > > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > > timing.
> > > > > > So if we don't yet see the expected connector state on a given
> > > > > > connector
> > > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > > state.
> > > > > 
> > > > > This changes makes good sense to me, so:
> > > > > 
> > > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > 
> > > > > And it brings back hard feelings about the hotplug interface we have
> > > > > today...
> > > > 
> > > > Yes, I was surprised too not find any way to identify which connector(s)
> > > > the hotplug event is associated with. We discussed with Ville if it'd
> > > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > > be useful outside of this test, or how that would align with Chris'
> > > > connector probe state generation idea.
> > > 
> > > Either way, I'm definitely in favor of having something more reliable
> > > to expose to userspace. Having something to correlate each event to one
> > > (or more) connector(s) sounds good to me.
> > > 
> > > And I think there is a general need for this, it's not just IGT.
> > > 
> > > Currently, every userspace application using DRM has to do a full
> > > reprobe once a hotplug uevent is received, which is really not optimal.
> > > If an entry identifying the connector was also provided, userspace
> > > could only reprobe that connector. The step after that would be having
> > > the indication of whether the connector was connected or disconnected
> > > directly in uevent too, so that no probing needs to be done at all when
> > > a given connector is disconnected.
> > > 
> > > Anyway, if you're interested in the idea and that Ville and Chris are
> > > in favor, I really think it would be worth fixing that. And we'd only
> > > be adding new elements to uevent, so that would stay backward-
> > > compatible too.
> > > 
> > > Cheers,
> > > 
> > > Paul
> > > 
> > > > > > v2:
> > > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > > in
> > > > > >   the debugging output. (Lyude)
> > > > > > 
> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > > ---
> > > > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > > -
> > > > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > > index 714e5e06..502f1efa 100644
> > > > > > --- a/tests/kms_chamelium.c
> > > > > > +++ b/tests/kms_chamelium.c
> > > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > > chamelium_port *port,
> > > > > >  	drmModeFreeConnector(connector);
> > > > > >  }
> > > > > >  
> > > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > > */
> > > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > > +{
> > > > > > +	struct timespec start, end;
> > > > > > +	int elapsed;
> > > > > > +	bool detected;
> > > > > > +
> > > > > > +	igt_assert_eq(igt_gettime(&start), 0);
> > > > > > +	detected = igt_hotplug_detected(mon, *timeout);
> > > > > > +	igt_assert_eq(igt_gettime(&end), 0);
> > > > > > +
> > > > > > +	elapsed = igt_time_elapsed(&start, &end);
> > > > > > +	igt_assert_lte(0, elapsed);
> > > > > > +	*timeout = max(0, *timeout - elapsed);
> > > > > > +
> > > > > > +	return detected;
> > > > > > +}
> > > > > > +
> > > > > >  static void
> > > > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > > >  		       enum igt_suspend_state state, enum
> > > > > > igt_suspend_test test,
> > > > > >  		       struct udev_monitor *mon, bool connected)
> > > > > >  {
> > > > > > +	drmModeConnection target_state = connected ?
> > > > > > DRM_MODE_DISCONNECTED :
> > > > > > +						     DRM_MODE_CONNECTE
> > > > > > D;
> > > > > > +	int timeout = HOTPLUG_TIMEOUT;
> > > > > >  	int delay;
> > > > > >  	int p;
> > > > > >  
> > > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > > chamelium_port *port,
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_system_suspend_autoresume(state, test);
> > > > > > +	igt_assert(wait_for_hotplug(mon, &timeout));
> > > > > >  
> > > > > > -	igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > > >  	if (port) {
> > > > > > -		igt_assert_eq(reprobe_connector(data, port), connected
> > > > > > ?
> > > > > > -			      DRM_MODE_DISCONNECTED :
> > > > > > DRM_MODE_CONNECTED);
> > > > > > +		igt_assert_eq(reprobe_connector(data, port),
> > > > > > target_state);
> > > > > >  	} else {
> > > > > >  		for (p = 0; p < data->port_count; p++) {
> > > > > > +			drmModeConnection current_state;
> > > > > > +
> > > > > >  			port = data->ports[p];
> > > > > > -			igt_assert_eq(reprobe_connector(data, port),
> > > > > > connected ?
> > > > > > -				      DRM_MODE_DISCONNECTED :
> > > > > > -				      DRM_MODE_CONNECTED);
> > > > > > +			/*
> > > > > > +			 * There could be as many hotplug events sent
> > > > > > by
> > > > > > +			 * driver as connectors we scheduled an HPD
> > > > > > toggle on
> > > > > > +			 * above, depending on timing. So if we're not
> > > > > > seeing
> > > > > > +			 * the expected connector state try to wait
> > > > > > for an HPD
> > > > > > +			 * event for each connector/port.
> > > > > > +			 */
> > > > > > +			current_state = reprobe_connector(data, port);
> > > > > > +			if (p > 0 && current_state != target_state) {
> > > > > > +				igt_assert(wait_for_hotplug(mon,
> > > > > > &timeout));
> > > > > > +				current_state =
> > > > > > reprobe_connector(data, port);
> > > > > > +			}
> > > > > > +
> > > > > > +			igt_assert_eq(current_state, target_state);
> > > > > >  		}
> > > > > >  
> > > > > >  		port = NULL;
> > > > > -- 
> > > > > Paul Kocialkowski, Bootlin
> > > > > Embedded Linux and kernel engineering
> > > > > https://bootlin.com
> > > > > 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: Improving DRM connector hotplug uevents

[Daniel Vetter]

On Fri, May 10, 2019 at 9:42 AM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> So this thread has drifted off from IGT to a general DRM improvement,
> so renaming the thread and adding some CCs.
>
> It's about avoiding full reprobes (which may include slow EDID reads,
> etc) when a connector change was detected, by providing information
> about the connector and the new state to userspace. This way, userspace
> can reprobe the changed connector alone (or not reprobe at all in case
> of a disconnect).
>
> It feels like there is growing temptation to have per-driver hacks to
> avoid full reprobes when the driver knows that nothing changed, but I
> think it makes more sense to fixup the uevent we send to make sure that
> userspace knows what to probe exactly.
>
> On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> > On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > > I'm in support of this as well, we really could use better hotplug events.
> > > Feel free to cc patches to me for review :)
> >
> > Any idea(s) how the API would look like?
>
> From a quick chat on IRC yesterday, it seems that just adding entries
> to the uevent would do and would be backwards-compatible (well,
> provided that the userspace uevent parsers will not fail if something
> extra to "HOTPLUG=1" is provided).
>
> Currently, the notification is sent globally at each round of detected
> connector change (which may concern multiple connectors). So I think we
> should just list the connectors that changed in the uevent and their
> new state. So my proposal here is something like:
>
> HOTPLUG=1
> CONNECTOR_ID=47
> STATUS=Connected
> CONNECTOR_ID=48
> STATUS=Disconnected
>
> Where each STATUS entry would refer to the previous CONNECTOR_ID entry.
>
> What do you think?

We have the better uevent already, all we need is wiring up, lots of
code, and userspace for it. Ok, not quite, patch is still in flight:

https://patchwork.kernel.org/patch/10906677/

The real trouble isn't the new event, but making sure it goes out for
anything that might have changed. There's a lot more connector status
than just the STATUS property.
-Daniel

>
> Cheers,
>
> Paul
>
> > > On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > > > Hi,
> > > >
> > > > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > > > hotplug event for each connector that may be sent by the driver.
> > > > > > >
> > > > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > > > as
> > > > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > > > timing.
> > > > > > > So if we don't yet see the expected connector state on a given
> > > > > > > connector
> > > > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > > > state.
> > > > > >
> > > > > > This changes makes good sense to me, so:
> > > > > >
> > > > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > >
> > > > > > And it brings back hard feelings about the hotplug interface we have
> > > > > > today...
> > > > >
> > > > > Yes, I was surprised too not find any way to identify which connector(s)
> > > > > the hotplug event is associated with. We discussed with Ville if it'd
> > > > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > > > be useful outside of this test, or how that would align with Chris'
> > > > > connector probe state generation idea.
> > > >
> > > > Either way, I'm definitely in favor of having something more reliable
> > > > to expose to userspace. Having something to correlate each event to one
> > > > (or more) connector(s) sounds good to me.
> > > >
> > > > And I think there is a general need for this, it's not just IGT.
> > > >
> > > > Currently, every userspace application using DRM has to do a full
> > > > reprobe once a hotplug uevent is received, which is really not optimal.
> > > > If an entry identifying the connector was also provided, userspace
> > > > could only reprobe that connector. The step after that would be having
> > > > the indication of whether the connector was connected or disconnected
> > > > directly in uevent too, so that no probing needs to be done at all when
> > > > a given connector is disconnected.
> > > >
> > > > Anyway, if you're interested in the idea and that Ville and Chris are
> > > > in favor, I really think it would be worth fixing that. And we'd only
> > > > be adding new elements to uevent, so that would stay backward-
> > > > compatible too.
> > > >
> > > > Cheers,
> > > >
> > > > Paul
> > > >
> > > > > > > v2:
> > > > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > > > in
> > > > > > >   the debugging output. (Lyude)
> > > > > > >
> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > > > ---
> > > > > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > > > -
> > > > > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > > > index 714e5e06..502f1efa 100644
> > > > > > > --- a/tests/kms_chamelium.c
> > > > > > > +++ b/tests/kms_chamelium.c
> > > > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > > > chamelium_port *port,
> > > > > > >     drmModeFreeConnector(connector);
> > > > > > >  }
> > > > > > >
> > > > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > > > */
> > > > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > > > +{
> > > > > > > +   struct timespec start, end;
> > > > > > > +   int elapsed;
> > > > > > > +   bool detected;
> > > > > > > +
> > > > > > > +   igt_assert_eq(igt_gettime(&start), 0);
> > > > > > > +   detected = igt_hotplug_detected(mon, *timeout);
> > > > > > > +   igt_assert_eq(igt_gettime(&end), 0);
> > > > > > > +
> > > > > > > +   elapsed = igt_time_elapsed(&start, &end);
> > > > > > > +   igt_assert_lte(0, elapsed);
> > > > > > > +   *timeout = max(0, *timeout - elapsed);
> > > > > > > +
> > > > > > > +   return detected;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static void
> > > > > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > > > >                    enum igt_suspend_state state, enum
> > > > > > > igt_suspend_test test,
> > > > > > >                    struct udev_monitor *mon, bool connected)
> > > > > > >  {
> > > > > > > +   drmModeConnection target_state = connected ?
> > > > > > > DRM_MODE_DISCONNECTED :
> > > > > > > +                                                DRM_MODE_CONNECTE
> > > > > > > D;
> > > > > > > +   int timeout = HOTPLUG_TIMEOUT;
> > > > > > >     int delay;
> > > > > > >     int p;
> > > > > > >
> > > > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > > > chamelium_port *port,
> > > > > > >     }
> > > > > > >
> > > > > > >     igt_system_suspend_autoresume(state, test);
> > > > > > > +   igt_assert(wait_for_hotplug(mon, &timeout));
> > > > > > >
> > > > > > > -   igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > > > >     if (port) {
> > > > > > > -           igt_assert_eq(reprobe_connector(data, port), connected
> > > > > > > ?
> > > > > > > -                         DRM_MODE_DISCONNECTED :
> > > > > > > DRM_MODE_CONNECTED);
> > > > > > > +           igt_assert_eq(reprobe_connector(data, port),
> > > > > > > target_state);
> > > > > > >     } else {
> > > > > > >             for (p = 0; p < data->port_count; p++) {
> > > > > > > +                   drmModeConnection current_state;
> > > > > > > +
> > > > > > >                     port = data->ports[p];
> > > > > > > -                   igt_assert_eq(reprobe_connector(data, port),
> > > > > > > connected ?
> > > > > > > -                                 DRM_MODE_DISCONNECTED :
> > > > > > > -                                 DRM_MODE_CONNECTED);
> > > > > > > +                   /*
> > > > > > > +                    * There could be as many hotplug events sent
> > > > > > > by
> > > > > > > +                    * driver as connectors we scheduled an HPD
> > > > > > > toggle on
> > > > > > > +                    * above, depending on timing. So if we're not
> > > > > > > seeing
> > > > > > > +                    * the expected connector state try to wait
> > > > > > > for an HPD
> > > > > > > +                    * event for each connector/port.
> > > > > > > +                    */
> > > > > > > +                   current_state = reprobe_connector(data, port);
> > > > > > > +                   if (p > 0 && current_state != target_state) {
> > > > > > > +                           igt_assert(wait_for_hotplug(mon,
> > > > > > > &timeout));
> > > > > > > +                           current_state =
> > > > > > > reprobe_connector(data, port);
> > > > > > > +                   }
> > > > > > > +
> > > > > > > +                   igt_assert_eq(current_state, target_state);
> > > > > > >             }
> > > > > > >
> > > > > > >             port = NULL;
> > > > > > --
> > > > > > Paul Kocialkowski, Bootlin
> > > > > > Embedded Linux and kernel engineering
> > > > > > https://bootlin.com
> > > > > >
> --
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [igt-dev] Improving DRM connector hotplug uevents

[Daniel Vetter]

On Fri, May 10, 2019 at 9:42 AM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> So this thread has drifted off from IGT to a general DRM improvement,
> so renaming the thread and adding some CCs.
>
> It's about avoiding full reprobes (which may include slow EDID reads,
> etc) when a connector change was detected, by providing information
> about the connector and the new state to userspace. This way, userspace
> can reprobe the changed connector alone (or not reprobe at all in case
> of a disconnect).
>
> It feels like there is growing temptation to have per-driver hacks to
> avoid full reprobes when the driver knows that nothing changed, but I
> think it makes more sense to fixup the uevent we send to make sure that
> userspace knows what to probe exactly.
>
> On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> > On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > > I'm in support of this as well, we really could use better hotplug events.
> > > Feel free to cc patches to me for review :)
> >
> > Any idea(s) how the API would look like?
>
> From a quick chat on IRC yesterday, it seems that just adding entries
> to the uevent would do and would be backwards-compatible (well,
> provided that the userspace uevent parsers will not fail if something
> extra to "HOTPLUG=1" is provided).
>
> Currently, the notification is sent globally at each round of detected
> connector change (which may concern multiple connectors). So I think we
> should just list the connectors that changed in the uevent and their
> new state. So my proposal here is something like:
>
> HOTPLUG=1
> CONNECTOR_ID=47
> STATUS=Connected
> CONNECTOR_ID=48
> STATUS=Disconnected
>
> Where each STATUS entry would refer to the previous CONNECTOR_ID entry.
>
> What do you think?

We have the better uevent already, all we need is wiring up, lots of
code, and userspace for it. Ok, not quite, patch is still in flight:

https://patchwork.kernel.org/patch/10906677/

The real trouble isn't the new event, but making sure it goes out for
anything that might have changed. There's a lot more connector status
than just the STATUS property.
-Daniel

>
> Cheers,
>
> Paul
>
> > > On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > > > Hi,
> > > >
> > > > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > > > hotplug event for each connector that may be sent by the driver.
> > > > > > >
> > > > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > > > as
> > > > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > > > timing.
> > > > > > > So if we don't yet see the expected connector state on a given
> > > > > > > connector
> > > > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > > > state.
> > > > > >
> > > > > > This changes makes good sense to me, so:
> > > > > >
> > > > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > >
> > > > > > And it brings back hard feelings about the hotplug interface we have
> > > > > > today...
> > > > >
> > > > > Yes, I was surprised too not find any way to identify which connector(s)
> > > > > the hotplug event is associated with. We discussed with Ville if it'd
> > > > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > > > be useful outside of this test, or how that would align with Chris'
> > > > > connector probe state generation idea.
> > > >
> > > > Either way, I'm definitely in favor of having something more reliable
> > > > to expose to userspace. Having something to correlate each event to one
> > > > (or more) connector(s) sounds good to me.
> > > >
> > > > And I think there is a general need for this, it's not just IGT.
> > > >
> > > > Currently, every userspace application using DRM has to do a full
> > > > reprobe once a hotplug uevent is received, which is really not optimal.
> > > > If an entry identifying the connector was also provided, userspace
> > > > could only reprobe that connector. The step after that would be having
> > > > the indication of whether the connector was connected or disconnected
> > > > directly in uevent too, so that no probing needs to be done at all when
> > > > a given connector is disconnected.
> > > >
> > > > Anyway, if you're interested in the idea and that Ville and Chris are
> > > > in favor, I really think it would be worth fixing that. And we'd only
> > > > be adding new elements to uevent, so that would stay backward-
> > > > compatible too.
> > > >
> > > > Cheers,
> > > >
> > > > Paul
> > > >
> > > > > > > v2:
> > > > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > > > in
> > > > > > >   the debugging output. (Lyude)
> > > > > > >
> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > > > ---
> > > > > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > > > -
> > > > > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > > > index 714e5e06..502f1efa 100644
> > > > > > > --- a/tests/kms_chamelium.c
> > > > > > > +++ b/tests/kms_chamelium.c
> > > > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > > > chamelium_port *port,
> > > > > > >     drmModeFreeConnector(connector);
> > > > > > >  }
> > > > > > >
> > > > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > > > */
> > > > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > > > +{
> > > > > > > +   struct timespec start, end;
> > > > > > > +   int elapsed;
> > > > > > > +   bool detected;
> > > > > > > +
> > > > > > > +   igt_assert_eq(igt_gettime(&start), 0);
> > > > > > > +   detected = igt_hotplug_detected(mon, *timeout);
> > > > > > > +   igt_assert_eq(igt_gettime(&end), 0);
> > > > > > > +
> > > > > > > +   elapsed = igt_time_elapsed(&start, &end);
> > > > > > > +   igt_assert_lte(0, elapsed);
> > > > > > > +   *timeout = max(0, *timeout - elapsed);
> > > > > > > +
> > > > > > > +   return detected;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static void
> > > > > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > > > >                    enum igt_suspend_state state, enum
> > > > > > > igt_suspend_test test,
> > > > > > >                    struct udev_monitor *mon, bool connected)
> > > > > > >  {
> > > > > > > +   drmModeConnection target_state = connected ?
> > > > > > > DRM_MODE_DISCONNECTED :
> > > > > > > +                                                DRM_MODE_CONNECTE
> > > > > > > D;
> > > > > > > +   int timeout = HOTPLUG_TIMEOUT;
> > > > > > >     int delay;
> > > > > > >     int p;
> > > > > > >
> > > > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > > > chamelium_port *port,
> > > > > > >     }
> > > > > > >
> > > > > > >     igt_system_suspend_autoresume(state, test);
> > > > > > > +   igt_assert(wait_for_hotplug(mon, &timeout));
> > > > > > >
> > > > > > > -   igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > > > >     if (port) {
> > > > > > > -           igt_assert_eq(reprobe_connector(data, port), connected
> > > > > > > ?
> > > > > > > -                         DRM_MODE_DISCONNECTED :
> > > > > > > DRM_MODE_CONNECTED);
> > > > > > > +           igt_assert_eq(reprobe_connector(data, port),
> > > > > > > target_state);
> > > > > > >     } else {
> > > > > > >             for (p = 0; p < data->port_count; p++) {
> > > > > > > +                   drmModeConnection current_state;
> > > > > > > +
> > > > > > >                     port = data->ports[p];
> > > > > > > -                   igt_assert_eq(reprobe_connector(data, port),
> > > > > > > connected ?
> > > > > > > -                                 DRM_MODE_DISCONNECTED :
> > > > > > > -                                 DRM_MODE_CONNECTED);
> > > > > > > +                   /*
> > > > > > > +                    * There could be as many hotplug events sent
> > > > > > > by
> > > > > > > +                    * driver as connectors we scheduled an HPD
> > > > > > > toggle on
> > > > > > > +                    * above, depending on timing. So if we're not
> > > > > > > seeing
> > > > > > > +                    * the expected connector state try to wait
> > > > > > > for an HPD
> > > > > > > +                    * event for each connector/port.
> > > > > > > +                    */
> > > > > > > +                   current_state = reprobe_connector(data, port);
> > > > > > > +                   if (p > 0 && current_state != target_state) {
> > > > > > > +                           igt_assert(wait_for_hotplug(mon,
> > > > > > > &timeout));
> > > > > > > +                           current_state =
> > > > > > > reprobe_connector(data, port);
> > > > > > > +                   }
> > > > > > > +
> > > > > > > +                   igt_assert_eq(current_state, target_state);
> > > > > > >             }
> > > > > > >
> > > > > > >             port = NULL;
> > > > > > --
> > > > > > Paul Kocialkowski, Bootlin
> > > > > > Embedded Linux and kernel engineering
> > > > > > https://bootlin.com
> > > > > >
> --
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: Improving DRM connector hotplug uevents

[Paul Kocialkowski]

Hi,

On Fri, 2019-05-10 at 09:56 +0200, Daniel Vetter wrote:
> On Fri, May 10, 2019 at 9:42 AM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > So this thread has drifted off from IGT to a general DRM improvement,
> > so renaming the thread and adding some CCs.
> > 
> > It's about avoiding full reprobes (which may include slow EDID reads,
> > etc) when a connector change was detected, by providing information
> > about the connector and the new state to userspace. This way, userspace
> > can reprobe the changed connector alone (or not reprobe at all in case
> > of a disconnect).
> > 
> > It feels like there is growing temptation to have per-driver hacks to
> > avoid full reprobes when the driver knows that nothing changed, but I
> > think it makes more sense to fixup the uevent we send to make sure that
> > userspace knows what to probe exactly.
> > 
> > On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> > > On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > > > I'm in support of this as well, we really could use better hotplug events.
> > > > Feel free to cc patches to me for review :)
> > > 
> > > Any idea(s) how the API would look like?
> > 
> > From a quick chat on IRC yesterday, it seems that just adding entries
> > to the uevent would do and would be backwards-compatible (well,
> > provided that the userspace uevent parsers will not fail if something
> > extra to "HOTPLUG=1" is provided).
> > 
> > Currently, the notification is sent globally at each round of detected
> > connector change (which may concern multiple connectors). So I think we
> > should just list the connectors that changed in the uevent and their
> > new state. So my proposal here is something like:
> > 
> > HOTPLUG=1
> > CONNECTOR_ID=47
> > STATUS=Connected
> > CONNECTOR_ID=48
> > STATUS=Disconnected
> > 
> > Where each STATUS entry would refer to the previous CONNECTOR_ID entry.
> > 
> > What do you think?
> 
> We have the better uevent already, all we need is wiring up, lots of
> code, and userspace for it. Ok, not quite, patch is still in flight:
> 
> https://patchwork.kernel.org/patch/10906677/

Ah great to see that!

> The real trouble isn't the new event, but making sure it goes out for
> anything that might have changed. There's a lot more connector status
> than just the STATUS property.

Why though? I thought this was a hotplug interface, not a general
"something-about-the-connector-changed". To me, hotplug is pretty
binary: either it's connected, either it's not.

Do we already send out hotplug uevents when something else than the
status changed? If so, maybe we should consider handling the rest
separately from hotplug.

Looking at drm_probe_helper.c's output_poll_execute, my understanding
is that drm_kms_helper_hotplug_event is only called when the status
changed, not other props. 

Cheers,

Paul

> -Daniel
> 
> > Cheers,
> > 
> > Paul
> > 
> > > > On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > > > > Hi,
> > > > > 
> > > > > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > > > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > > > > hotplug event for each connector that may be sent by the driver.
> > > > > > > > 
> > > > > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > > > > as
> > > > > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > > > > timing.
> > > > > > > > So if we don't yet see the expected connector state on a given
> > > > > > > > connector
> > > > > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > > > > state.
> > > > > > > 
> > > > > > > This changes makes good sense to me, so:
> > > > > > > 
> > > > > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > 
> > > > > > > And it brings back hard feelings about the hotplug interface we have
> > > > > > > today...
> > > > > > 
> > > > > > Yes, I was surprised too not find any way to identify which connector(s)
> > > > > > the hotplug event is associated with. We discussed with Ville if it'd
> > > > > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > > > > be useful outside of this test, or how that would align with Chris'
> > > > > > connector probe state generation idea.
> > > > > 
> > > > > Either way, I'm definitely in favor of having something more reliable
> > > > > to expose to userspace. Having something to correlate each event to one
> > > > > (or more) connector(s) sounds good to me.
> > > > > 
> > > > > And I think there is a general need for this, it's not just IGT.
> > > > > 
> > > > > Currently, every userspace application using DRM has to do a full
> > > > > reprobe once a hotplug uevent is received, which is really not optimal.
> > > > > If an entry identifying the connector was also provided, userspace
> > > > > could only reprobe that connector. The step after that would be having
> > > > > the indication of whether the connector was connected or disconnected
> > > > > directly in uevent too, so that no probing needs to be done at all when
> > > > > a given connector is disconnected.
> > > > > 
> > > > > Anyway, if you're interested in the idea and that Ville and Chris are
> > > > > in favor, I really think it would be worth fixing that. And we'd only
> > > > > be adding new elements to uevent, so that would stay backward-
> > > > > compatible too.
> > > > > 
> > > > > Cheers,
> > > > > 
> > > > > Paul
> > > > > 
> > > > > > > > v2:
> > > > > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > > > > in
> > > > > > > >   the debugging output. (Lyude)
> > > > > > > > 
> > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > > > > ---
> > > > > > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > > > > -
> > > > > > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > > > > index 714e5e06..502f1efa 100644
> > > > > > > > --- a/tests/kms_chamelium.c
> > > > > > > > +++ b/tests/kms_chamelium.c
> > > > > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > > > > chamelium_port *port,
> > > > > > > >     drmModeFreeConnector(connector);
> > > > > > > >  }
> > > > > > > > 
> > > > > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > > > > */
> > > > > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > > > > +{
> > > > > > > > +   struct timespec start, end;
> > > > > > > > +   int elapsed;
> > > > > > > > +   bool detected;
> > > > > > > > +
> > > > > > > > +   igt_assert_eq(igt_gettime(&start), 0);
> > > > > > > > +   detected = igt_hotplug_detected(mon, *timeout);
> > > > > > > > +   igt_assert_eq(igt_gettime(&end), 0);
> > > > > > > > +
> > > > > > > > +   elapsed = igt_time_elapsed(&start, &end);
> > > > > > > > +   igt_assert_lte(0, elapsed);
> > > > > > > > +   *timeout = max(0, *timeout - elapsed);
> > > > > > > > +
> > > > > > > > +   return detected;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static void
> > > > > > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > > > > >                    enum igt_suspend_state state, enum
> > > > > > > > igt_suspend_test test,
> > > > > > > >                    struct udev_monitor *mon, bool connected)
> > > > > > > >  {
> > > > > > > > +   drmModeConnection target_state = connected ?
> > > > > > > > DRM_MODE_DISCONNECTED :
> > > > > > > > +                                                DRM_MODE_CONNECTE
> > > > > > > > D;
> > > > > > > > +   int timeout = HOTPLUG_TIMEOUT;
> > > > > > > >     int delay;
> > > > > > > >     int p;
> > > > > > > > 
> > > > > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > > > > chamelium_port *port,
> > > > > > > >     }
> > > > > > > > 
> > > > > > > >     igt_system_suspend_autoresume(state, test);
> > > > > > > > +   igt_assert(wait_for_hotplug(mon, &timeout));
> > > > > > > > 
> > > > > > > > -   igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > > > > >     if (port) {
> > > > > > > > -           igt_assert_eq(reprobe_connector(data, port), connected
> > > > > > > > ?
> > > > > > > > -                         DRM_MODE_DISCONNECTED :
> > > > > > > > DRM_MODE_CONNECTED);
> > > > > > > > +           igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > target_state);
> > > > > > > >     } else {
> > > > > > > >             for (p = 0; p < data->port_count; p++) {
> > > > > > > > +                   drmModeConnection current_state;
> > > > > > > > +
> > > > > > > >                     port = data->ports[p];
> > > > > > > > -                   igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > connected ?
> > > > > > > > -                                 DRM_MODE_DISCONNECTED :
> > > > > > > > -                                 DRM_MODE_CONNECTED);
> > > > > > > > +                   /*
> > > > > > > > +                    * There could be as many hotplug events sent
> > > > > > > > by
> > > > > > > > +                    * driver as connectors we scheduled an HPD
> > > > > > > > toggle on
> > > > > > > > +                    * above, depending on timing. So if we're not
> > > > > > > > seeing
> > > > > > > > +                    * the expected connector state try to wait
> > > > > > > > for an HPD
> > > > > > > > +                    * event for each connector/port.
> > > > > > > > +                    */
> > > > > > > > +                   current_state = reprobe_connector(data, port);
> > > > > > > > +                   if (p > 0 && current_state != target_state) {
> > > > > > > > +                           igt_assert(wait_for_hotplug(mon,
> > > > > > > > &timeout));
> > > > > > > > +                           current_state =
> > > > > > > > reprobe_connector(data, port);
> > > > > > > > +                   }
> > > > > > > > +
> > > > > > > > +                   igt_assert_eq(current_state, target_state);
> > > > > > > >             }
> > > > > > > > 
> > > > > > > >             port = NULL;
> > > > > > > --
> > > > > > > Paul Kocialkowski, Bootlin
> > > > > > > Embedded Linux and kernel engineering
> > > > > > > https://bootlin.com
> > > > > > > 
> > --
> > Paul Kocialkowski, Bootlin
> > Embedded Linux and kernel engineering
> > https://bootlin.com
> > 
> 
> 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [igt-dev] Improving DRM connector hotplug uevents

[Paul Kocialkowski]

Hi,

On Fri, 2019-05-10 at 09:56 +0200, Daniel Vetter wrote:
> On Fri, May 10, 2019 at 9:42 AM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > So this thread has drifted off from IGT to a general DRM improvement,
> > so renaming the thread and adding some CCs.
> > 
> > It's about avoiding full reprobes (which may include slow EDID reads,
> > etc) when a connector change was detected, by providing information
> > about the connector and the new state to userspace. This way, userspace
> > can reprobe the changed connector alone (or not reprobe at all in case
> > of a disconnect).
> > 
> > It feels like there is growing temptation to have per-driver hacks to
> > avoid full reprobes when the driver knows that nothing changed, but I
> > think it makes more sense to fixup the uevent we send to make sure that
> > userspace knows what to probe exactly.
> > 
> > On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> > > On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > > > I'm in support of this as well, we really could use better hotplug events.
> > > > Feel free to cc patches to me for review :)
> > > 
> > > Any idea(s) how the API would look like?
> > 
> > From a quick chat on IRC yesterday, it seems that just adding entries
> > to the uevent would do and would be backwards-compatible (well,
> > provided that the userspace uevent parsers will not fail if something
> > extra to "HOTPLUG=1" is provided).
> > 
> > Currently, the notification is sent globally at each round of detected
> > connector change (which may concern multiple connectors). So I think we
> > should just list the connectors that changed in the uevent and their
> > new state. So my proposal here is something like:
> > 
> > HOTPLUG=1
> > CONNECTOR_ID=47
> > STATUS=Connected
> > CONNECTOR_ID=48
> > STATUS=Disconnected
> > 
> > Where each STATUS entry would refer to the previous CONNECTOR_ID entry.
> > 
> > What do you think?
> 
> We have the better uevent already, all we need is wiring up, lots of
> code, and userspace for it. Ok, not quite, patch is still in flight:
> 
> https://patchwork.kernel.org/patch/10906677/

Ah great to see that!

> The real trouble isn't the new event, but making sure it goes out for
> anything that might have changed. There's a lot more connector status
> than just the STATUS property.

Why though? I thought this was a hotplug interface, not a general
"something-about-the-connector-changed". To me, hotplug is pretty
binary: either it's connected, either it's not.

Do we already send out hotplug uevents when something else than the
status changed? If so, maybe we should consider handling the rest
separately from hotplug.

Looking at drm_probe_helper.c's output_poll_execute, my understanding
is that drm_kms_helper_hotplug_event is only called when the status
changed, not other props. 

Cheers,

Paul

> -Daniel
> 
> > Cheers,
> > 
> > Paul
> > 
> > > > On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > > > > Hi,
> > > > > 
> > > > > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > > > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > > > > hotplug event for each connector that may be sent by the driver.
> > > > > > > > 
> > > > > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > > > > as
> > > > > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > > > > timing.
> > > > > > > > So if we don't yet see the expected connector state on a given
> > > > > > > > connector
> > > > > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > > > > state.
> > > > > > > 
> > > > > > > This changes makes good sense to me, so:
> > > > > > > 
> > > > > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > 
> > > > > > > And it brings back hard feelings about the hotplug interface we have
> > > > > > > today...
> > > > > > 
> > > > > > Yes, I was surprised too not find any way to identify which connector(s)
> > > > > > the hotplug event is associated with. We discussed with Ville if it'd
> > > > > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > > > > be useful outside of this test, or how that would align with Chris'
> > > > > > connector probe state generation idea.
> > > > > 
> > > > > Either way, I'm definitely in favor of having something more reliable
> > > > > to expose to userspace. Having something to correlate each event to one
> > > > > (or more) connector(s) sounds good to me.
> > > > > 
> > > > > And I think there is a general need for this, it's not just IGT.
> > > > > 
> > > > > Currently, every userspace application using DRM has to do a full
> > > > > reprobe once a hotplug uevent is received, which is really not optimal.
> > > > > If an entry identifying the connector was also provided, userspace
> > > > > could only reprobe that connector. The step after that would be having
> > > > > the indication of whether the connector was connected or disconnected
> > > > > directly in uevent too, so that no probing needs to be done at all when
> > > > > a given connector is disconnected.
> > > > > 
> > > > > Anyway, if you're interested in the idea and that Ville and Chris are
> > > > > in favor, I really think it would be worth fixing that. And we'd only
> > > > > be adding new elements to uevent, so that would stay backward-
> > > > > compatible too.
> > > > > 
> > > > > Cheers,
> > > > > 
> > > > > Paul
> > > > > 
> > > > > > > > v2:
> > > > > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > > > > in
> > > > > > > >   the debugging output. (Lyude)
> > > > > > > > 
> > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > > > > ---
> > > > > > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > > > > -
> > > > > > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > > > > index 714e5e06..502f1efa 100644
> > > > > > > > --- a/tests/kms_chamelium.c
> > > > > > > > +++ b/tests/kms_chamelium.c
> > > > > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > > > > chamelium_port *port,
> > > > > > > >     drmModeFreeConnector(connector);
> > > > > > > >  }
> > > > > > > > 
> > > > > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > > > > */
> > > > > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > > > > +{
> > > > > > > > +   struct timespec start, end;
> > > > > > > > +   int elapsed;
> > > > > > > > +   bool detected;
> > > > > > > > +
> > > > > > > > +   igt_assert_eq(igt_gettime(&start), 0);
> > > > > > > > +   detected = igt_hotplug_detected(mon, *timeout);
> > > > > > > > +   igt_assert_eq(igt_gettime(&end), 0);
> > > > > > > > +
> > > > > > > > +   elapsed = igt_time_elapsed(&start, &end);
> > > > > > > > +   igt_assert_lte(0, elapsed);
> > > > > > > > +   *timeout = max(0, *timeout - elapsed);
> > > > > > > > +
> > > > > > > > +   return detected;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static void
> > > > > > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > > > > >                    enum igt_suspend_state state, enum
> > > > > > > > igt_suspend_test test,
> > > > > > > >                    struct udev_monitor *mon, bool connected)
> > > > > > > >  {
> > > > > > > > +   drmModeConnection target_state = connected ?
> > > > > > > > DRM_MODE_DISCONNECTED :
> > > > > > > > +                                                DRM_MODE_CONNECTE
> > > > > > > > D;
> > > > > > > > +   int timeout = HOTPLUG_TIMEOUT;
> > > > > > > >     int delay;
> > > > > > > >     int p;
> > > > > > > > 
> > > > > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > > > > chamelium_port *port,
> > > > > > > >     }
> > > > > > > > 
> > > > > > > >     igt_system_suspend_autoresume(state, test);
> > > > > > > > +   igt_assert(wait_for_hotplug(mon, &timeout));
> > > > > > > > 
> > > > > > > > -   igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > > > > >     if (port) {
> > > > > > > > -           igt_assert_eq(reprobe_connector(data, port), connected
> > > > > > > > ?
> > > > > > > > -                         DRM_MODE_DISCONNECTED :
> > > > > > > > DRM_MODE_CONNECTED);
> > > > > > > > +           igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > target_state);
> > > > > > > >     } else {
> > > > > > > >             for (p = 0; p < data->port_count; p++) {
> > > > > > > > +                   drmModeConnection current_state;
> > > > > > > > +
> > > > > > > >                     port = data->ports[p];
> > > > > > > > -                   igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > connected ?
> > > > > > > > -                                 DRM_MODE_DISCONNECTED :
> > > > > > > > -                                 DRM_MODE_CONNECTED);
> > > > > > > > +                   /*
> > > > > > > > +                    * There could be as many hotplug events sent
> > > > > > > > by
> > > > > > > > +                    * driver as connectors we scheduled an HPD
> > > > > > > > toggle on
> > > > > > > > +                    * above, depending on timing. So if we're not
> > > > > > > > seeing
> > > > > > > > +                    * the expected connector state try to wait
> > > > > > > > for an HPD
> > > > > > > > +                    * event for each connector/port.
> > > > > > > > +                    */
> > > > > > > > +                   current_state = reprobe_connector(data, port);
> > > > > > > > +                   if (p > 0 && current_state != target_state) {
> > > > > > > > +                           igt_assert(wait_for_hotplug(mon,
> > > > > > > > &timeout));
> > > > > > > > +                           current_state =
> > > > > > > > reprobe_connector(data, port);
> > > > > > > > +                   }
> > > > > > > > +
> > > > > > > > +                   igt_assert_eq(current_state, target_state);
> > > > > > > >             }
> > > > > > > > 
> > > > > > > >             port = NULL;
> > > > > > > --
> > > > > > > Paul Kocialkowski, Bootlin
> > > > > > > Embedded Linux and kernel engineering
> > > > > > > https://bootlin.com
> > > > > > > 
> > --
> > Paul Kocialkowski, Bootlin
> > Embedded Linux and kernel engineering
> > https://bootlin.com
> > 
> 
> 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: Improving DRM connector hotplug uevents

[Daniel Vetter]

On Fri, May 10, 2019 at 10:01 AM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Fri, 2019-05-10 at 09:56 +0200, Daniel Vetter wrote:
> > On Fri, May 10, 2019 at 9:42 AM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > Hi,
> > >
> > > So this thread has drifted off from IGT to a general DRM improvement,
> > > so renaming the thread and adding some CCs.
> > >
> > > It's about avoiding full reprobes (which may include slow EDID reads,
> > > etc) when a connector change was detected, by providing information
> > > about the connector and the new state to userspace. This way, userspace
> > > can reprobe the changed connector alone (or not reprobe at all in case
> > > of a disconnect).
> > >
> > > It feels like there is growing temptation to have per-driver hacks to
> > > avoid full reprobes when the driver knows that nothing changed, but I
> > > think it makes more sense to fixup the uevent we send to make sure that
> > > userspace knows what to probe exactly.
> > >
> > > On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> > > > On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > > > > I'm in support of this as well, we really could use better hotplug events.
> > > > > Feel free to cc patches to me for review :)
> > > >
> > > > Any idea(s) how the API would look like?
> > >
> > > From a quick chat on IRC yesterday, it seems that just adding entries
> > > to the uevent would do and would be backwards-compatible (well,
> > > provided that the userspace uevent parsers will not fail if something
> > > extra to "HOTPLUG=1" is provided).
> > >
> > > Currently, the notification is sent globally at each round of detected
> > > connector change (which may concern multiple connectors). So I think we
> > > should just list the connectors that changed in the uevent and their
> > > new state. So my proposal here is something like:
> > >
> > > HOTPLUG=1
> > > CONNECTOR_ID=47
> > > STATUS=Connected
> > > CONNECTOR_ID=48
> > > STATUS=Disconnected
> > >
> > > Where each STATUS entry would refer to the previous CONNECTOR_ID entry.
> > >
> > > What do you think?
> >
> > We have the better uevent already, all we need is wiring up, lots of
> > code, and userspace for it. Ok, not quite, patch is still in flight:
> >
> > https://patchwork.kernel.org/patch/10906677/
>
> Ah great to see that!
>
> > The real trouble isn't the new event, but making sure it goes out for
> > anything that might have changed. There's a lot more connector status
> > than just the STATUS property.
>
> Why though? I thought this was a hotplug interface, not a general
> "something-about-the-connector-changed". To me, hotplug is pretty
> binary: either it's connected, either it's not.
>
> Do we already send out hotplug uevents when something else than the
> status changed? If so, maybe we should consider handling the rest
> separately from hotplug.

We have plenty of bugs around "changed screen over s/r and no output".
Imo if we rework hotplug, we should get all the mistakes right.

> Looking at drm_probe_helper.c's output_poll_execute, my understanding
> is that drm_kms_helper_hotplug_event is only called when the status
> changed, not other props.

Yeah, and that's why userspace has to reprobe all the time. One idea
that we floated around is to have an epoche counter, which changes
every time anything with the connector changes wrt hotplug related
state (edid, sink id, whatever). The more specific hotplug uevent
would then give you an event on that epoch counter, so userspace knows
it needs to reprobe that specific connector for all the new state.
-Daniel


> Cheers,
>
> Paul
>
> > -Daniel
> >
> > > Cheers,
> > >
> > > Paul
> > >
> > > > > On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > > > > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > > > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > > > > > hotplug event for each connector that may be sent by the driver.
> > > > > > > > >
> > > > > > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > > > > > as
> > > > > > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > > > > > timing.
> > > > > > > > > So if we don't yet see the expected connector state on a given
> > > > > > > > > connector
> > > > > > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > > > > > state.
> > > > > > > >
> > > > > > > > This changes makes good sense to me, so:
> > > > > > > >
> > > > > > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > >
> > > > > > > > And it brings back hard feelings about the hotplug interface we have
> > > > > > > > today...
> > > > > > >
> > > > > > > Yes, I was surprised too not find any way to identify which connector(s)
> > > > > > > the hotplug event is associated with. We discussed with Ville if it'd
> > > > > > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > > > > > be useful outside of this test, or how that would align with Chris'
> > > > > > > connector probe state generation idea.
> > > > > >
> > > > > > Either way, I'm definitely in favor of having something more reliable
> > > > > > to expose to userspace. Having something to correlate each event to one
> > > > > > (or more) connector(s) sounds good to me.
> > > > > >
> > > > > > And I think there is a general need for this, it's not just IGT.
> > > > > >
> > > > > > Currently, every userspace application using DRM has to do a full
> > > > > > reprobe once a hotplug uevent is received, which is really not optimal.
> > > > > > If an entry identifying the connector was also provided, userspace
> > > > > > could only reprobe that connector. The step after that would be having
> > > > > > the indication of whether the connector was connected or disconnected
> > > > > > directly in uevent too, so that no probing needs to be done at all when
> > > > > > a given connector is disconnected.
> > > > > >
> > > > > > Anyway, if you're interested in the idea and that Ville and Chris are
> > > > > > in favor, I really think it would be worth fixing that. And we'd only
> > > > > > be adding new elements to uevent, so that would stay backward-
> > > > > > compatible too.
> > > > > >
> > > > > > Cheers,
> > > > > >
> > > > > > Paul
> > > > > >
> > > > > > > > > v2:
> > > > > > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > > > > > in
> > > > > > > > >   the debugging output. (Lyude)
> > > > > > > > >
> > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > > > > > ---
> > > > > > > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > > > > > -
> > > > > > > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > > > > > index 714e5e06..502f1efa 100644
> > > > > > > > > --- a/tests/kms_chamelium.c
> > > > > > > > > +++ b/tests/kms_chamelium.c
> > > > > > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > > > > > chamelium_port *port,
> > > > > > > > >     drmModeFreeConnector(connector);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > > > > > */
> > > > > > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > > > > > +{
> > > > > > > > > +   struct timespec start, end;
> > > > > > > > > +   int elapsed;
> > > > > > > > > +   bool detected;
> > > > > > > > > +
> > > > > > > > > +   igt_assert_eq(igt_gettime(&start), 0);
> > > > > > > > > +   detected = igt_hotplug_detected(mon, *timeout);
> > > > > > > > > +   igt_assert_eq(igt_gettime(&end), 0);
> > > > > > > > > +
> > > > > > > > > +   elapsed = igt_time_elapsed(&start, &end);
> > > > > > > > > +   igt_assert_lte(0, elapsed);
> > > > > > > > > +   *timeout = max(0, *timeout - elapsed);
> > > > > > > > > +
> > > > > > > > > +   return detected;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  static void
> > > > > > > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > > > > > >                    enum igt_suspend_state state, enum
> > > > > > > > > igt_suspend_test test,
> > > > > > > > >                    struct udev_monitor *mon, bool connected)
> > > > > > > > >  {
> > > > > > > > > +   drmModeConnection target_state = connected ?
> > > > > > > > > DRM_MODE_DISCONNECTED :
> > > > > > > > > +                                                DRM_MODE_CONNECTE
> > > > > > > > > D;
> > > > > > > > > +   int timeout = HOTPLUG_TIMEOUT;
> > > > > > > > >     int delay;
> > > > > > > > >     int p;
> > > > > > > > >
> > > > > > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > > > > > chamelium_port *port,
> > > > > > > > >     }
> > > > > > > > >
> > > > > > > > >     igt_system_suspend_autoresume(state, test);
> > > > > > > > > +   igt_assert(wait_for_hotplug(mon, &timeout));
> > > > > > > > >
> > > > > > > > > -   igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > > > > > >     if (port) {
> > > > > > > > > -           igt_assert_eq(reprobe_connector(data, port), connected
> > > > > > > > > ?
> > > > > > > > > -                         DRM_MODE_DISCONNECTED :
> > > > > > > > > DRM_MODE_CONNECTED);
> > > > > > > > > +           igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > > target_state);
> > > > > > > > >     } else {
> > > > > > > > >             for (p = 0; p < data->port_count; p++) {
> > > > > > > > > +                   drmModeConnection current_state;
> > > > > > > > > +
> > > > > > > > >                     port = data->ports[p];
> > > > > > > > > -                   igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > > connected ?
> > > > > > > > > -                                 DRM_MODE_DISCONNECTED :
> > > > > > > > > -                                 DRM_MODE_CONNECTED);
> > > > > > > > > +                   /*
> > > > > > > > > +                    * There could be as many hotplug events sent
> > > > > > > > > by
> > > > > > > > > +                    * driver as connectors we scheduled an HPD
> > > > > > > > > toggle on
> > > > > > > > > +                    * above, depending on timing. So if we're not
> > > > > > > > > seeing
> > > > > > > > > +                    * the expected connector state try to wait
> > > > > > > > > for an HPD
> > > > > > > > > +                    * event for each connector/port.
> > > > > > > > > +                    */
> > > > > > > > > +                   current_state = reprobe_connector(data, port);
> > > > > > > > > +                   if (p > 0 && current_state != target_state) {
> > > > > > > > > +                           igt_assert(wait_for_hotplug(mon,
> > > > > > > > > &timeout));
> > > > > > > > > +                           current_state =
> > > > > > > > > reprobe_connector(data, port);
> > > > > > > > > +                   }
> > > > > > > > > +
> > > > > > > > > +                   igt_assert_eq(current_state, target_state);
> > > > > > > > >             }
> > > > > > > > >
> > > > > > > > >             port = NULL;
> > > > > > > > --
> > > > > > > > Paul Kocialkowski, Bootlin
> > > > > > > > Embedded Linux and kernel engineering
> > > > > > > > https://bootlin.com
> > > > > > > >
> > > --
> > > Paul Kocialkowski, Bootlin
> > > Embedded Linux and kernel engineering
> > > https://bootlin.com
> > >
> >
> >
> --
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [igt-dev] Improving DRM connector hotplug uevents

[Daniel Vetter]

On Fri, May 10, 2019 at 10:01 AM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Fri, 2019-05-10 at 09:56 +0200, Daniel Vetter wrote:
> > On Fri, May 10, 2019 at 9:42 AM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > Hi,
> > >
> > > So this thread has drifted off from IGT to a general DRM improvement,
> > > so renaming the thread and adding some CCs.
> > >
> > > It's about avoiding full reprobes (which may include slow EDID reads,
> > > etc) when a connector change was detected, by providing information
> > > about the connector and the new state to userspace. This way, userspace
> > > can reprobe the changed connector alone (or not reprobe at all in case
> > > of a disconnect).
> > >
> > > It feels like there is growing temptation to have per-driver hacks to
> > > avoid full reprobes when the driver knows that nothing changed, but I
> > > think it makes more sense to fixup the uevent we send to make sure that
> > > userspace knows what to probe exactly.
> > >
> > > On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> > > > On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > > > > I'm in support of this as well, we really could use better hotplug events.
> > > > > Feel free to cc patches to me for review :)
> > > >
> > > > Any idea(s) how the API would look like?
> > >
> > > From a quick chat on IRC yesterday, it seems that just adding entries
> > > to the uevent would do and would be backwards-compatible (well,
> > > provided that the userspace uevent parsers will not fail if something
> > > extra to "HOTPLUG=1" is provided).
> > >
> > > Currently, the notification is sent globally at each round of detected
> > > connector change (which may concern multiple connectors). So I think we
> > > should just list the connectors that changed in the uevent and their
> > > new state. So my proposal here is something like:
> > >
> > > HOTPLUG=1
> > > CONNECTOR_ID=47
> > > STATUS=Connected
> > > CONNECTOR_ID=48
> > > STATUS=Disconnected
> > >
> > > Where each STATUS entry would refer to the previous CONNECTOR_ID entry.
> > >
> > > What do you think?
> >
> > We have the better uevent already, all we need is wiring up, lots of
> > code, and userspace for it. Ok, not quite, patch is still in flight:
> >
> > https://patchwork.kernel.org/patch/10906677/
>
> Ah great to see that!
>
> > The real trouble isn't the new event, but making sure it goes out for
> > anything that might have changed. There's a lot more connector status
> > than just the STATUS property.
>
> Why though? I thought this was a hotplug interface, not a general
> "something-about-the-connector-changed". To me, hotplug is pretty
> binary: either it's connected, either it's not.
>
> Do we already send out hotplug uevents when something else than the
> status changed? If so, maybe we should consider handling the rest
> separately from hotplug.

We have plenty of bugs around "changed screen over s/r and no output".
Imo if we rework hotplug, we should get all the mistakes right.

> Looking at drm_probe_helper.c's output_poll_execute, my understanding
> is that drm_kms_helper_hotplug_event is only called when the status
> changed, not other props.

Yeah, and that's why userspace has to reprobe all the time. One idea
that we floated around is to have an epoche counter, which changes
every time anything with the connector changes wrt hotplug related
state (edid, sink id, whatever). The more specific hotplug uevent
would then give you an event on that epoch counter, so userspace knows
it needs to reprobe that specific connector for all the new state.
-Daniel


> Cheers,
>
> Paul
>
> > -Daniel
> >
> > > Cheers,
> > >
> > > Paul
> > >
> > > > > On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > > > > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > > > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > > > > > hotplug event for each connector that may be sent by the driver.
> > > > > > > > >
> > > > > > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > > > > > as
> > > > > > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > > > > > timing.
> > > > > > > > > So if we don't yet see the expected connector state on a given
> > > > > > > > > connector
> > > > > > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > > > > > state.
> > > > > > > >
> > > > > > > > This changes makes good sense to me, so:
> > > > > > > >
> > > > > > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > >
> > > > > > > > And it brings back hard feelings about the hotplug interface we have
> > > > > > > > today...
> > > > > > >
> > > > > > > Yes, I was surprised too not find any way to identify which connector(s)
> > > > > > > the hotplug event is associated with. We discussed with Ville if it'd
> > > > > > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > > > > > be useful outside of this test, or how that would align with Chris'
> > > > > > > connector probe state generation idea.
> > > > > >
> > > > > > Either way, I'm definitely in favor of having something more reliable
> > > > > > to expose to userspace. Having something to correlate each event to one
> > > > > > (or more) connector(s) sounds good to me.
> > > > > >
> > > > > > And I think there is a general need for this, it's not just IGT.
> > > > > >
> > > > > > Currently, every userspace application using DRM has to do a full
> > > > > > reprobe once a hotplug uevent is received, which is really not optimal.
> > > > > > If an entry identifying the connector was also provided, userspace
> > > > > > could only reprobe that connector. The step after that would be having
> > > > > > the indication of whether the connector was connected or disconnected
> > > > > > directly in uevent too, so that no probing needs to be done at all when
> > > > > > a given connector is disconnected.
> > > > > >
> > > > > > Anyway, if you're interested in the idea and that Ville and Chris are
> > > > > > in favor, I really think it would be worth fixing that. And we'd only
> > > > > > be adding new elements to uevent, so that would stay backward-
> > > > > > compatible too.
> > > > > >
> > > > > > Cheers,
> > > > > >
> > > > > > Paul
> > > > > >
> > > > > > > > > v2:
> > > > > > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > > > > > in
> > > > > > > > >   the debugging output. (Lyude)
> > > > > > > > >
> > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > > > > > ---
> > > > > > > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > > > > > -
> > > > > > > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > > > > > index 714e5e06..502f1efa 100644
> > > > > > > > > --- a/tests/kms_chamelium.c
> > > > > > > > > +++ b/tests/kms_chamelium.c
> > > > > > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > > > > > chamelium_port *port,
> > > > > > > > >     drmModeFreeConnector(connector);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > > > > > */
> > > > > > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > > > > > +{
> > > > > > > > > +   struct timespec start, end;
> > > > > > > > > +   int elapsed;
> > > > > > > > > +   bool detected;
> > > > > > > > > +
> > > > > > > > > +   igt_assert_eq(igt_gettime(&start), 0);
> > > > > > > > > +   detected = igt_hotplug_detected(mon, *timeout);
> > > > > > > > > +   igt_assert_eq(igt_gettime(&end), 0);
> > > > > > > > > +
> > > > > > > > > +   elapsed = igt_time_elapsed(&start, &end);
> > > > > > > > > +   igt_assert_lte(0, elapsed);
> > > > > > > > > +   *timeout = max(0, *timeout - elapsed);
> > > > > > > > > +
> > > > > > > > > +   return detected;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  static void
> > > > > > > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > > > > > >                    enum igt_suspend_state state, enum
> > > > > > > > > igt_suspend_test test,
> > > > > > > > >                    struct udev_monitor *mon, bool connected)
> > > > > > > > >  {
> > > > > > > > > +   drmModeConnection target_state = connected ?
> > > > > > > > > DRM_MODE_DISCONNECTED :
> > > > > > > > > +                                                DRM_MODE_CONNECTE
> > > > > > > > > D;
> > > > > > > > > +   int timeout = HOTPLUG_TIMEOUT;
> > > > > > > > >     int delay;
> > > > > > > > >     int p;
> > > > > > > > >
> > > > > > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > > > > > chamelium_port *port,
> > > > > > > > >     }
> > > > > > > > >
> > > > > > > > >     igt_system_suspend_autoresume(state, test);
> > > > > > > > > +   igt_assert(wait_for_hotplug(mon, &timeout));
> > > > > > > > >
> > > > > > > > > -   igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > > > > > >     if (port) {
> > > > > > > > > -           igt_assert_eq(reprobe_connector(data, port), connected
> > > > > > > > > ?
> > > > > > > > > -                         DRM_MODE_DISCONNECTED :
> > > > > > > > > DRM_MODE_CONNECTED);
> > > > > > > > > +           igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > > target_state);
> > > > > > > > >     } else {
> > > > > > > > >             for (p = 0; p < data->port_count; p++) {
> > > > > > > > > +                   drmModeConnection current_state;
> > > > > > > > > +
> > > > > > > > >                     port = data->ports[p];
> > > > > > > > > -                   igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > > connected ?
> > > > > > > > > -                                 DRM_MODE_DISCONNECTED :
> > > > > > > > > -                                 DRM_MODE_CONNECTED);
> > > > > > > > > +                   /*
> > > > > > > > > +                    * There could be as many hotplug events sent
> > > > > > > > > by
> > > > > > > > > +                    * driver as connectors we scheduled an HPD
> > > > > > > > > toggle on
> > > > > > > > > +                    * above, depending on timing. So if we're not
> > > > > > > > > seeing
> > > > > > > > > +                    * the expected connector state try to wait
> > > > > > > > > for an HPD
> > > > > > > > > +                    * event for each connector/port.
> > > > > > > > > +                    */
> > > > > > > > > +                   current_state = reprobe_connector(data, port);
> > > > > > > > > +                   if (p > 0 && current_state != target_state) {
> > > > > > > > > +                           igt_assert(wait_for_hotplug(mon,
> > > > > > > > > &timeout));
> > > > > > > > > +                           current_state =
> > > > > > > > > reprobe_connector(data, port);
> > > > > > > > > +                   }
> > > > > > > > > +
> > > > > > > > > +                   igt_assert_eq(current_state, target_state);
> > > > > > > > >             }
> > > > > > > > >
> > > > > > > > >             port = NULL;
> > > > > > > > --
> > > > > > > > Paul Kocialkowski, Bootlin
> > > > > > > > Embedded Linux and kernel engineering
> > > > > > > > https://bootlin.com
> > > > > > > >
> > > --
> > > Paul Kocialkowski, Bootlin
> > > Embedded Linux and kernel engineering
> > > https://bootlin.com
> > >
> >
> >
> --
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: Improving DRM connector hotplug uevents

[Paul Kocialkowski]

Hi,

Resending this with the right CC.

Also changed my mind in the meantime and my latest proposal is at:
https://lists.freedesktop.org/archives/dri-devel/2019-May/217442.html

On Fri, 2019-05-10 at 10:06 +0200, Daniel Vetter wrote:
> On Fri, May 10, 2019 at 10:01 AM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > On Fri, 2019-05-10 at 09:56 +0200, Daniel Vetter wrote:
> > > On Fri, May 10, 2019 at 9:42 AM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > > Hi,
> > > > 
> > > > So this thread has drifted off from IGT to a general DRM improvement,
> > > > so renaming the thread and adding some CCs.
> > > > 
> > > > It's about avoiding full reprobes (which may include slow EDID reads,
> > > > etc) when a connector change was detected, by providing information
> > > > about the connector and the new state to userspace. This way, userspace
> > > > can reprobe the changed connector alone (or not reprobe at all in case
> > > > of a disconnect).
> > > > 
> > > > It feels like there is growing temptation to have per-driver hacks to
> > > > avoid full reprobes when the driver knows that nothing changed, but I
> > > > think it makes more sense to fixup the uevent we send to make sure that
> > > > userspace knows what to probe exactly.
> > > > 
> > > > On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> > > > > On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > > > > > I'm in support of this as well, we really could use better hotplug events.
> > > > > > Feel free to cc patches to me for review :)
> > > > > 
> > > > > Any idea(s) how the API would look like?
> > > > 
> > > > From a quick chat on IRC yesterday, it seems that just adding entries
> > > > to the uevent would do and would be backwards-compatible (well,
> > > > provided that the userspace uevent parsers will not fail if something
> > > > extra to "HOTPLUG=1" is provided).
> > > > 
> > > > Currently, the notification is sent globally at each round of detected
> > > > connector change (which may concern multiple connectors). So I think we
> > > > should just list the connectors that changed in the uevent and their
> > > > new state. So my proposal here is something like:
> > > > 
> > > > HOTPLUG=1
> > > > CONNECTOR_ID=47
> > > > STATUS=Connected
> > > > CONNECTOR_ID=48
> > > > STATUS=Disconnected
> > > > 
> > > > Where each STATUS entry would refer to the previous CONNECTOR_ID entry.
> > > > 
> > > > What do you think?
> > > 
> > > We have the better uevent already, all we need is wiring up, lots of
> > > code, and userspace for it. Ok, not quite, patch is still in flight:
> > > 
> > > https://patchwork.kernel.org/patch/10906677/
> > 
> > Ah great to see that!
> > 
> > > The real trouble isn't the new event, but making sure it goes out for
> > > anything that might have changed. There's a lot more connector status
> > > than just the STATUS property.
> > 
> > Why though? I thought this was a hotplug interface, not a general
> > "something-about-the-connector-changed". To me, hotplug is pretty
> > binary: either it's connected, either it's not.
> > 
> > Do we already send out hotplug uevents when something else than the
> > status changed? If so, maybe we should consider handling the rest
> > separately from hotplug.
> 
> We have plenty of bugs around "changed screen over s/r and no output".
> Imo if we rework hotplug, we should get all the mistakes right.

I think suspend/resume is one specific case where hotplug just can't be
detected by the driver. I think one solution to that would be to always
issue a hotplug even for all connectors on resume. I don't really see
how we could do better than that anyway.

> > Looking at drm_probe_helper.c's output_poll_execute, my understanding
> > is that drm_kms_helper_hotplug_event is only called when the status
> > changed, not other props.
> 
> Yeah, and that's why userspace has to reprobe all the time.

I'm a bit confused then, how can userspace be notified that something
else than the connected status changed if that's the only case where a
uevent is issued?

If something else changed that needs userspace attention, maybe we
could send out a uevent with the CONNECTOR_ID but without the STATUS
part so that userspace has to reprobe that connector to know what's
what.

>  One idea
> that we floated around is to have an epoche counter, which changes
> every time anything with the connector changes wrt hotplug related
> state (edid, sink id, whatever). The more specific hotplug uevent
> would then give you an event on that epoch counter, so userspace knows
> it needs to reprobe that specific connector for all the new state.

That would work, but I find that a bit overkill for the issue at hand.
At least I'm not convinced that something simpler would be a bad fit.

The idea of adding STATUS to the uevent is just to provide a fast-path
to avoid reprobe for the basic unplug case. If it's a plug or something
more complex that has happened, then I think userspace should do a
reprobe of the connector anyway. If we properly notify userspace only
when something changed, I don't think we need an epoch counter in
particular.

Cheers,

Paul

> -Daniel
> 
> 
> > Cheers,
> > 
> > Paul
> > 
> > > -Daniel
> > > 
> > > > Cheers,
> > > > 
> > > > Paul
> > > > 
> > > > > > On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > > > > > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > > > > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > > > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > > > > > > hotplug event for each connector that may be sent by the driver.
> > > > > > > > > > 
> > > > > > > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > > > > > > as
> > > > > > > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > > > > > > timing.
> > > > > > > > > > So if we don't yet see the expected connector state on a given
> > > > > > > > > > connector
> > > > > > > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > > > > > > state.
> > > > > > > > > 
> > > > > > > > > This changes makes good sense to me, so:
> > > > > > > > > 
> > > > > > > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > > > 
> > > > > > > > > And it brings back hard feelings about the hotplug interface we have
> > > > > > > > > today...
> > > > > > > > 
> > > > > > > > Yes, I was surprised too not find any way to identify which connector(s)
> > > > > > > > the hotplug event is associated with. We discussed with Ville if it'd
> > > > > > > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > > > > > > be useful outside of this test, or how that would align with Chris'
> > > > > > > > connector probe state generation idea.
> > > > > > > 
> > > > > > > Either way, I'm definitely in favor of having something more reliable
> > > > > > > to expose to userspace. Having something to correlate each event to one
> > > > > > > (or more) connector(s) sounds good to me.
> > > > > > > 
> > > > > > > And I think there is a general need for this, it's not just IGT.
> > > > > > > 
> > > > > > > Currently, every userspace application using DRM has to do a full
> > > > > > > reprobe once a hotplug uevent is received, which is really not optimal.
> > > > > > > If an entry identifying the connector was also provided, userspace
> > > > > > > could only reprobe that connector. The step after that would be having
> > > > > > > the indication of whether the connector was connected or disconnected
> > > > > > > directly in uevent too, so that no probing needs to be done at all when
> > > > > > > a given connector is disconnected.
> > > > > > > 
> > > > > > > Anyway, if you're interested in the idea and that Ville and Chris are
> > > > > > > in favor, I really think it would be worth fixing that. And we'd only
> > > > > > > be adding new elements to uevent, so that would stay backward-
> > > > > > > compatible too.
> > > > > > > 
> > > > > > > Cheers,
> > > > > > > 
> > > > > > > Paul
> > > > > > > 
> > > > > > > > > > v2:
> > > > > > > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > > > > > > in
> > > > > > > > > >   the debugging output. (Lyude)
> > > > > > > > > > 
> > > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > > > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > > > > > > ---
> > > > > > > > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > > > > > > -
> > > > > > > > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > > > > > > index 714e5e06..502f1efa 100644
> > > > > > > > > > --- a/tests/kms_chamelium.c
> > > > > > > > > > +++ b/tests/kms_chamelium.c
> > > > > > > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > > > > > > chamelium_port *port,
> > > > > > > > > >     drmModeFreeConnector(connector);
> > > > > > > > > >  }
> > > > > > > > > > 
> > > > > > > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > > > > > > */
> > > > > > > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > > > > > > +{
> > > > > > > > > > +   struct timespec start, end;
> > > > > > > > > > +   int elapsed;
> > > > > > > > > > +   bool detected;
> > > > > > > > > > +
> > > > > > > > > > +   igt_assert_eq(igt_gettime(&start), 0);
> > > > > > > > > > +   detected = igt_hotplug_detected(mon, *timeout);
> > > > > > > > > > +   igt_assert_eq(igt_gettime(&end), 0);
> > > > > > > > > > +
> > > > > > > > > > +   elapsed = igt_time_elapsed(&start, &end);
> > > > > > > > > > +   igt_assert_lte(0, elapsed);
> > > > > > > > > > +   *timeout = max(0, *timeout - elapsed);
> > > > > > > > > > +
> > > > > > > > > > +   return detected;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >  static void
> > > > > > > > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > > > > > > >                    enum igt_suspend_state state, enum
> > > > > > > > > > igt_suspend_test test,
> > > > > > > > > >                    struct udev_monitor *mon, bool connected)
> > > > > > > > > >  {
> > > > > > > > > > +   drmModeConnection target_state = connected ?
> > > > > > > > > > DRM_MODE_DISCONNECTED :
> > > > > > > > > > +                                                DRM_MODE_CONNECTE
> > > > > > > > > > D;
> > > > > > > > > > +   int timeout = HOTPLUG_TIMEOUT;
> > > > > > > > > >     int delay;
> > > > > > > > > >     int p;
> > > > > > > > > > 
> > > > > > > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > > > > > > chamelium_port *port,
> > > > > > > > > >     }
> > > > > > > > > > 
> > > > > > > > > >     igt_system_suspend_autoresume(state, test);
> > > > > > > > > > +   igt_assert(wait_for_hotplug(mon, &timeout));
> > > > > > > > > > 
> > > > > > > > > > -   igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > > > > > > >     if (port) {
> > > > > > > > > > -           igt_assert_eq(reprobe_connector(data, port), connected
> > > > > > > > > > ?
> > > > > > > > > > -                         DRM_MODE_DISCONNECTED :
> > > > > > > > > > DRM_MODE_CONNECTED);
> > > > > > > > > > +           igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > > > target_state);
> > > > > > > > > >     } else {
> > > > > > > > > >             for (p = 0; p < data->port_count; p++) {
> > > > > > > > > > +                   drmModeConnection current_state;
> > > > > > > > > > +
> > > > > > > > > >                     port = data->ports[p];
> > > > > > > > > > -                   igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > > > connected ?
> > > > > > > > > > -                                 DRM_MODE_DISCONNECTED :
> > > > > > > > > > -                                 DRM_MODE_CONNECTED);
> > > > > > > > > > +                   /*
> > > > > > > > > > +                    * There could be as many hotplug events sent
> > > > > > > > > > by
> > > > > > > > > > +                    * driver as connectors we scheduled an HPD
> > > > > > > > > > toggle on
> > > > > > > > > > +                    * above, depending on timing. So if we're not
> > > > > > > > > > seeing
> > > > > > > > > > +                    * the expected connector state try to wait
> > > > > > > > > > for an HPD
> > > > > > > > > > +                    * event for each connector/port.
> > > > > > > > > > +                    */
> > > > > > > > > > +                   current_state = reprobe_connector(data, port);
> > > > > > > > > > +                   if (p > 0 && current_state != target_state) {
> > > > > > > > > > +                           igt_assert(wait_for_hotplug(mon,
> > > > > > > > > > &timeout));
> > > > > > > > > > +                           current_state =
> > > > > > > > > > reprobe_connector(data, port);
> > > > > > > > > > +                   }
> > > > > > > > > > +
> > > > > > > > > > +                   igt_assert_eq(current_state, target_state);
> > > > > > > > > >             }
> > > > > > > > > > 
> > > > > > > > > >             port = NULL;
> > > > > > > > > --
> > > > > > > > > Paul Kocialkowski, Bootlin
> > > > > > > > > Embedded Linux and kernel engineering
> > > > > > > > > https://bootlin.com
> > > > > > > > > 
> > > > --
> > > > Paul Kocialkowski, Bootlin
> > > > Embedded Linux and kernel engineering
> > > > https://bootlin.com
> > > > 
> > --
> > Paul Kocialkowski, Bootlin
> > Embedded Linux and kernel engineering
> > https://bootlin.com
> > 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [igt-dev] Improving DRM connector hotplug uevents

[Paul Kocialkowski]

Hi,

Resending this with the right CC.

Also changed my mind in the meantime and my latest proposal is at:
https://lists.freedesktop.org/archives/dri-devel/2019-May/217442.html

On Fri, 2019-05-10 at 10:06 +0200, Daniel Vetter wrote:
> On Fri, May 10, 2019 at 10:01 AM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > On Fri, 2019-05-10 at 09:56 +0200, Daniel Vetter wrote:
> > > On Fri, May 10, 2019 at 9:42 AM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > > Hi,
> > > > 
> > > > So this thread has drifted off from IGT to a general DRM improvement,
> > > > so renaming the thread and adding some CCs.
> > > > 
> > > > It's about avoiding full reprobes (which may include slow EDID reads,
> > > > etc) when a connector change was detected, by providing information
> > > > about the connector and the new state to userspace. This way, userspace
> > > > can reprobe the changed connector alone (or not reprobe at all in case
> > > > of a disconnect).
> > > > 
> > > > It feels like there is growing temptation to have per-driver hacks to
> > > > avoid full reprobes when the driver knows that nothing changed, but I
> > > > think it makes more sense to fixup the uevent we send to make sure that
> > > > userspace knows what to probe exactly.
> > > > 
> > > > On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> > > > > On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > > > > > I'm in support of this as well, we really could use better hotplug events.
> > > > > > Feel free to cc patches to me for review :)
> > > > > 
> > > > > Any idea(s) how the API would look like?
> > > > 
> > > > From a quick chat on IRC yesterday, it seems that just adding entries
> > > > to the uevent would do and would be backwards-compatible (well,
> > > > provided that the userspace uevent parsers will not fail if something
> > > > extra to "HOTPLUG=1" is provided).
> > > > 
> > > > Currently, the notification is sent globally at each round of detected
> > > > connector change (which may concern multiple connectors). So I think we
> > > > should just list the connectors that changed in the uevent and their
> > > > new state. So my proposal here is something like:
> > > > 
> > > > HOTPLUG=1
> > > > CONNECTOR_ID=47
> > > > STATUS=Connected
> > > > CONNECTOR_ID=48
> > > > STATUS=Disconnected
> > > > 
> > > > Where each STATUS entry would refer to the previous CONNECTOR_ID entry.
> > > > 
> > > > What do you think?
> > > 
> > > We have the better uevent already, all we need is wiring up, lots of
> > > code, and userspace for it. Ok, not quite, patch is still in flight:
> > > 
> > > https://patchwork.kernel.org/patch/10906677/
> > 
> > Ah great to see that!
> > 
> > > The real trouble isn't the new event, but making sure it goes out for
> > > anything that might have changed. There's a lot more connector status
> > > than just the STATUS property.
> > 
> > Why though? I thought this was a hotplug interface, not a general
> > "something-about-the-connector-changed". To me, hotplug is pretty
> > binary: either it's connected, either it's not.
> > 
> > Do we already send out hotplug uevents when something else than the
> > status changed? If so, maybe we should consider handling the rest
> > separately from hotplug.
> 
> We have plenty of bugs around "changed screen over s/r and no output".
> Imo if we rework hotplug, we should get all the mistakes right.

I think suspend/resume is one specific case where hotplug just can't be
detected by the driver. I think one solution to that would be to always
issue a hotplug even for all connectors on resume. I don't really see
how we could do better than that anyway.

> > Looking at drm_probe_helper.c's output_poll_execute, my understanding
> > is that drm_kms_helper_hotplug_event is only called when the status
> > changed, not other props.
> 
> Yeah, and that's why userspace has to reprobe all the time.

I'm a bit confused then, how can userspace be notified that something
else than the connected status changed if that's the only case where a
uevent is issued?

If something else changed that needs userspace attention, maybe we
could send out a uevent with the CONNECTOR_ID but without the STATUS
part so that userspace has to reprobe that connector to know what's
what.

>  One idea
> that we floated around is to have an epoche counter, which changes
> every time anything with the connector changes wrt hotplug related
> state (edid, sink id, whatever). The more specific hotplug uevent
> would then give you an event on that epoch counter, so userspace knows
> it needs to reprobe that specific connector for all the new state.

That would work, but I find that a bit overkill for the issue at hand.
At least I'm not convinced that something simpler would be a bad fit.

The idea of adding STATUS to the uevent is just to provide a fast-path
to avoid reprobe for the basic unplug case. If it's a plug or something
more complex that has happened, then I think userspace should do a
reprobe of the connector anyway. If we properly notify userspace only
when something changed, I don't think we need an epoch counter in
particular.

Cheers,

Paul

> -Daniel
> 
> 
> > Cheers,
> > 
> > Paul
> > 
> > > -Daniel
> > > 
> > > > Cheers,
> > > > 
> > > > Paul
> > > > 
> > > > > > On Thu, 2019-05-09 at 14:24 +0200, Paul Kocialkowski wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Thu, 2019-05-09 at 15:08 +0300, Imre Deak wrote:
> > > > > > > > On Thu, May 09, 2019 at 09:25:41AM +0200, Paul Kocialkowski wrote:
> > > > > > > > > On Wed, 2019-05-08 at 11:24 +0300, Imre Deak wrote:
> > > > > > > > > > After scheduling an HPD toggle event, make sure that we wait for the
> > > > > > > > > > hotplug event for each connector that may be sent by the driver.
> > > > > > > > > > 
> > > > > > > > > > Depending on the scheduling there could be 1 event or as many events
> > > > > > > > > > as
> > > > > > > > > > connectors we scheduled an HPD toggle event on, depending on the
> > > > > > > > > > timing.
> > > > > > > > > > So if we don't yet see the expected connector state on a given
> > > > > > > > > > connector
> > > > > > > > > > try to wait for an additional hotplug event and reprobe/recheck the
> > > > > > > > > > state.
> > > > > > > > > 
> > > > > > > > > This changes makes good sense to me, so:
> > > > > > > > > 
> > > > > > > > > Reviewed-By: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > > > 
> > > > > > > > > And it brings back hard feelings about the hotplug interface we have
> > > > > > > > > today...
> > > > > > > > 
> > > > > > > > Yes, I was surprised too not find any way to identify which connector(s)
> > > > > > > > the hotplug event is associated with. We discussed with Ville if it'd
> > > > > > > > make sense to add a connector ID map to the uevent, but not sure if it'd
> > > > > > > > be useful outside of this test, or how that would align with Chris'
> > > > > > > > connector probe state generation idea.
> > > > > > > 
> > > > > > > Either way, I'm definitely in favor of having something more reliable
> > > > > > > to expose to userspace. Having something to correlate each event to one
> > > > > > > (or more) connector(s) sounds good to me.
> > > > > > > 
> > > > > > > And I think there is a general need for this, it's not just IGT.
> > > > > > > 
> > > > > > > Currently, every userspace application using DRM has to do a full
> > > > > > > reprobe once a hotplug uevent is received, which is really not optimal.
> > > > > > > If an entry identifying the connector was also provided, userspace
> > > > > > > could only reprobe that connector. The step after that would be having
> > > > > > > the indication of whether the connector was connected or disconnected
> > > > > > > directly in uevent too, so that no probing needs to be done at all when
> > > > > > > a given connector is disconnected.
> > > > > > > 
> > > > > > > Anyway, if you're interested in the idea and that Ville and Chris are
> > > > > > > in favor, I really think it would be worth fixing that. And we'd only
> > > > > > > be adding new elements to uevent, so that would stay backward-
> > > > > > > compatible too.
> > > > > > > 
> > > > > > > Cheers,
> > > > > > > 
> > > > > > > Paul
> > > > > > > 
> > > > > > > > > > v2:
> > > > > > > > > > - s/igt_assert(x >= y)/igt_assert_lte(y, x)/ to see the actual limits
> > > > > > > > > > in
> > > > > > > > > >   the debugging output. (Lyude)
> > > > > > > > > > 
> > > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110534
> > > > > > > > > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > > > > > Cc: Lyude Paul <lyude@redhat.com>
> > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > > Reviewed-by: Lyude Paul <lyude@redhat.com>
> > > > > > > > > > ---
> > > > > > > > > >  tests/kms_chamelium.c | 45 +++++++++++++++++++++++++++++++++++++-----
> > > > > > > > > > -
> > > > > > > > > >  1 file changed, 39 insertions(+), 6 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > > > > > > > > index 714e5e06..502f1efa 100644
> > > > > > > > > > --- a/tests/kms_chamelium.c
> > > > > > > > > > +++ b/tests/kms_chamelium.c
> > > > > > > > > > @@ -284,11 +284,32 @@ test_edid_read(data_t *data, struct
> > > > > > > > > > chamelium_port *port,
> > > > > > > > > >     drmModeFreeConnector(connector);
> > > > > > > > > >  }
> > > > > > > > > > 
> > > > > > > > > > +/* Wait for hotplug and return the remaining time left from timeout
> > > > > > > > > > */
> > > > > > > > > > +static bool wait_for_hotplug(struct udev_monitor *mon, int *timeout)
> > > > > > > > > > +{
> > > > > > > > > > +   struct timespec start, end;
> > > > > > > > > > +   int elapsed;
> > > > > > > > > > +   bool detected;
> > > > > > > > > > +
> > > > > > > > > > +   igt_assert_eq(igt_gettime(&start), 0);
> > > > > > > > > > +   detected = igt_hotplug_detected(mon, *timeout);
> > > > > > > > > > +   igt_assert_eq(igt_gettime(&end), 0);
> > > > > > > > > > +
> > > > > > > > > > +   elapsed = igt_time_elapsed(&start, &end);
> > > > > > > > > > +   igt_assert_lte(0, elapsed);
> > > > > > > > > > +   *timeout = max(0, *timeout - elapsed);
> > > > > > > > > > +
> > > > > > > > > > +   return detected;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >  static void
> > > > > > > > > >  try_suspend_resume_hpd(data_t *data, struct chamelium_port *port,
> > > > > > > > > >                    enum igt_suspend_state state, enum
> > > > > > > > > > igt_suspend_test test,
> > > > > > > > > >                    struct udev_monitor *mon, bool connected)
> > > > > > > > > >  {
> > > > > > > > > > +   drmModeConnection target_state = connected ?
> > > > > > > > > > DRM_MODE_DISCONNECTED :
> > > > > > > > > > +                                                DRM_MODE_CONNECTE
> > > > > > > > > > D;
> > > > > > > > > > +   int timeout = HOTPLUG_TIMEOUT;
> > > > > > > > > >     int delay;
> > > > > > > > > >     int p;
> > > > > > > > > > 
> > > > > > > > > > @@ -310,17 +331,29 @@ try_suspend_resume_hpd(data_t *data, struct
> > > > > > > > > > chamelium_port *port,
> > > > > > > > > >     }
> > > > > > > > > > 
> > > > > > > > > >     igt_system_suspend_autoresume(state, test);
> > > > > > > > > > +   igt_assert(wait_for_hotplug(mon, &timeout));
> > > > > > > > > > 
> > > > > > > > > > -   igt_assert(igt_hotplug_detected(mon, HOTPLUG_TIMEOUT));
> > > > > > > > > >     if (port) {
> > > > > > > > > > -           igt_assert_eq(reprobe_connector(data, port), connected
> > > > > > > > > > ?
> > > > > > > > > > -                         DRM_MODE_DISCONNECTED :
> > > > > > > > > > DRM_MODE_CONNECTED);
> > > > > > > > > > +           igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > > > target_state);
> > > > > > > > > >     } else {
> > > > > > > > > >             for (p = 0; p < data->port_count; p++) {
> > > > > > > > > > +                   drmModeConnection current_state;
> > > > > > > > > > +
> > > > > > > > > >                     port = data->ports[p];
> > > > > > > > > > -                   igt_assert_eq(reprobe_connector(data, port),
> > > > > > > > > > connected ?
> > > > > > > > > > -                                 DRM_MODE_DISCONNECTED :
> > > > > > > > > > -                                 DRM_MODE_CONNECTED);
> > > > > > > > > > +                   /*
> > > > > > > > > > +                    * There could be as many hotplug events sent
> > > > > > > > > > by
> > > > > > > > > > +                    * driver as connectors we scheduled an HPD
> > > > > > > > > > toggle on
> > > > > > > > > > +                    * above, depending on timing. So if we're not
> > > > > > > > > > seeing
> > > > > > > > > > +                    * the expected connector state try to wait
> > > > > > > > > > for an HPD
> > > > > > > > > > +                    * event for each connector/port.
> > > > > > > > > > +                    */
> > > > > > > > > > +                   current_state = reprobe_connector(data, port);
> > > > > > > > > > +                   if (p > 0 && current_state != target_state) {
> > > > > > > > > > +                           igt_assert(wait_for_hotplug(mon,
> > > > > > > > > > &timeout));
> > > > > > > > > > +                           current_state =
> > > > > > > > > > reprobe_connector(data, port);
> > > > > > > > > > +                   }
> > > > > > > > > > +
> > > > > > > > > > +                   igt_assert_eq(current_state, target_state);
> > > > > > > > > >             }
> > > > > > > > > > 
> > > > > > > > > >             port = NULL;
> > > > > > > > > --
> > > > > > > > > Paul Kocialkowski, Bootlin
> > > > > > > > > Embedded Linux and kernel engineering
> > > > > > > > > https://bootlin.com
> > > > > > > > > 
> > > > --
> > > > Paul Kocialkowski, Bootlin
> > > > Embedded Linux and kernel engineering
> > > > https://bootlin.com
> > > > 
> > --
> > Paul Kocialkowski, Bootlin
> > Embedded Linux and kernel engineering
> > https://bootlin.com
> > 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

Re: Improving DRM connector hotplug uevents

[Ser, Simon]

On Fri, 2019-05-10 at 09:56 +0200, Daniel Vetter wrote:
> On Fri, May 10, 2019 at 9:42 AM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > So this thread has drifted off from IGT to a general DRM improvement,
> > so renaming the thread and adding some CCs.
> > 
> > It's about avoiding full reprobes (which may include slow EDID reads,
> > etc) when a connector change was detected, by providing information
> > about the connector and the new state to userspace. This way, userspace
> > can reprobe the changed connector alone (or not reprobe at all in case
> > of a disconnect).
> > 
> > It feels like there is growing temptation to have per-driver hacks to
> > avoid full reprobes when the driver knows that nothing changed, but I
> > think it makes more sense to fixup the uevent we send to make sure that
> > userspace knows what to probe exactly.
> > 
> > On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> > > On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > > > I'm in support of this as well, we really could use better hotplug events.
> > > > Feel free to cc patches to me for review :)
> > > 
> > > Any idea(s) how the API would look like?
> > 
> > From a quick chat on IRC yesterday, it seems that just adding entries
> > to the uevent would do and would be backwards-compatible (well,
> > provided that the userspace uevent parsers will not fail if something
> > extra to "HOTPLUG=1" is provided).
> > 
> > Currently, the notification is sent globally at each round of detected
> > connector change (which may concern multiple connectors). So I think we
> > should just list the connectors that changed in the uevent and their
> > new state. So my proposal here is something like:
> > 
> > HOTPLUG=1
> > CONNECTOR_ID=47
> > STATUS=Connected
> > CONNECTOR_ID=48
> > STATUS=Disconnected
> > 
> > Where each STATUS entry would refer to the previous CONNECTOR_ID entry.
> > 
> > What do you think?
> 
> We have the better uevent already, all we need is wiring up, lots of
> code, and userspace for it. Ok, not quite, patch is still in flight:
> 
> https://patchwork.kernel.org/patch/10906677/

Excellent.

I can help with a userspace implementation if needed.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [igt-dev] Improving DRM connector hotplug uevents

[Ser, Simon]

On Fri, 2019-05-10 at 09:56 +0200, Daniel Vetter wrote:
> On Fri, May 10, 2019 at 9:42 AM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > Hi,
> > 
> > So this thread has drifted off from IGT to a general DRM improvement,
> > so renaming the thread and adding some CCs.
> > 
> > It's about avoiding full reprobes (which may include slow EDID reads,
> > etc) when a connector change was detected, by providing information
> > about the connector and the new state to userspace. This way, userspace
> > can reprobe the changed connector alone (or not reprobe at all in case
> > of a disconnect).
> > 
> > It feels like there is growing temptation to have per-driver hacks to
> > avoid full reprobes when the driver knows that nothing changed, but I
> > think it makes more sense to fixup the uevent we send to make sure that
> > userspace knows what to probe exactly.
> > 
> > On Fri, 2019-05-10 at 06:55 +0000, Ser, Simon wrote:
> > > On Thu, 2019-05-09 at 11:56 -0400, Lyude Paul wrote:
> > > > I'm in support of this as well, we really could use better hotplug events.
> > > > Feel free to cc patches to me for review :)
> > > 
> > > Any idea(s) how the API would look like?
> > 
> > From a quick chat on IRC yesterday, it seems that just adding entries
> > to the uevent would do and would be backwards-compatible (well,
> > provided that the userspace uevent parsers will not fail if something
> > extra to "HOTPLUG=1" is provided).
> > 
> > Currently, the notification is sent globally at each round of detected
> > connector change (which may concern multiple connectors). So I think we
> > should just list the connectors that changed in the uevent and their
> > new state. So my proposal here is something like:
> > 
> > HOTPLUG=1
> > CONNECTOR_ID=47
> > STATUS=Connected
> > CONNECTOR_ID=48
> > STATUS=Disconnected
> > 
> > Where each STATUS entry would refer to the previous CONNECTOR_ID entry.
> > 
> > What do you think?
> 
> We have the better uevent already, all we need is wiring up, lots of
> code, and userspace for it. Ok, not quite, patch is still in flight:
> 
> https://patchwork.kernel.org/patch/10906677/

Excellent.

I can help with a userspace implementation if needed.
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev