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