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

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

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)" ?

[snip]

> @@ -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 ?

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

> +	connector->name = malloc(len + 1);
> +	if (connector->name)
> +		snprintf(connector->name, len + 1, "%s-%u", type_name,  index);
> +}
> +
>  static struct resources *get_resources(struct device *dev)
>  {
>  	struct resources *res;

[snip]

> @@ -1405,6 +1483,32 @@ static int cursor_supported(void)
>  	return 1;
>  }
> 
> +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'.

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

-- 
Regards,

Laurent Pinchart

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