[PATCH 0/3] modetest: Allow selecting a driver manually

[PATCH 0/3] modetest: Allow selecting a driver manually

[Laurent Pinchart]

Hello,

Here's a small patch set that adds an argument to the modetest command line to
specify the driver name instead of using the builtin list of drivers. This is
particularly handy during development of new DRM/KMS drivers.

Laurent Pinchart (3):
  modetest: Remove extern declarations of opt(arg|ind|err|opt)
  modetest: Sort command line arguments
  modetest: Add a command line parameter to select the driver

 tests/modetest/modetest.c | 81 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 29 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/3] modetest: Remove extern declarations of opt(arg|ind|err|opt)

[Laurent Pinchart]

Those variables are declared in unistd.h, there's no need to redeclare
them here.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 tests/modetest/modetest.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index c91bb9d..489918e 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -835,8 +835,6 @@ set_mode(struct connector *c, int count, struct plane *p, int plane_count,
 	kms_destroy(&kms);
 }
 
-extern char *optarg;
-extern int optind, opterr, optopt;
 static char optstr[] = "ecpmfs:P:v";
 
 #define min(a, b)	((a) < (b) ? (a) : (b))
-- 
1.7.12.4

[PATCH 2/3] modetest: Sort command line arguments

[Laurent Pinchart]

The current mostly random sort order hinders code readability. Sort the
options alphabetically in the code, and by group in the help message.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 tests/modetest/modetest.c | 49 ++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 489918e..34457e2 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -835,8 +835,6 @@ set_mode(struct connector *c, int count, struct plane *p, int plane_count,
 	kms_destroy(&kms);
 }
 
-static char optstr[] = "ecpmfs:P:v";
-
 #define min(a, b)	((a) < (b) ? (a) : (b))
 
 static int parse_connector(struct connector *c, const char *arg)
@@ -896,15 +894,20 @@ static int parse_plane(struct plane *p, const char *arg)
 
 void usage(char *name)
 {
-	fprintf(stderr, "usage: %s [-ecpmf]\n", name);
-	fprintf(stderr, "\t-e\tlist encoders\n");
+	fprintf(stderr, "usage: %s [-cefmPpsv]\n", name);
+
+	fprintf(stderr, "\n Query options:\n\n");
 	fprintf(stderr, "\t-c\tlist connectors\n");
-	fprintf(stderr, "\t-p\tlist CRTCs and planes (pipes)\n");
-	fprintf(stderr, "\t-m\tlist modes\n");
+	fprintf(stderr, "\t-e\tlist encoders\n");
 	fprintf(stderr, "\t-f\tlist framebuffers\n");
-	fprintf(stderr, "\t-v\ttest vsynced page flipping\n");
-	fprintf(stderr, "\t-s <connector_id>[@<crtc_id>]:<mode>[@<format>]\tset a mode\n");
+	fprintf(stderr, "\t-m\tlist modes\n");
+
+	fprintf(stderr, "\n Test options:\n\n");
+	fprintf(stderr, "\t-p\tlist CRTCs and planes (pipes)\n");
 	fprintf(stderr, "\t-P <connector_id>:<w>x<h>[@<format>]\tset a plane\n");
+	fprintf(stderr, "\t-s <connector_id>[@<crtc_id>]:<mode>[@<format>]\tset a mode\n");
+	fprintf(stderr, "\t-v\ttest vsynced page flipping\n");
+
 	fprintf(stderr, "\n\tDefault is to dump all info.\n");
 	exit(0);
 }
@@ -932,6 +935,8 @@ static int page_flipping_supported(void)
 #endif
 }
 
