[igt-dev] [PATCH i-g-t v2 9/9] lib/igt_chamelium: autodiscover Chamelium port mappings
Arkadiusz Hiler
arkadiusz.hiler at intel.com
Tue Jun 25 07:03:35 UTC 2019
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.
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