[igt-dev] [PATCH i-g-t v2 0/9] Chamelium port mapping auto-discovery

[igt-dev] [PATCH i-g-t v2 0/9] Chamelium port mapping auto-discovery

[Simon Ser]

6 words: No More Manual Chamelium Port Configuration!

<insert Petri-style advertizing saying how awesome this patchset is>

This patchset adds auto-discovery of the mapping between Chamelium port IDs and
DRM connectors. This is a new step performed on initialization. Per-port EDIDs
are used to recognize connectors.

In case a static .igtrc mapping is set up, igt_chamelium now checks that it's
not mis-configured.

MST support is still a TODO.

Simon Ser (9):
  lib/igt_edid: add edid_get_size
  lib/igt_chamelium: fix chamelium_port_set_edid docs
  lib/igt_chamelium: allow EDIDs to be mutated for each port
  lib/igt_chamelium: split chmelium_new_edid
  lib/igt_chamelium: add CHAMELIUM_MAX_PORTS
  lib/igt_chamelium: upload one EDID per port
  lib/igt_chamelium: set EDID serial
  lib/igt_edid: add edid_get_mfg
  lib/igt_chamelium: autodiscover Chamelium port mappings

 lib/igt_chamelium.c   | 313 ++++++++++++++++++++++++++++++++++++++----
 lib/igt_chamelium.h   |   9 ++
 lib/igt_edid.c        |  23 ++++
 lib/igt_edid.h        |   2 +
 tests/kms_chamelium.c |  17 ++-
 5 files changed, 327 insertions(+), 37 deletions(-)

--
2.22.0

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

[igt-dev] [PATCH i-g-t v2 1/9] lib/igt_edid: add edid_get_size

[Simon Ser]

This is a simple helper to get the size in bytes of an arbitrary EDID.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_chamelium.c |  6 ++----
 lib/igt_edid.c      | 10 ++++++++++
 lib/igt_edid.h      |  1 +
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index b83ff395d44b..966d78dce146 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -538,12 +538,10 @@ struct chamelium_edid *chamelium_new_edid(struct chamelium *chamelium,
 	xmlrpc_value *res;
 	struct chamelium_edid *chamelium_edid;
 	int edid_id;
-	struct edid *edid = (struct edid *) raw_edid;
-	size_t edid_size = sizeof(struct edid) +
-			   edid->extensions_len * sizeof(struct edid_ext);
+	const struct edid *edid = (struct edid *) raw_edid;
 
 	res = chamelium_rpc(chamelium, NULL, "CreateEdid", "(6)",
-			    raw_edid, edid_size);
+			    raw_edid, edid_get_size(edid));
 
 	xmlrpc_read_int(&chamelium->env, res, &edid_id);
 	xmlrpc_DECREF(res);
diff --git a/lib/igt_edid.c b/lib/igt_edid.c
index e71136f48e14..6cc5e7dd42c4 100644
--- a/lib/igt_edid.c
+++ b/lib/igt_edid.c
@@ -274,6 +274,16 @@ void edid_update_checksum(struct edid *edid)
 					  sizeof(struct edid));
 }
 
+/**
+ * edid_get_size: return the size of the EDID block in bytes including EDID
+ * extensions, if any.
+ */
+size_t edid_get_size(const struct edid *edid)
+{
+	return sizeof(struct edid) +
+	       edid->extensions_len * sizeof(struct edid_ext);
+}
+
 /**
  * cea_sad_init_pcm:
  * @channels: the number of supported channels (max. 8)
diff --git a/lib/igt_edid.h b/lib/igt_edid.h
index 00596ef1a46f..8d8e30ec0554 100644
--- a/lib/igt_edid.h
+++ b/lib/igt_edid.h
@@ -297,6 +297,7 @@ struct edid {
 void edid_init(struct edid *edid);
 void edid_init_with_mode(struct edid *edid, drmModeModeInfo *mode);
 void edid_update_checksum(struct edid *edid);
+size_t edid_get_size(const struct edid *edid);
 void detailed_timing_set_mode(struct detailed_timing *dt, drmModeModeInfo *mode,
 			      int width_mm, int height_mm);
 void detailed_timing_set_monitor_range_mode(struct detailed_timing *dt,
-- 
2.22.0

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

[igt-dev] [PATCH i-g-t v2 2/9] lib/igt_chamelium: fix chamelium_port_set_edid docs

[Simon Ser]

There were two issues:

1. The documentation was wrong, the EDID ID 0 doesn't disable EDIDs, it just
   resets the EDID to Chamelium's default one.
2. My previous patch updating these docs missed the second line of the argument
   description. The result was that only reading the first line was fine, but
   reading both lines felt weird.

Signed-off-by: Simon Ser <simon.ser@intel.com>
Fixes: 1f67ee0d09d6 ("lib/igt_chamelium: replace EDID IDs with opaque structs")
---
 lib/igt_chamelium.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index 966d78dce146..4a3f64b3585d 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -564,21 +564,22 @@ static void chamelium_destroy_edid(struct chamelium *chamelium, int edid_id)
  * chamelium_port_set_edid:
  * @chamelium: The Chamelium instance to use
  * @port: The port on the Chamelium to set the EDID on
- * @edid: The Chamelium EDID to set
- * #chamelium_new_edid, or 0 to disable the EDID on the port
+ * @edid: The Chamelium EDID to set or NULL to use the default Chamelium EDID
  *
  * Sets a port on the chamelium to use the specified EDID. This does not fire a
  * hotplug pulse on it's own, and merely changes what EDID the chamelium port
  * will report to us the next time we probe it. Users will need to reprobe the
  * connectors themselves if they want to see the EDID reported by the port
  * change.
+ *
+ * To create an EDID, see #chamelium_new_edid.
  */
 void chamelium_port_set_edid(struct chamelium *chamelium,
 			     struct chamelium_port *port,
 			     struct chamelium_edid *edid)
 {
 	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "ApplyEdid", "(ii)",
-				    port->id, edid->id));
+				    port->id, edid ? edid->id : 0));
 }
 
 /**
-- 
2.22.0

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

[igt-dev] [PATCH i-g-t v2 3/9] lib/igt_chamelium: allow EDIDs to be mutated for each port

[Simon Ser]

This adds the infrastructure necessary to change EDIDs provided to
chamelium_new_edid, without actually doing it yet. This commit just updates the
API to make it possible, documents expectations and updates callers
accordingly. Mutating EDIDs is necessary to add an identifier to them (e.g. by
adding a serial number) and to be able to map Chamelium ports to DRM
connectors.

A new function chamelium_edid_get_raw allows callers to retrieve the exact EDID
used for a particular port.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_chamelium.c   | 30 ++++++++++++++++++++++++++++--
 lib/igt_chamelium.h   |  2 ++
 tests/kms_chamelium.c |  7 +++++--
 3 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index 4a3f64b3585d..9ea483eee717 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -83,6 +83,7 @@
 
 struct chamelium_edid {
 	int id;
+	struct edid *raw;
 	struct igt_list link;
 };
 
@@ -530,6 +531,11 @@ void chamelium_schedule_hpd_toggle(struct chamelium *chamelium,
  * Uploads and registers a new EDID with the chamelium. The EDID will be
  * destroyed automatically when #chamelium_deinit is called.
  *
+ * Callers shouldn't assume that the raw EDID they provide is uploaded as-is to
+ * the Chamelium. The EDID may be mutated (e.g. a serial number can be appended
+ * to be able to uniquely identify the EDID). To retrieve the exact EDID that
+ * will be applied to a particular port, use #chamelium_edid_get_raw.
+ *
  * Returns: An opaque pointer to the Chamelium EDID
  */
 struct chamelium_edid *chamelium_new_edid(struct chamelium *chamelium,
@@ -539,16 +545,18 @@ struct chamelium_edid *chamelium_new_edid(struct chamelium *chamelium,
 	struct chamelium_edid *chamelium_edid;
 	int edid_id;
 	const struct edid *edid = (struct edid *) raw_edid;
+	size_t edid_size = edid_get_size(edid);
 
 	res = chamelium_rpc(chamelium, NULL, "CreateEdid", "(6)",
-			    raw_edid, edid_get_size(edid));
+			    raw_edid, edid_size);
 
 	xmlrpc_read_int(&chamelium->env, res, &edid_id);
 	xmlrpc_DECREF(res);
 
 	chamelium_edid = calloc(1, sizeof(struct chamelium_edid));
 	chamelium_edid->id = edid_id;
-
+	chamelium_edid->raw = malloc(edid_size);
+	memcpy(chamelium_edid->raw, raw_edid, edid_size);
 	igt_list_add(&chamelium_edid->link, &chamelium->edids);
 
 	return chamelium_edid;
@@ -560,6 +568,23 @@ static void chamelium_destroy_edid(struct chamelium *chamelium, int edid_id)
 				    edid_id));
 }
 
+/**
+ * chamelium_edid_get_raw: get the raw EDID
+ * @edid: the Chamelium EDID
+ * @port: the Chamelium port
+ *
+ * The EDID provided to #chamelium_new_edid may be mutated for identification
+ * purposes. This function allows to retrieve the exact EDID that will be set
+ * for a given port.
+ *
+ * The returned raw EDID is only valid until the next call to this function.
+ */
+const struct edid *chamelium_edid_get_raw(struct chamelium_edid *edid,
+					  struct chamelium_port *port)
+{
+	return edid->raw;
+}
+
 /**
  * chamelium_port_set_edid:
  * @chamelium: The Chamelium instance to use
@@ -1898,6 +1923,7 @@ void chamelium_deinit(struct chamelium *chamelium)
 	/* Destroy any EDIDs we created to make sure we don't leak them */
 	igt_list_for_each_safe(pos, tmp, &chamelium->edids, link) {
 		chamelium_destroy_edid(chamelium, pos->id);
+		free(pos->raw);
 		free(pos);
 	}
 
diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
index ce9e9ced75d9..f58e4f1f0c75 100644
--- a/lib/igt_chamelium.h
+++ b/lib/igt_chamelium.h
@@ -102,6 +102,8 @@ void chamelium_schedule_hpd_toggle(struct chamelium *chamelium,
 				   bool rising_edge);
 struct chamelium_edid *chamelium_new_edid(struct chamelium *chamelium,
 					  const unsigned char *edid);
+const struct edid *chamelium_edid_get_raw(struct chamelium_edid *edid,
+					  struct chamelium_port *port);
 void chamelium_port_set_edid(struct chamelium *chamelium,
 			     struct chamelium_port *port,
 			     struct chamelium_edid *edid);
diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index 378024d8fd05..175e5032c973 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -276,9 +276,10 @@ static void
 test_edid_read(data_t *data, struct chamelium_port *port, enum test_edid edid)
 {
 	drmModePropertyBlobPtr edid_blob = NULL;
-	const unsigned char *raw_edid = get_edid(edid);
 	drmModeConnector *connector = chamelium_port_get_connector(
 	    data->chamelium, port, false);
+	size_t raw_edid_size;
+	const struct edid *raw_edid;
 	uint64_t edid_blob_id;
 
 	reset_state(data, port);
@@ -295,7 +296,9 @@ test_edid_read(data_t *data, struct chamelium_port *port, enum test_edid edid)
 	igt_assert(edid_blob = drmModeGetPropertyBlob(data->drm_fd,
 						      edid_blob_id));
 
-	igt_assert(memcmp(raw_edid, edid_blob->data, EDID_LENGTH) == 0);
+	raw_edid = chamelium_edid_get_raw(data->edids[edid], port);
+	raw_edid_size = edid_get_size(raw_edid);
+	igt_assert(memcmp(raw_edid, edid_blob->data, raw_edid_size) == 0);
 
 	drmModeFreePropertyBlob(edid_blob);
 	drmModeFreeConnector(connector);
-- 
2.22.0

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

[igt-dev] [PATCH i-g-t v2 4/9] lib/igt_chamelium: split chmelium_new_edid

[Simon Ser]

Split the part that uploads an EDID to the Chamelium board into a new
chamelium_upload_edid function. The function will be called in
chamelium_set_edid instead of chamelium_new_edid when automatic Chamelium port
mapping is implemented.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_chamelium.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index 9ea483eee717..8e6db445330e 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -523,6 +523,26 @@ void chamelium_schedule_hpd_toggle(struct chamelium *chamelium,
 				    "(iii)", port->id, delay_ms, rising_edge));
 }
 
+static int chamelium_upload_edid(struct chamelium *chamelium,
+				 const struct edid *edid)
+{
+	xmlrpc_value *res;
+	int edid_id;
+
+	res = chamelium_rpc(chamelium, NULL, "CreateEdid", "(6)",
+			    edid, edid_get_size(edid));
+	xmlrpc_read_int(&chamelium->env, res, &edid_id);
+	xmlrpc_DECREF(res);
+
+	return edid_id;
+}
+
+static void chamelium_destroy_edid(struct chamelium *chamelium, int edid_id)
+{
+	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "DestroyEdid", "(i)",
+				    edid_id));
+}
+
 /**
  * chamelium_new_edid:
  * @chamelium: The Chamelium instance to use
@@ -541,33 +561,22 @@ void chamelium_schedule_hpd_toggle(struct chamelium *chamelium,
 struct chamelium_edid *chamelium_new_edid(struct chamelium *chamelium,
 					  const unsigned char *raw_edid)
 {
-	xmlrpc_value *res;
 	struct chamelium_edid *chamelium_edid;
 	int edid_id;
 	const struct edid *edid = (struct edid *) raw_edid;
 	size_t edid_size = edid_get_size(edid);
 
-	res = chamelium_rpc(chamelium, NULL, "CreateEdid", "(6)",
-			    raw_edid, edid_size);
-
-	xmlrpc_read_int(&chamelium->env, res, &edid_id);
-	xmlrpc_DECREF(res);
+	edid_id = chamelium_upload_edid(chamelium, edid);
 
 	chamelium_edid = calloc(1, sizeof(struct chamelium_edid));
 	chamelium_edid->id = edid_id;
 	chamelium_edid->raw = malloc(edid_size);
-	memcpy(chamelium_edid->raw, raw_edid, edid_size);
+	memcpy(chamelium_edid->raw, edid, edid_get_size(edid));
 	igt_list_add(&chamelium_edid->link, &chamelium->edids);
 
 	return chamelium_edid;
 }
 
-static void chamelium_destroy_edid(struct chamelium *chamelium, int edid_id)
-{
-	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "DestroyEdid", "(i)",
-				    edid_id));
-}
-
 /**
  * chamelium_edid_get_raw: get the raw EDID
  * @edid: the Chamelium EDID
-- 
2.22.0

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

[igt-dev] [PATCH i-g-t v2 5/9] lib/igt_chamelium: add CHAMELIUM_MAX_PORTS

[Simon Ser]

We will always be able to find an upper bound for the number of connectors
supported by a Chamelium board. Instead of using dynamic arrays, simplify the
code by using static ones.

More code will need to have arrays of ports in the future.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_chamelium.c | 7 ++-----
 lib/igt_chamelium.h | 7 +++++++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index 8e6db445330e..1b5566f42f44 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -120,7 +120,7 @@ struct chamelium {
 	int drm_fd;
 
 	struct igt_list edids;
-	struct chamelium_port *ports;
+	struct chamelium_port ports[CHAMELIUM_MAX_PORTS];
 	int port_count;
 };
 
@@ -1738,11 +1738,9 @@ static bool chamelium_read_port_mappings(struct chamelium *chamelium,
 		if (strstr(group_list[i], "Chamelium:"))
 			chamelium->port_count++;
 	}
+	igt_assert(chamelium->port_count <= CHAMELIUM_MAX_PORTS);
 
-	chamelium->ports = calloc(sizeof(struct chamelium_port),
-				  chamelium->port_count);
 	port_i = 0;
-
 	for (i = 0; group_list[i] != NULL; i++) {
 		group = group_list[i];
 
@@ -1942,7 +1940,6 @@ void chamelium_deinit(struct chamelium *chamelium)
 	for (i = 0; i < chamelium->port_count; i++)
 		free(chamelium->ports[i].name);
 
-	free(chamelium->ports);
 	free(chamelium);
 }
 
diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h
index f58e4f1f0c75..c034d72c8910 100644
--- a/lib/igt_chamelium.h
+++ b/lib/igt_chamelium.h
@@ -61,6 +61,13 @@ struct chamelium_audio_file {
 
 struct chamelium_edid;
 
+/**
+ * CHAMELIUM_MAX_PORTS: the maximum number of ports supported by igt_chamelium.
+ *
+ * For now, we have 1 VGA, 1 HDMI and 2 DisplayPort ports.
+ */
+#define CHAMELIUM_MAX_PORTS 4
+
 /**
  * CHAMELIUM_DEFAULT_EDID: provide this ID to #chamelium_port_set_edid to use
  * the default EDID.
-- 
2.22.0

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

[igt-dev] [PATCH i-g-t v2 6/9] lib/igt_chamelium: upload one EDID per port

[Simon Ser]

Instead of uploading the EDID in chamelium_new_edid, do it in
chamelium_port_set_edid so that we can customize the EDID depending on the
port.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_chamelium.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index 1b5566f42f44..467f1a458516 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -82,8 +82,8 @@
  */
 
 struct chamelium_edid {
-	int id;
 	struct edid *raw;
+	int ids[CHAMELIUM_MAX_PORTS];
 	struct igt_list link;
 };
 
@@ -562,14 +562,10 @@ struct chamelium_edid *chamelium_new_edid(struct chamelium *chamelium,
 					  const unsigned char *raw_edid)
 {
 	struct chamelium_edid *chamelium_edid;
-	int edid_id;
 	const struct edid *edid = (struct edid *) raw_edid;
 	size_t edid_size = edid_get_size(edid);
 
-	edid_id = chamelium_upload_edid(chamelium, edid);
-
 	chamelium_edid = calloc(1, sizeof(struct chamelium_edid));
-	chamelium_edid->id = edid_id;
 	chamelium_edid->raw = malloc(edid_size);
 	memcpy(chamelium_edid->raw, edid, edid_get_size(edid));
 	igt_list_add(&chamelium_edid->link, &chamelium->edids);
@@ -612,8 +608,23 @@ void chamelium_port_set_edid(struct chamelium *chamelium,
 			     struct chamelium_port *port,
 			     struct chamelium_edid *edid)
 {
+	int edid_id;
+	size_t port_index;
+	const struct edid *raw_edid;
+
+	if (edid) {
+		port_index = port - chamelium->ports;
+		edid_id = edid->ids[port_index];
+		if (edid_id == 0) {
+			raw_edid = chamelium_edid_get_raw(edid, port);
+			edid_id = chamelium_upload_edid(chamelium, raw_edid);
+		}
+	} else {
+		edid_id = 0;
+	}
+
 	xmlrpc_DECREF(chamelium_rpc(chamelium, NULL, "ApplyEdid", "(ii)",
-				    port->id, edid ? edid->id : 0));
+				    port->id, edid_id));
 }
 
 /**
@@ -1929,7 +1940,10 @@ void chamelium_deinit(struct chamelium *chamelium)
 
 	/* Destroy any EDIDs we created to make sure we don't leak them */
 	igt_list_for_each_safe(pos, tmp, &chamelium->edids, link) {
-		chamelium_destroy_edid(chamelium, pos->id);
+		for (i = 0; i < CHAMELIUM_MAX_PORTS; i++) {
+			if (pos->ids[i])
+				chamelium_destroy_edid(chamelium, pos->ids[i]);
+		}
 		free(pos->raw);
 		free(pos);
 	}
-- 
2.22.0

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

[igt-dev] [PATCH i-g-t v2 7/9] lib/igt_chamelium: set EDID serial

[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 <simon.ser@intel.com>
---
 lib/igt_chamelium.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index 467f1a458516..dcc59e9f0b04 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -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 */
+	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;
 }
 
-- 
2.22.0

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

[igt-dev] [PATCH i-g-t v2 8/9] lib/igt_edid: add edid_get_mfg

[Simon Ser]

This returns the 3-letter manufacturer identifier of an EDID.

Signed-off-by: Simon Ser <simon.ser@intel.com>
---
 lib/igt_edid.c        | 13 +++++++++++++
 lib/igt_edid.h        |  1 +
 tests/kms_chamelium.c | 10 +++-------
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/lib/igt_edid.c b/lib/igt_edid.c
index 6cc5e7dd42c4..cbb7eefff70d 100644
--- a/lib/igt_edid.c
+++ b/lib/igt_edid.c
@@ -172,6 +172,19 @@ void detailed_timing_set_string(struct detailed_timing *dt,
 		ds->str[len] = '\n';
 }
 
+/**
+ * edid_get_mfg: reads the 3-letter manufacturer identifier
+ *
+ * The string is *not* NULL-terminated.
+ */
+void edid_get_mfg(const struct edid *edid, char out[static 3])
+{
+	out[0] = ((edid->mfg_id[0] & 0x7C) >> 2) + '@';
+	out[1] = (((edid->mfg_id[0] & 0x03) << 3) |
+		 ((edid->mfg_id[1] & 0xE0) >> 5)) + '@';
+	out[2] = (edid->mfg_id[1] & 0x1F) + '@';
+}
+
 static void edid_set_mfg(struct edid *edid, const char mfg[static 3])
 {
 	edid->mfg_id[0] = (mfg[0] - '@') << 2 | (mfg[1] - '@') >> 3;
diff --git a/lib/igt_edid.h b/lib/igt_edid.h
index 8d8e30ec0554..47581bb778b0 100644
--- a/lib/igt_edid.h
+++ b/lib/igt_edid.h
@@ -298,6 +298,7 @@ void edid_init(struct edid *edid);
 void edid_init_with_mode(struct edid *edid, drmModeModeInfo *mode);
 void edid_update_checksum(struct edid *edid);
 size_t edid_get_size(const struct edid *edid);
+void edid_get_mfg(const struct edid *edid, char out[static 3]);
 void detailed_timing_set_mode(struct detailed_timing *dt, drmModeModeInfo *mode,
 			      int width_mm, int height_mm);
 void detailed_timing_set_monitor_range_mode(struct detailed_timing *dt,
diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
index 175e5032c973..1d5c0ded8a56 100644
--- a/tests/kms_chamelium.c
+++ b/tests/kms_chamelium.c
@@ -184,7 +184,7 @@ check_analog_bridge(data_t *data, struct chamelium_port *port)
 	drmModeConnector *connector = chamelium_port_get_connector(
 	    data->chamelium, port, false);
 	uint64_t edid_blob_id;
-	unsigned char *edid;
+	const struct edid *edid;
 	char edid_vendor[3];
 
 	if (chamelium_port_get_type(port) != DRM_MODE_CONNECTOR_VGA) {
@@ -198,12 +198,8 @@ check_analog_bridge(data_t *data, struct chamelium_port *port)
 	igt_assert(edid_blob = drmModeGetPropertyBlob(data->drm_fd,
 						      edid_blob_id));
 
-	edid = (unsigned char *) edid_blob->data;
-
-	edid_vendor[0] = ((edid[8] & 0x7c) >> 2) + '@';
-	edid_vendor[1] = (((edid[8] & 0x03) << 3) |
-			  ((edid[9] & 0xe0) >> 5)) + '@';
-	edid_vendor[2] = (edid[9] & 0x1f) + '@';
+	edid = (const struct edid *) edid_blob->data;
+	edid_get_mfg(edid, edid_vendor);
 
 	drmModeFreePropertyBlob(edid_blob);
 	drmModeFreeConnector(connector);
-- 
2.22.0

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

[igt-dev] [PATCH i-g-t v2 9/9] lib/igt_chamelium: autodiscover Chamelium port mappings

[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 <simon.ser@intel.com>
---
 lib/igt_chamelium.c | 205 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 203 insertions(+), 2 deletions(-)

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index dcc59e9f0b04..ddea1ba47d4e 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -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;
 }
 
+/**
+ * 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]))
+
+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);
+	return port_id;
+}
+
+/**
+ * chamelium_autodiscover: TODO
+ */
+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) {
+				found = true;
+				break;
+			}
+		}
+		if (found)
+			continue;
+
+		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;
+
+	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;
+
+		/* 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++) {
+			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;
+			}
+		}
+		if (found)
+			continue;
+
+		/* Get got a new mapping */
+		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);
 }
 
 /**
-- 
2.22.0

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

[igt-dev] ✗ Fi.CI.BAT: failure for Chamelium port mapping auto-discovery

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_6310 -> IGTPW_3178
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_3178 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_3178, 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/1/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_3178:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-skl-6700k2:      [PASS][1] -> [FAIL][2] +3 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-skl-6700k2/igt@kms_chamelium@hdmi-crc-fast.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-skl-6700k2/igt@kms_chamelium@hdmi-crc-fast.html

  
#### Warnings ####

  * igt@kms_chamelium@dp-hpd-fast:
    - fi-skl-6700k2:      [SKIP][3] ([fdo#109271]) -> [FAIL][4] +4 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-skl-6700k2/igt@kms_chamelium@dp-hpd-fast.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-skl-6700k2/igt@kms_chamelium@dp-hpd-fast.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_close_race@basic-process:
    - fi-icl-dsi:         [PASS][5] -> [INCOMPLETE][6] ([fdo#107713])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-icl-dsi/igt@gem_close_race@basic-process.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-icl-dsi/igt@gem_close_race@basic-process.html

  * igt@gem_cpu_reloc@basic:
    - fi-icl-u3:          [PASS][7] -> [INCOMPLETE][8] ([fdo#107713] / [fdo#110246])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-icl-u3/igt@gem_cpu_reloc@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-icl-u3/igt@gem_cpu_reloc@basic.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       [PASS][9] -> [FAIL][10] ([fdo#109635 ])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       [PASS][11] -> [DMESG-WARN][12] ([fdo#102614])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html

  
#### Possible fixes ####

  * igt@debugfs_test@read_all_entries:
    - fi-ilk-650:         [DMESG-WARN][13] ([fdo#106387]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-ilk-650/igt@debugfs_test@read_all_entries.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-ilk-650/igt@debugfs_test@read_all_entries.html

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       [INCOMPLETE][15] ([fdo#107718]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@gem_sync@basic-store-each:
    - fi-apl-guc:         [FAIL][17] ([fdo#110512]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-apl-guc/igt@gem_sync@basic-store-each.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-apl-guc/igt@gem_sync@basic-store-each.html

  * igt@i915_selftest@live_contexts:
    - fi-bdw-gvtdvm:      [DMESG-FAIL][19] ([fdo#110235]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html
    - fi-skl-gvtdvm:      [DMESG-FAIL][21] ([fdo#110235]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-skl-gvtdvm/igt@i915_selftest@live_contexts.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-skl-gvtdvm/igt@i915_selftest@live_contexts.html

  * igt@i915_selftest@live_evict:
    - fi-kbl-8809g:       [INCOMPLETE][23] ([fdo#110938]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6310/fi-kbl-8809g/igt@i915_selftest@live_evict.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/fi-kbl-8809g/igt@i915_selftest@live_evict.html

  
  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109635 ]: https://bugs.freedesktop.org/show_bug.cgi?id=109635 
  [fdo#110235]: https://bugs.freedesktop.org/show_bug.cgi?id=110235
  [fdo#110246]: https://bugs.freedesktop.org/show_bug.cgi?id=110246
  [fdo#110512]: https://bugs.freedesktop.org/show_bug.cgi?id=110512
  [fdo#110938]: https://bugs.freedesktop.org/show_bug.cgi?id=110938


Participating hosts (52 -> 44)
------------------------------

  Additional (2): fi-skl-guc fi-cml-u 
  Missing    (10): fi-kbl-soraka fi-cml-u2 fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-icl-guc fi-byt-clapper fi-bdw-samus 


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

  * IGT: IGT_5061 -> IGTPW_3178

  CI_DRM_6310: dc88bdb699786ec5c75f0fd0fb6f8b0c3e58eb8e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3178: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3178/
  IGT_5061: c88ced79a7b71aec58f1d9c5c599ac2f431bcf7a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

[igt-dev] [PATCH i-g-t v3] lib/igt_chamelium: autodiscover Chamelium port mappings

[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 <simon.ser@intel.com>
---

Changes from v2 to v3:
- Fix assertion failure when EDID blob is zero
- Log auto-discovery duration
- Fix already-configured mappings being skipped
- Optimize auto-discovery by not resetting the Chamelium before starting tests
  (the Chamelium had ports enabled and wasn't reset before testing before this
  patch, so this should be fine)

On my setup auto-discovery takes a little less than 500ms. As a side note, the
Chamelium teardown sequence takes roughly the same time.

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

diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
index dcc59e9f0b04..8bb37ad3cf9b 100644
--- a/lib/igt_chamelium.c
+++ b/lib/igt_chamelium.c
@@ -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,208 @@ out:
 	return ret;
 }

+/**
+ * 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]))
+
+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 || !edid_blob_id) {
+		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);
+	return port_id;
+}
+
+/**
+ * chamelium_autodiscover: TODO
+ */
+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];
+	struct timespec start;
+	uint64_t elapsed_ns;
+
+	igt_debug("Starting Chamelium port auto-discovery\n");
+	igt_gettime(&start);
+
+	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];
+
+		/* Get or add a chamelium_port slot */
+		port = NULL;
+		for (j = 0; j < chamelium->port_count; j++) {
+			if (chamelium->ports[j].id == port_id) {
+				port = &chamelium->ports[j];
+				break;
+			}
+		}
+		if (!port) {
+			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;
+
+	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 = drmModeGetConnectorCurrent(drm_fd,
+						       res->connectors[i]);
+		port_id = port_id_from_edid(drm_fd, connector);
+		drmModeFreeConnector(connector);
+		if (port_id < 0)
+			continue;
+
+		/* 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++) {
+			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;
+			}
+		}
+		if (found)
+			continue;
+
+		/* Get got a new mapping */
+		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);
+	}
+
+	elapsed_ns = igt_nsec_elapsed(&start);
+	igt_debug("Auto-discovery took %fms\n",
+		  (float) elapsed_ns / (1000 * 1000));
+
+	return true;
+}
+
 static bool chamelium_read_config(struct chamelium *chamelium, int drm_fd)
 {
 	GError *error = NULL;
@@ -1845,7 +2047,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);
 }

 /**
--
2.22.0

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

[igt-dev] ✓ Fi.CI.BAT: success for Chamelium port mapping auto-discovery (rev2)

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from IGT_5062 -> IGTPW_3180
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_create@basic-files:
    - fi-icl-dsi:         [PASS][1] -> [INCOMPLETE][2] ([fdo#107713] / [fdo#109100])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5062/fi-icl-dsi/igt@gem_ctx_create@basic-files.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/fi-icl-dsi/igt@gem_ctx_create@basic-files.html

  * igt@gem_exec_basic@basic-all:
    - fi-icl-u2:          [PASS][3] -> [INCOMPLETE][4] ([fdo#107713])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5062/fi-icl-u2/igt@gem_exec_basic@basic-all.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/fi-icl-u2/igt@gem_exec_basic@basic-all.html
    - fi-cml-u2:          [PASS][5] -> [INCOMPLETE][6] ([fdo#110566])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5062/fi-cml-u2/igt@gem_exec_basic@basic-all.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/fi-cml-u2/igt@gem_exec_basic@basic-all.html

  * igt@gem_exec_suspend@basic-s3:
    - fi-cfl-8109u:       [PASS][7] -> [FAIL][8] ([fdo#103375])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5062/fi-cfl-8109u/igt@gem_exec_suspend@basic-s3.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/fi-cfl-8109u/igt@gem_exec_suspend@basic-s3.html

  * igt@i915_selftest@live_contexts:
    - fi-bdw-gvtdvm:      [PASS][9] -> [DMESG-FAIL][10] ([fdo#110235])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5062/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/fi-bdw-gvtdvm/igt@i915_selftest@live_contexts.html

  
#### Possible fixes ####

  * igt@gem_ctx_switch@basic-default:
    - fi-icl-guc:         [INCOMPLETE][11] ([fdo#107713] / [fdo#108569]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5062/fi-icl-guc/igt@gem_ctx_switch@basic-default.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/fi-icl-guc/igt@gem_ctx_switch@basic-default.html

  * igt@gem_mmap_gtt@basic-read-no-prefault:
    - fi-glk-dsi:         [INCOMPLETE][13] ([fdo#103359] / [k.org#198133]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5062/fi-glk-dsi/igt@gem_mmap_gtt@basic-read-no-prefault.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/fi-glk-dsi/igt@gem_mmap_gtt@basic-read-no-prefault.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         [DMESG-WARN][15] ([fdo#106387]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5062/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html

  
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#110235]: https://bugs.freedesktop.org/show_bug.cgi?id=110235
  [fdo#110566]: https://bugs.freedesktop.org/show_bug.cgi?id=110566
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (51 -> 45)
------------------------------

  Additional (2): fi-byt-j1900 fi-bxt-j4205 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-icl-u3 fi-icl-y fi-byt-clapper fi-bdw-samus 


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

  * IGT: IGT_5062 -> IGTPW_3180

  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_5062: 771cd83f4ae4299cebfb09b89b4d044835c6ea80 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

[igt-dev] ✗ Fi.CI.IGT: failure for Chamelium port mapping auto-discovery (rev2)

[Patchwork]

== 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@gem_ctx_switch@basic-queue-heavy.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-glk4/igt@gem_ctx_switch@basic-queue-heavy.html

  * 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@gem_workarounds@suspend-resume.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-apl6/igt@gem_workarounds@suspend-resume.html

  * 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@gem_userptr_blits@map-fixed-invalidate-busy-gup.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-apl5/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html

  * 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@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-apl5/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  
  [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/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] ✗ Fi.CI.IGT: failure for Chamelium port mapping auto-discovery (rev2)

[Ser, Simon]

Cc Martin

On Mon, 2019-06-24 at 08:58 +0000, Patchwork wrote:
> == 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@gem_ctx_switch@basic-queue-heavy.html
>    [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-glk4/igt@gem_ctx_switch@basic-queue-heavy.html
> 
>   * 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@gem_workarounds@suspend-resume.html
>    [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-apl6/igt@gem_workarounds@suspend-resume.html
> 
>   * 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@gem_userptr_blits@map-fixed-invalidate-busy-gup.html
>    [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-apl5/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html
> 
>   * 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@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
>    [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3180/shard-apl5/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
> 
>   
>   [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/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2 7/9] lib/igt_chamelium: set EDID serial

[Arkadiusz Hiler]

On Wed, Jun 19, 2019 at 06:55:16PM +0300, Simon Ser wrote:
> 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 <simon.ser@intel.com>
> ---
>  lib/igt_chamelium.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index 467f1a458516..dcc59e9f0b04 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -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 */
> +	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.

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

