[igt-dev] [PATCH i-g-t v2 9/9] lib/igt_chamelium: autodiscover Chamelium port mappings
Ser, Simon
simon.ser at intel.com
Tue Jun 25 07:23:06 UTC 2019
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 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.
>
> 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
> > > >
More information about the igt-dev
mailing list