+static char optstr[] = "cefmP:ps:v";
+
 int main(int argc, char **argv)
 {
 	int c;
@@ -946,34 +951,34 @@ int main(int argc, char **argv)
 	opterr = 0;
 	while ((c = getopt(argc, argv, optstr)) != -1) {
 		switch (c) {
-		case 'e':
-			encoders = 1;
-			break;
 		case 'c':
 			connectors = 1;
 			break;
-		case 'p':
-			crtcs = 1;
-			planes = 1;
+		case 'e':
+			encoders = 1;
+			break;
+		case 'f':
+			framebuffers = 1;
 			break;
 		case 'm':
 			modes = 1;
 			break;
-		case 'f':
-			framebuffers = 1;
+		case 'P':
+			if (parse_plane(&plane_args[plane_count], optarg) < 0)
+				usage(argv[0]);
+			plane_count++;
 			break;
-		case 'v':
-			test_vsync = 1;
+		case 'p':
+			crtcs = 1;
+			planes = 1;
 			break;
 		case 's':
 			if (parse_connector(&con_args[count], optarg) < 0)
 				usage(argv[0]);
 			count++;				      
 			break;
-		case 'P':
-			if (parse_plane(&plane_args[plane_count], optarg) < 0)
-				usage(argv[0]);
-			plane_count++;
+		case 'v':
+			test_vsync = 1;
 			break;
 		default:
 			usage(argv[0]);
-- 
1.7.12.4

[PATCH 3/3] modetest: Add a command line parameter to select the driver

[Laurent Pinchart]

If the -M parameter is specific, modetest will use the requested device
name instead of trying its builtin list of device names.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 tests/modetest/modetest.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 34457e2..b6298fc 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -908,6 +908,9 @@ void usage(char *name)
 	fprintf(stderr, "\t-s <connector_id>[@<crtc_id>]:<mode>[@<format>]\tset a mode\n");
 	fprintf(stderr, "\t-v\ttest vsynced page flipping\n");
 
+	fprintf(stderr, "\n Generic options:\n\n");
+	fprintf(stderr, "\t-M module\tuse the given driver\n");
+
 	fprintf(stderr, "\n\tDefault is to dump all info.\n");
 	exit(0);
 }
@@ -935,7 +938,7 @@ static int page_flipping_supported(void)
 #endif
 }
 
-static char optstr[] = "cefmP:ps:v";
+static char optstr[] = "cefM:mP:ps:v";
 
 int main(int argc, char **argv)
 {
@@ -943,6 +946,7 @@ int main(int argc, char **argv)
 	int encoders = 0, connectors = 0, crtcs = 0, planes = 0, framebuffers = 0;
 	int test_vsync = 0;
 	char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "omapdrm", "exynos" };
+	char *module = NULL;
 	unsigned int i;
 	int count = 0, plane_count = 0;
 	struct connector con_args[2];
@@ -960,6 +964,9 @@ int main(int argc, char **argv)
 		case 'f':
 			framebuffers = 1;
 			break;
+		case 'M':
+			module = optarg;
+			break;
 		case 'm':
 			modes = 1;
 			break;
@@ -989,14 +996,27 @@ int main(int argc, char **argv)
 	if (argc == 1)
 		encoders = connectors = crtcs = planes = modes = framebuffers = 1;
 
-	for (i = 0; i < ARRAY_SIZE(modules); i++) {
-		printf("trying to load module %s...", modules[i]);
+	if (module) {
 		fd = drmOpen(modules[i], NULL);
 		if (fd < 0) {
-			printf("failed.\n");
-		} else {
-			printf("success.\n");
-			break;
+			printf("failed to open device '%s'.\n", module);
+			return 1;
+		}
+	} else {
+		for (i = 0; i < ARRAY_SIZE(modules); i++) {
+			printf("trying to open device '%s'...", modules[i]);
+			fd = drmOpen(modules[i], NULL);
+			if (fd < 0) {
+				printf("failed.\n");
+			} else {
+				printf("success.\n");
+				break;
+			}
+		}
+
+		if (fd < 0) {
+			printf("no device found.\n", module);
+			return 1;
 		}
 	}
 
-- 
1.7.12.4

Re: [PATCH 3/3] modetest: Add a command line parameter to select the driver

[Jani Nikula]

On Fri, 08 Feb 2013, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> If the -M parameter is specific, modetest will use the requested device
> name instead of trying its builtin list of device names.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  tests/modetest/modetest.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> index 34457e2..b6298fc 100644
> --- a/tests/modetest/modetest.c
> +++ b/tests/modetest/modetest.c
> @@ -908,6 +908,9 @@ void usage(char *name)
>  	fprintf(stderr, "\t-s <connector_id>[@<crtc_id>]:<mode>[@<format>]\tset a mode\n");
>  	fprintf(stderr, "\t-v\ttest vsynced page flipping\n");
>  
> +	fprintf(stderr, "\n Generic options:\n\n");
> +	fprintf(stderr, "\t-M module\tuse the given driver\n");
> +
>  	fprintf(stderr, "\n\tDefault is to dump all info.\n");
>  	exit(0);
>  }
> @@ -935,7 +938,7 @@ static int page_flipping_supported(void)
>  #endif
>  }
>  
> -static char optstr[] = "cefmP:ps:v";
> +static char optstr[] = "cefM:mP:ps:v";
>  
>  int main(int argc, char **argv)
>  {
> @@ -943,6 +946,7 @@ int main(int argc, char **argv)
>  	int encoders = 0, connectors = 0, crtcs = 0, planes = 0, framebuffers = 0;
>  	int test_vsync = 0;
>  	char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "omapdrm", "exynos" };
> +	char *module = NULL;
>  	unsigned int i;
>  	int count = 0, plane_count = 0;
>  	struct connector con_args[2];
> @@ -960,6 +964,9 @@ int main(int argc, char **argv)
>  		case 'f':
>  			framebuffers = 1;
>  			break;
> +		case 'M':
> +			module = optarg;
> +			break;
>  		case 'm':
>  			modes = 1;
>  			break;
> @@ -989,14 +996,27 @@ int main(int argc, char **argv)
>  	if (argc == 1)
>  		encoders = connectors = crtcs = planes = modes = framebuffers = 1;
>  
> -	for (i = 0; i < ARRAY_SIZE(modules); i++) {
> -		printf("trying to load module %s...", modules[i]);
> +	if (module) {
>  		fd = drmOpen(modules[i], NULL);

If this worked for you, I presume you have some uncommitted changes in
your tree. ;) The compiler should cry about uninitialized use of i too.

fd = drmOpen(module, NULL); ?

>  		if (fd < 0) {
> -			printf("failed.\n");
> -		} else {
> -			printf("success.\n");
> -			break;
> +			printf("failed to open device '%s'.\n", module);
> +			return 1;
> +		}
> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(modules); i++) {
> +			printf("trying to open device '%s'...", modules[i]);
> +			fd = drmOpen(modules[i], NULL);
> +			if (fd < 0) {
> +				printf("failed.\n");
> +			} else {
> +				printf("success.\n");
> +				break;
> +			}
> +		}
> +
> +		if (fd < 0) {
> +			printf("no device found.\n", module);

Extra param to printf.


BR,
Jani.

> +			return 1;
>  		}
>  	}
>  
> -- 
> 1.7.12.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] modetest: Add a command line parameter to select the driver

[Laurent Pinchart]

Hi Jani,

On Friday 08 February 2013 15:15:50 Jani Nikula wrote:
> On Fri, 08 Feb 2013, Laurent Pinchart wrote:
> > If the -M parameter is specific, modetest will use the requested device
> > name instead of trying its builtin list of device names.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  tests/modetest/modetest.c | 34 +++++++++++++++++++++++++++-------
> >  1 file changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > index 34457e2..b6298fc 100644
> > --- a/tests/modetest/modetest.c
> > +++ b/tests/modetest/modetest.c

[snip]

> > @@ -989,14 +996,27 @@ int main(int argc, char **argv)
> >  	if (argc == 1)
> >  		encoders = connectors = crtcs = planes = modes = framebuffers =
> >  		1;
> > 
> > -	for (i = 0; i < ARRAY_SIZE(modules); i++) {
> > -		printf("trying to load module %s...", modules[i]);
> > +	if (module) {
> > 
> >  		fd = drmOpen(modules[i], NULL);
> 
> If this worked for you, I presume you have some uncommitted changes in
> your tree. ;) The compiler should cry about uninitialized use of i too.
> 
> fd = drmOpen(module, NULL); ?

Oops :-)

modetest is compiled without any -W flag. I'll send a v4 that enables warnings 
and fixes them (please ignore the v3, sorry for the noise).

> >  		if (fd < 0) {
> > 
> > -			printf("failed.\n");
> > -		} else {
> > -			printf("success.\n");
> > -			break;
> > +			printf("failed to open device '%s'.\n", module);
> > +			return 1;
> > +		}
> > +	} else {
> > +		for (i = 0; i < ARRAY_SIZE(modules); i++) {
> > +			printf("trying to open device '%s'...", modules[i]);
> > +			fd = drmOpen(modules[i], NULL);
> > +			if (fd < 0) {
> > +				printf("failed.\n");
> > +			} else {
> > +				printf("success.\n");
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (fd < 0) {
> > +			printf("no device found.\n", module);
> 
> Extra param to printf.

Will be fixed in v4.

-- 
Regards,

Laurent Pinchart