Re: [igt-dev] [PATCH i-g-t v2 9/9] lib/igt_chamelium: autodiscover Chamelium port mappings

[Arkadiusz Hiler]

On Wed, Jun 19, 2019 at 06:55:18PM +0300, Simon Ser wrote:
> 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 <simon.ser@intel.com>
> ---
>  lib/igt_chamelium.c | 205 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 203 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> index dcc59e9f0b04..ddea1ba47d4e 100644
> --- a/lib/igt_chamelium.c
> +++ b/lib/igt_chamelium.c
> @@ -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;
>  }
>  
> +/**
> + * 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?

> +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);
> +	return port_id;
> +}
> +
> +/**
> + * chamelium_autodiscover: TODO
> + */

Are you planning to get this description done?

> +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) {
> +				found = true;
> +				break;
> +			}
> +		}
> +		if (found)
> +			continue;

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

> +
> +		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;
> +
> +	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;

> +
> +		/* 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++) {
> +			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?

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

Get got?

> +		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);
>  }
>  
>  /**
> -- 
> 2.22.0
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2 9/9] lib/igt_chamelium: autodiscover Chamelium port mappings

[Ser, Simon]

On Mon, 2019-06-24 at 16:22 +0300, Arkadiusz Hiler wrote:
> On Wed, Jun 19, 2019 at 06:55:18PM +0300, Simon Ser wrote:
> > 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 <simon.ser@intel.com>
> > ---
> >  lib/igt_chamelium.c | 205 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 203 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > index dcc59e9f0b04..ddea1ba47d4e 100644
> > --- a/lib/igt_chamelium.c
> > +++ b/lib/igt_chamelium.c
> > @@ -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;
> >  }
> >  
> > +/**
> > + * 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.

> > +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);
> > +	return port_id;
> > +}
> > +
> > +/**
> > + * chamelium_autodiscover: TODO
> > + */
> 
> Are you planning to get this description done?

