Chamelium port mapping auto-discovery

Set a different EDID serial string for each Chamelium port, so that we can easily tell which DRM connector maps to a Chamelium port.

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_chamelium.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

lib/igt_chamelium.c +3 -0

index 467f1a458516..dcc59e9f0b04 100644
@@ -587,6 +587,17 @@ struct chamelium_edid *chamelium_new_edid(struct chamelium *chamelium,
 const struct edid *chamelium_edid_get_raw(struct chamelium_edid *edid,
 					  struct chamelium_port *port)
 {
+	uint32_t *serial;
+
+	/* Product code: Chamelium */

Quoted 19 lines · Simon Ser

Set a different EDID serial string for each Chamelium port, so that we can easily tell which DRM connector maps to a Chamelium port.

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_chamelium.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

lib/igt_chamelium.c +3 -0

index 467f1a458516..dcc59e9f0b04 100644
@@ -587,6 +587,17 @@ struct chamelium_edid *chamelium_new_edid(struct chamelium *chamelium,
 const struct edid *chamelium_edid_get_raw(struct chamelium_edid *edid,
 					  struct chamelium_port *port)
 {
+	uint32_t *serial;
+
+	/* Product code: Chamelium */

lib/igt_chamelium.c struct chamelium_edid *chamelium_new_edid(struct chamelium *chamelium, · line 602

Quoted 10 lines

lib/igt_chamelium.c +8 -0

+	edid->raw->prod_code[0] = 'C';
+	edid->raw->prod_code[1] = 'H';
+
+	/* Serial: Chamelium port ID */
+	serial = (uint32_t *) &edid->raw->serial;
+	*serial = port->id;
+
+	edid_update_checksum(edid->raw);
 	return edid->raw;
 }

Reusing the same memory and modifying it with each call is a pitfall for consumers of this API. Either store a copy for each connector, memcpy to a user provided pointer or return a copy and make the caller responsible for free()-ing it.

I vaugely remember fixing issues caused by abuse of similar functions that modifed a static memory and returning a pointer to it.

This removes the need for configuring Chamelium ports in .igtrc, making it both easier and less error-prone to setup Chamelium boards.

A new function chamelium_autodiscover sets a different EDID on each Chamelium port and plugs them. We then walk the list of DRM connectors, read back the EDID and infer the mapping.

The EDID serial number is used to tell each port apart. In case a mapping is already configured in the .igtrc file, we check that it's consistent.

MST is not handled yet (requires refactoring existing tests since connector IDs aren't stable).

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_chamelium.c | 205 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 203 insertions(+), 2 deletions(-)

lib/igt_chamelium.c +1 -1

index dcc59e9f0b04..ddea1ba47d4e 100644
@@ -358,7 +358,7 @@ static xmlrpc_value *chamelium_rpc(struct chamelium *chamelium,
  */
 void chamelium_plug(struct chamelium *chamelium, struct chamelium_port *port)
 {
-	igt_debug("Plugging %s\n", port->name);
+	igt_debug("Plugging %s (Chamelium port ID %d)\n", port->name, port->id);
 	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "Plug", "(i)", port->id));
 }
 

@@ -1828,6 +1828,204 @@ out:
 	return ret;
 }

lib/igt_chamelium.c +54 -0

+static int port_id_from_edid(int drm_fd, drmModeConnector *connector)
+{
+	int port_id = -1;
+	bool ok;
+	uint64_t edid_blob_id;
+	drmModePropertyBlobRes *edid_blob;
+	const struct edid *edid;
+	char mfg[3];
+
+	if (connector->connection != DRM_MODE_CONNECTED) {
+		igt_debug("Skipping auto-discovery for connector %s-%d: "
+			  "connector status is not connected\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		return -1;
+	}
+
+	ok = kmstest_get_property(drm_fd, connector->connector_id,
+				  DRM_MODE_OBJECT_CONNECTOR, "EDID",
+				  NULL, &edid_blob_id, NULL);
+	if (!ok) {
+		igt_debug("Skipping auto-discovery for connector %s-%d: "
+			  "missing the EDID property\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		return -1;
+	}
+
+	edid_blob = drmModeGetPropertyBlob(drm_fd, edid_blob_id);
+	igt_assert(edid_blob);
+
+	edid = (const struct edid *) edid_blob->data;
+
+	edid_get_mfg(edid, mfg);
+	if (memcmp(mfg, "IGT", 3) != 0) {
+		igt_debug("Skipping connector %s-%d for auto-discovery: "
+			  "manufacturer is %.3s, not IGT\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id, mfg);
+		goto out;
+	}
+
+	if (edid->prod_code[0] != 'C' || edid->prod_code[1] != 'H') {
+		igt_warn("Invalid EDID for IGT connector %s-%d: "
+			  "invalid product code\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		goto out;
+	}
+
+	port_id = *(uint32_t *) &edid->serial;
+	igt_debug("Auto-discovery mapped connector %s-%d to Chamelium "
+		  "port ID %d\n",
+		  kmstest_connector_type_str(connector->connector_type),

lib/igt_chamelium.c +23 -0

+static bool chamelium_autodiscover(struct chamelium *chamelium, int drm_fd)
+{
+	drmModeRes *res;
+	drmModeConnector *connector;
+	struct chamelium_port *port;
+	size_t i, j, port_index;
+	int port_id;
+	uint32_t conn_id;
+	struct chamelium_edid *edid;
+	bool found;
+	uint32_t autodiscovered[AUTODISCOVER_PORTS_LEN] = {0};
+	char conn_name[64];
+
+	igt_debug("Starting Chamelium port auto-discovery\n");
+
+	edid = chamelium_new_edid(chamelium, igt_kms_get_base_edid());
+
+	/* Set EDID and plug ports we want to auto-discover */
+	port_index = chamelium->port_count;
+	for (i = 0; i < AUTODISCOVER_PORTS_LEN; i++) {
+		port_id = autodiscover_ports[i];
+
+		/* No need to auto-discover ports explicitly configured in the

lib/igt_chamelium.c +16 -0

+
+		igt_assert(port_index < CHAMELIUM_MAX_PORTS);
+		port = &chamelium->ports[port_index];
+		port_index++;
+
+		port->id = port_id;
+
+		chamelium_port_set_edid(chamelium, port, edid);
+		chamelium_plug(chamelium, port);
+	}
+
+	/* Reprobe connectors and build the mapping */
+	res = drmModeGetResources(drm_fd);
+	if (!res)
+		return false;
+

lib/igt_chamelium.c +5 -0

+
+		/* If we already have a mapping from the config file, check
+		 * that it's consistent. */
+		found = false;
+		for (j = 0; j < chamelium->port_count; j++) {

lib/igt_chamelium.c +48 -1

+		found = false;
+		for (j = 0; j < AUTODISCOVER_PORTS_LEN; j++) {
+			if (port_id == autodiscover_ports[j]) {
+				found = true;
+				autodiscovered[j] = conn_id;
+				break;
+			}
+		}
+		igt_assert_f(found, "Auto-discovered a port (%d) we haven't "
+			     "setup\n", port_id);
+	}
+
+	drmModeFreeResources(res);
+
+	/* We now have a Chamelium port ID ↔ DRM connector ID mapping:
+	 * autodiscover_ports contains the Chamelium port IDs and
+	 * autodiscovered contains the DRM connector IDs. */
+	for (i = 0; i < AUTODISCOVER_PORTS_LEN; i++) {
+		port_id = autodiscover_ports[i];
+		conn_id = autodiscovered[i];
+		if (!conn_id) {
+			continue;
+		}
+
+		port = &chamelium->ports[chamelium->port_count];
+		chamelium->port_count++;
+
+		port->id = port_id;
+		port->type = chamelium_get_port_type(chamelium, port);
+		port->connector_id = conn_id;
+
+		connector = drmModeGetConnectorCurrent(drm_fd, conn_id);
+		snprintf(conn_name, sizeof(conn_name), "%s-%u",
+			 kmstest_connector_type_str(connector->connector_type),
+			 connector->connector_type_id);
+		drmModeFreeConnector(connector);
+		port->name = strdup(conn_name);
+	}
+
+	chamelium_reset(chamelium);
+
+	return true;
+}
+
 static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
 {
 	GError *error = NULL;

@@ -1845,7 +2043,10 @@ static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
 		return false;
 	}
 
-	return chamelium_read_port_mappings(chamelium, drm_fd);
+	if (!chamelium_read_port_mappings(chamelium, drm_fd)) {
+		return false;
+	}
+	return chamelium_autodiscover(chamelium, drm_fd);
 }
 
 /**

Signature · 1 line

2.22.0

Quoted 35 lines · Simon Ser

This removes the need for configuring Chamelium ports in .igtrc, making it both easier and less error-prone to setup Chamelium boards.

A new function chamelium_autodiscover sets a different EDID on each Chamelium port and plugs them. We then walk the list of DRM connectors, read back the EDID and infer the mapping.

The EDID serial number is used to tell each port apart. In case a mapping is already configured in the .igtrc file, we check that it's consistent.

MST is not handled yet (requires refactoring existing tests since connector IDs aren't stable).

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_chamelium.c | 205 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 203 insertions(+), 2 deletions(-)

lib/igt_chamelium.c +1 -1

index dcc59e9f0b04..ddea1ba47d4e 100644
@@ -358,7 +358,7 @@ static xmlrpc_value *chamelium_rpc(struct chamelium *chamelium,
  */
 void chamelium_plug(struct chamelium *chamelium, struct chamelium_port *port)
 {
-	igt_debug("Plugging %s\n", port->name);
+	igt_debug("Plugging %s (Chamelium port ID %d)\n", port->name, port->id);
 	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "Plug", "(i)", port->id));
 }
 

@@ -1828,6 +1828,204 @@ out:
 	return ret;
 }
 

lib/igt_chamelium.c out: · line 1839

Quoted 9 lines

lib/igt_chamelium.c +9 -0

+/**
+ * autodiscover_ports: list of ports which can be auto-discovered
+ *
+ * See this file for a list of Chamelium ports:
+ * https://chromium.googlesource.com/chromiumos/platform/chameleon/+/master/chameleond/utils/ids.py
+ */
+static const int autodiscover_ports[] = {1, 2, 3, 4};
+
+#define AUTODISCOVER_PORTS_LEN (sizeof(autodiscover_ports) / sizeof(autodiscover_ports[0]))

We also have CHAMELIUM_MAX_PORTS which counts the number of display ports. Are we anticipating any non-autodiscoverable ports? Any other reasons to keep this separate?

Quoted 58 lines

lib/igt_chamelium.c +58 -0

+static int port_id_from_edid(int drm_fd, drmModeConnector *connector)
+{
+	int port_id = -1;
+	bool ok;
+	uint64_t edid_blob_id;
+	drmModePropertyBlobRes *edid_blob;
+	const struct edid *edid;
+	char mfg[3];
+
+	if (connector->connection != DRM_MODE_CONNECTED) {
+		igt_debug("Skipping auto-discovery for connector %s-%d: "
+			  "connector status is not connected\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		return -1;
+	}
+
+	ok = kmstest_get_property(drm_fd, connector->connector_id,
+				  DRM_MODE_OBJECT_CONNECTOR, "EDID",
+				  NULL, &edid_blob_id, NULL);
+	if (!ok) {
+		igt_debug("Skipping auto-discovery for connector %s-%d: "
+			  "missing the EDID property\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		return -1;
+	}
+
+	edid_blob = drmModeGetPropertyBlob(drm_fd, edid_blob_id);
+	igt_assert(edid_blob);
+
+	edid = (const struct edid *) edid_blob->data;
+
+	edid_get_mfg(edid, mfg);
+	if (memcmp(mfg, "IGT", 3) != 0) {
+		igt_debug("Skipping connector %s-%d for auto-discovery: "
+			  "manufacturer is %.3s, not IGT\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id, mfg);
+		goto out;
+	}
+
+	if (edid->prod_code[0] != 'C' || edid->prod_code[1] != 'H') {
+		igt_warn("Invalid EDID for IGT connector %s-%d: "
+			  "invalid product code\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		goto out;
+	}
+
+	port_id = *(uint32_t *) &edid->serial;
+	igt_debug("Auto-discovery mapped connector %s-%d to Chamelium "
+		  "port ID %d\n",
+		  kmstest_connector_type_str(connector->connector_type),
+		  connector->connector_type_id, port_id);
+
+out:
+	drmModeFreePropertyBlob(edid_blob);

lib/igt_chamelium.c out: · line 1904

Quoted 6 lines

lib/igt_chamelium.c +6 -0

+	return port_id;
+}
+
+/**
+ * chamelium_autodiscover: TODO
+ */

Are you planning to get this description done?

Quoted 27 lines

lib/igt_chamelium.c +27 -0

+static bool chamelium_autodiscover(struct chamelium *chamelium, int drm_fd)
+{
+	drmModeRes *res;
+	drmModeConnector *connector;
+	struct chamelium_port *port;
+	size_t i, j, port_index;
+	int port_id;
+	uint32_t conn_id;
+	struct chamelium_edid *edid;
+	bool found;
+	uint32_t autodiscovered[AUTODISCOVER_PORTS_LEN] = {0};
+	char conn_name[64];
+
+	igt_debug("Starting Chamelium port auto-discovery\n");
+
+	edid = chamelium_new_edid(chamelium, igt_kms_get_base_edid());
+
+	/* Set EDID and plug ports we want to auto-discover */
+	port_index = chamelium->port_count;
+	for (i = 0; i < AUTODISCOVER_PORTS_LEN; i++) {
+		port_id = autodiscover_ports[i];
+
+		/* No need to auto-discover ports explicitly configured in the
+		 * .igtrc file. */
+		found = false;
+		for (j = 0; j < chamelium->port_count; j++) {
+			if (chamelium->ports[j].id == port_id) {

lib/igt_chamelium.c out: · line 1937

Quoted 6 lines

lib/igt_chamelium.c +6 -0

+				found = true;
+				break;
+			}
+		}
+		if (found)
+			continue;

we are skipping seting edid and pluging the port if it was defined in .igtrc

Quoted 16 lines

lib/igt_chamelium.c +16 -0

+
+		igt_assert(port_index < CHAMELIUM_MAX_PORTS);
+		port = &chamelium->ports[port_index];
+		port_index++;
+
+		port->id = port_id;
+
+		chamelium_port_set_edid(chamelium, port, edid);
+		chamelium_plug(chamelium, port);
+	}
+
+	/* Reprobe connectors and build the mapping */
+	res = drmModeGetResources(drm_fd);
+	if (!res)
+		return false;
+

lib/igt_chamelium.c out: · line 1963

Quoted 10 lines

lib/igt_chamelium.c +10 -0

+	for (i = 0; i < res->count_connectors; i++) {
+		conn_id = res->connectors[i];
+
+		/* Read the EDID and parse the Chamelium port ID we stored
+		 * there. */
+		connector = drmModeGetConnector(drm_fd, res->connectors[i]);
+		port_id = port_id_from_edid(drm_fd, connector);
+		drmModeFreeConnector(connector);
+		if (port_id < 0)
+			continue;

here we won't get a sensible value and continue;

Quoted 5 lines

lib/igt_chamelium.c +5 -0

+
+		/* If we already have a mapping from the config file, check
+		 * that it's consistent. */
+		found = false;
+		for (j = 0; j < chamelium->port_count; j++) {

lib/igt_chamelium.c out: · line 1978

Quoted 10 lines

lib/igt_chamelium.c +10 -0

+			port = &chamelium->ports[j];
+			if (port->connector_id == conn_id) {
+				found = true;
+				igt_assert_f(port->id == port_id,
+					     "Inconsistency detected in .igtrc: "
+					     "connector %s is configured with "
+					     "Chamelium port %d, but is "
+					     "connected to port %d\n",
+					     port->name, port->id, port_id);
+				break;

so this whole section checking consistency seems to make no sense

Or am I reading this wrong?

lib/igt_chamelium.c out: · line 1984

Quoted 6 lines

lib/igt_chamelium.c +6 -0

+			}
+		}
+		if (found)
+			continue;
+
+		/* Get got a new mapping */

Get got?

Quoted 58 lines

lib/igt_chamelium.c +48 -1

+		found = false;
+		for (j = 0; j < AUTODISCOVER_PORTS_LEN; j++) {
+			if (port_id == autodiscover_ports[j]) {
+				found = true;
+				autodiscovered[j] = conn_id;
+				break;
+			}
+		}
+		igt_assert_f(found, "Auto-discovered a port (%d) we haven't "
+			     "setup\n", port_id);
+	}
+
+	drmModeFreeResources(res);
+
+	/* We now have a Chamelium port ID ↔ DRM connector ID mapping:
+	 * autodiscover_ports contains the Chamelium port IDs and
+	 * autodiscovered contains the DRM connector IDs. */
+	for (i = 0; i < AUTODISCOVER_PORTS_LEN; i++) {
+		port_id = autodiscover_ports[i];
+		conn_id = autodiscovered[i];
+		if (!conn_id) {
+			continue;
+		}
+
+		port = &chamelium->ports[chamelium->port_count];
+		chamelium->port_count++;
+
+		port->id = port_id;
+		port->type = chamelium_get_port_type(chamelium, port);
+		port->connector_id = conn_id;
+
+		connector = drmModeGetConnectorCurrent(drm_fd, conn_id);
+		snprintf(conn_name, sizeof(conn_name), "%s-%u",
+			 kmstest_connector_type_str(connector->connector_type),
+			 connector->connector_type_id);
+		drmModeFreeConnector(connector);
+		port->name = strdup(conn_name);
+	}
+
+	chamelium_reset(chamelium);
+
+	return true;
+}
+
 static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
 {
 	GError *error = NULL;

@@ -1845,7 +2043,10 @@ static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
 		return false;
 	}
 
-	return chamelium_read_port_mappings(chamelium, drm_fd);
+	if (!chamelium_read_port_mappings(chamelium, drm_fd)) {
+		return false;
+	}
+	return chamelium_autodiscover(chamelium, drm_fd);
 }
 

Quoted 4 lines

lib/igt_chamelium.c +0 -0

 /**

Signature · 1 line

2.22.0

Quoted 36 lines · Arkadiusz Hiler

Quoted 35 lines · Simon Ser

This removes the need for configuring Chamelium ports in .igtrc, making it both easier and less error-prone to setup Chamelium boards.

A new function chamelium_autodiscover sets a different EDID on each Chamelium port and plugs them. We then walk the list of DRM connectors, read back the EDID and infer the mapping.

The EDID serial number is used to tell each port apart. In case a mapping is already configured in the .igtrc file, we check that it's consistent.

MST is not handled yet (requires refactoring existing tests since connector IDs aren't stable).

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_chamelium.c | 205 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 203 insertions(+), 2 deletions(-)

lib/igt_chamelium.c +1 -1

index dcc59e9f0b04..ddea1ba47d4e 100644
@@ -358,7 +358,7 @@ static xmlrpc_value *chamelium_rpc(struct chamelium *chamelium,
  */
 void chamelium_plug(struct chamelium *chamelium, struct chamelium_port *port)
 {
-	igt_debug("Plugging %s\n", port->name);
+	igt_debug("Plugging %s (Chamelium port ID %d)\n", port->name, port->id);
 	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "Plug", "(i)", port->id));
 }
 

@@ -1828,6 +1828,204 @@ out:
 	return ret;
 }
 

Quoted 13 lines

lib/igt_chamelium.c out: · line 1839

Quoted 9 lines

lib/igt_chamelium.c +9 -0

+/**
+ * autodiscover_ports: list of ports which can be auto-discovered
+ *
+ * See this file for a list of Chamelium ports:
+ * https://chromium.googlesource.com/chromiumos/platform/chameleon/+/master/chameleond/utils/ids.py
+ */
+static const int autodiscover_ports[] = {1, 2, 3, 4};
+
+#define AUTODISCOVER_PORTS_LEN (sizeof(autodiscover_ports) / sizeof(autodiscover_ports[0]))

We also have CHAMELIUM_MAX_PORTS which counts the number of display ports. Are we anticipating any non-autodiscoverable ports? Any other reasons to keep this separate?

We could have a video_ports array. However I'd like not to expose it in the header, because port IDs aren't something library users should care about. Then we'd need to define the size of the array in the header and the array itself in the C file, which is annoying and error-prone.

For this reason, I'd prefer to keep CHAMELIUM_MAX_PORTS.

That said, an enum for all video ports we care about is probably a good idea. Or maybe we could just call GetSupportedInputs and HasVideoSupport to get a list of ports to auto-discover?

That would be a little bit better since the port IDs could change for a different Chamelium board.

Quoted 58 lines

Quoted 58 lines

lib/igt_chamelium.c +58 -0

+static int port_id_from_edid(int drm_fd, drmModeConnector *connector)
+{
+	int port_id = -1;
+	bool ok;
+	uint64_t edid_blob_id;
+	drmModePropertyBlobRes *edid_blob;
+	const struct edid *edid;
+	char mfg[3];
+
+	if (connector->connection != DRM_MODE_CONNECTED) {
+		igt_debug("Skipping auto-discovery for connector %s-%d: "
+			  "connector status is not connected\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		return -1;
+	}
+
+	ok = kmstest_get_property(drm_fd, connector->connector_id,
+				  DRM_MODE_OBJECT_CONNECTOR, "EDID",
+				  NULL, &edid_blob_id, NULL);
+	if (!ok) {
+		igt_debug("Skipping auto-discovery for connector %s-%d: "
+			  "missing the EDID property\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		return -1;
+	}
+
+	edid_blob = drmModeGetPropertyBlob(drm_fd, edid_blob_id);
+	igt_assert(edid_blob);
+
+	edid = (const struct edid *) edid_blob->data;
+
+	edid_get_mfg(edid, mfg);
+	if (memcmp(mfg, "IGT", 3) != 0) {
+		igt_debug("Skipping connector %s-%d for auto-discovery: "
+			  "manufacturer is %.3s, not IGT\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id, mfg);
+		goto out;
+	}
+
+	if (edid->prod_code[0] != 'C' || edid->prod_code[1] != 'H') {
+		igt_warn("Invalid EDID for IGT connector %s-%d: "
+			  "invalid product code\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		goto out;
+	}
+
+	port_id = *(uint32_t *) &edid->serial;
+	igt_debug("Auto-discovery mapped connector %s-%d to Chamelium "
+		  "port ID %d\n",
+		  kmstest_connector_type_str(connector->connector_type),
+		  connector->connector_type_id, port_id);
+
+out:
+	drmModeFreePropertyBlob(edid_blob);

Quoted 8 lines

lib/igt_chamelium.c out: · line 1904

Quoted 6 lines

lib/igt_chamelium.c +6 -0

+	return port_id;
+}
+
+/**
+ * chamelium_autodiscover: TODO
+ */

Are you planning to get this description done?

Derp

Quoted 27 lines

Quoted 27 lines

lib/igt_chamelium.c +27 -0

+static bool chamelium_autodiscover(struct chamelium *chamelium, int drm_fd)
+{
+	drmModeRes *res;
+	drmModeConnector *connector;
+	struct chamelium_port *port;
+	size_t i, j, port_index;
+	int port_id;
+	uint32_t conn_id;
+	struct chamelium_edid *edid;
+	bool found;
+	uint32_t autodiscovered[AUTODISCOVER_PORTS_LEN] = {0};
+	char conn_name[64];
+
+	igt_debug("Starting Chamelium port auto-discovery\n");
+
+	edid = chamelium_new_edid(chamelium, igt_kms_get_base_edid());
+
+	/* Set EDID and plug ports we want to auto-discover */
+	port_index = chamelium->port_count;
+	for (i = 0; i < AUTODISCOVER_PORTS_LEN; i++) {
+		port_id = autodiscover_ports[i];
+
+		/* No need to auto-discover ports explicitly configured in the
+		 * .igtrc file. */
+		found = false;
+		for (j = 0; j < chamelium->port_count; j++) {
+			if (chamelium->ports[j].id == port_id) {

Quoted 8 lines

lib/igt_chamelium.c out: · line 1937

Quoted 6 lines

lib/igt_chamelium.c +6 -0

+				found = true;
+				break;
+			}
+		}
+		if (found)
+			continue;

we are skipping seting edid and pluging the port if it was defined in .igtrc

Good catch. In fact you've been reviewing v2. I've posted v3 which fixes this issue.

Quoted 6 lines

lib/igt_chamelium.c out: · line 1943

Quoted 6 lines

lib/igt_chamelium.c +6 -0

+
+		igt_assert(port_index < CHAMELIUM_MAX_PORTS);
+		port = &chamelium->ports[port_index];
+		port_index++;
+
+		port->id = port_id;

Quoted 110 lines

Quoted 10 lines

lib/igt_chamelium.c +10 -0

+
+		chamelium_port_set_edid(chamelium, port, edid);
+		chamelium_plug(chamelium, port);
+	}
+
+	/* Reprobe connectors and build the mapping */
+	res = drmModeGetResources(drm_fd);
+	if (!res)
+		return false;
+

lib/igt_chamelium.c out: · line 1963

Quoted 10 lines

lib/igt_chamelium.c +10 -0

+	for (i = 0; i < res->count_connectors; i++) {
+		conn_id = res->connectors[i];
+
+		/* Read the EDID and parse the Chamelium port ID we stored
+		 * there. */
+		connector = drmModeGetConnector(drm_fd, res->connectors[i]);
+		port_id = port_id_from_edid(drm_fd, connector);
+		drmModeFreeConnector(connector);
+		if (port_id < 0)
+			continue;

here we won't get a sensible value and continue;

Quoted 5 lines

lib/igt_chamelium.c +5 -0

+
+		/* If we already have a mapping from the config file, check
+		 * that it's consistent. */
+		found = false;
+		for (j = 0; j < chamelium->port_count; j++) {

lib/igt_chamelium.c out: · line 1978

Quoted 10 lines

lib/igt_chamelium.c +10 -0

+			port = &chamelium->ports[j];
+			if (port->connector_id == conn_id) {
+				found = true;
+				igt_assert_f(port->id == port_id,
+					     "Inconsistency detected in .igtrc: "
+					     "connector %s is configured with "
+					     "Chamelium port %d, but is "
+					     "connected to port %d\n",
+					     port->name, port->id, port_id);
+				break;

so this whole section checking consistency seems to make no sense

Or am I reading this wrong?

lib/igt_chamelium.c out: · line 1984

Quoted 6 lines

lib/igt_chamelium.c +6 -0

+			}
+		}
+		if (found)
+			continue;
+
+		/* Get got a new mapping */

Get got?

Quoted 58 lines

lib/igt_chamelium.c +48 -1

+		found = false;
+		for (j = 0; j < AUTODISCOVER_PORTS_LEN; j++) {
+			if (port_id == autodiscover_ports[j]) {
+				found = true;
+				autodiscovered[j] = conn_id;
+				break;
+			}
+		}
+		igt_assert_f(found, "Auto-discovered a port (%d) we haven't "
+			     "setup\n", port_id);
+	}
+
+	drmModeFreeResources(res);
+
+	/* We now have a Chamelium port ID ↔ DRM connector ID mapping:
+	 * autodiscover_ports contains the Chamelium port IDs and
+	 * autodiscovered contains the DRM connector IDs. */
+	for (i = 0; i < AUTODISCOVER_PORTS_LEN; i++) {
+		port_id = autodiscover_ports[i];
+		conn_id = autodiscovered[i];
+		if (!conn_id) {
+			continue;
+		}
+
+		port = &chamelium->ports[chamelium->port_count];
+		chamelium->port_count++;
+
+		port->id = port_id;
+		port->type = chamelium_get_port_type(chamelium, port);
+		port->connector_id = conn_id;
+
+		connector = drmModeGetConnectorCurrent(drm_fd, conn_id);
+		snprintf(conn_name, sizeof(conn_name), "%s-%u",
+			 kmstest_connector_type_str(connector->connector_type),
+			 connector->connector_type_id);
+		drmModeFreeConnector(connector);
+		port->name = strdup(conn_name);
+	}
+
+	chamelium_reset(chamelium);
+
+	return true;
+}
+
 static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
 {
 	GError *error = NULL;

@@ -1845,7 +2043,10 @@ static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
 		return false;
 	}
 
-	return chamelium_read_port_mappings(chamelium, drm_fd);
+	if (!chamelium_read_port_mappings(chamelium, drm_fd)) {
+		return false;
+	}
+	return chamelium_autodiscover(chamelium, drm_fd);
 }
 

Quoted 4 lines

Quoted 4 lines

lib/igt_chamelium.c +0 -0

 /**

Signature · 1 line

2.22.0

Quoted 56 lines · Simon

Quoted 36 lines · Arkadiusz Hiler

Quoted 35 lines · Simon Ser

This removes the need for configuring Chamelium ports in .igtrc, making it both easier and less error-prone to setup Chamelium boards.

A new function chamelium_autodiscover sets a different EDID on each Chamelium port and plugs them. We then walk the list of DRM connectors, read back the EDID and infer the mapping.

The EDID serial number is used to tell each port apart. In case a mapping is already configured in the .igtrc file, we check that it's consistent.

MST is not handled yet (requires refactoring existing tests since connector IDs aren't stable).

Signed-off-by Simon Ser <[email protected]>

---
 lib/igt_chamelium.c | 205 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 203 insertions(+), 2 deletions(-)

lib/igt_chamelium.c +1 -1

index dcc59e9f0b04..ddea1ba47d4e 100644
@@ -358,7 +358,7 @@ static xmlrpc_value *chamelium_rpc(struct chamelium *chamelium,
  */
 void chamelium_plug(struct chamelium *chamelium, struct chamelium_port *port)
 {
-	igt_debug("Plugging %s\n", port->name);
+	igt_debug("Plugging %s (Chamelium port ID %d)\n", port->name, port->id);
 	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "Plug", "(i)", port->id));
 }
 

@@ -1828,6 +1828,204 @@ out:
 	return ret;
 }
 

Quoted 13 lines

lib/igt_chamelium.c out: · line 1839

Quoted 9 lines

lib/igt_chamelium.c +9 -0

+/**
+ * autodiscover_ports: list of ports which can be auto-discovered
+ *
+ * See this file for a list of Chamelium ports:
+ * https://chromium.googlesource.com/chromiumos/platform/chameleon/+/master/chameleond/utils/ids.py
+ */
+static const int autodiscover_ports[] = {1, 2, 3, 4};
+
+#define AUTODISCOVER_PORTS_LEN (sizeof(autodiscover_ports) / sizeof(autodiscover_ports[0]))

We also have CHAMELIUM_MAX_PORTS which counts the number of display ports. Are we anticipating any non-autodiscoverable ports? Any other reasons to keep this separate?

We could have a video_ports array. However I'd like not to expose it in the header, because port IDs aren't something library users should care about. Then we'd need to define the size of the array in the header and the array itself in the C file, which is annoying and error-prone.

Quoted 1 line

For this reason, I'd prefer to keep CHAMELIUM_MAX_PORTS.

There are still ways of hiding it behind extern or a function, but I am not sure the added complexity is worth it.

Anyway, no strong feeling on this one, just seems a bit like a duplication.

Quoted 6 lines

That said, an enum for all video ports we care about is probably a good idea. Or maybe we could just call GetSupportedInputs and HasVideoSupport to get a list of ports to auto-discover?

That would be a little bit better since the port IDs could change for a different Chamelium board.

Sounds ok

Quoted 105 lines

Quoted 58 lines

Quoted 58 lines

lib/igt_chamelium.c +58 -0

+static int port_id_from_edid(int drm_fd, drmModeConnector *connector)
+{
+	int port_id = -1;
+	bool ok;
+	uint64_t edid_blob_id;
+	drmModePropertyBlobRes *edid_blob;
+	const struct edid *edid;
+	char mfg[3];
+
+	if (connector->connection != DRM_MODE_CONNECTED) {
+		igt_debug("Skipping auto-discovery for connector %s-%d: "
+			  "connector status is not connected\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		return -1;
+	}
+
+	ok = kmstest_get_property(drm_fd, connector->connector_id,
+				  DRM_MODE_OBJECT_CONNECTOR, "EDID",
+				  NULL, &edid_blob_id, NULL);
+	if (!ok) {
+		igt_debug("Skipping auto-discovery for connector %s-%d: "
+			  "missing the EDID property\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		return -1;
+	}
+
+	edid_blob = drmModeGetPropertyBlob(drm_fd, edid_blob_id);
+	igt_assert(edid_blob);
+
+	edid = (const struct edid *) edid_blob->data;
+
+	edid_get_mfg(edid, mfg);
+	if (memcmp(mfg, "IGT", 3) != 0) {
+		igt_debug("Skipping connector %s-%d for auto-discovery: "
+			  "manufacturer is %.3s, not IGT\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id, mfg);
+		goto out;
+	}
+
+	if (edid->prod_code[0] != 'C' || edid->prod_code[1] != 'H') {
+		igt_warn("Invalid EDID for IGT connector %s-%d: "
+			  "invalid product code\n",
+			  kmstest_connector_type_str(connector->connector_type),
+			  connector->connector_type_id);
+		goto out;
+	}
+
+	port_id = *(uint32_t *) &edid->serial;
+	igt_debug("Auto-discovery mapped connector %s-%d to Chamelium "
+		  "port ID %d\n",
+		  kmstest_connector_type_str(connector->connector_type),
+		  connector->connector_type_id, port_id);
+
+out:
+	drmModeFreePropertyBlob(edid_blob);

Quoted 8 lines

lib/igt_chamelium.c out: · line 1904

Quoted 6 lines

lib/igt_chamelium.c +6 -0

+	return port_id;
+}
+
+/**
+ * chamelium_autodiscover: TODO
+ */

Are you planning to get this description done?

Derp

Quoted 27 lines

Quoted 27 lines

lib/igt_chamelium.c +27 -0

+static bool chamelium_autodiscover(struct chamelium *chamelium, int drm_fd)
+{
+	drmModeRes *res;
+	drmModeConnector *connector;
+	struct chamelium_port *port;
+	size_t i, j, port_index;
+	int port_id;
+	uint32_t conn_id;
+	struct chamelium_edid *edid;
+	bool found;
+	uint32_t autodiscovered[AUTODISCOVER_PORTS_LEN] = {0};
+	char conn_name[64];
+
+	igt_debug("Starting Chamelium port auto-discovery\n");
+
+	edid = chamelium_new_edid(chamelium, igt_kms_get_base_edid());
+
+	/* Set EDID and plug ports we want to auto-discover */
+	port_index = chamelium->port_count;
+	for (i = 0; i < AUTODISCOVER_PORTS_LEN; i++) {
+		port_id = autodiscover_ports[i];
+
+		/* No need to auto-discover ports explicitly configured in the
+		 * .igtrc file. */
+		found = false;
+		for (j = 0; j < chamelium->port_count; j++) {
+			if (chamelium->ports[j].id == port_id) {

Quoted 8 lines

lib/igt_chamelium.c out: · line 1937

Quoted 6 lines

lib/igt_chamelium.c +6 -0

+				found = true;
+				break;
+			}
+		}
+		if (found)
+			continue;

we are skipping seting edid and pluging the port if it was defined in .igtrc

Quoted 2 lines

Good catch. In fact you've been reviewing v2. I've posted v3 which fixes this issue.

Ah, right. I have read v3 now and it looks ok. Thanks!

Quoted 6 lines

Quoted 6 lines

lib/igt_chamelium.c out: · line 1943

Quoted 6 lines

lib/igt_chamelium.c +6 -0

+
+		igt_assert(port_index < CHAMELIUM_MAX_PORTS);
+		port = &chamelium->ports[port_index];
+		port_index++;
+
+		port->id = port_id;

One more nit:

Naming port_index, port, port->id and port_id. We also have 3 ways of iterating over ports using i, j and port_index. While reading v3 I found it as hard to follow as when I was reading v2

streamlinining it a bit would be nice

Quoted 110 lines

Quoted 110 lines

Quoted 10 lines

lib/igt_chamelium.c +10 -0

+
+		chamelium_port_set_edid(chamelium, port, edid);
+		chamelium_plug(chamelium, port);
+	}
+
+	/* Reprobe connectors and build the mapping */
+	res = drmModeGetResources(drm_fd);
+	if (!res)
+		return false;
+

lib/igt_chamelium.c out: · line 1963

Quoted 10 lines

lib/igt_chamelium.c +10 -0

+	for (i = 0; i < res->count_connectors; i++) {
+		conn_id = res->connectors[i];
+
+		/* Read the EDID and parse the Chamelium port ID we stored
+		 * there. */
+		connector = drmModeGetConnector(drm_fd, res->connectors[i]);
+		port_id = port_id_from_edid(drm_fd, connector);
+		drmModeFreeConnector(connector);
+		if (port_id < 0)
+			continue;

here we won't get a sensible value and continue;

Quoted 5 lines

lib/igt_chamelium.c +5 -0

+
+		/* If we already have a mapping from the config file, check
+		 * that it's consistent. */
+		found = false;
+		for (j = 0; j < chamelium->port_count; j++) {

lib/igt_chamelium.c out: · line 1978

Quoted 10 lines

lib/igt_chamelium.c +10 -0

+			port = &chamelium->ports[j];
+			if (port->connector_id == conn_id) {
+				found = true;
+				igt_assert_f(port->id == port_id,
+					     "Inconsistency detected in .igtrc: "
+					     "connector %s is configured with "
+					     "Chamelium port %d, but is "
+					     "connected to port %d\n",
+					     port->name, port->id, port_id);
+				break;

so this whole section checking consistency seems to make no sense

Or am I reading this wrong?

lib/igt_chamelium.c out: · line 1984

Quoted 6 lines

lib/igt_chamelium.c +6 -0

+			}
+		}
+		if (found)
+			continue;
+
+		/* Get got a new mapping */

Get got?

Quoted 58 lines

lib/igt_chamelium.c +48 -1

+		found = false;
+		for (j = 0; j < AUTODISCOVER_PORTS_LEN; j++) {
+			if (port_id == autodiscover_ports[j]) {
+				found = true;
+				autodiscovered[j] = conn_id;
+				break;
+			}
+		}
+		igt_assert_f(found, "Auto-discovered a port (%d) we haven't "
+			     "setup\n", port_id);
+	}
+
+	drmModeFreeResources(res);
+
+	/* We now have a Chamelium port ID ↔ DRM connector ID mapping:
+	 * autodiscover_ports contains the Chamelium port IDs and
+	 * autodiscovered contains the DRM connector IDs. */
+	for (i = 0; i < AUTODISCOVER_PORTS_LEN; i++) {
+		port_id = autodiscover_ports[i];
+		conn_id = autodiscovered[i];
+		if (!conn_id) {
+			continue;
+		}
+
+		port = &chamelium->ports[chamelium->port_count];
+		chamelium->port_count++;
+
+		port->id = port_id;
+		port->type = chamelium_get_port_type(chamelium, port);
+		port->connector_id = conn_id;
+
+		connector = drmModeGetConnectorCurrent(drm_fd, conn_id);
+		snprintf(conn_name, sizeof(conn_name), "%s-%u",
+			 kmstest_connector_type_str(connector->connector_type),
+			 connector->connector_type_id);
+		drmModeFreeConnector(connector);
+		port->name = strdup(conn_name);
+	}
+
+	chamelium_reset(chamelium);
+
+	return true;
+}
+
 static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
 {
 	GError *error = NULL;

@@ -1845,7 +2043,10 @@ static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
 		return false;
 	}
 
-	return chamelium_read_port_mappings(chamelium, drm_fd);
+	if (!chamelium_read_port_mappings(chamelium, drm_fd)) {
+		return false;
+	}
+	return chamelium_autodiscover(chamelium, drm_fd);
 }
 

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

lib/igt_chamelium.c +0 -0

 /**

Signature · 1 line

2.22.0

== Series Details ==

Series: Chamelium port mapping auto-discovery (rev2)
URL : https://patchwork.freedesktop.org/series/62393/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6312_full -> IGTPW_3180_full
====================================================

Summary
-------

**FAILURE**

Serious unknown changes coming with IGTPW_3180_full absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in IGTPW_3180_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

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

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

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

### IGT changes ###

#### Issues hit ####

* igt@gem_ctx_switch@basic-queue-heavy:
- shard-glk: [PASS][1] -> [INCOMPLETE][2] ([fdo#103359] / [k.org#198133])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6312/shard-glk5/igt@[email protected]
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-glk4/igt@[email protected]

* igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing:
- shard-apl: [PASS][3] -> [DMESG-WARN][4] ([fdo#110913 ]) +2 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6312/shard-apl8/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-apl7/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html

* igt@gem_workarounds@suspend-resume:
- shard-apl: [PASS][5] -> [DMESG-WARN][6] ([fdo#108566]) +3 similar issues
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6312/shard-apl7/igt@[email protected]
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-apl6/igt@[email protected]

* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
- shard-apl: [PASS][7] -> [FAIL][8] ([fdo#103167])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6312/shard-apl4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-apl8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite.html

#### Possible fixes ####

* igt@gem_userptr_blits@map-fixed-invalidate-busy-gup:
- shard-apl: [DMESG-WARN][9] ([fdo#110913 ]) -> [PASS][10] +2 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6312/shard-apl3/igt@[email protected]
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-apl5/igt@[email protected]

* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- shard-apl: [DMESG-WARN][11] ([fdo#108566]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6312/shard-apl5/igt@[email protected]
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-apl5/igt@[email protected]

[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
[fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
[fdo#110913 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110913
[k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133

Participating hosts (10 -> 2)
------------------------------

ERROR: It appears as if the changes made in IGTPW_3180_full prevented too many machines from booting.

Missing (8): shard-skl pig-hsw-4770r pig-glk-j5005 shard-iclb pig-skl-6260u shard-hsw shard-kbl shard-snb

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

* IGT: IGT_5061 -> IGTPW_3180
* Piglit: piglit_4509 -> None

CI_DRM_6312: 034e3ac6a2d180d188da927388b60c7e62c5655b @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_3180: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/
IGT_5061: c88ced79a7b71aec58f1d9c5c599ac2f431bcf7a @ 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_3180/