[igt-dev] [PATCH] Refactor Display Outputs to handle MST Connectors.

* [igt-dev] [PATCH] Refactor Display Outputs to handle MST Connectors.
@ 2022-10-18 14:59 Mark Yacoub
  2022-10-18 15:49 ` [igt-dev] ✗ Fi.CI.BAT: failure for " Patchwork
                   ` (8 more replies)
  0 siblings, 9 replies; 14+ messages in thread
From: Mark Yacoub @ 2022-10-18 14:59 UTC (permalink / raw)
  To: igt-dev, lyude
  Cc: robdclark, petri.latvala, ihf, amstan, seanpaul, matthewtlam,
	khaled.almahallawy, markyacoub

[Why]
Chamelium can be connected to the DUT through MST.
MST connectors can appear to the kernel only after it's been plugged in
through Chamelium.
The older implementation gets the output at display init and doesn't
handle any new connectors being added.

[How]
Save the connector path for each display output to check for MSTs (used
for comparison and retrieval as connector ID can change but Path would
not.)
Refresh display outputs at chamelium init. This retrieves all newly
plugged connectors.

Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
---
 lib/igt_chamelium.c                   |  36 +++--
 lib/igt_chamelium.h                   |   2 +-
 lib/igt_kms.c                         | 223 +++++++++++++++++---------
 lib/igt_kms.h                         |   2 +
 tests/chamelium/kms_chamelium.c       |   7 +-
 tests/chamelium/kms_color_chamelium.c |   2 +-
 tests/feature_discovery.c             |   3 +-
 tests/kms_dp_tiled_display.c          |  18 ++-
 8 files changed, 185 insertions(+), 108 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index 26244bd5..d998a1f1 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -233,15 +233,18 @@ drmModeConnector *chamelium_port_get_connector(struct chamelium *chamelium,
 	/* In case the connector ID is still valid, do a quick check if we're have the connector we expect. 
 	 * Otherwise, read the new resources and find the new connector we're looking for. */
 	if (connector) {
-		drmModePropertyBlobPtr path_blob =
-			kmstest_get_path_blob(drm_fd, connector->connector_id);
-		if (path_blob) {
-			bool is_correct_connector =
-				strcmp(port->connector_path, path_blob->data) ==
-				0;
-			drmModeFreePropertyBlob(path_blob);
-			if (is_correct_connector)
-				return connector;
+		if (connector->connection == DRM_MODE_CONNECTED) {
+			drmModePropertyBlobPtr path_blob =
+				kmstest_get_path_blob(drm_fd,
+						      connector->connector_id);
+			if (path_blob) {
+				bool is_correct_connector =
+					strcmp(port->connector_path,
+					       path_blob->data) == 0;
+				drmModeFreePropertyBlob(path_blob);
+				if (is_correct_connector)
+					return connector;
+			}
 		}
 
 		drmModeFreeConnector(connector);
@@ -413,11 +416,8 @@ chamelium_reprobe_connector(igt_display_t *display,
 	/* If we still have a connector, let's make sure that igt_display and
 	   the port are up to date too */
 	output = igt_output_from_connector(display, connector);
-	if (output)
-	{
-		output->force_reprobe = true;
-		igt_output_refresh(output);
-	}
+	output->force_reprobe = true;
+	igt_output_refresh(output);
 
 	drmModeFreeConnector(connector);
 	return connection_status;
@@ -2886,7 +2886,7 @@ error:
  *
  * Returns: A newly initialized chamelium struct, or NULL on error
  */
-struct chamelium *chamelium_init(int drm_fd)
+struct chamelium *chamelium_init(int drm_fd, igt_display_t *display)
 {
 	struct chamelium *chamelium = chamelium_init_rpc_only();
 	bool mismatching_ports_found = false;
@@ -2952,6 +2952,12 @@ struct chamelium *chamelium_init(int drm_fd)
 		       "port mapping manually in .igtrc with AdapterAllowed=1. See "
 		       "docs/chamelium.txt for more information.\n");
 
+	/* After a Chamelium init, all ports are now connected, and MST
+	 * connectors are now known to the kernel. MST connectors would spawn
+	 * new connectors that were previously unknown to the kernel. Refresh
+	 * the outputs to grab all supported connectors.*/
+	igt_display_reset_outputs(display);
+
 	return chamelium;
 error:
 	close(chamelium->drm_fd);
diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
index f40e848e..e1ee2b59 100644
--- a/lib/igt_chamelium.h
+++ b/lib/igt_chamelium.h
@@ -107,7 +107,7 @@ extern bool igt_chamelium_allow_fsm_handling;
 
 void chamelium_deinit_rpc_only(struct chamelium *chamelium);
 struct chamelium *chamelium_init_rpc_only(void);
-struct chamelium *chamelium_init(int drm_fd);
+struct chamelium *chamelium_init(int drm_fd, igt_display_t *display);
 void chamelium_deinit(struct chamelium *chamelium);
 void chamelium_reset(struct chamelium *chamelium);
 
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 55fcd811..921a623d 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1698,6 +1698,7 @@ static bool _kmstest_connector_config(int drm_fd, uint32_t connector_id,
 {
 	drmModeRes *resources;
 	drmModeConnector *connector;
+	drmModePropertyBlobPtr path_blob;
 
 	config->pipe = PIPE_NONE;
 
@@ -1722,6 +1723,13 @@ static bool _kmstest_connector_config(int drm_fd, uint32_t connector_id,
 		goto err3;
 	}
 
+	/* Set connector path for MST connectors. */
+	path_blob = kmstest_get_path_blob(drm_fd, connector_id);
+	if (path_blob) {
+		config->connector_path = strdup(path_blob->data);
+		drmModeFreePropertyBlob(path_blob);
+	}
+
 	/*
 	 * Find given CRTC if crtc_id != 0 or else the first CRTC not in use.
 	 * In both cases find the first compatible encoder and skip the CRTC
@@ -2347,16 +2355,119 @@ static bool igt_pipe_has_valid_output(igt_display_t *display, enum pipe pipe)
 	return false;
 }
 
+/**
+ * igt_display_require:
+ * @display: a pointer to an initialized #igt_display_t structure
+ *
+ * Initialize @display outputs with their connectors and pipes.
+ * This function clears any previously allocated outputs.
+ */
+void igt_display_reset_outputs(igt_display_t *display)
+{
+	int i;
+	drmModeRes *resources;
+
+	/* Clear any existing outputs*/
+	if (display->n_outputs) {
+		for (i = 0; i < display->n_outputs; i++) {
+			struct kmstest_connector_config *config =
+				&display->outputs[i].config;
+			drmModeFreeConnector(config->connector);
+			drmModeFreeEncoder(config->encoder);
+			drmModeFreeCrtc(config->crtc);
+			free(config->connector_path);
+		}
+		free(display->outputs);
+	}
+
+	resources = drmModeGetResources(display->drm_fd);
+	if (!resources)
+		return;
+
+	display->n_outputs = resources->count_connectors;
+	display->outputs = calloc(display->n_outputs, sizeof(igt_output_t));
+	igt_assert_f(display->outputs,
+		     "Failed to allocate memory for %d outputs\n",
+		     display->n_outputs);
+
+	for (i = 0; i < display->n_outputs; i++) {
+		igt_output_t *output = &display->outputs[i];
+		drmModeConnector *connector;
+
+		/*
+		 * We don't assign each output a pipe unless
+		 * a pipe is set with igt_output_set_pipe().
+		 */
+		output->pending_pipe = PIPE_NONE;
+		output->id = resources->connectors[i];
+		output->display = display;
+
+		igt_output_refresh(output);
+
+		connector = output->config.connector;
+		if (connector &&
+		    (!connector->count_modes ||
+		     connector->connection == DRM_MODE_UNKNOWNCONNECTION)) {
+			output->force_reprobe = true;
+			igt_output_refresh(output);
+		}
+	}
+
+	/* Set reasonable default values for every object in the
+	 * display. */
+	igt_display_reset(display);
+
+	for_each_pipe(display, i) {
+		igt_pipe_t *pipe = &display->pipes[i];
+		igt_output_t *output;
+
+		if (!igt_pipe_has_valid_output(display, i))
+			continue;
+
+		output = igt_get_single_output_for_pipe(display, i);
+
+		if (pipe->num_primary_planes > 1) {
+			igt_plane_t *primary =
+				&pipe->planes[pipe->plane_primary];
+			igt_plane_t *assigned_primary =
+				igt_get_assigned_primary(output, pipe);
+			int assigned_primary_index = assigned_primary->index;
+
+			/*
+			 * If the driver-assigned primary plane isn't at
+			 * the pipe->plane_primary index, swap it with
+			 * the plane that's currently at the
+			 * plane_primary index and update plane->index
+			 * accordingly.
+			 *
+			 * This way, we can preserve pipe->plane_primary
+			 * as 0 so that tests that assume
+			 * pipe->plane_primary is always 0 won't break.
+			 */
+			if (assigned_primary_index != pipe->plane_primary) {
+				assigned_primary->index = pipe->plane_primary;
+				primary->index = assigned_primary_index;
+
+				igt_swap(pipe->planes[assigned_primary_index],
+					 pipe->planes[pipe->plane_primary]);
+			}
+		}
+	}
+
+	drmModeFreeResources(resources);
+}
+
 /**
  * igt_display_require:
  * @display: a pointer to an #igt_display_t structure
  * @drm_fd: a drm file descriptor
  *
  * Initialize @display and allocate the various resources required. Use
- * #igt_display_fini to release the resources when they are no longer required.
+ * #igt_display_fini to release the resources when they are no longer
+ * required.
  *
- * This function automatically skips if the kernel driver doesn't support any
- * CRTC or outputs.
+ * This function automatically skips if the kernel driver doesn't
+ * support any CRTC or outputs.
  */
 void igt_display_require(igt_display_t *display, int drm_fd)
 {
@@ -2458,6 +2569,8 @@ void igt_display_require(igt_display_t *display, int drm_fd)
 		 */
 	}
 
+	drmModeFreePlaneResources(plane_resources);
+
 	for_each_pipe(display, i) {
 		igt_pipe_t *pipe = &display->pipes[i];
 		igt_plane_t *plane;
@@ -2556,78 +2669,11 @@ void igt_display_require(igt_display_t *display, int drm_fd)
 		pipe->n_planes = n_planes;
 	}
 
-	igt_fill_display_format_mod(display);
-
-	/*
-	 * The number of connectors is set, so we just initialize the outputs
-	 * array in _init(). This may change when we need dynamic connectors
-	 * (say DisplayPort MST).
-	 */
-	display->n_outputs = resources->count_connectors;
-	display->outputs = calloc(display->n_outputs, sizeof(igt_output_t));
-	igt_assert_f(display->outputs, "Failed to allocate memory for %d outputs\n", display->n_outputs);
-
-	for (i = 0; i < display->n_outputs; i++) {
-		igt_output_t *output = &display->outputs[i];
-		drmModeConnector *connector;
-
-		/*
-		 * We don't assign each output a pipe unless
-		 * a pipe is set with igt_output_set_pipe().
-		 */
-		output->pending_pipe = PIPE_NONE;
-		output->id = resources->connectors[i];
-		output->display = display;
-
-		igt_output_refresh(output);
-
-		connector = output->config.connector;
-		if (connector && (!connector->count_modes ||
-		    connector->connection == DRM_MODE_UNKNOWNCONNECTION)) {
-			output->force_reprobe = true;
-			igt_output_refresh(output);
-		}
-	}
-
-	drmModeFreePlaneResources(plane_resources);
 	drmModeFreeResources(resources);
 
-	/* Set reasonable default values for every object in the display. */
-	igt_display_reset(display);
-
-	for_each_pipe(display, i) {
-		igt_pipe_t *pipe = &display->pipes[i];
-		igt_output_t *output;
-
-		if (!igt_pipe_has_valid_output(display, i))
-			continue;
-
-		output = igt_get_single_output_for_pipe(display, i);
-
-		if (pipe->num_primary_planes > 1) {
-			igt_plane_t *primary = &pipe->planes[pipe->plane_primary];
-			igt_plane_t *assigned_primary = igt_get_assigned_primary(output, pipe);
-			int assigned_primary_index = assigned_primary->index;
-
-			/*
-			 * If the driver-assigned primary plane isn't at the
-			 * pipe->plane_primary index, swap it with the plane
-			 * that's currently at the plane_primary index and
-			 * update plane->index accordingly.
-			 *
-			 * This way, we can preserve pipe->plane_primary as 0
-			 * so that tests that assume pipe->plane_primary is always 0
-			 * won't break.
-			 */
-			if (assigned_primary_index != pipe->plane_primary) {
-				assigned_primary->index = pipe->plane_primary;
-				primary->index = assigned_primary_index;
+	igt_fill_display_format_mod(display);
 
-				igt_swap(pipe->planes[assigned_primary_index],
-						pipe->planes[pipe->plane_primary]);
-			}
-		}
-	}
+	igt_display_reset_outputs(display);
 
 out:
 	LOG_UNINDENT(display);
@@ -2695,17 +2741,36 @@ void igt_display_require_output_on_pipe(igt_display_t *display, enum pipe pipe)
 igt_output_t *igt_output_from_connector(igt_display_t *display,
 					drmModeConnector *connector)
 {
-	igt_output_t *output, *found = NULL;
 	int i;
+	igt_output_t *found = NULL;
 
 	for (i = 0; i < display->n_outputs; i++) {
-		output = &display->outputs[i];
+		igt_output_t *output = &display->outputs[i];
+		bool is_mst = !!output->config.connector_path;
+
+		if (is_mst) {
+			drmModePropertyBlobPtr path_blob =
+				kmstest_get_path_blob(display->drm_fd,
+						      connector->connector_id);
+			if (path_blob) {
+				bool is_same_connector =
+					strcmp(output->config.connector_path,
+					       path_blob->data) == 0;
+				drmModeFreePropertyBlob(path_blob);
+				if (is_same_connector) {
+					output->id = connector->connector_id;
+					found = output;
+					break;
+				}
+			}
 
-		if (output->config.connector &&
-		    output->config.connector->connector_id ==
-		    connector->connector_id) {
-			found = output;
-			break;
+		} else {
+			if (output->config.connector &&
+			    output->config.connector->connector_id ==
+				    connector->connector_id) {
+				found = output;
+				break;
+			}
 		}
 	}
 
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index eddfb6fc..221bcb55 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -173,6 +173,7 @@ struct kmstest_connector_config {
 
 	int pipe;
 	unsigned valid_crtc_idx_mask;
+	char *connector_path;
 };
 
 struct kmstest_plane {
@@ -451,6 +452,7 @@ typedef struct {
 	uint16_t tile_h_size, tile_v_size;
 } igt_tile_info_t;
 
+void igt_display_reset_outputs(igt_display_t *display);
 void igt_display_require(igt_display_t *display, int drm_fd);
 void igt_display_fini(igt_display_t *display);
 void igt_display_reset(igt_display_t *display);
diff --git a/tests/chamelium/kms_chamelium.c b/tests/chamelium/kms_chamelium.c
index d97b439a..0b541d34 100644
--- a/tests/chamelium/kms_chamelium.c
+++ b/tests/chamelium/kms_chamelium.c
@@ -315,8 +315,8 @@ static drmModeModeInfo get_mode_for_port(struct chamelium *chamelium,
 static igt_output_t *get_output_for_port(data_t *data,
 					 struct chamelium_port *port)
 {
-	drmModeConnector *connector = chamelium_port_get_connector(data->chamelium,
-								   port, false);
+	drmModeConnector *connector =
+		chamelium_port_get_connector(data->chamelium, port, true);
 	igt_output_t *output = igt_output_from_connector(&data->display,
 							 connector);
 	drmModeFreeConnector(connector);
@@ -406,6 +406,7 @@ test_hotplug(data_t *data, struct chamelium_port *port, int toggle_count,
 		    (modeset_mode == TEST_MODESET_ON && i == 0 )) {
 			if (i == 0) {
 				/* We can only get mode and pipe once we are connected */
+				output = get_output_for_port(data, port);
 				pipe = get_pipe_for_output(&data->display, output);
 				mode = get_mode_for_port(data->chamelium, port);
 				create_fb_for_mode(data, &fb, &mode);
@@ -2582,7 +2583,7 @@ igt_main
 		igt_display_commit2(&data.display, COMMIT_ATOMIC);
 
 		/* we need to initalize chamelium after igt_display_require */
-		data.chamelium = chamelium_init(data.drm_fd);
+		data.chamelium = chamelium_init(data.drm_fd, &data.display);
 		igt_require(data.chamelium);
 
 		data.ports = chamelium_get_ports(data.chamelium,
diff --git a/tests/chamelium/kms_color_chamelium.c b/tests/chamelium/kms_color_chamelium.c
index 678931aa..9d7765cd 100644
--- a/tests/chamelium/kms_color_chamelium.c
+++ b/tests/chamelium/kms_color_chamelium.c
@@ -680,7 +680,7 @@ igt_main
 		igt_chamelium_allow_fsm_handling = false;
 
 		/* we need to initalize chamelium after igt_display_require */
-		data.chamelium = chamelium_init(data.drm_fd);
+		data.chamelium = chamelium_init(data.drm_fd, &data.display);
 		igt_require(data.chamelium);
 
 		data.ports = chamelium_get_ports(data.chamelium,
diff --git a/tests/feature_discovery.c b/tests/feature_discovery.c
index 1978ce89..c13ccc7b 100644
--- a/tests/feature_discovery.c
+++ b/tests/feature_discovery.c
@@ -96,7 +96,8 @@ igt_main {
 #ifdef HAVE_CHAMELIUM
 		igt_describe("Make sure that Chamelium is configured and reachable.");
 		igt_subtest("chamelium") {
-			struct chamelium *chamelium = chamelium_init(fd);
+			struct chamelium *chamelium =
+				chamelium_init(fd, &display);
 			igt_require(chamelium);
 			chamelium_deinit(chamelium);
 		}
diff --git a/tests/kms_dp_tiled_display.c b/tests/kms_dp_tiled_display.c
index 8f503e40..bb638050 100644
--- a/tests/kms_dp_tiled_display.c
+++ b/tests/kms_dp_tiled_display.c
@@ -70,10 +70,11 @@ typedef struct {
 
 } data_t;
 
-void basic_test(data_t *data, drmEventContext *drm_event, struct pollfd *pfd);
+void basic_test(data_t *data, drmEventContext *drm_event, struct pollfd *pfd,
+		igt_display_t *display);
 
 #ifdef HAVE_CHAMELIUM
-static void test_with_chamelium(data_t *data);
+static void test_with_chamelium(data_t *data, igt_display_t *display);
 #endif
 
 static int drm_property_is_tile(drmModePropertyPtr prop)
@@ -388,13 +389,13 @@ static bool got_all_page_flips(data_t *data)
 }
 
 #ifdef HAVE_CHAMELIUM
-static void test_with_chamelium(data_t *data)
+static void test_with_chamelium(data_t *data, igt_display_t *display)
 {
 	int i, count = 0;
 	uint8_t htile = 2, vtile = 1;
 	struct edid **edid;
 
-	data->chamelium = chamelium_init(data->drm_fd);
+	data->chamelium = chamelium_init(data->drm_fd, display);
 	igt_require(data->chamelium);
 	data->ports = chamelium_get_ports
 		(data->chamelium, &data->port_count);
@@ -427,7 +428,8 @@ static void test_with_chamelium(data_t *data)
 }
 #endif
 
-void basic_test(data_t *data, drmEventContext *drm_event, struct pollfd *pfd)
+void basic_test(data_t *data, drmEventContext *drm_event, struct pollfd *pfd,
+		igt_display_t *display)
 {
 		int ret;
 
@@ -476,7 +478,7 @@ igt_main
 	igt_describe("Make sure the Tiled CRTCs are synchronized and we get "
 		     "page flips for all tiled CRTCs in one vblank.");
 	igt_subtest("basic-test-pattern") {
-		basic_test(&data, &drm_event, &pfd);
+		basic_test(&data, &drm_event, &pfd, &display);
 		test_cleanup(&data);
 	}
 
@@ -486,8 +488,8 @@ igt_main
 	igt_subtest_f("basic-test-pattern-with-chamelium") {
 		int i;
 
-		test_with_chamelium(&data);
-		basic_test(&data, &drm_event, &pfd);
+		test_with_chamelium(&data, &display);
+		basic_test(&data, &drm_event, &pfd, &display);
 		test_cleanup(&data);
 		for (i = 0; i < data.port_count; i++)
 			chamelium_reset_state(data.display, data.chamelium,
-- 
2.38.0.413.g74048e4d9e-goog

^ permalink raw reply related	[flat|nested] 14+ messages in thread