More fixes for igt pm_rps

PATCH In review [email protected] 2 participants 7 messages Jan 17, 2014 – Jan 19, 2014

Share & tools

[email protected]

author

2014-01-17 22:49 UTC #0 lore raw

Jeff McGee (3):
pm_rps: Add read back checking on sysfs writes
pm_rps: Convert to subtest structure
pm_rps: Fix verbose option and streamline its use

 tests/pm_rps.c | 142 +++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 92 insertions(+), 50 deletions(-)

Signature · 1 line

1.8.5.2

[email protected]

author

2014-01-17 22:49 UTC #1 lore raw

PATCH 1/3 pm_rps: Add read back checking on sysfs writes

From: Jeff McGee <[email protected]>

Signed-off-by Jeff McGee <[email protected]>

---
 tests/pm_rps.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

tests/pm_rps.c +6 -1

index 7c739b6..ec07dd2 100644
@@ -68,15 +68,20 @@ static int readval(FILE *filp)
 
 static int do_writeval(FILE *filp, int val, int lerrno)
 {
-	int ret;
+	int ret, orig;
+
+	orig = readval(filp);
 	rewind(filp);
 	ret = fprintf(filp, "%d", val);
+
 	if (lerrno) {
 		/* Expecting specific error */
 		igt_assert(ret == EOF && errno == lerrno);
+		igt_assert(readval(filp) == orig);
 	} else {
 		/* Expecting no error */
 		igt_assert(ret != EOF);
+		igt_assert(readval(filp) == val);
 	}
 
 	return ret;

Signature · 1 line

1.8.5.2

[email protected]

author

2014-01-17 22:49 UTC #2 lore raw

PATCH 2/3 pm_rps: Convert to subtest structure

In reply to [email protected]

From: Jeff McGee <[email protected]>

Signed-off-by Jeff McGee <[email protected]>

---
 tests/pm_rps.c | 119 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 62 insertions(+), 57 deletions(-)

tests/pm_rps.c +62 ~~-57~~

index ec07dd2..4d64f0a 100644
@@ -142,66 +142,71 @@ static void pm_rps_exit_handler(int sig)
 	}
 }
 
