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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Thierry,

On Monday 26 January 2015 11:14:32 Thierry Reding wrote:
> On Sun, Jan 25, 2015 at 01:56:51AM +0200, Laurent Pinchart wrote:
> > 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.

My bad.

> >> @@ -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).

Isn't the index dependent on probe ordering as well ?

> 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.

Yes, I think that's a good idea.

> >> +	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.

We're talking about 8 bytes per connector for a test tool. With no more than a 
dozen of connectors. Right ? :-)

> 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.

-- 
Regards,

Laurent Pinchart

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