Re: [PATCH libdrm 08/11] tests: modetest: Accept connector names

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5417 bytes --]

On Sun, Jan 25, 2015 at 01:56:51AM +0200, Laurent Pinchart wrote:
> Hi Thierry,
> 
> Thank you for the patch.
> 
> On Friday 23 January 2015 17:08:21 Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > 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).
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  tests/modetest/modetest.c | 134 +++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 123 insertions(+), 11 deletions(-)
> > 
> > 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
> 
> [snip]
> 
> > @@ -327,7 +328,7 @@ static void dump_connectors(struct device *dev)
> >  	int i, j;
> > 
> >  	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 = 0; i < dev->resources->res->count_connectors; i++) {
> >  		struct connector *_connector = &dev->resources->connectors[i];
> >  		drmModeConnector *connector = _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);
> 
> As this is a low-level test tool I believe it would be useful to print both 
> 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);
> >  }
> > 
> > +static unsigned int get_connector_index(struct resources *res, uint32_t
> > type)
> > +{
> > +	unsigned int index = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < res->res->count_connectors; i++)
> > +		if (res->connectors[i].connector->connector_type == type)
> > +			index++;
> > +
> > +	return index - 1;
> > +}
> > +
> > +static unsigned int get_order(unsigned int value)
> > +{
> > +	unsigned int order = 0;
> > +
> > +	do {
> > +		value /= 10;
> > +		order++;
> > +	} while (value > 0);
> > +
> > +	return order - 1;
> > +}
> > +
> > +static void connector_set_name(struct connector *connector,
> > +			       struct resources *res)
> > +{
> > +	uint32_t type = connector->connector->connector_type;
> > +	const char *type_name;
> > +	unsigned int index;
> > +	int len;
> > +
> > +	type_name = util_lookup_connector_type_name(type);
> > +	index = get_connector_index(res, type);
> 
> The kernel's connector name is created using connector_type_id, not the 
> 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 = strlen(type_name) + get_order(index) + 2;
> 
> This looks like an over-optimization to me, can't you just add 9 to account 
> for the largest possible index ? Or, even better, use asprintf ? The function 
> 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 = 0; i < pipe->num_cons; i++) {
> > +		id = strtoul(pipe->cons[i], &endp, 10);
> > +		if (endp == pipe->cons[i]) {
> 
> This won't have the expected behaviour for 9-pin DIN connectors, as the name 
> starts with a digit. You should instead test for *endp == '\0'.

Good catch. I've changed this check to:

	if (*endp != '\0') {
		... look up by name ...
	}

> > +			connector = get_connector_by_name(dev, pipe->cons[i]);
> > +			if (!connector) {
> > +				fprintf(stderr, "no connector named %s\n",
> > +					pipe->cons[i]);
> > +				return -ENODEV;
> > +			}
> > +
> > +			id = connector->connector_id;
> > +		}
> > +
> > +		pipe->con_ids[i] = id;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Could update the help text in usage() to reflect the new usage ?

Done.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel