diff mbox

tests: modetest: Accept connector names in addition to connector IDs

Message ID 1437411380-10015-1-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Pinchart July 20, 2015, 4:56 p.m. UTC
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).

Connector names are constructed from the connector type name and
connector type ID using the same format as used internally in the
Linux kernel.

Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 tests/modetest/modetest.c | 94 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 82 insertions(+), 12 deletions(-)

I've carried this patch in my tree for too long and I'm now tired of rebasing
it. It's thus time to get it in mainline.

Compared to the original version developed by Thierry, I've

- rebased the patch on top of latest master
- dropped dependencies on other WIP patches in Thierry's tree
- fixed printf alignment
- used asprintf to simplify name string allocation
- constructed the connector name using the same format as the Linux kernel

Comments

Thierry Reding July 21, 2015, 9:05 a.m. UTC | #1
On Mon, Jul 20, 2015 at 07:56:20PM +0300, Laurent Pinchart 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).
> 
> Connector names are constructed from the connector type name and
> connector type ID using the same format as used internally in the
> Linux kernel.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  tests/modetest/modetest.c | 94 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 82 insertions(+), 12 deletions(-)
> 
> I've carried this patch in my tree for too long and I'm now tired of rebasing
> it. It's thus time to get it in mainline.
> 
> Compared to the original version developed by Thierry, I've
> 
> - rebased the patch on top of latest master
> - dropped dependencies on other WIP patches in Thierry's tree
> - fixed printf alignment
> - used asprintf to simplify name string allocation
> - constructed the connector name using the same format as the Linux kernel

Heh, I started to get all these patches into shape last week, and this
will probably give me more rebase headaches. But I guess that's what I
deserve for letting things slide...

Thierry
Laurent Pinchart July 21, 2015, 12:27 p.m. UTC | #2
Hi Thierry,

On Tuesday 21 July 2015 11:05:59 Thierry Reding wrote:
> On Mon, Jul 20, 2015 at 07:56:20PM +0300, Laurent Pinchart 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).
> > 
> > Connector names are constructed from the connector type name and
> > connector type ID using the same format as used internally in the
> > Linux kernel.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  tests/modetest/modetest.c | 94 ++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 82 insertions(+), 12 deletions(-)
> > 
> > I've carried this patch in my tree for too long and I'm now tired of
> > rebasing it. It's thus time to get it in mainline.
> > 
> > Compared to the original version developed by Thierry, I've
> > 
> > - rebased the patch on top of latest master
> > - dropped dependencies on other WIP patches in Thierry's tree
> > - fixed printf alignment
> > - used asprintf to simplify name string allocation
> > - constructed the connector name using the same format as the Linux kernel
> 
> Heh, I started to get all these patches into shape last week, and this
> will probably give me more rebase headaches.

It shouldn't hurt too much as rebasing your patch on top of the master branch 
without any other dependency was pretty easy.

> But I guess that's what I deserve for letting things slide...

Should I consider this as an Acked-by ? :-)
Thierry Reding July 21, 2015, 3:10 p.m. UTC | #3
On Tue, Jul 21, 2015 at 03:27:25PM +0300, Laurent Pinchart wrote:
> Hi Thierry,
> 
> On Tuesday 21 July 2015 11:05:59 Thierry Reding wrote:
> > On Mon, Jul 20, 2015 at 07:56:20PM +0300, Laurent Pinchart 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).
> > > 
> > > Connector names are constructed from the connector type name and
> > > connector type ID using the same format as used internally in the
> > > Linux kernel.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  tests/modetest/modetest.c | 94 ++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 82 insertions(+), 12 deletions(-)
> > > 
> > > I've carried this patch in my tree for too long and I'm now tired of
> > > rebasing it. It's thus time to get it in mainline.
> > > 
> > > Compared to the original version developed by Thierry, I've
> > > 
> > > - rebased the patch on top of latest master
> > > - dropped dependencies on other WIP patches in Thierry's tree
> > > - fixed printf alignment
> > > - used asprintf to simplify name string allocation
> > > - constructed the connector name using the same format as the Linux kernel
> > 
> > Heh, I started to get all these patches into shape last week, and this
> > will probably give me more rebase headaches.
> 
> It shouldn't hurt too much as rebasing your patch on top of the master branch 
> without any other dependency was pretty easy.
> 
> > But I guess that's what I deserve for letting things slide...
> 
> Should I consider this as an Acked-by ? :-)

I don't think you need it since I authored it, but yeah, if this works,
just go ahead and I'll figure out what to do about it when rebasing on
top of your version.

Thierry
diff mbox

Patch

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index c6e3cffdf89b..81ae923cd53a 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -77,6 +77,7 @@  struct connector {
 	drmModeConnector *connector;
 	drmModeObjectProperties *props;
 	drmModePropertyRes **props_info;
+	char *name;
 };
 
 struct fb {
@@ -372,18 +373,18 @@  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;
 		if (!connector)
 			continue;
 
-		printf("%d\t%d\t%s\t%s\t%dx%d\t\t%d\t",
+		printf("%d\t%d\t%s\t%-15s\t%dx%d\t\t%d\t",
 		       connector->connector_id,
 		       connector->encoder_id,
 		       connector_status_str(connector->connection),
-		       connector_type_str(connector->connector_type),
+		       _connector->name,
 		       connector->mmWidth, connector->mmHeight,
 		       connector->count_modes);
 
@@ -509,12 +510,13 @@  static void dump_planes(struct device *dev)
 
 static void free_resources(struct resources *res)
 {
+	int i;
+
 	if (!res)
 		return;
 
 #define free_resource(_res, __res, type, Type)					\
 	do {									\
-		int i;								\
 		if (!(_res)->type##s)						\
 			break;							\
 		for (i = 0; i < (int)(_res)->__res->count_##type##s; ++i) {	\
@@ -527,7 +529,6 @@  static void free_resources(struct resources *res)
 
 #define free_properties(_res, __res, type)					\
 	do {									\
-		int i;								\
 		for (i = 0; i < (int)(_res)->__res->count_##type##s; ++i) {	\
 			drmModeFreeObjectProperties(res->type##s[i].props);	\
 			free(res->type##s[i].props_info);			\
@@ -539,6 +540,10 @@  static void free_resources(struct resources *res)
 
 		free_resource(res, res, crtc, Crtc);
 		free_resource(res, res, encoder, Encoder);
+
+		for (i = 0; i < res->res->count_connectors; i++)
+			free(res->connectors[i].name);
+
 		free_resource(res, res, connector, Connector);
 		free_resource(res, res, fb, FB);
 
@@ -600,6 +605,15 @@  static struct resources *get_resources(struct device *dev)
 	get_resource(res, res, connector, Connector);
 	get_resource(res, res, fb, FB);
 
+	/* Set the name of all connectors based on the type name and the per-type ID. */
+	for (i = 0; i < res->res->count_connectors; i++) {
+		struct connector *connector = &res->connectors[i];
+
+		asprintf(&connector->name, "%s-%u",
+			 connector_type_str(connector->connector->connector_type),
+			 connector->connector->connector_type_id);
+	}
+
 #define get_properties(_res, __res, type, Type)					\
 	do {									\
 		int i;								\
@@ -666,6 +680,21 @@  static int get_crtc_index(struct device *dev, uint32_t id)
 	return -1;
 }
 
+static drmModeConnector *get_connector_by_name(struct device *dev, const char *name)
+{
+	struct connector *connector;
+	int i;
+
+	for (i = 0; i < dev->resources->res->count_connectors; i++) {
+		connector = &dev->resources->connectors[i];
+
+		if (strcmp(connector->name, name) == 0)
+			return connector->connector;
+	}
+
+	return NULL;
+}
+
 static drmModeConnector *get_connector_by_id(struct device *dev, uint32_t id)
 {
 	drmModeConnector *connector;
@@ -706,6 +735,7 @@  static drmModeEncoder *get_encoder_by_id(struct device *dev, uint32_t id)
  * can bind it with a free crtc.
  */
 struct pipe_arg {
+	const char **cons;
 	uint32_t *con_ids;
 	unsigned int num_cons;
 	uint32_t crtc_id;
@@ -821,8 +851,8 @@  static int pipe_find_crtc_and_mode(struct device *dev, struct pipe_arg *pipe)
 					   pipe->mode_str, pipe->vrefresh);
 		if (mode == NULL) {
 			fprintf(stderr,
-				"failed to find mode \"%s\" for connector %u\n",
-				pipe->mode_str, pipe->con_ids[i]);
+				"failed to find mode \"%s\" for connector %s\n",
+				pipe->mode_str, pipe->cons[i]);
 			return -EINVAL;
 		}
 	}
@@ -1126,7 +1156,7 @@  static void set_mode(struct device *dev, struct pipe_arg *pipes, unsigned int co
 		printf("setting mode %s-%dHz@%s on connectors ",
 		       pipe->mode_str, pipe->mode->vrefresh, pipe->format_str);
 		for (j = 0; j < pipe->num_cons; ++j)
-			printf("%u, ", pipe->con_ids[j]);
+			printf("%s, ", pipe->cons[j]);
 		printf("crtc %d\n", pipe->crtc->crtc->crtc_id);
 
 		ret = drmModeSetCrtc(dev->fd, pipe->crtc->crtc->crtc_id, fb_id,
@@ -1315,18 +1345,24 @@  static int parse_connector(struct pipe_arg *pipe, const char *arg)
 
 	/* Count the number of connectors and allocate them. */
 	pipe->num_cons = 1;
-	for (p = arg; isdigit(*p) || *p == ','; ++p) {
+	for (p = arg; *p && *p != ':' && *p != '@'; ++p) {
 		if (*p == ',')
 			pipe->num_cons++;
 	}
 
 	pipe->con_ids = calloc(pipe->num_cons, sizeof(*pipe->con_ids));
-	if (pipe->con_ids == NULL)
+	pipe->cons = calloc(pipe->num_cons, sizeof(*pipe->cons));
+	if (pipe->con_ids == NULL || pipe->cons == NULL)
 		return -1;
 
 	/* Parse the connectors. */
 	for (i = 0, p = arg; i < pipe->num_cons; ++i, p = endp + 1) {
-		pipe->con_ids[i] = strtoul(p, &endp, 10);
+		endp = strpbrk(p, ",@:");
+		if (!endp)
+			break;
+
+		pipe->cons[i] = strndup(p, endp - p);
+
 		if (*endp != ',')
 			break;
 	}
@@ -1488,6 +1524,32 @@  static int cursor_supported(void)
 	return 1;
 }
 
+static int pipe_resolve_connectors(struct device *dev, struct pipe_arg *pipe)
+{
+	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]) {
+			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;
+}
+
 static char optstr[] = "cdD:efM:P:ps:Cvw:";
 
 int main(int argc, char **argv)
@@ -1503,7 +1565,7 @@  int main(int argc, char **argv)
 	char *device = NULL;
 	char *module = NULL;
 	unsigned int i;
-	int count = 0, plane_count = 0;
+	unsigned int count = 0, plane_count = 0;
 	unsigned int prop_count = 0;
 	struct pipe_arg *pipe_args = NULL;
 	struct plane_arg *plane_args = NULL;
@@ -1645,6 +1707,14 @@  int main(int argc, char **argv)
 		return 1;
 	}
 
+	for (i = 0; i < count; i++) {
+		if (pipe_resolve_connectors(&dev, &pipe_args[i]) < 0) {
+			free_resources(dev.resources);
+			drmClose(dev.fd);
+			return 1;
+		}
+	}
+
 #define dump_resource(dev, res) if (res) dump_##res(dev)
 
 	dump_resource(&dev, encoders);