[igt-dev] [PATCH i-g-t 1/2] lib/igt_chamelium: Sleep when doing autodiscovery

[igt-dev] [PATCH i-g-t 1/2] lib/igt_chamelium: Sleep when doing autodiscovery

[Arkadiusz Hiler]

Autodiscovery was wrongly assuming that whenever we do chamelium_plug()
the connector state change is immediate.

It was working most of the time for native connectors, but may explain
some of the flip-flopping skips.

The problem got only more serious with the advent of LSPcons as USB-C,
where we have much things happening on the signal path, introducing
delays.

So for the sake of reliability this change introduces sleep(10) between
plug and reprobe. The number is about 2 * the_most_pathological_case_observed.

Also the discovery step is performed only if there is no static port
mapping set up. After the discovery is done, IGT prints the mapping
ready to be pasted into .igtrc.

Cc: Kunal Joshi <kunal1.joshi@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 lib/igt_chamelium.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index c704b84f..28be5248 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -82,6 +82,14 @@
  *
  */
 
+/*
+ * We cannot expect chamelium_plug() to take effect imediately.
+ *
+ * Especially with modern, more complex hardware where we may have LSPcons and
+ * USB controllers in the way.
+ */
+#define CHAMELIUM_HOTPLUG_DETECTION_DELAY 10
+
 struct chamelium_edid {
 	struct chamelium *chamelium;
 	struct edid *base;
@@ -2231,6 +2239,10 @@ static bool chamelium_autodiscover(struct chamelium *chamelium, int drm_fd)
 		chamelium_plug(chamelium, port);
 	}
 
+	igt_info("Sleeping %d seconds for the hotplug to take effect.\n",
+		 CHAMELIUM_HOTPLUG_DETECTION_DELAY);
+	sleep(CHAMELIUM_HOTPLUG_DETECTION_DELAY);
+
 	/* Reprobe connectors and build the mapping */
 	res = drmModeGetResources(drm_fd);
 	if (!res)
@@ -2448,8 +2460,20 @@ struct chamelium *chamelium_init(int drm_fd)
 	if (!chamelium_read_port_mappings(chamelium, drm_fd))
 		goto error;
 
-	if (!chamelium_autodiscover(chamelium, drm_fd))
-		goto error;
+	if (chamelium->port_count == 0) {
+		igt_info("Chamelium configured without port mapping, "
+			 "performing autodiscovery\n");
+
+		if (!chamelium_autodiscover(chamelium, drm_fd))
+			goto error;
+
+		if (chamelium->port_count != 0)
+			igt_info("\nConsider adding the following to your .igtrc:\n");
+		for (int i = 0; i < chamelium->port_count; ++i) {
+			igt_info("[Chamelium:%s]\n", chamelium->ports[i].name);
+			igt_info("ChameliumPortID=%d\n\n", chamelium->ports[i].id);
+		}
+	}
 
 	cleanup_instance = chamelium;
 	igt_install_exit_handler(chamelium_exit_handler);
-- 
2.25.2

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

[igt-dev] [PATCH i-g-t 2/2] lib/igt_kms: Make igt_display_require() + chamelium more robust

[Arkadiusz Hiler]

1. We don't reset Chamelium, as this generates extra unplug events if
   any of the ports is already connected which is often the case

2. We try to plug all the chamelium ports, it's a noop if they are
   already plugged

2. We wait for all the ports being connected:
   - if the port mapping is provided: wait for the ports to be connected
   - if there is no mapping: sleep(10) and hope for the best

Cc: Petri Latvala <petri.latvala@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 lib/igt_chamelium.c | 87 +++++++++++++++++++++++++++++++++++++++------
 lib/igt_chamelium.h |  2 ++
 lib/igt_kms.c       |  2 ++
 3 files changed, 80 insertions(+), 11 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index 28be5248..69ad8378 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -2532,17 +2532,6 @@ bool chamelium_plug_all(struct chamelium *chamelium)
 	size_t port_count;
 	int port_ids[CHAMELIUM_MAX_PORTS];
 	xmlrpc_value *v;
-	v = __chamelium_rpc(chamelium, NULL, "Reset", "()");
-
-	if (v != NULL)
-		xmlrpc_DECREF(v);
-
-	if (chamelium->env.fault_occurred) {
-		igt_debug("Chamelium RPC call failed: %s\n",
-		     chamelium->env.fault_string);
-
-		return false;
-	}
 
 	port_count = chamelium_get_video_ports(chamelium, port_ids);
 	if (port_count <= 0)
@@ -2565,6 +2554,82 @@ bool chamelium_plug_all(struct chamelium *chamelium)
 	return true;
 }
 
