[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 at 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
> >