From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH libdrm 08/11] tests: modetest: Accept connector names Date: Mon, 26 Jan 2015 11:14:32 +0100 Message-ID: <20150126101430.GB3508@ulmo> References: <1422029304-1926-1-git-send-email-thierry.reding@gmail.com> <1422029304-1926-9-git-send-email-thierry.reding@gmail.com> <10536857.Rtq57mgypf@avalon> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1328417146==" Return-path: Received: from mail-we0-f174.google.com (mail-we0-f174.google.com [74.125.82.174]) by gabe.freedesktop.org (Postfix) with ESMTP id 129A16E223 for ; Mon, 26 Jan 2015 02:14:35 -0800 (PST) Received: by mail-we0-f174.google.com with SMTP id w55so2693682wes.5 for ; Mon, 26 Jan 2015 02:14:34 -0800 (PST) In-Reply-To: <10536857.Rtq57mgypf@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1328417146== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="f0KYrhQ4vYSV2aJu" Content-Disposition: inline --f0KYrhQ4vYSV2aJu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Jan 25, 2015 at 01:56:51AM +0200, Laurent Pinchart wrote: > Hi Thierry, >=20 > Thank you for the patch. >=20 > On Friday 23 January 2015 17:08:21 Thierry Reding wrote: > > From: Thierry Reding > >=20 > > Allow connector names to be used in the specification of the -s option. > > This requires storing the string passed on the command-line so that it > > can later be resolved to a connector ID (after the DRM device has been > > opened). > >=20 > > Signed-off-by: Thierry Reding > > --- > > tests/modetest/modetest.c | 134 ++++++++++++++++++++++++++++++++++++++= +---- > > 1 file changed, 123 insertions(+), 11 deletions(-) > >=20 > > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c > > index d5fd99ebe1fd..a7cc94f8938c 100644 > > --- a/tests/modetest/modetest.c > > +++ b/tests/modetest/modetest.c >=20 > [snip] >=20 > > @@ -327,7 +328,7 @@ static void dump_connectors(struct device *dev) > > int i, j; > >=20 > > printf("Connectors:\n"); > > - printf("id\tencoder\tstatus\t\ttype\tsize (mm)\tmodes\tencoders\n"); > > + printf("id\tencoder\tstatus\t\tname\t\tsize (mm)\tmodes\tencoders\n"); > > for (i =3D 0; i < dev->resources->res->count_connectors; i++) { > > struct connector *_connector =3D &dev->resources->connectors[i]; > > drmModeConnector *connector =3D _connector->connector; > > @@ -338,7 +339,7 @@ static void dump_connectors(struct device *dev) > > connector->connector_id, > > connector->encoder_id, > > util_lookup_connector_status_name(connector->connection), > > - util_lookup_connector_type_name(connector->connector_type), > > + _connector->name, > > connector->mmWidth, connector->mmHeight, > > connector->count_modes); >=20 > As this is a low-level test tool I believe it would be useful to print bo= th=20 > the name and the ID. Maybe something like "name (id)" ? The ID is already printed in the very first column. > > @@ -511,6 +516,47 @@ static void free_resources(struct resources *res) > > free(res); > > } > >=20 > > +static unsigned int get_connector_index(struct resources *res, uint32_t > > type) > > +{ > > + unsigned int index =3D 0; > > + int i; > > + > > + for (i =3D 0; i < res->res->count_connectors; i++) > > + if (res->connectors[i].connector->connector_type =3D=3D type) > > + index++; > > + > > + return index - 1; > > +} > > + > > +static unsigned int get_order(unsigned int value) > > +{ > > + unsigned int order =3D 0; > > + > > + do { > > + value /=3D 10; > > + order++; > > + } while (value > 0); > > + > > + return order - 1; > > +} > > + > > +static void connector_set_name(struct connector *connector, > > + struct resources *res) > > +{ > > + uint32_t type =3D connector->connector->connector_type; > > + const char *type_name; > > + unsigned int index; > > + int len; > > + > > + type_name =3D util_lookup_connector_type_name(type); > > + index =3D get_connector_index(res, type); >=20 > The kernel's connector name is created using connector_type_id, not the= =20 > connector index. Shouldn't we do the same here ? The idea was to mirror what X was doing so that people familiar with the xrandr tool would feel right at home. Note that the index here is by type, not global. So you'd end up with something like this: HDMI-A-1 HDMI-A-2 LVDS-1 I think that's what most people would find to be the least surprising. Using the connector_type_id would again introduce the potential to get no-deterministic names (dependent on driver probe ordering in case of multiple cards). That said I now realize that this actually starts numbering connectors at 0, so get_connector_index() should probably return index rather than index - 1 to be consistent with what X does. > > + len =3D strlen(type_name) + get_order(index) + 2; >=20 > This looks like an over-optimization to me, can't you just add 9 to accou= nt=20 > for the largest possible index ? Or, even better, use asprintf ? The func= tion=20 > is a GNU extension but is available on BSD according to its manpage. Always using 9 characters would be wasting 8 bytes per connector for something like 99% of the systems. I had thought about using asprintf but decided not to use it because it isn't always available and it is pretty simple to compute the actual length. > > +static int pipe_resolve_connectors(struct pipe_arg *pipe, struct device > > *dev) > > +{ > > + drmModeConnector *connector; > > + unsigned int i; > > + uint32_t id; > > + char *endp; > > + > > + for (i =3D 0; i < pipe->num_cons; i++) { > > + id =3D strtoul(pipe->cons[i], &endp, 10); > > + if (endp =3D=3D pipe->cons[i]) { >=20 > This won't have the expected behaviour for 9-pin DIN connectors, as the n= ame=20 > starts with a digit. You should instead test for *endp =3D=3D '\0'. Good catch. I've changed this check to: if (*endp !=3D '\0') { ... look up by name ... } > > + connector =3D get_connector_by_name(dev, pipe->cons[i]); > > + if (!connector) { > > + fprintf(stderr, "no connector named %s\n", > > + pipe->cons[i]); > > + return -ENODEV; > > + } > > + > > + id =3D connector->connector_id; > > + } > > + > > + pipe->con_ids[i] =3D id; > > + } > > + > > + return 0; > > +} >=20 > Could update the help text in usage() to reflect the new usage ? Done. Thierry --f0KYrhQ4vYSV2aJu Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUxhOGAAoJEN0jrNd/PrOhQMgP/2KQGjKE1KdEqOlDa3ZA43xq hZsccjf7jBky2HpSQL342O1tBVhXCP5mA/Di2MKEZ/nof93iK5+8pc0F92ol0tdI DBnXvF9SE8YCfrnNi9VHKCTIjW646BVo7r+PsWmTnhbJ6zObUwaeF1nHVlsnsmu9 MYqYPNJb3L5rZT8Q4O+nb9MyJms7cYzyBvr3VWT+FWQIGYx1OoulTqsnNgL7C3mf eGRDoKpypY7TqLFTZPfFIjZcVPVUQFaO28kiMAVRhjFqZcXx5qQ57DyrDn5JDfrA KCD7OTjuOI4Phuq8ofmLYBuVg0VtSGPe69Fdy+XkAwNFmlmIZa0HdQyFQ2HsIGKO k15heIWzUF60ITPU7bl7Qyq6W72KkteVLcvmX1RkH/IUPXPbzOTgbLbXql2VVYXR JLBdpKOVz7tatyg5EB2qXONp5rawVe3mcIruLDy3OCd5v0E5lQAPCAf1JcWTunba QNvfv8vXBYeAdRYS3y2BQP3TV0togXuYWzrMbRle/kCMHexUNPbpqovk7/Z5BB7C z27Y5dnDeZDP0x21inVlfdTpNgRO5t/LL84ftKGgzqcM88Bju7yDOD/AIorU2QaK 49IizHI4v/zGcE8ELGyijhHyWONe8yLvNLmrmOCE1fWI3pFxoZK9gjzwg0zJWsoS ktdBRN8PhxHPUSSghnlg =stwo -----END PGP SIGNATURE----- --f0KYrhQ4vYSV2aJu-- --===============1328417146== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1328417146==--