+bool chamelium_wait_all_configured_ports_connected(struct chamelium *chamelium, int drm_fd)
+{
+	drmModeRes *res;
+	drmModeConnector *connector;
+	char **group_list;
+	char *group;
+	bool ret = true;
+
+	int connectors[CHAMELIUM_MAX_PORTS];
+	int connectors_count = 0;
+
+	res = drmModeGetResources(drm_fd);
+
+	group_list = g_key_file_get_groups(igt_key_file, NULL);
+
+	for (int i = 0; group_list[i] != NULL; i++) {
+		char *map_name;
+		group = group_list[i];
+
+		if (!strstr(group, "Chamelium:"))
+			continue;
+
+		igt_assert(chamelium->port_count <= CHAMELIUM_MAX_PORTS);
+
+		map_name = group + (sizeof("Chamelium:") - 1);
+
+		for (int j = 0;
+		     j < res->count_connectors;
+		     j++) {
+			char name[50];
+
+			connector = drmModeGetConnectorCurrent(
+			    drm_fd, res->connectors[j]);
+
+			/* We have to generate the connector name on our own */
+			snprintf(name, 50, "%s-%u",
+				 kmstest_connector_type_str(connector->connector_type),
+				 connector->connector_type_id);
+
+
+			if (strcmp(name, map_name) == 0) {
+				igt_assert(connectors_count < CHAMELIUM_MAX_PORTS);
+				connectors[connectors_count++] = connector->connector_id;
+				break;
+			}
+
+			drmModeFreeConnector(connector);
+		}
+	}
+
+	drmModeFreeResources(res);
+
+	if (connectors_count == 0) {
+		igt_info("No chamelium port mappping, sleeping for %d seconds "
+			 "for the hotplug to take effect",
+			 CHAMELIUM_HOTPLUG_DETECTION_DELAY);
+		sleep(CHAMELIUM_HOTPLUG_DETECTION_DELAY);
+		return true;
+	}
+
+	igt_until_timeout(CHAMELIUM_HOTPLUG_DETECTION_DELAY) {
+		ret = true;
+		for (int i = 0; i < connectors_count; ++i) {
+			connector = drmModeGetConnector(drm_fd, connectors[i]);
+			if (connector->connection != DRM_MODE_CONNECTED)
+				ret = false;
+			drmModeFreeConnector(connector);
+		}
+
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
 igt_constructor {
 	/* Frame dumps can be large, so we need to be able to handle very large
 	 * responses
diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
index c29741b4..359f4ab3 100644
--- a/lib/igt_chamelium.h
+++ b/lib/igt_chamelium.h
@@ -215,5 +215,7 @@ void chamelium_destroy_frame_dump(struct chamelium_frame_dump *dump);
 void chamelium_destroy_audio_file(struct chamelium_audio_file *audio_file);
 void chamelium_infoframe_destroy(struct chamelium_infoframe *infoframe);
 bool chamelium_plug_all(struct chamelium *chamelium);
+bool chamelium_wait_all_configured_ports_connected(struct chamelium *chamelium,
+						   int drm_fd);
 
 #endif /* IGT_CHAMELIUM_H */
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index e9621e7e..d4cbc1c5 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1899,6 +1899,8 @@ void igt_display_require(igt_display_t *display, int drm_fd)
 				       "cannot reach the configured chamelium!\n");
 			igt_abort_on_f(!chamelium_plug_all(chamelium),
 				       "failed to plug all the chamelium ports!\n");
+			igt_abort_on_f(!chamelium_wait_all_configured_ports_connected(chamelium, drm_fd),
+				       "not all configured chamelium ports are connected!\n");
 			chamelium_deinit_rpc_only(chamelium);
 		}
 	}
-- 
2.25.2

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

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] lib/igt_chamelium: Sleep when doing autodiscovery

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/2] lib/igt_chamelium: Sleep when doing autodiscovery
URL   : https://patchwork.freedesktop.org/series/77002/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8434 -> IGTPW_4540
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/index.html


Changes
-------

  No changes found


Participating hosts (50 -> 43)
------------------------------

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


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

  * CI: CI-20190529 -> None
  * IGT: IGT_5635 -> IGTPW_4540

  CI-20190529: 20190529
  CI_DRM_8434: 2951bac393beb4f095468de8b7cc53c8e3a092c2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_4540: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/index.html
  IGT_5635: e83abfca61d407d12eee4d25bb0e8686337a7791 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

[igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/2] lib/igt_chamelium: Sleep when doing autodiscovery

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/2] lib/igt_chamelium: Sleep when doing autodiscovery
URL   : https://patchwork.freedesktop.org/series/77002/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8434_full -> IGTPW_4540_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/index.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@suspend:
    - shard-iclb:         [PASS][1] -> [INCOMPLETE][2] ([i915#1185])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-iclb3/igt@gem_eio@suspend.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-iclb3/igt@gem_eio@suspend.html

  * igt@gem_softpin@noreloc-s3:
    - shard-apl:          [PASS][3] -> [DMESG-WARN][4] ([i915#180]) +5 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-apl4/igt@gem_softpin@noreloc-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-apl6/igt@gem_softpin@noreloc-s3.html

  * igt@kms_cursor_crc@pipe-a-cursor-256x85-offscreen:
    - shard-kbl:          [PASS][5] -> [FAIL][6] ([i915#54] / [i915#93] / [i915#95]) +3 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-256x85-offscreen.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-kbl3/igt@kms_cursor_crc@pipe-a-cursor-256x85-offscreen.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [PASS][7] -> [DMESG-WARN][8] ([i915#180])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-kbl1/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_draw_crc@draw-method-xrgb8888-render-untiled:
    - shard-kbl:          [PASS][9] -> [FAIL][10] ([i915#177] / [i915#52] / [i915#54] / [i915#93] / [i915#95])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-kbl6/igt@kms_draw_crc@draw-method-xrgb8888-render-untiled.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-kbl6/igt@kms_draw_crc@draw-method-xrgb8888-render-untiled.html
    - shard-apl:          [PASS][11] -> [FAIL][12] ([i915#52] / [i915#54] / [i915#95])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-apl8/igt@kms_draw_crc@draw-method-xrgb8888-render-untiled.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-apl4/igt@kms_draw_crc@draw-method-xrgb8888-render-untiled.html

  * igt@kms_flip_tiling@flip-changes-tiling-y:
    - shard-apl:          [PASS][13] -> [FAIL][14] ([i915#95])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-apl6/igt@kms_flip_tiling@flip-changes-tiling-y.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-apl4/igt@kms_flip_tiling@flip-changes-tiling-y.html
    - shard-kbl:          [PASS][15] -> [FAIL][16] ([i915#699] / [i915#93] / [i915#95])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-kbl4/igt@kms_flip_tiling@flip-changes-tiling-y.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-kbl6/igt@kms_flip_tiling@flip-changes-tiling-y.html

  * igt@kms_plane_cursor@pipe-a-viewport-size-128:
    - shard-apl:          [PASS][17] -> [FAIL][18] ([i915#1559] / [i915#95])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-apl8/igt@kms_plane_cursor@pipe-a-viewport-size-128.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-apl7/igt@kms_plane_cursor@pipe-a-viewport-size-128.html
    - shard-kbl:          [PASS][19] -> [FAIL][20] ([i915#1559] / [i915#93] / [i915#95])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-kbl6/igt@kms_plane_cursor@pipe-a-viewport-size-128.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-kbl7/igt@kms_plane_cursor@pipe-a-viewport-size-128.html

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         [PASS][21] -> [SKIP][22] ([fdo#109441]) +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-iclb2/igt@kms_psr@psr2_primary_page_flip.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-iclb8/igt@kms_psr@psr2_primary_page_flip.html

  
#### Possible fixes ####

  * igt@gem_ctx_persistence@engines-mixed-process@bcs0:
    - shard-glk:          [FAIL][23] ([i915#1528]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-glk4/igt@gem_ctx_persistence@engines-mixed-process@bcs0.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-glk8/igt@gem_ctx_persistence@engines-mixed-process@bcs0.html

  * {igt@gem_exec_fence@parallel@bcs0}:
    - shard-tglb:         [FAIL][25] -> [PASS][26] +4 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-tglb5/igt@gem_exec_fence@parallel@bcs0.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-tglb6/igt@gem_exec_fence@parallel@bcs0.html

  * igt@gem_exec_suspend@basic-s3:
    - shard-kbl:          [DMESG-WARN][27] ([i915#180]) -> [PASS][28] +2 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-kbl7/igt@gem_exec_suspend@basic-s3.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-kbl6/igt@gem_exec_suspend@basic-s3.html

  * igt@kms_big_fb@linear-32bpp-rotate-180:
    - shard-apl:          [FAIL][29] ([i915#1119] / [i915#95]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-apl6/igt@kms_big_fb@linear-32bpp-rotate-180.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-apl4/igt@kms_big_fb@linear-32bpp-rotate-180.html
    - shard-kbl:          [FAIL][31] ([i915#1119] / [i915#93] / [i915#95]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-kbl4/igt@kms_big_fb@linear-32bpp-rotate-180.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-kbl6/igt@kms_big_fb@linear-32bpp-rotate-180.html

  * igt@kms_color@pipe-b-legacy-gamma:
    - shard-kbl:          [FAIL][33] ([i915#71]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-kbl4/igt@kms_color@pipe-b-legacy-gamma.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-kbl2/igt@kms_color@pipe-b-legacy-gamma.html
    - shard-glk:          [FAIL][35] ([i915#71]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-glk1/igt@kms_color@pipe-b-legacy-gamma.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-glk7/igt@kms_color@pipe-b-legacy-gamma.html
    - shard-apl:          [FAIL][37] ([i915#71]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-apl4/igt@kms_color@pipe-b-legacy-gamma.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-apl6/igt@kms_color@pipe-b-legacy-gamma.html

  * igt@kms_cursor_crc@pipe-a-cursor-64x64-onscreen:
    - shard-kbl:          [FAIL][39] ([i915#54] / [i915#93] / [i915#95]) -> [PASS][40] +4 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-64x64-onscreen.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-64x64-onscreen.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
    - shard-apl:          [FAIL][41] ([i915#49] / [i915#95]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-apl3/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-apl7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite.html
    - shard-kbl:          [FAIL][43] ([i915#49]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-kbl7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-kbl7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-apl:          [DMESG-WARN][45] ([i915#180]) -> [PASS][46] +2 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-apl6/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-apl6/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html

  * igt@kms_plane_cursor@pipe-a-viewport-size-64:
    - shard-kbl:          [FAIL][47] ([i915#1559] / [i915#93] / [i915#95]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-kbl2/igt@kms_plane_cursor@pipe-a-viewport-size-64.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-kbl6/igt@kms_plane_cursor@pipe-a-viewport-size-64.html
    - shard-apl:          [FAIL][49] ([i915#1559] / [i915#95]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-apl6/igt@kms_plane_cursor@pipe-a-viewport-size-64.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-apl4/igt@kms_plane_cursor@pipe-a-viewport-size-64.html

  * igt@kms_psr@psr2_sprite_plane_move:
    - shard-iclb:         [SKIP][51] ([fdo#109441]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-iclb1/igt@kms_psr@psr2_sprite_plane_move.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html

  * {igt@perf@blocking-parameterized}:
    - shard-iclb:         [FAIL][53] ([i915#1542]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-iclb1/igt@perf@blocking-parameterized.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-iclb5/igt@perf@blocking-parameterized.html
    - shard-hsw:          [FAIL][55] ([i915#1542]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-hsw8/igt@perf@blocking-parameterized.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-hsw7/igt@perf@blocking-parameterized.html

  
#### Warnings ####

  * igt@i915_pm_rpm@pm-tiling:
    - shard-snb:          [INCOMPLETE][57] ([i915#82]) -> [SKIP][58] ([fdo#109271])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-snb6/igt@i915_pm_rpm@pm-tiling.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-snb6/igt@i915_pm_rpm@pm-tiling.html

  * igt@kms_content_protection@legacy:
    - shard-apl:          [FAIL][59] ([fdo#110321] / [fdo#110336]) -> [TIMEOUT][60] ([i915#1319])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-apl6/igt@kms_content_protection@legacy.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-apl7/igt@kms_content_protection@legacy.html

  * igt@kms_content_protection@lic:
    - shard-apl:          [TIMEOUT][61] ([i915#1319]) -> [FAIL][62] ([fdo#110321]) +1 similar issue
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8434/shard-apl3/igt@kms_content_protection@lic.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/shard-apl2/igt@kms_content_protection@lic.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#110321]: https://bugs.freedesktop.org/show_bug.cgi?id=110321
  [fdo#110336]: https://bugs.freedesktop.org/show_bug.cgi?id=110336
  [i915#1119]: https://gitlab.freedesktop.org/drm/intel/issues/1119
  [i915#1185]: https://gitlab.freedesktop.org/drm/intel/issues/1185
  [i915#1319]: https://gitlab.freedesktop.org/drm/intel/issues/1319
  [i915#1528]: https://gitlab.freedesktop.org/drm/intel/issues/1528
  [i915#1542]: https://gitlab.freedesktop.org/drm/intel/issues/1542
  [i915#1559]: https://gitlab.freedesktop.org/drm/intel/issues/1559
  [i915#177]: https://gitlab.freedesktop.org/drm/intel/issues/177
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#49]: https://gitlab.freedesktop.org/drm/intel/issues/49
  [i915#52]: https://gitlab.freedesktop.org/drm/intel/issues/52
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#699]: https://gitlab.freedesktop.org/drm/intel/issues/699
  [i915#71]: https://gitlab.freedesktop.org/drm/intel/issues/71
  [i915#82]: https://gitlab.freedesktop.org/drm/intel/issues/82
  [i915#93]: https://gitlab.freedesktop.org/drm/intel/issues/93
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (11 -> 8)
------------------------------

  Missing    (3): pig-skl-6260u pig-glk-j5005 pig-icl-1065g7 


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

  * CI: CI-20190529 -> None
  * IGT: IGT_5635 -> IGTPW_4540
  * Piglit: piglit_4509 -> None

  CI-20190529: 20190529
  CI_DRM_8434: 2951bac393beb4f095468de8b7cc53c8e3a092c2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_4540: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4540/index.html
  IGT_5635: e83abfca61d407d12eee4d25bb0e8686337a7791 @ 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_4540/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/2] lib/igt_chamelium: Sleep when doing autodiscovery

[Imre Deak]

On Wed, May 06, 2020 at 06:58:02PM +0300, Arkadiusz Hiler wrote:
> Autodiscovery was wrongly assuming that whenever we do chamelium_plug()
> the connector state change is immediate.
> 
> It was working most of the time for native connectors, but may explain
> some of the flip-flopping skips.
> 
> The problem got only more serious with the advent of LSPcons as USB-C,
> where we have much things happening on the signal path, introducing
> delays.
> 
> So for the sake of reliability this change introduces sleep(10) between
> plug and reprobe. The number is about 2 * the_most_pathological_case_observed.
> 
> Also the discovery step is performed only if there is no static port
> mapping set up. After the discovery is done, IGT prints the mapping
> ready to be pasted into .igtrc.
> 
> Cc: Kunal Joshi <kunal1.joshi@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  lib/igt_chamelium.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index c704b84f..28be5248 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -82,6 +82,14 @@
>   *
>   */
>  
> +/*
> + * We cannot expect chamelium_plug() to take effect imediately.
> + *
> + * Especially with modern, more complex hardware where we may have LSPcons and
> + * USB controllers in the way.
> + */
> +#define CHAMELIUM_HOTPLUG_DETECTION_DELAY 10
> +
>  struct chamelium_edid {
>  	struct chamelium *chamelium;
>  	struct edid *base;
> @@ -2231,6 +2239,10 @@ static bool chamelium_autodiscover(struct chamelium *chamelium, int drm_fd)
>  		chamelium_plug(chamelium, port);
>  	}
>  
> +	igt_info("Sleeping %d seconds for the hotplug to take effect.\n",
> +		 CHAMELIUM_HOTPLUG_DETECTION_DELAY);
> +	sleep(CHAMELIUM_HOTPLUG_DETECTION_DELAY);
> +
>  	/* Reprobe connectors and build the mapping */
>  	res = drmModeGetResources(drm_fd);
>  	if (!res)
> @@ -2448,8 +2460,20 @@ struct chamelium *chamelium_init(int drm_fd)
>  	if (!chamelium_read_port_mappings(chamelium, drm_fd))
>  		goto error;
>  
> -	if (!chamelium_autodiscover(chamelium, drm_fd))
> -		goto error;
> +	if (chamelium->port_count == 0) {
> +		igt_info("Chamelium configured without port mapping, "
> +			 "performing autodiscovery\n");
> +
> +		if (!chamelium_autodiscover(chamelium, drm_fd))
> +			goto error;
> +
> +		if (chamelium->port_count != 0)
> +			igt_info("\nConsider adding the following to your .igtrc:\n");
> +		for (int i = 0; i < chamelium->port_count; ++i) {
> +			igt_info("[Chamelium:%s]\n", chamelium->ports[i].name);
> +			igt_info("ChameliumPortID=%d\n\n", chamelium->ports[i].id);
> +		}
> +	}
>  
>  	cleanup_instance = chamelium;
>  	igt_install_exit_handler(chamelium_exit_handler);
> -- 
> 2.25.2
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_kms: Make igt_display_require() + chamelium more robust

[Imre Deak]

On Wed, May 06, 2020 at 06:58:03PM +0300, Arkadiusz Hiler wrote:
> 1. We don't reset Chamelium, as this generates extra unplug events if
>    any of the ports is already connected which is often the case
> 
> 2. We try to plug all the chamelium ports, it's a noop if they are
>    already plugged
> 
> 2. We wait for all the ports being connected:
>    - if the port mapping is provided: wait for the ports to be connected
>    - if there is no mapping: sleep(10) and hope for the best
> 
> Cc: Petri Latvala <petri.latvala@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> ---
>  lib/igt_chamelium.c | 87 +++++++++++++++++++++++++++++++++++++++------
>  lib/igt_chamelium.h |  2 ++
>  lib/igt_kms.c       |  2 ++
>  3 files changed, 80 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index 28be5248..69ad8378 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -2532,17 +2532,6 @@ bool chamelium_plug_all(struct chamelium *chamelium)
>  	size_t port_count;
>  	int port_ids[CHAMELIUM_MAX_PORTS];
>  	xmlrpc_value *v;
> -	v = __chamelium_rpc(chamelium, NULL, "Reset", "()");
> -
> -	if (v != NULL)
> -		xmlrpc_DECREF(v);
> -
> -	if (chamelium->env.fault_occurred) {
> -		igt_debug("Chamelium RPC call failed: %s\n",
> -		     chamelium->env.fault_string);
> -
> -		return false;
> -	}

The reset could've been added for some stability issue in Chamelium, so
imo better to remove it in a separate patch.

>  	port_count = chamelium_get_video_ports(chamelium, port_ids);
>  	if (port_count <= 0)
> @@ -2565,6 +2554,82 @@ bool chamelium_plug_all(struct chamelium *chamelium)
>  	return true;
>  }
>  
> +bool chamelium_wait_all_configured_ports_connected(struct chamelium *chamelium, int drm_fd)
> +{
> +	drmModeRes *res;
> +	drmModeConnector *connector;
> +	char **group_list;
> +	char *group;
> +	bool ret = true;
> +
> +	int connectors[CHAMELIUM_MAX_PORTS];
> +	int connectors_count = 0;
> +
> +	res = drmModeGetResources(drm_fd);
> +
> +	group_list = g_key_file_get_groups(igt_key_file, NULL);
> +
> +	for (int i = 0; group_list[i] != NULL; i++) {
> +		char *map_name;
> +		group = group_list[i];
> +
> +		if (!strstr(group, "Chamelium:"))
> +			continue;
> +
> +		igt_assert(chamelium->port_count <= CHAMELIUM_MAX_PORTS);
> +
> +		map_name = group + (sizeof("Chamelium:") - 1);
> +
> +		for (int j = 0;
> +		     j < res->count_connectors;
> +		     j++) {
> +			char name[50];
> +
> +			connector = drmModeGetConnectorCurrent(
> +			    drm_fd, res->connectors[j]);
> +
> +			/* We have to generate the connector name on our own */
> +			snprintf(name, 50, "%s-%u",
> +				 kmstest_connector_type_str(connector->connector_type),
> +				 connector->connector_type_id);
> +
> +
> +			if (strcmp(name, map_name) == 0) {
> +				igt_assert(connectors_count < CHAMELIUM_MAX_PORTS);
> +				connectors[connectors_count++] = connector->connector_id;
> +				break;
> +			}
> +
> +			drmModeFreeConnector(connector);
> +		}
> +	}
> +
> +	drmModeFreeResources(res);

Couldn't you reuse chamelium_read_port_mappings()?

> +	if (connectors_count == 0) {
> +		igt_info("No chamelium port mappping, sleeping for %d seconds "
> +			 "for the hotplug to take effect",
> +			 CHAMELIUM_HOTPLUG_DETECTION_DELAY);
> +		sleep(CHAMELIUM_HOTPLUG_DETECTION_DELAY);
> +		return true;
> +	}

In igt_chamlium this makes the startup delay 20 sec now with the
detection delay added in the previous patch.

Would it make sense to change autodiscover to retry detection for 10 sec
instead, which you could call from here? Then you could also call
autodiscover without the unconditional sleep in the previous patch.

> +
> +	igt_until_timeout(CHAMELIUM_HOTPLUG_DETECTION_DELAY) {
> +		ret = true;
> +		for (int i = 0; i < connectors_count; ++i) {
> +			connector = drmModeGetConnector(drm_fd, connectors[i]);
> +			if (connector->connection != DRM_MODE_CONNECTED)
> +				ret = false;
> +			drmModeFreeConnector(connector);
> +		}
> +
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
>  igt_constructor {
>  	/* Frame dumps can be large, so we need to be able to handle very large
>  	 * responses
> diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
> index c29741b4..359f4ab3 100644
> --- a/lib/igt_chamelium.h
> +++ b/lib/igt_chamelium.h
> @@ -215,5 +215,7 @@ void chamelium_destroy_frame_dump(struct chamelium_frame_dump *dump);
>  void chamelium_destroy_audio_file(struct chamelium_audio_file *audio_file);
>  void chamelium_infoframe_destroy(struct chamelium_infoframe *infoframe);
>  bool chamelium_plug_all(struct chamelium *chamelium);
> +bool chamelium_wait_all_configured_ports_connected(struct chamelium *chamelium,
> +						   int drm_fd);
>  
>  #endif /* IGT_CHAMELIUM_H */
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index e9621e7e..d4cbc1c5 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1899,6 +1899,8 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>  				       "cannot reach the configured chamelium!\n");
>  			igt_abort_on_f(!chamelium_plug_all(chamelium),
>  				       "failed to plug all the chamelium ports!\n");
> +			igt_abort_on_f(!chamelium_wait_all_configured_ports_connected(chamelium, drm_fd),
> +				       "not all configured chamelium ports are connected!\n");
>  			chamelium_deinit_rpc_only(chamelium);
>  		}
>  	}
> -- 
> 2.25.2
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_kms: Make igt_display_require() + chamelium more robust

[Imre Deak]

On Thu, May 07, 2020 at 10:53:05PM +0300, Imre Deak wrote:
> On Wed, May 06, 2020 at 06:58:03PM +0300, Arkadiusz Hiler wrote:
> > 1. We don't reset Chamelium, as this generates extra unplug events if
> >    any of the ports is already connected which is often the case
> > 
> > 2. We try to plug all the chamelium ports, it's a noop if they are
> >    already plugged
> > 
> > 2. We wait for all the ports being connected:
> >    - if the port mapping is provided: wait for the ports to be connected
> >    - if there is no mapping: sleep(10) and hope for the best
> > 
> > Cc: Petri Latvala <petri.latvala@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > ---
> >  lib/igt_chamelium.c | 87 +++++++++++++++++++++++++++++++++++++++------
> >  lib/igt_chamelium.h |  2 ++
> >  lib/igt_kms.c       |  2 ++
> >  3 files changed, 80 insertions(+), 11 deletions(-)
> > 
> > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > index 28be5248..69ad8378 100644
> > --- a/lib/igt_chamelium.c
> > +++ b/lib/igt_chamelium.c
> > @@ -2532,17 +2532,6 @@ bool chamelium_plug_all(struct chamelium *chamelium)
> >  	size_t port_count;
> >  	int port_ids[CHAMELIUM_MAX_PORTS];
> >  	xmlrpc_value *v;
> > -	v = __chamelium_rpc(chamelium, NULL, "Reset", "()");
> > -
> > -	if (v != NULL)
> > -		xmlrpc_DECREF(v);
> > -
> > -	if (chamelium->env.fault_occurred) {
> > -		igt_debug("Chamelium RPC call failed: %s\n",
> > -		     chamelium->env.fault_string);
> > -
> > -		return false;
> > -	}
> 
> The reset could've been added for some stability issue in Chamelium, so
> imo better to remove it in a separate patch.
> 
> >  	port_count = chamelium_get_video_ports(chamelium, port_ids);
> >  	if (port_count <= 0)
> > @@ -2565,6 +2554,82 @@ bool chamelium_plug_all(struct chamelium *chamelium)
> >  	return true;
> >  }
> >  
> > +bool chamelium_wait_all_configured_ports_connected(struct chamelium *chamelium, int drm_fd)
> > +{
> > +	drmModeRes *res;
> > +	drmModeConnector *connector;
> > +	char **group_list;
> > +	char *group;
> > +	bool ret = true;
> > +
> > +	int connectors[CHAMELIUM_MAX_PORTS];
> > +	int connectors_count = 0;
> > +
> > +	res = drmModeGetResources(drm_fd);
> > +
> > +	group_list = g_key_file_get_groups(igt_key_file, NULL);
> > +
> > +	for (int i = 0; group_list[i] != NULL; i++) {
> > +		char *map_name;
> > +		group = group_list[i];
> > +
> > +		if (!strstr(group, "Chamelium:"))
> > +			continue;
> > +
> > +		igt_assert(chamelium->port_count <= CHAMELIUM_MAX_PORTS);
> > +
> > +		map_name = group + (sizeof("Chamelium:") - 1);
> > +
> > +		for (int j = 0;
> > +		     j < res->count_connectors;
> > +		     j++) {
> > +			char name[50];
> > +
> > +			connector = drmModeGetConnectorCurrent(
> > +			    drm_fd, res->connectors[j]);
> > +
> > +			/* We have to generate the connector name on our own */
> > +			snprintf(name, 50, "%s-%u",
> > +				 kmstest_connector_type_str(connector->connector_type),
> > +				 connector->connector_type_id);
> > +
> > +
> > +			if (strcmp(name, map_name) == 0) {
> > +				igt_assert(connectors_count < CHAMELIUM_MAX_PORTS);
> > +				connectors[connectors_count++] = connector->connector_id;
> > +				break;
> > +			}
> > +
> > +			drmModeFreeConnector(connector);
> > +		}
> > +	}
> > +
> > +	drmModeFreeResources(res);
> 
> Couldn't you reuse chamelium_read_port_mappings()?
> 
> > +	if (connectors_count == 0) {
> > +		igt_info("No chamelium port mappping, sleeping for %d seconds "
> > +			 "for the hotplug to take effect",
> > +			 CHAMELIUM_HOTPLUG_DETECTION_DELAY);
> > +		sleep(CHAMELIUM_HOTPLUG_DETECTION_DELAY);
> > +		return true;
> > +	}
> 
> In igt_chamlium this makes the startup delay 20 sec now with the
> detection delay added in the previous patch.
> 
> Would it make sense to change autodiscover to retry detection for 10 sec
> instead, which you could call from here? Then you could also call
> autodiscover without the unconditional sleep in the previous patch.

Ah that wouldn't work, since we don't know how many connectors to wait
for:/ But I'd avoid doing twice the detection delay, for instance by
removing it from the previous patch, since chamelium_init() is always
called after igt_display_require().

> > +
> > +	igt_until_timeout(CHAMELIUM_HOTPLUG_DETECTION_DELAY) {
> > +		ret = true;
> > +		for (int i = 0; i < connectors_count; ++i) {
> > +			connector = drmModeGetConnector(drm_fd, connectors[i]);
> > +			if (connector->connection != DRM_MODE_CONNECTED)
> > +				ret = false;
> > +			drmModeFreeConnector(connector);
> > +		}
> > +
> > +		if (ret)
> > +			break;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  igt_constructor {
> >  	/* Frame dumps can be large, so we need to be able to handle very large
> >  	 * responses
> > diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
> > index c29741b4..359f4ab3 100644
> > --- a/lib/igt_chamelium.h
> > +++ b/lib/igt_chamelium.h
> > @@ -215,5 +215,7 @@ void chamelium_destroy_frame_dump(struct chamelium_frame_dump *dump);
> >  void chamelium_destroy_audio_file(struct chamelium_audio_file *audio_file);
> >  void chamelium_infoframe_destroy(struct chamelium_infoframe *infoframe);
> >  bool chamelium_plug_all(struct chamelium *chamelium);
> > +bool chamelium_wait_all_configured_ports_connected(struct chamelium *chamelium,
> > +						   int drm_fd);
> >  
> >  #endif /* IGT_CHAMELIUM_H */
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index e9621e7e..d4cbc1c5 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -1899,6 +1899,8 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> >  				       "cannot reach the configured chamelium!\n");
> >  			igt_abort_on_f(!chamelium_plug_all(chamelium),
> >  				       "failed to plug all the chamelium ports!\n");
> > +			igt_abort_on_f(!chamelium_wait_all_configured_ports_connected(chamelium, drm_fd),
> > +				       "not all configured chamelium ports are connected!\n");
> >  			chamelium_deinit_rpc_only(chamelium);
> >  		}
> >  	}
> > -- 
> > 2.25.2
> > 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_kms: Make igt_display_require() + chamelium more robust

[Arkadiusz Hiler]

On Thu, May 07, 2020 at 11:16:28PM +0300, Imre Deak wrote:
> On Thu, May 07, 2020 at 10:53:05PM +0300, Imre Deak wrote:
> > On Wed, May 06, 2020 at 06:58:03PM +0300, Arkadiusz Hiler wrote:
> > > 1. We don't reset Chamelium, as this generates extra unplug events if
> > >    any of the ports is already connected which is often the case
> > > 
> > > 2. We try to plug all the chamelium ports, it's a noop if they are
> > >    already plugged
> > > 
> > > 2. We wait for all the ports being connected:
> > >    - if the port mapping is provided: wait for the ports to be connected
> > >    - if there is no mapping: sleep(10) and hope for the best
> > > 
> > > Cc: Petri Latvala <petri.latvala@intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > > ---
> > >  lib/igt_chamelium.c | 87 +++++++++++++++++++++++++++++++++++++++------
> > >  lib/igt_chamelium.h |  2 ++
> > >  lib/igt_kms.c       |  2 ++
> > >  3 files changed, 80 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > > index 28be5248..69ad8378 100644
> > > --- a/lib/igt_chamelium.c
> > > +++ b/lib/igt_chamelium.c
> > > @@ -2532,17 +2532,6 @@ bool chamelium_plug_all(struct chamelium *chamelium)
> > >  	size_t port_count;
> > >  	int port_ids[CHAMELIUM_MAX_PORTS];
> > >  	xmlrpc_value *v;
> > > -	v = __chamelium_rpc(chamelium, NULL, "Reset", "()");
> > > -
> > > -	if (v != NULL)
> > > -		xmlrpc_DECREF(v);
> > > -
> > > -	if (chamelium->env.fault_occurred) {
> > > -		igt_debug("Chamelium RPC call failed: %s\n",
> > > -		     chamelium->env.fault_string);
> > > -
> > > -		return false;
> > > -	}
> > 
> > The reset could've been added for some stability issue in Chamelium, so
> > imo better to remove it in a separate patch.

The reset was there just to get the default EDIDs, but I think it's
worth skipping it to avoid extra unplugs. I can spin it off into a
separate patch though.

> > >  	port_count = chamelium_get_video_ports(chamelium, port_ids);
> > >  	if (port_count <= 0)
> > > @@ -2565,6 +2554,82 @@ bool chamelium_plug_all(struct chamelium *chamelium)
> > >  	return true;
> > >  }
> > >  
> > > +bool chamelium_wait_all_configured_ports_connected(struct chamelium *chamelium, int drm_fd)
> > > +{
> > > +	drmModeRes *res;
> > > +	drmModeConnector *connector;
> > > +	char **group_list;
> > > +	char *group;
> > > +	bool ret = true;
> > > +
> > > +	int connectors[CHAMELIUM_MAX_PORTS];
> > > +	int connectors_count = 0;
> > > +
> > > +	res = drmModeGetResources(drm_fd);
> > > +
> > > +	group_list = g_key_file_get_groups(igt_key_file, NULL);
> > > +
> > > +	for (int i = 0; group_list[i] != NULL; i++) {
> > > +		char *map_name;
> > > +		group = group_list[i];
> > > +
> > > +		if (!strstr(group, "Chamelium:"))
> > > +			continue;
> > > +
> > > +		igt_assert(chamelium->port_count <= CHAMELIUM_MAX_PORTS);
> > > +
> > > +		map_name = group + (sizeof("Chamelium:") - 1);
> > > +
> > > +		for (int j = 0;
> > > +		     j < res->count_connectors;
> > > +		     j++) {
> > > +			char name[50];
> > > +
> > > +			connector = drmModeGetConnectorCurrent(
> > > +			    drm_fd, res->connectors[j]);
> > > +
> > > +			/* We have to generate the connector name on our own */
> > > +			snprintf(name, 50, "%s-%u",
> > > +				 kmstest_connector_type_str(connector->connector_type),
> > > +				 connector->connector_type_id);
> > > +
> > > +
> > > +			if (strcmp(name, map_name) == 0) {
> > > +				igt_assert(connectors_count < CHAMELIUM_MAX_PORTS);
> > > +				connectors[connectors_count++] = connector->connector_id;
> > > +				break;
> > > +			}
> > > +
> > > +			drmModeFreeConnector(connector);
> > > +		}
> > > +	}
> > > +
> > > +	drmModeFreeResources(res);
> > 
> > Couldn't you reuse chamelium_read_port_mappings()?

I did that first but...

1. I want to avoid calling chamelium_get_port_type() which does
   chamelium XMLRPC calls.

2. I don't to half initalize chamleium->ports

Then I have attempted to rework chamelium_read_port_mappings() to
extract a common helpers and make those things optional, but it ended up
much longer and much harder to follow than what we have here now.

> > > +	if (connectors_count == 0) {
> > > +		igt_info("No chamelium port mappping, sleeping for %d seconds "
> > > +			 "for the hotplug to take effect",
> > > +			 CHAMELIUM_HOTPLUG_DETECTION_DELAY);
> > > +		sleep(CHAMELIUM_HOTPLUG_DETECTION_DELAY);
> > > +		return true;
> > > +	}
> > 
> > In igt_chamlium this makes the startup delay 20 sec now with the
> > detection delay added in the previous patch.
> > 
> > Would it make sense to change autodiscover to retry detection for 10 sec
> > instead, which you could call from here? Then you could also call
> > autodiscover without the unconditional sleep in the previous patch.
> 
> Ah that wouldn't work, since we don't know how many connectors to wait
> for:/ But I'd avoid doing twice the detection delay, for instance by
> removing it from the previous patch, since chamelium_init() is always
> called after igt_display_require().

They play two different roles.

In igt_display_require() we just plug the ports for the sake of
non-chamelium tests.

In chamelium's discovery we are resetting the state, setting up custom
EDIDs, etc.

We cannot use connectors considered by igt_display_require() as
connected for the sake of autodiscovery - we still need a hard sleep()
there as we may not get the updated state/EDID immediately with TypeC.

We could create something like this though:

extern const bool __igt_chamelium_test = false;
#define IGT_CHAMELIUM_TEST __igt_chamelium_test = true

and then skip the wait in igt_display_require() basing on the contents.

IMO that feels too hacky, and I am more keen on with making the
autodiscovery annoyingly long to incentivize people to put the snippet
kms_chamelium is printing out in their .igtrc.


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