[PATCH v3 0/4] modetest enhancements

[PATCH v3 0/4] modetest enhancements

[Laurent Pinchart]

Hello,

Here's the third version of 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.

This version enables compiler warnings for modetest and fix the many warning
messages it creates. This should make sure that no v4 will be needed to fix a
stupid issue.

Laurent Pinchart (4):
  modetest: Fix warnings
  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/Makefile.am |   4 +-
 tests/modetest/buffers.c   |  13 ++---
 tests/modetest/buffers.h   |   5 +-
 tests/modetest/modetest.c  | 132 +++++++++++++++++++++++++--------------------
 4 files changed, 88 insertions(+), 66 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v3 1/4] modetest: Fix warnings

[Laurent Pinchart]

Enable all standard automake warnings except for -Wpointer-arith (as the
test pattern generation code uses void pointer arithmetics) and fix
them.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 tests/modetest/Makefile.am |  4 +++-
 tests/modetest/buffers.c   | 13 +++++++------
 tests/modetest/buffers.h   |  5 +++--
 tests/modetest/modetest.c  | 44 ++++++++++++++++++++++----------------------
 4 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/tests/modetest/Makefile.am b/tests/modetest/Makefile.am
index 065ae13..9376767 100644
--- a/tests/modetest/Makefile.am
+++ b/tests/modetest/Makefile.am
@@ -1,4 +1,6 @@
-AM_CFLAGS = \
+AM_CFLAGS = $(filter-out -Wpointer-arith, $(WARN_CFLAGS))
+
+AM_CFLAGS += \
 	-I$(top_srcdir)/include/drm \
 	-I$(top_srcdir)/libkms/ \
 	-I$(top_srcdir)
diff --git a/tests/modetest/buffers.c b/tests/modetest/buffers.c
index 5086381..3c93994 100644
--- a/tests/modetest/buffers.c
+++ b/tests/modetest/buffers.c
@@ -882,8 +882,8 @@ fill_pattern(unsigned int format, enum fill_pattern pattern, void *planes[3],
  */
 
 static struct kms_bo *
-allocate_buffer(struct kms_driver *kms,
-		int width, int height, int *stride)
+allocate_buffer(struct kms_driver *kms, unsigned int width, unsigned int height,
+		unsigned int *stride)
 {
 	struct kms_bo *bo;
 	unsigned bo_attribs[] = {
@@ -917,13 +917,14 @@ allocate_buffer(struct kms_driver *kms,
 
 struct kms_bo *
 create_test_buffer(struct kms_driver *kms, unsigned int format,
-		   int width, int height, int handles[4],
-		   int pitches[4], int offsets[4], enum fill_pattern pattern)
+		   unsigned int width, unsigned int height,
+		   unsigned int handles[4], unsigned int pitches[4],
+		   unsigned int offsets[4], enum fill_pattern pattern)
 {
 	struct kms_bo *bo;
-	int ret, stride;
-	void *planes[3];
+	void *planes[3] = { 0, };
 	void *virtual;
+	int ret;
 
 	bo = allocate_buffer(kms, width, height, &pitches[0]);
 	if (!bo)
diff --git a/tests/modetest/buffers.h b/tests/modetest/buffers.h
index 2b15ce5..e320389 100644
--- a/tests/modetest/buffers.h
+++ b/tests/modetest/buffers.h
@@ -37,8 +37,9 @@ enum fill_pattern {
 };
 
 struct kms_bo *create_test_buffer(struct kms_driver *kms, unsigned int format,
-		   int width, int height, int handles[4], int pitches[4],
-		   int offsets[4], enum fill_pattern pattern);
+		   unsigned int width, unsigned int height,
+		   unsigned int handles[4], unsigned int pitches[4],
+		   unsigned int offsets[4], enum fill_pattern pattern);
 
 unsigned int format_fourcc(const char *name);
 
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index c91bb9d..1081c78 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -64,11 +64,11 @@ int fd, modes;
 
 struct type_name {
 	int type;
-	char *name;
+	const char *name;
 };
 
 #define type_name_fn(res) \
-char * res##_str(int type) {			\
+const char * res##_str(int type) {			\
 	unsigned int i;					\
 	for (i = 0; i < ARRAY_SIZE(res##_names); i++) { \
 		if (res##_names[i].type == type)	\
@@ -85,7 +85,7 @@ struct type_name encoder_type_names[] = {
 	{ DRM_MODE_ENCODER_TVDAC, "TVDAC" },
 };
 
-type_name_fn(encoder_type)
+static type_name_fn(encoder_type)
 
 struct type_name connector_status_names[] = {
 	{ DRM_MODE_CONNECTED, "connected" },
@@ -93,7 +93,7 @@ struct type_name connector_status_names[] = {
 	{ DRM_MODE_UNKNOWNCONNECTION, "unknown" },
 };
 
-type_name_fn(connector_status)
+static type_name_fn(connector_status)
 
 struct type_name connector_type_names[] = {
 	{ DRM_MODE_CONNECTOR_Unknown, "unknown" },
@@ -113,11 +113,11 @@ struct type_name connector_type_names[] = {
 	{ DRM_MODE_CONNECTOR_eDP, "embedded displayport" },
 };
 
-type_name_fn(connector_type)
+static type_name_fn(connector_type)
 
 #define bit_name_fn(res)					\
-char * res##_str(int type) {					\
-	int i;							\
+const char * res##_str(int type) {				\
+	unsigned int i;						\
 	const char *sep = "";					\
 	for (i = 0; i < ARRAY_SIZE(res##_names); i++) {		\
 		if (type & (1 << i)) {				\
@@ -138,7 +138,7 @@ static const char *mode_type_names[] = {
 	"driver",
 };
 
-bit_name_fn(mode_type)
+static bit_name_fn(mode_type)
 
 static const char *mode_flag_names[] = {
 	"phsync",
@@ -157,9 +157,9 @@ static const char *mode_flag_names[] = {
 	"clkdiv2"
 };
 
-bit_name_fn(mode_flag)
+static bit_name_fn(mode_flag)
 
-void dump_encoders(void)
+static void dump_encoders(void)
 {
 	drmModeEncoder *encoder;
 	int i;
@@ -185,7 +185,7 @@ void dump_encoders(void)
 	printf("\n");
 }
 
-void dump_mode(drmModeModeInfo *mode)
+static void dump_mode(drmModeModeInfo *mode)
 {
 	printf("  %s %d %d %d %d %d %d %d %d %d",
 	       mode->name,
@@ -301,7 +301,7 @@ dump_prop(uint32_t prop_id, uint64_t value)
 	drmModeFreeProperty(prop);
 }
 
-void dump_connectors(void)
+static void dump_connectors(void)
 {
 	drmModeConnector *connector;
 	int i, j;
@@ -347,7 +347,7 @@ void dump_connectors(void)
 	printf("\n");
 }
 
-void dump_crtcs(void)
+static void dump_crtcs(void)
 {
 	drmModeCrtc *crtc;
 	drmModeObjectPropertiesPtr props;
@@ -389,7 +389,7 @@ void dump_crtcs(void)
 	printf("\n");
 }
 
-void dump_framebuffers(void)
+static void dump_framebuffers(void)
 {
 	drmModeFB *fb;
 	int i;
@@ -574,7 +574,7 @@ connector_find_mode(struct connector *c)
 
 	/* and figure out which crtc index it is: */
 	for (i = 0; i < resources->count_crtcs; i++) {
-		if (c->crtc == resources->crtcs[i]) {
+		if (c->crtc == (int)resources->crtcs[i]) {
 			c->pipe = i;
 			break;
 		}
@@ -584,7 +584,7 @@ connector_find_mode(struct connector *c)
 
 /* -------------------------------------------------------------------------- */
 
-void
+static void
 page_flip_handler(int fd, unsigned int frame,
 		  unsigned int sec, unsigned int usec, void *data)
 {
@@ -622,7 +622,7 @@ set_plane(struct kms_driver *kms, struct connector *c, struct plane *p)
 	uint32_t plane_id = 0;
 	struct kms_bo *plane_bo;
 	uint32_t plane_flags = 0;
-	int ret, crtc_x, crtc_y, crtc_w, crtc_h;
+	int crtc_x, crtc_y, crtc_w, crtc_h;
 	unsigned int i;
 
 	/* find an unused plane which can be connected to our crtc */
@@ -861,7 +861,7 @@ static int parse_connector(struct connector *c, const char *arg)
 	arg = endp + 1;
 
 	p = strchrnul(arg, '@');
-	len = min(sizeof c->mode_str - 1, p - arg);
+	len = min(sizeof c->mode_str - 1, (unsigned int)(p - arg));
 	strncpy(c->mode_str, arg, len);
 	c->mode_str[len] = '\0';
 
@@ -883,7 +883,7 @@ static int parse_plane(struct plane *p, const char *arg)
 {
 	strcpy(p->format_str, "XR24");
 
-	if (sscanf(arg, "%d:%dx%d@%4s", &p->con_id, &p->w, &p->h, &p->format_str) != 4 &&
+	if (sscanf(arg, "%d:%dx%d@%4s", &p->con_id, &p->w, &p->h, p->format_str) != 4 &&
 	    sscanf(arg, "%d:%dx%d", &p->con_id, &p->w, &p->h) != 3)
 		return -1;
 
@@ -896,7 +896,7 @@ static int parse_plane(struct plane *p, const char *arg)
 	return 0;
 }
 
-void usage(char *name)
+static void usage(char *name)
 {
 	fprintf(stderr, "usage: %s [-ecpmf]\n", name);
 	fprintf(stderr, "\t-e\tlist encoders\n");
@@ -939,11 +939,11 @@ int main(int argc, char **argv)
 	int c;
 	int encoders = 0, connectors = 0, crtcs = 0, planes = 0, framebuffers = 0;
 	int test_vsync = 0;
-	char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "omapdrm", "exynos" };
+	const char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "omapdrm", "exynos" };
 	unsigned int i;
 	int count = 0, plane_count = 0;
 	struct connector con_args[2];
-	struct plane plane_args[2] = {0};
+	struct plane plane_args[2] = { { 0, }, };
 	
 	opterr = 0;
 	while ((c = getopt(argc, argv, optstr)) != -1) {
-- 
1.7.12.4

[PATCH v3 2/4] 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>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 tests/modetest/modetest.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 1081c78..92c758b 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 v3 3/4] 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>
Reviewed-by: Jani Nikula <jani.nikula@intel.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 92c758b..f77261d 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)
 
 static 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 v3 4/4] 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>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 tests/modetest/modetest.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index f77261d..dbba77c 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -908,6 +908,9 @@ static 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;
 	const 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]);
-		fd = drmOpen(modules[i], NULL);
+	if (module) {
+		fd = drmOpen(module, NULL);
 		if (fd < 0) {
-			printf("failed.\n");
-		} else {
-			printf("success.\n");
-			break;
+			fprintf(stderr, "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) {
+			fprintf(stderr, "no device found.\n");
+			return 1;
 		}
 	}
 
@@ -1005,11 +1025,6 @@ int main(int argc, char **argv)
 		return -1;
 	}
 
-	if (i == ARRAY_SIZE(modules)) {
-		fprintf(stderr, "failed to load any modules, aborting.\n");
-		return -1;
-	}
-
 	resources = drmModeGetResources(fd);
 	if (!resources) {
 		fprintf(stderr, "drmModeGetResources failed: %s\n",
-- 
1.7.12.4