Derp

> > +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) {
> > +				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.

> > +
> > +		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;
> > +
> > +	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;
> 
> > +
> > +		/* 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++) {
> > +			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?
> 
> > +			}
> > +		}
> > +		if (found)
> > +			continue;
> > +
> > +		/* Get got a new mapping */
> 
> Get got?
> 
> > +		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);
> >  }
> >  
> >  /**
> > -- 
> > 2.22.0
> > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2 9/9] lib/igt_chamelium: autodiscover Chamelium port mappings

[Arkadiusz Hiler]

On Mon, Jun 24, 2019 at 05:15:45PM +0300, Ser, Simon wrote:
> On Mon, 2019-06-24 at 16:22 +0300, Arkadiusz Hiler wrote:
> > On Wed, Jun 19, 2019 at 06:55:18PM +0300, Simon Ser wrote:
> > > 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 <simon.ser@intel.com>
> > > ---
> > >  lib/igt_chamelium.c | 205 +++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 203 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > > index dcc59e9f0b04..ddea1ba47d4e 100644
> > > --- a/lib/igt_chamelium.c
> > > +++ b/lib/igt_chamelium.c
> > > @@ -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;
> > >  }
> > >  
> > > +/**
> > > + * 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.

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.

> 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

> > > +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);
> > > +	return port_id;
> > > +}
> > > +
> > > +/**
> > > + * chamelium_autodiscover: TODO
> > > + */
> > 
> > Are you planning to get this description done?
> 
> Derp
> 
> > > +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) {
> > > +				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.

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

> > > +
> > > +		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

> > > +
> > > +		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;
> > > +
> > > +	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;
> > 
> > > +
> > > +		/* 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++) {
> > > +			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?
> > 
> > > +			}
> > > +		}
> > > +		if (found)
> > > +			continue;
> > > +
> > > +		/* Get got a new mapping */
> > 
> > Get got?
> > 
> > > +		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);
> > >  }
> > >  
> > >  /**
> > > -- 
> > > 2.22.0
> > > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2 9/9] lib/igt_chamelium: autodiscover Chamelium port mappings

[Ser, Simon]

On Tue, 2019-06-25 at 10:03 +0300, Arkadiusz Hiler wrote:
> On Mon, Jun 24, 2019 at 05:15:45PM +0300, Ser, Simon wrote:
> > On Mon, 2019-06-24 at 16:22 +0300, Arkadiusz Hiler wrote:
> > > On Wed, Jun 19, 2019 at 06:55:18PM +0300, Simon Ser wrote:
> > > > 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 <simon.ser@intel.com>
> > > > ---
> > > >  lib/igt_chamelium.c | 205 +++++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 203 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > > > index dcc59e9f0b04..ddea1ba47d4e 100644
> > > > --- a/lib/igt_chamelium.c
> > > > +++ b/lib/igt_chamelium.c
> > > > @@ -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;
> > > >  }
> > > >  
> > > > +/**
> > > > + * 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.
> 
> There are still ways of hiding it behind extern or a function, but I am
> not sure the added complexity is worth it.

This kinds of defeat its purpose, since we can't define static arrays
with the length set to the constant or function return value.

> Anyway, no strong feeling on this one, just seems a bit like a
> duplication.
> 
> > 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
> 
> > > > +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);
> > > > +	return port_id;
> > > > +}
> > > > +
> > > > +/**
> > > > + * chamelium_autodiscover: TODO
> > > > + */
> > > 
> > > Are you planning to get this description done?
> > 
> > Derp
> > 
> > > > +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) {
> > > > +				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.
> 
> Ah, right. I have read v3 now and it looks ok. Thanks!
> 
> > > > +
> > > > +		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
> 
> > > > +
> > > > +		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;
> > > > +
> > > > +	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;
> > > 
> > > > +
> > > > +		/* 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++) {
> > > > +			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?
> > > 
> > > > +			}
> > > > +		}
> > > > +		if (found)
> > > > +			continue;
> > > > +
> > > > +		/* Get got a new mapping */
> > > 
> > > Get got?
> > > 
> > > > +		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);
> > > >  }
> > > >  
> > > >  /**
> > > > -- 
> > > > 2.22.0
> > > > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v2 7/9] lib/igt_chamelium: set EDID serial

[Ser, Simon]

On Mon, 2019-06-24 at 15:16 +0300, Arkadiusz Hiler wrote:
> On Wed, Jun 19, 2019 at 06:55:16PM +0300, Simon Ser wrote:
> > 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 <simon.ser@intel.com>
> > ---
> >  lib/igt_chamelium.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > index 467f1a458516..dcc59e9f0b04 100644
> > --- a/lib/igt_chamelium.c
> > +++ b/lib/igt_chamelium.c
> > @@ -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 */
> > +	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.

Fair enough. I think I'll go with storing a copy for each connector,
since it allows to keep the same simple API.
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev