[PATCH] [I-G-T]Add rc6_residency_counter subtest

[PATCH] [I-G-T]Add rc6_residency_counter subtest

[Wendy Wang]

Move rc6_residency_check to subtest, add new rc6_residency_counter subtest
for pm_rc6_residency IGT case.

Test results run on platforms show as below:
On HSW
---------------------------------------
[root@x-hswu opt]# ./pm_rc6_residency
IGT-Version: 1.6-g35b31df (x86_64) (Linux: 3.15.0-rc3_drm-intel-nightly_0791a3_20140520+ x86_64)
Subtest rc6-residency-check: SUCCESS
This machine doesn't support rc6pp
This machine doesn't support rc6p
This machine entry  rc6 status.
The residency counter : 0.999667
Subtest rc6-residency-counter: SUCCESS

On IVB
----------------------------------------
[root@IVB tests]# ./pm_rc6_residency
IGT-Version: 1.6-g35b31df (x86_64) (Linux: 3.13.6_20140318+ x86_64)
Subtest rc6-residency-check: SUCCESS
This machine entry  rc6p status.
The residency counter : 0.997000
Subtest rc6-residency-counter: SUCCESS

On BYT
----------------------------------------
root@x-byt:/opt# ./pm_rc6_residency
IGT-Version: 1.6-g0d39021 (x86_64) (Linux: 3.14.0_kcloud_ceabbb_20140521+ x86_64)
Subtest rc6-residency-check: SUCCESS
This machine doesn't support rc6pp
This machine doesn't support rc6p
The residency counter : 1.144333
Test assertion failure function rc6_residency_counter, file pm_rc6_residency.c:131:
Last errno: 0, Success
Failed assertion: counter_result <=1
Debug files must be wrong,
Subtest rc6-residency-counter: FAIL

On BDW
---------------------------------------
[root@x-bdw01 opt]# ./pm_rc6_residency
IGT-Version: 1.6-g0d39021 (x86_64) (Linux: 3.15.0-rc5_drm-intel-nightly_367653_20140521+ x86_64)
Subtest rc6-residency-check: SUCCESS
This machine doesn't support rc6pp
This machine doesn't support rc6p
The residency counter : 0.994333
This machine entry rc6 state.
Subtest rc6-residency-counter: SUCCESS

Signed-off-by: Liu, Lei A <lei.a.liu@intel.com>, Wendy Wang <wendy.wang@intel.com>
Signed-off-by: Wendy Wang <wendy.wang@intel.com>

diff --git a/tests/pm_rc6_residency.c b/tests/pm_rc6_residency.c
index 197ab00..550e3ad 100644
--- a/tests/pm_rc6_residency.c
+++ b/tests/pm_rc6_residency.c
@@ -34,9 +34,11 @@
 
 #include "drmtest.h"
 
+#define NUMBER_OF_RC6_RESIDENCY 3
 #define SLEEP_DURATION 3000 // in milliseconds
 #define RC6_FUDGE 900 // in milliseconds
 
+
 static unsigned int readit(const char *path)
 {
 	unsigned int ret;
@@ -44,8 +46,10 @@ static unsigned int readit(const char *path)
 
 	FILE *file;
 	file = fopen(path, "r");
-	igt_assert_f(file,
-		     "Couldn't open %s (%d)\n", path, errno);
+	if (file == NULL) {
+		fprintf(stderr, "Couldn't open %s (%d)\n", path, errno);
+		abort();
+	}
 	scanned = fscanf(file, "%u", &ret);
 	igt_assert(scanned == 1);
 
@@ -54,62 +58,112 @@ static unsigned int readit(const char *path)
 	return ret;
 }
 
-igt_simple_main
+static void read_rc6_residency( int value[], const char *name_of_rc6_residency[])
 {
 	const int device = drm_get_card();
-	char *path, *pathp, *pathpp;
-	int fd, ret;
-	unsigned int value1, value1p, value1pp, value2, value2p, value2pp;
+	char *path ;
+	int  ret;
 	FILE *file;
-	int diff;
-
-	igt_skip_on_simulation();
-
-	/* Use drm_open_any to verify device existence */
-	fd = drm_open_any();
-	close(fd);
-
-	ret = asprintf(&path, "/sys/class/drm/card%d/power/rc6_enable", device);
-	igt_assert(ret != -1);
 
 	/* For some reason my ivb isn't idle even after syncing up with the gpu.
 	 * Let's add a sleept just to make it happy. */
 	sleep(5);
 
-	file = fopen(path, "r");
+	ret = asprintf(&path, "/sys/class/drm/card%d/power/rc6_enable", device);
+	igt_assert(ret != -1);
+
+	file = fopen(path, "r");//open
 	igt_require(file);
 
 	/* claim success if no rc6 enabled. */
 	if (readit(path) == 0)
 		igt_success();
 
-	ret = asprintf(&path, "/sys/class/drm/card%d/power/rc6_residency_ms", device);
-	igt_assert(ret != -1);
-	ret = asprintf(&pathp, "/sys/class/drm/card%d/power/rc6p_residency_ms", device);
-	igt_assert(ret != -1);
-	ret = asprintf(&pathpp, "/sys/class/drm/card%d/power/rc6pp_residency_ms", device);
-	igt_assert(ret != -1);
+	for(unsigned int i = 0; i < 6; i++)
+	{
+		if(i == 3)
+			sleep(SLEEP_DURATION / 1000);
+		ret = asprintf(&path, "/sys/class/drm/card%d/power/%s_residency_ms",device,name_of_rc6_residency[i % 3] );
+		igt_assert(ret != -1);
+		value[i] = readit(path);
+	}
+	free(path);
+}
 
-	value1 = readit(path);
-	value1p = readit(pathp);
-	value1pp = readit(pathpp);
-	sleep(SLEEP_DURATION / 1000);
-	value2 = readit(path);
-	value2p = readit(pathp);
-	value2pp = readit(pathpp);
+static void rc6_residency_counter(int value[],const char * name_of_rc6_residency[])
+{
+	unsigned int flag_counter,flag_support;
+	double  counter_result = 0;
+	flag_counter = 0;
+	flag_support = 0;
+
+	for(int flag = NUMBER_OF_RC6_RESIDENCY-1; flag >= 0 ; flag --)
+	{
+		unsigned int  tmp_counter, tmp_support;
+		double counter;
+		counter = ((double)value[flag + 3] - (double)value[flag]) /(double) SLEEP_DURATION;
+
+		if( counter > 0.9 ){
+			counter_result = counter;
+			tmp_counter = 1;
+		}
+		else
+			tmp_counter = 0;
+
+		if( value [flag + 3] == 0){
+			tmp_support = 0;
+			printf("This machine doesn't support %s\n",name_of_rc6_residency[flag]);
+		}
+		else
+			tmp_support = 1;
+
+		flag_counter = flag_counter + tmp_counter;
+		flag_counter = flag_counter << 1;
+
+		flag_support = flag_support + tmp_support;
+		flag_support = flag_support << 1;
+	}
+
+	printf("The residency counter : %f \n", counter_result);
+
+	igt_assert_f(flag_counter != 0 , "The RC6 residency counter is not good.\n");
+	igt_assert_f(flag_support != 0 , "This machine doesn't support any RC6 state!\n");
+	igt_assert_f(counter_result <=1  , "Debug files must be wrong \n");
+
+	printf("This machine entry %s state.\n", name_of_rc6_residency[(flag_counter / 2) - 1]);
+}
 
-	free(pathpp);
-	free(pathp);
-	free(path);
+static void rc6_residency_check(int value[])
+{
+	unsigned int diff;
+	diff = (value[3] - value[0]) +
+			(value[4] - value[1]) +
+			(value[5] - value[2]);
+
+	igt_assert_f(diff <= (SLEEP_DURATION + RC6_FUDGE),"Diff was too high. That is unpossible\n");
+	igt_assert_f(diff >= (SLEEP_DURATION - RC6_FUDGE),"GPU was not in RC6 long enough. Check that "
+							"the GPU is as idle as possible(ie. no X, "
+							"running and running no other tests)\n");
+}
+
+igt_main
+{
+	int value[6];
+	int fd;
+	const char * name_of_rc6_residency[3]={"rc6","rc6p","rc6pp"};
+
+	igt_skip_on_simulation();
+
+	/* Use drm_open_any to verify device existence */
+	fd = drm_open_any();
+	close(fd);
+
+	read_rc6_residency(value, name_of_rc6_residency);
+
+	igt_subtest("rc6-residency-check")
+		rc6_residency_check(value);
 
-	diff = (value2pp - value1pp) +
-		(value2p - value1p) +
-		(value2 - value1);
+	igt_subtest("rc6-residency-counter")
+		rc6_residency_counter(value, name_of_rc6_residency);
 
-	igt_assert_f(diff <= (SLEEP_DURATION + RC6_FUDGE),
-		     "Diff was too high. That is unpossible\n");
-	igt_assert_f(diff >= (SLEEP_DURATION - RC6_FUDGE),
-		     "GPU was not in RC6 long enough. Check that "
-		     "the GPU is as idle as possible (ie. no X, "
-		     "running and running no other tests)\n");
 }
-- 
1.8.3.1

Re: [PATCH] [I-G-T]Add rc6_residency_counter subtest

[Daniel Vetter]

On Thu, Jun 05, 2014 at 10:27:42AM +0800, Wendy Wang wrote:
> Move rc6_residency_check to subtest, add new rc6_residency_counter subtest
> for pm_rc6_residency IGT case.

I don't really understand what you're trying to do here really. Please
explain better in the commit message not just what the patch does, but
_why_ we need this new subtest.

Also the conversion to tests with subtests has a few issues:
- Everything outside of subtests that touches hw state must be wrapped in
  igt_fixture.
- You didn't move the test to the multi-test target in Makefile.sources.
- It looks like the 2nd subtest depends upon the first, they must be
  independent.
- Your new tests uses igt_assert where it probably should use igt_skip or
  something similar.

Please test that the subtest enumeration works correctly:
- With parameter --list all subtest should be enumareated, but nothing
  else. This _must_ work as non-root on non-intel machines.
- Subtests must work individually, you can run them with --run-subtest
  <name>

Cheers, Daniel
> 
> Test results run on platforms show as below:
> On HSW
> ---------------------------------------
> [root@x-hswu opt]# ./pm_rc6_residency
> IGT-Version: 1.6-g35b31df (x86_64) (Linux: 3.15.0-rc3_drm-intel-nightly_0791a3_20140520+ x86_64)
> Subtest rc6-residency-check: SUCCESS
> This machine doesn't support rc6pp
> This machine doesn't support rc6p
> This machine entry  rc6 status.
> The residency counter : 0.999667
> Subtest rc6-residency-counter: SUCCESS
> 
> On IVB
> ----------------------------------------
> [root@IVB tests]# ./pm_rc6_residency
> IGT-Version: 1.6-g35b31df (x86_64) (Linux: 3.13.6_20140318+ x86_64)
> Subtest rc6-residency-check: SUCCESS
> This machine entry  rc6p status.
> The residency counter : 0.997000
> Subtest rc6-residency-counter: SUCCESS
> 
> On BYT
> ----------------------------------------
> root@x-byt:/opt# ./pm_rc6_residency
> IGT-Version: 1.6-g0d39021 (x86_64) (Linux: 3.14.0_kcloud_ceabbb_20140521+ x86_64)
> Subtest rc6-residency-check: SUCCESS
> This machine doesn't support rc6pp
> This machine doesn't support rc6p
> The residency counter : 1.144333
> Test assertion failure function rc6_residency_counter, file pm_rc6_residency.c:131:
> Last errno: 0, Success
> Failed assertion: counter_result <=1
> Debug files must be wrong,
> Subtest rc6-residency-counter: FAIL
> 
> On BDW
> ---------------------------------------
> [root@x-bdw01 opt]# ./pm_rc6_residency
> IGT-Version: 1.6-g0d39021 (x86_64) (Linux: 3.15.0-rc5_drm-intel-nightly_367653_20140521+ x86_64)
> Subtest rc6-residency-check: SUCCESS
> This machine doesn't support rc6pp
> This machine doesn't support rc6p
> The residency counter : 0.994333
> This machine entry rc6 state.
> Subtest rc6-residency-counter: SUCCESS
> 
> Signed-off-by: Liu, Lei A <lei.a.liu@intel.com>, Wendy Wang <wendy.wang@intel.com>
> Signed-off-by: Wendy Wang <wendy.wang@intel.com>
> 
> diff --git a/tests/pm_rc6_residency.c b/tests/pm_rc6_residency.c
> index 197ab00..550e3ad 100644
> --- a/tests/pm_rc6_residency.c
> +++ b/tests/pm_rc6_residency.c
> @@ -34,9 +34,11 @@
>  
>  #include "drmtest.h"
>  
> +#define NUMBER_OF_RC6_RESIDENCY 3
>  #define SLEEP_DURATION 3000 // in milliseconds
>  #define RC6_FUDGE 900 // in milliseconds
>  
> +
>  static unsigned int readit(const char *path)
>  {
>  	unsigned int ret;
> @@ -44,8 +46,10 @@ static unsigned int readit(const char *path)
>  
>  	FILE *file;
>  	file = fopen(path, "r");
> -	igt_assert_f(file,
> -		     "Couldn't open %s (%d)\n", path, errno);
> +	if (file == NULL) {
> +		fprintf(stderr, "Couldn't open %s (%d)\n", path, errno);
> +		abort();
> +	}
>  	scanned = fscanf(file, "%u", &ret);
>  	igt_assert(scanned == 1);
>  
> @@ -54,62 +58,112 @@ static unsigned int readit(const char *path)
>  	return ret;
>  }
>  
> -igt_simple_main
> +static void read_rc6_residency( int value[], const char *name_of_rc6_residency[])
>  {
>  	const int device = drm_get_card();
> -	char *path, *pathp, *pathpp;
> -	int fd, ret;
> -	unsigned int value1, value1p, value1pp, value2, value2p, value2pp;
> +	char *path ;
> +	int  ret;
>  	FILE *file;
> -	int diff;
> -
> -	igt_skip_on_simulation();
> -
> -	/* Use drm_open_any to verify device existence */
> -	fd = drm_open_any();
> -	close(fd);
> -
> -	ret = asprintf(&path, "/sys/class/drm/card%d/power/rc6_enable", device);
> -	igt_assert(ret != -1);
>  
>  	/* For some reason my ivb isn't idle even after syncing up with the gpu.
>  	 * Let's add a sleept just to make it happy. */
>  	sleep(5);
>  
> -	file = fopen(path, "r");
> +	ret = asprintf(&path, "/sys/class/drm/card%d/power/rc6_enable", device);
> +	igt_assert(ret != -1);
> +
> +	file = fopen(path, "r");//open
>  	igt_require(file);
>  
>  	/* claim success if no rc6 enabled. */
>  	if (readit(path) == 0)
>  		igt_success();
>  
> -	ret = asprintf(&path, "/sys/class/drm/card%d/power/rc6_residency_ms", device);
> -	igt_assert(ret != -1);
> -	ret = asprintf(&pathp, "/sys/class/drm/card%d/power/rc6p_residency_ms", device);
> -	igt_assert(ret != -1);
> -	ret = asprintf(&pathpp, "/sys/class/drm/card%d/power/rc6pp_residency_ms", device);
> -	igt_assert(ret != -1);
> +	for(unsigned int i = 0; i < 6; i++)
> +	{
> +		if(i == 3)
> +			sleep(SLEEP_DURATION / 1000);
> +		ret = asprintf(&path, "/sys/class/drm/card%d/power/%s_residency_ms",device,name_of_rc6_residency[i % 3] );
> +		igt_assert(ret != -1);
> +		value[i] = readit(path);
> +	}
> +	free(path);
> +}
>  
> -	value1 = readit(path);
> -	value1p = readit(pathp);
> -	value1pp = readit(pathpp);
> -	sleep(SLEEP_DURATION / 1000);
> -	value2 = readit(path);
> -	value2p = readit(pathp);
> -	value2pp = readit(pathpp);
> +static void rc6_residency_counter(int value[],const char * name_of_rc6_residency[])
> +{
> +	unsigned int flag_counter,flag_support;
> +	double  counter_result = 0;
> +	flag_counter = 0;
> +	flag_support = 0;
> +
> +	for(int flag = NUMBER_OF_RC6_RESIDENCY-1; flag >= 0 ; flag --)
> +	{
> +		unsigned int  tmp_counter, tmp_support;
> +		double counter;
> +		counter = ((double)value[flag + 3] - (double)value[flag]) /(double) SLEEP_DURATION;
> +
> +		if( counter > 0.9 ){
> +			counter_result = counter;
> +			tmp_counter = 1;
> +		}
> +		else
> +			tmp_counter = 0;
> +
> +		if( value [flag + 3] == 0){
> +			tmp_support = 0;
> +			printf("This machine doesn't support %s\n",name_of_rc6_residency[flag]);
> +		}
> +		else
> +			tmp_support = 1;
> +
> +		flag_counter = flag_counter + tmp_counter;
> +		flag_counter = flag_counter << 1;
> +
> +		flag_support = flag_support + tmp_support;
> +		flag_support = flag_support << 1;
> +	}
> +
> +	printf("The residency counter : %f \n", counter_result);
> +
> +	igt_assert_f(flag_counter != 0 , "The RC6 residency counter is not good.\n");
> +	igt_assert_f(flag_support != 0 , "This machine doesn't support any RC6 state!\n");
> +	igt_assert_f(counter_result <=1  , "Debug files must be wrong \n");
> +
> +	printf("This machine entry %s state.\n", name_of_rc6_residency[(flag_counter / 2) - 1]);
> +}
>  
> -	free(pathpp);
> -	free(pathp);
> -	free(path);
> +static void rc6_residency_check(int value[])
> +{
> +	unsigned int diff;
> +	diff = (value[3] - value[0]) +
> +			(value[4] - value[1]) +
> +			(value[5] - value[2]);
> +
> +	igt_assert_f(diff <= (SLEEP_DURATION + RC6_FUDGE),"Diff was too high. That is unpossible\n");
> +	igt_assert_f(diff >= (SLEEP_DURATION - RC6_FUDGE),"GPU was not in RC6 long enough. Check that "
> +							"the GPU is as idle as possible(ie. no X, "
> +							"running and running no other tests)\n");
> +}
> +
> +igt_main
> +{
> +	int value[6];
> +	int fd;
> +	const char * name_of_rc6_residency[3]={"rc6","rc6p","rc6pp"};
> +
> +	igt_skip_on_simulation();
> +
> +	/* Use drm_open_any to verify device existence */
> +	fd = drm_open_any();
> +	close(fd);
> +
> +	read_rc6_residency(value, name_of_rc6_residency);
> +
> +	igt_subtest("rc6-residency-check")
> +		rc6_residency_check(value);
>  
> -	diff = (value2pp - value1pp) +
> -		(value2p - value1p) +
> -		(value2 - value1);
> +	igt_subtest("rc6-residency-counter")
> +		rc6_residency_counter(value, name_of_rc6_residency);
>  
> -	igt_assert_f(diff <= (SLEEP_DURATION + RC6_FUDGE),
> -		     "Diff was too high. That is unpossible\n");
> -	igt_assert_f(diff >= (SLEEP_DURATION - RC6_FUDGE),
> -		     "GPU was not in RC6 long enough. Check that "
> -		     "the GPU is as idle as possible (ie. no X, "
> -		     "running and running no other tests)\n");
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] [I-G-T]Add rc6_residency_counter subtest

[Ben Widawsky]

On Thu, Jun 05, 2014 at 10:27:42AM +0800, Wendy Wang wrote:
> Move rc6_residency_check to subtest, add new rc6_residency_counter subtest
> for pm_rc6_residency IGT case.
> 

Looks good to me. You have some formatting issues:
 for loops don't use the right coding style
 useless comment to fopen
 bad sign-off in mail
 added unnecessary whitespace
 replace assert with open coded abort

So I've merged it with all those warts (except I fixed the
signed-off-by). Please keep these things in mind next patch, and feel
free to submit patches to fix these issues as well.

[snip]

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] [I-G-T]Add rc6_residency_counter subtest

[Daniel Vetter]

On Thu, Jun 5, 2014 at 7:19 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Thu, Jun 05, 2014 at 10:27:42AM +0800, Wendy Wang wrote:
>> Move rc6_residency_check to subtest, add new rc6_residency_counter subtest
>> for pm_rc6_residency IGT case.
>>
>
> Looks good to me. You have some formatting issues:
>  for loops don't use the right coding style
>  useless comment to fopen
>  bad sign-off in mail
>  added unnecessary whitespace
>  replace assert with open coded abort
>
> So I've merged it with all those warts (except I fixed the
> signed-off-by). Please keep these things in mind next patch, and feel
> free to submit patches to fix these issues as well.

I guess you've missed my reply to Wendy's patch. Since you've gone
ahead and merged it, can you please address these issues?

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