-igt_simple_main
+igt_main
 {
-	const int device = drm_get_card();
-	struct junk *junk = stuff;
-	int fd, ret;
-
 	igt_skip_on_simulation();
 
-	/* Use drm_open_any to verify device existence */
-	fd = drm_open_any();
-	close(fd);
-
-	do {
-		int val = -1;
-		char *path;
-		ret = asprintf(&path, sysfs_base_path, device, junk->name);
-		igt_assert(ret != -1);
-		junk->filp = fopen(path, junk->mode);
-		igt_require(junk->filp);
-		setbuf(junk->filp, NULL);
-
-		val = readval(junk->filp);
-		igt_assert(val >= 0);
-		junk++;
-	} while(junk->name != NULL);
-
-	origmin = fmin;
-	origmax = fmax;
-
-	igt_install_exit_handler(pm_rps_exit_handler);
-
-	if (verbose)
-		printf("Original min = %d\nOriginal max = %d\n", origmin, origmax);
-
-	if (verbose)
-		dumpit();
-
-	checkit();
-	setfreq(origmin);
-	if (verbose)
-		dumpit();
-	igt_assert(fcur == fmin);
-	setfreq(origmax);
-	if (verbose)
-		dumpit();
-	igt_assert(fcur == fmax);
-	checkit();
-
-	/* And some errors */
-	writeval_inval(stuff[MIN].filp, frpn - 1);
-	writeval_inval(stuff[MAX].filp, frp0 + 1000);
-	checkit();
-
-	writeval_inval(stuff[MIN].filp, fmax + 1000);
-	writeval_inval(stuff[MAX].filp, fmin - 1);
-	checkit();
+	igt_fixture {
+		const int device = drm_get_card();
+		struct junk *junk = stuff;
+		int fd, ret;
+
+		/* Use drm_open_any to verify device existence */
+		fd = drm_open_any();
+		close(fd);
+
+		do {
+			int val = -1;
+			char *path;
+			ret = asprintf(&path, sysfs_base_path, device, junk->name);
+			igt_assert(ret != -1);
+			junk->filp = fopen(path, junk->mode);
+			igt_require(junk->filp);
+			setbuf(junk->filp, NULL);
+
+			val = readval(junk->filp);
+			igt_assert(val >= 0);
+			junk++;
+		} while(junk->name != NULL);
+
+		origmin = fmin;
+		origmax = fmax;
+
+		igt_install_exit_handler(pm_rps_exit_handler);
+	}
 
-	writeval_inval(stuff[MIN].filp, 0x11111110);
-	writeval_inval(stuff[MAX].filp, 0);
+	igt_subtest("min-max-config-at-idle") {
+		if (verbose)
+			printf("Original min = %d\nOriginal max = %d\n",
+			        origmin, origmax);
+
+		if (verbose)
+			dumpit();
+
+		checkit();
+		setfreq(origmin);
+		if (verbose)
+			dumpit();
+		igt_assert(fcur == fmin);
+		setfreq(origmax);
+		if (verbose)
+			dumpit();
+		igt_assert(fcur == fmax);
+		checkit();
+
+		/* And some errors */
+		writeval_inval(stuff[MIN].filp, frpn - 1);
+		writeval_inval(stuff[MAX].filp, frp0 + 1000);
+		checkit();
+
+		writeval_inval(stuff[MIN].filp, fmax + 1000);
+		writeval_inval(stuff[MAX].filp, fmin - 1);
+		checkit();
+
+		writeval_inval(stuff[MIN].filp, 0x11111110);
+		writeval_inval(stuff[MAX].filp, 0);
 
-	writeval(stuff[MIN].filp, origmin);
-	writeval(stuff[MAX].filp, origmax);
+		writeval(stuff[MIN].filp, origmin);
+		writeval(stuff[MAX].filp, origmax);
+	}
 }

Signature · 1 line

1.8.5.2

Daniel Vetter

2014-01-19 14:29 UTC #3 lore raw

Re: [PATCH 2/3] pm_rps: Convert to subtest structure

Quoted 3 lines · On Fri, Jan 17, 2014 at 04:56:05PM -0600, [email protected] wrote:

From: Jeff McGee <[email protected]>
Signed-off-by Jeff McGee <[email protected]>

I've missed that you've forgotten to move the test to the _M Makefile target. Without that testrunners like piglit won't enumerate the subtests and run the individually. I've fixed this up now.
-Daniel

Quoted 144 lines

---
 tests/pm_rps.c | 119 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 62 insertions(+), 57 deletions(-)

tests/pm_rps.c +62 ~~-57~~

index ec07dd2..4d64f0a 100644
@@ -142,66 +142,71 @@ static void pm_rps_exit_handler(int sig)
 	}
 }
 
-igt_simple_main
+igt_main
 {
-	const int device = drm_get_card();
-	struct junk *junk = stuff;
-	int fd, ret;
-
 	igt_skip_on_simulation();
 
-	/* Use drm_open_any to verify device existence */
-	fd = drm_open_any();
-	close(fd);
-
-	do {
-		int val = -1;
-		char *path;
-		ret = asprintf(&path, sysfs_base_path, device, junk->name);
-		igt_assert(ret != -1);
-		junk->filp = fopen(path, junk->mode);
-		igt_require(junk->filp);
-		setbuf(junk->filp, NULL);
-
-		val = readval(junk->filp);
-		igt_assert(val >= 0);
-		junk++;
-	} while(junk->name != NULL);
-
-	origmin = fmin;
-	origmax = fmax;
-
-	igt_install_exit_handler(pm_rps_exit_handler);
-
-	if (verbose)
-		printf("Original min = %d\nOriginal max = %d\n", origmin, origmax);
-
-	if (verbose)
-		dumpit();
-
-	checkit();
-	setfreq(origmin);
-	if (verbose)
-		dumpit();
-	igt_assert(fcur == fmin);
-	setfreq(origmax);
-	if (verbose)
-		dumpit();
-	igt_assert(fcur == fmax);
-	checkit();
-
-	/* And some errors */
-	writeval_inval(stuff[MIN].filp, frpn - 1);
-	writeval_inval(stuff[MAX].filp, frp0 + 1000);
-	checkit();
-
-	writeval_inval(stuff[MIN].filp, fmax + 1000);
-	writeval_inval(stuff[MAX].filp, fmin - 1);
-	checkit();
+	igt_fixture {
+		const int device = drm_get_card();
+		struct junk *junk = stuff;
+		int fd, ret;
+
+		/* Use drm_open_any to verify device existence */
+		fd = drm_open_any();
+		close(fd);
+
+		do {
+			int val = -1;
+			char *path;
+			ret = asprintf(&path, sysfs_base_path, device, junk->name);
+			igt_assert(ret != -1);
+			junk->filp = fopen(path, junk->mode);
+			igt_require(junk->filp);
+			setbuf(junk->filp, NULL);
+
+			val = readval(junk->filp);
+			igt_assert(val >= 0);
+			junk++;
+		} while(junk->name != NULL);
+
+		origmin = fmin;
+		origmax = fmax;
+
+		igt_install_exit_handler(pm_rps_exit_handler);
+	}
 
-	writeval_inval(stuff[MIN].filp, 0x11111110);
-	writeval_inval(stuff[MAX].filp, 0);
+	igt_subtest("min-max-config-at-idle") {
+		if (verbose)
+			printf("Original min = %d\nOriginal max = %d\n",
+			        origmin, origmax);
+
+		if (verbose)
+			dumpit();
+
+		checkit();
+		setfreq(origmin);
+		if (verbose)
+			dumpit();
+		igt_assert(fcur == fmin);
+		setfreq(origmax);
+		if (verbose)
+			dumpit();
+		igt_assert(fcur == fmax);
+		checkit();
+
+		/* And some errors */
+		writeval_inval(stuff[MIN].filp, frpn - 1);
+		writeval_inval(stuff[MAX].filp, frp0 + 1000);
+		checkit();
+
+		writeval_inval(stuff[MIN].filp, fmax + 1000);
+		writeval_inval(stuff[MAX].filp, fmin - 1);
+		checkit();
+
+		writeval_inval(stuff[MIN].filp, 0x11111110);
+		writeval_inval(stuff[MAX].filp, 0);
 
-	writeval(stuff[MIN].filp, origmin);
-	writeval(stuff[MAX].filp, origmax);
+		writeval(stuff[MIN].filp, origmin);
+		writeval(stuff[MAX].filp, origmax);
+	}
 }

Signature · 6 lines

1.8.5.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[email protected]

author

2014-01-17 22:49 UTC #4 lore raw

PATCH 3/3 pm_rps: Fix verbose option and streamline its use

In reply to [email protected]

From: Jeff McGee <[email protected]>

Signed-off-by Jeff McGee <[email protected]>

---
 tests/pm_rps.c | 52 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 10 deletions(-)

tests/pm_rps.c +42 ~~-10~~

index 4d64f0a..192dca7 100644
@@ -31,6 +31,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <getopt.h>
 #include "drmtest.h"
 
 static bool verbose = false;

@@ -130,6 +131,8 @@ static void dumpit(void)
 
 	printf("\n");
 }
+#define dump() if (verbose) dumpit()
+#define log(...) if (verbose) printf(__VA_ARGS__)
 
 static void pm_rps_exit_handler(int sig)
 {

@@ -142,8 +145,40 @@ static void pm_rps_exit_handler(int sig)
 	}
 }
 
-igt_main
+static int opt_handler(int opt, int opt_index)
 {
+	switch (opt) {
+	case 'v':
+		verbose = true;
+		break;
+	default:
+		assert(0);
+	}
+
+	return 0;
+}
+
+/* Mod of igt_subtest_init that adds our extra options */
+void subtest_init(int argc, char **argv)
+{
+	struct option long_opts[] = {
+		{"verbose", 0, 0, 'v'}
+	};
+	const char *help_str = "  -v, --verbose";
+	int ret;
+
+	ret = igt_subtest_init_parse_opts(argc, argv, "v", long_opts,
+					  help_str, opt_handler);
+
+	if (ret < 0)
+		/* exit with no error for -h/--help */
+		exit(ret == -1 ? 0 : ret);
+}
+
+int main(int argc, char **argv)
+{
+	subtest_init(argc, argv);
+
 	igt_skip_on_simulation();
 
 	igt_fixture {

@@ -176,21 +211,16 @@ igt_main
 	}
 
 	igt_subtest("min-max-config-at-idle") {
-		if (verbose)
-			printf("Original min = %d\nOriginal max = %d\n",
-			        origmin, origmax);
+		log("Original min = %d\nOriginal max = %d\n", origmin, origmax);
 
-		if (verbose)
-			dumpit();
+		dump();
 
 		checkit();
 		setfreq(origmin);
-		if (verbose)
-			dumpit();
+		dump();
 		igt_assert(fcur == fmin);
 		setfreq(origmax);
-		if (verbose)
-			dumpit();
+		dump();
 		igt_assert(fcur == fmax);
 		checkit();

@@ -209,4 +239,6 @@ igt_main
 		writeval(stuff[MIN].filp, origmin);
 		writeval(stuff[MAX].filp, origmax);
 	}
+
+	igt_exit();
 }

Signature · 1 line

1.8.5.2

Daniel Vetter

2014-01-18 14:23 UTC #5 lore raw

Re: [PATCH 3/3] pm_rps: Fix verbose option and streamline its use

Quoted 3 lines · On Fri, Jan 17, 2014 at 04:56:06PM -0600, [email protected] wrote:

From: Jeff McGee <[email protected]>
Signed-off-by Jeff McGee <[email protected]>

Ah, now I understand your question for a unified verbose handling. I guess if someone is bored (or if there's a need in testrunner frameworks for it) we could add a bit of infrastructure for verbose logging and similar things. I'll add a note on our i-g-t ideas wiki page.

All patches merged, thanks.
-Daniel

Quoted 107 lines

---
 tests/pm_rps.c | 52 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 10 deletions(-)

tests/pm_rps.c +42 ~~-10~~

index 4d64f0a..192dca7 100644
@@ -31,6 +31,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <getopt.h>
 #include "drmtest.h"
 
 static bool verbose = false;

@@ -130,6 +131,8 @@ static void dumpit(void)
 
 	printf("\n");
 }
+#define dump() if (verbose) dumpit()
+#define log(...) if (verbose) printf(__VA_ARGS__)
 
 static void pm_rps_exit_handler(int sig)
 {

@@ -142,8 +145,40 @@ static void pm_rps_exit_handler(int sig)
 	}
 }
 
-igt_main
+static int opt_handler(int opt, int opt_index)
 {
+	switch (opt) {
+	case 'v':
+		verbose = true;
+		break;
+	default:
+		assert(0);
+	}
+
+	return 0;
+}
+
+/* Mod of igt_subtest_init that adds our extra options */
+void subtest_init(int argc, char **argv)
+{
+	struct option long_opts[] = {
+		{"verbose", 0, 0, 'v'}
+	};
+	const char *help_str = "  -v, --verbose";
+	int ret;
+
+	ret = igt_subtest_init_parse_opts(argc, argv, "v", long_opts, 
+					  help_str, opt_handler);
+
+	if (ret < 0)
+		/* exit with no error for -h/--help */
+		exit(ret == -1 ? 0 : ret);
+}
+
+int main(int argc, char **argv)
+{
+	subtest_init(argc, argv);
+
 	igt_skip_on_simulation();
 
 	igt_fixture {

@@ -176,21 +211,16 @@ igt_main
 	}
 
 	igt_subtest("min-max-config-at-idle") {
-		if (verbose)
-			printf("Original min = %d\nOriginal max = %d\n",
-			        origmin, origmax);
+		log("Original min = %d\nOriginal max = %d\n", origmin, origmax);
 
-		if (verbose)
-			dumpit();
+		dump();
 
 		checkit();
 		setfreq(origmin);
-		if (verbose)
-			dumpit();
+		dump();
 		igt_assert(fcur == fmin);
 		setfreq(origmax);
-		if (verbose)
-			dumpit();
+		dump();
 		igt_assert(fcur == fmax);
 		checkit();

@@ -209,4 +239,6 @@ igt_main
 		writeval(stuff[MIN].filp, origmin);
 		writeval(stuff[MAX].filp, origmax);
 	}
+
+	igt_exit();
 }

Signature · 6 lines

1.8.5.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Daniel Vetter

2014-01-19 21:33 UTC #6 lore raw

Re: [PATCH 3/3] pm_rps: Fix verbose option and streamline its use

In reply to [email protected]

Quoted 38 lines · On Fri, Jan 17, 2014 at 04:56:06PM -0600, [email protected] wrote:

From: Jeff McGee <[email protected]>

Signed-off-by Jeff McGee <[email protected]>

---
 tests/pm_rps.c | 52 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 10 deletions(-)

tests/pm_rps.c +7 -1

index 4d64f0a..192dca7 100644
@@ -31,6 +31,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <getopt.h>
 #include "drmtest.h"
 
 static bool verbose = false;

@@ -130,6 +131,8 @@ static void dumpit(void)
 
 	printf("\n");
 }
+#define dump() if (verbose) dumpit()
+#define log(...) if (verbose) printf(__VA_ARGS__)
 
 static void pm_rps_exit_handler(int sig)
 {

@@ -142,8 +145,40 @@ static void pm_rps_exit_handler(int sig)
 	}
 }
 
-igt_main
+static int opt_handler(int opt, int opt_index)
 {
+	switch (opt) {
+	case 'v':
+		verbose = true;

tests/pm_rps.c static void pm_rps_exit_handler(int sig) · line 162

Quoted 10 lines

tests/pm_rps.c +10 -0

+		break;
+	default:
+		assert(0);
+	}
+
+	return 0;
+}
+
+/* Mod of igt_subtest_init that adds our extra options */
+void subtest_init(int argc, char **argv)

Missing static here. I've added it back in.
-Daniel

Quoted 62 lines

tests/pm_rps.c +25 -9

+{
+	struct option long_opts[] = {
+		{"verbose", 0, 0, 'v'}
+	};
+	const char *help_str = "  -v, --verbose";
+	int ret;
+
+	ret = igt_subtest_init_parse_opts(argc, argv, "v", long_opts,
+					  help_str, opt_handler);
+
+	if (ret < 0)
+		/* exit with no error for -h/--help */
+		exit(ret == -1 ? 0 : ret);
+}
+
+int main(int argc, char **argv)
+{
+	subtest_init(argc, argv);
+
 	igt_skip_on_simulation();
 
 	igt_fixture {

@@ -176,21 +211,16 @@ igt_main
 	}
 
 	igt_subtest("min-max-config-at-idle") {
-		if (verbose)
-			printf("Original min = %d\nOriginal max = %d\n",
-			        origmin, origmax);
+		log("Original min = %d\nOriginal max = %d\n", origmin, origmax);
 
-		if (verbose)
-			dumpit();
+		dump();
 
 		checkit();
 		setfreq(origmin);
-		if (verbose)
-			dumpit();
+		dump();
 		igt_assert(fcur == fmin);
 		setfreq(origmax);
-		if (verbose)
-			dumpit();
+		dump();
 		igt_assert(fcur == fmax);
 		checkit();

@@ -209,4 +239,6 @@ igt_main
 		writeval(stuff[MIN].filp, origmin);
 		writeval(stuff[MAX].filp, origmax);
 	}
+
+	igt_exit();
 }

Signature · 6 lines

1.8.5.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch