Expansion of pm_rps subtest min-max-config-at-idle

Expansion of pm_rps subtest min-max-config-at-idle

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

Fill out the subtest with more min/max combinations and check that
current frequency tracks with minimum after each perturbation.

Jeff McGee (4):
  pm_rps: Expand on min and max config testing
  pm_rps: Remove repeat sysfs reads
  pm_rps: Make frequency logging more compact
  pm_rps: Require that cur reaches min at idle

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

-- 
1.8.5.2

[PATCH 1/4] pm_rps: Expand on min and max config testing

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

Add a function that methodically varies min and max to exercise
several valid and invalid combinations. Allow the caller to
define what is to be checked between each step.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
---
 tests/pm_rps.c | 106 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 77 insertions(+), 29 deletions(-)

diff --git a/tests/pm_rps.c b/tests/pm_rps.c
index f7f119c..e5cbfd1 100644
--- a/tests/pm_rps.c
+++ b/tests/pm_rps.c
@@ -134,6 +134,81 @@ static void dumpit(void)
 #define dump() if (verbose) dumpit()
 #define log(...) if (verbose) printf(__VA_ARGS__)
 
+static void min_max_config(void (*check)(void))
+{
+	int fmid = (frpn + frp0) / 2;
+
+	log("Check original min and max...\n");
+	check();
+
+	log("Set min=RPn and max=RP0...\n");
+	writeval(stuff[MIN].filp, frpn);
+	writeval(stuff[MAX].filp, frp0);
+	check();
+
+	log("Increase min to midpoint...\n");
+	writeval(stuff[MIN].filp, fmid);
+	check();
+
+	log("Increase min to RP0...\n");
+	writeval(stuff[MIN].filp, frp0);
+	check();
+
+	log("Increase min above RP0 (invalid)...\n");
+	writeval_inval(stuff[MIN].filp, frp0 + 1000);
+	check();
+
+	log("Decrease max to RPn (invalid)...\n");
+	writeval_inval(stuff[MAX].filp, frpn);
+	check();
+
+	log("Decrease min to midpoint...\n");
+	writeval(stuff[MIN].filp, fmid);
+	check();
+
+	log("Decrease min to RPn...\n");
+	writeval(stuff[MIN].filp, frpn);
+	check();
+
+	log("Decrease min below RPn (invalid)...\n");
+	writeval_inval(stuff[MIN].filp, 0);
+	check();
+
+	log("Decrease max to midpoint...\n");
+	writeval(stuff[MAX].filp, fmid);
+	check();
+
+	log("Decrease max to RPn...\n");
+	writeval(stuff[MAX].filp, frpn);
+	check();
+
+	log("Decrease max below RPn (invalid)...\n");
+	writeval_inval(stuff[MAX].filp, 0);
+	check();
+
+	log("Increase min to RP0 (invalid)...\n");
+	writeval_inval(stuff[MIN].filp, frp0);
+	check();
+
+	log("Increase max to midpoint...\n");
+	writeval(stuff[MAX].filp, fmid);
+	check();
+
+	log("Increase max to RP0...\n");
+	writeval(stuff[MAX].filp, frp0);
+	check();
+
+	log("Increase max above RP0 (invalid)...\n");
+	writeval_inval(stuff[MAX].filp, frp0 + 1000);
+	check();
+}
+
+static void idle_check(void)
+{
+	dump();
+	checkit();
+}
+
 static void pm_rps_exit_handler(int sig)
 {
 	if (origmin > fmax) {
@@ -210,35 +285,8 @@ int main(int argc, char **argv)
 		igt_install_exit_handler(pm_rps_exit_handler);
 	}
 
-	igt_subtest("min-max-config-at-idle") {
-		log("Original min = %d\nOriginal max = %d\n", origmin, origmax);
-
-		dump();
-
-		checkit();
-		setfreq(origmin);
-		dump();
-		igt_assert(fcur == fmin);
-		setfreq(origmax);
-		dump();
-		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);
-	}
+	igt_subtest("min-max-config-at-idle")
+		min_max_config(idle_check);
 
 	igt_exit();
 }
-- 
1.8.5.2

[PATCH 2/4] pm_rps: Remove repeat sysfs reads

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

Storing values avoids some unnecessary overhead but more importantly
allows all of our processing to be atomic.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
---
 tests/pm_rps.c | 98 ++++++++++++++++++++++++++++++----------------------------
 1 file changed, 51 insertions(+), 47 deletions(-)

diff --git a/tests/pm_rps.c b/tests/pm_rps.c
index e5cbfd1..37f7020 100644
--- a/tests/pm_rps.c
+++ b/tests/pm_rps.c
@@ -35,7 +35,6 @@
 #include "drmtest.h"
 
 static bool verbose = false;
-static int origmin, origmax;
 
 static const char sysfs_base_path[] = "/sys/class/drm/card%d/gt_%s_freq_mhz";
 enum {
@@ -44,9 +43,12 @@ enum {
 	MAX,
 	RP0,
 	RP1,
-	RPn
+	RPn,
+	NUMFREQ
 };
 
+static int origfreqs[NUMFREQ];
+
 struct junk {
 	const char *name;
 	const char *mode;
@@ -67,6 +69,14 @@ static int readval(FILE *filp)
 	return val;
 }
 
+static void read_freqs(int *freqs)
+{
+	int i;
+
+	for (i = 0; i < NUMFREQ; i++)
+		freqs[i] = readval(stuff[i].filp);
+}
+
 static int do_writeval(FILE *filp, int val, int lerrno)
 {
 	int ret, orig;
@@ -90,16 +100,9 @@ static int do_writeval(FILE *filp, int val, int lerrno)
 #define writeval(filp, val) do_writeval(filp, val, 0)
 #define writeval_inval(filp, val) do_writeval(filp, val, EINVAL)
 
-#define fcur (readval(stuff[CUR].filp))
-#define fmin (readval(stuff[MIN].filp))
-#define fmax (readval(stuff[MAX].filp))
-#define frp0 (readval(stuff[RP0].filp))
-#define frp1 (readval(stuff[RP1].filp))
-#define frpn (readval(stuff[RPn].filp))
-
 static void setfreq(int val)
 {
-	if (val > fmax) {
+	if (val > readval(stuff[MAX].filp)) {
 		writeval(stuff[MAX].filp, val);
 		writeval(stuff[MIN].filp, val);
 	} else {
@@ -108,42 +111,41 @@ static void setfreq(int val)
 	}
 }
 
-static void checkit(void)
+static void checkit(const int *freqs)
 {
-	igt_assert(fmin <= fmax);
-	igt_assert(fcur <= fmax);
-	igt_assert(fmin <= fcur);
-	igt_assert(frpn <= fmin);
-	igt_assert(fmax <= frp0);
-	igt_assert(frp1 <= frp0);
-	igt_assert(frpn <= frp1);
-	igt_assert(frp0 != 0);
-	igt_assert(frp1 != 0);
+	igt_assert(freqs[MIN] <= freqs[MAX]);
+	igt_assert(freqs[CUR] <= freqs[MAX]);
+	igt_assert(freqs[MIN] <= freqs[CUR]);
+	igt_assert(freqs[RPn] <= freqs[MIN]);
+	igt_assert(freqs[MAX] <= freqs[RP0]);
+	igt_assert(freqs[RP1] <= freqs[RP0]);
+	igt_assert(freqs[RPn] <= freqs[RP1]);
+	igt_assert(freqs[RP0] != 0);
+	igt_assert(freqs[RP1] != 0);
 }
 
-static void dumpit(void)
+static void dumpit(const int *freqs)
 {
-	struct junk *junk = stuff;
-	do {
-		printf("gt frequency %s (MHz):  %d\n", junk->name, readval(junk->filp));
-		junk++;
-	} while(junk->name != NULL);
+	int i;
+
+	for (i = 0; i < NUMFREQ; i++)
+		printf("gt frequency %s (MHz):  %d\n", stuff[i].name, freqs[i]);
 
 	printf("\n");
 }
-#define dump() if (verbose) dumpit()
+#define dump(x) if (verbose) dumpit(x)
 #define log(...) if (verbose) printf(__VA_ARGS__)
 
 static void min_max_config(void (*check)(void))
 {
-	int fmid = (frpn + frp0) / 2;
+	int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2;
 
 	log("Check original min and max...\n");
 	check();
 
 	log("Set min=RPn and max=RP0...\n");
-	writeval(stuff[MIN].filp, frpn);
-	writeval(stuff[MAX].filp, frp0);
+	writeval(stuff[MIN].filp, origfreqs[RPn]);
+	writeval(stuff[MAX].filp, origfreqs[RP0]);
 	check();
 
 	log("Increase min to midpoint...\n");
@@ -151,15 +153,15 @@ static void min_max_config(void (*check)(void))
 	check();
 
 	log("Increase min to RP0...\n");
-	writeval(stuff[MIN].filp, frp0);
+	writeval(stuff[MIN].filp, origfreqs[RP0]);
 	check();
 
 	log("Increase min above RP0 (invalid)...\n");
-	writeval_inval(stuff[MIN].filp, frp0 + 1000);
+	writeval_inval(stuff[MIN].filp, origfreqs[RP0] + 1000);
 	check();
 
 	log("Decrease max to RPn (invalid)...\n");
-	writeval_inval(stuff[MAX].filp, frpn);
+	writeval_inval(stuff[MAX].filp, origfreqs[RPn]);
 	check();
 
 	log("Decrease min to midpoint...\n");
@@ -167,7 +169,7 @@ static void min_max_config(void (*check)(void))
 	check();
 
 	log("Decrease min to RPn...\n");
-	writeval(stuff[MIN].filp, frpn);
+	writeval(stuff[MIN].filp, origfreqs[RPn]);
 	check();
 
 	log("Decrease min below RPn (invalid)...\n");
@@ -179,7 +181,7 @@ static void min_max_config(void (*check)(void))
 	check();
 
 	log("Decrease max to RPn...\n");
-	writeval(stuff[MAX].filp, frpn);
+	writeval(stuff[MAX].filp, origfreqs[RPn]);
 	check();
 
 	log("Decrease max below RPn (invalid)...\n");
@@ -187,7 +189,7 @@ static void min_max_config(void (*check)(void))
 	check();
 
 	log("Increase min to RP0 (invalid)...\n");
-	writeval_inval(stuff[MIN].filp, frp0);
+	writeval_inval(stuff[MIN].filp, origfreqs[RP0]);
 	check();
 
 	log("Increase max to midpoint...\n");
@@ -195,28 +197,31 @@ static void min_max_config(void (*check)(void))
 	check();
 
 	log("Increase max to RP0...\n");
-	writeval(stuff[MAX].filp, frp0);
+	writeval(stuff[MAX].filp, origfreqs[RP0]);
 	check();
 
 	log("Increase max above RP0 (invalid)...\n");
-	writeval_inval(stuff[MAX].filp, frp0 + 1000);
+	writeval_inval(stuff[MAX].filp, origfreqs[RP0] + 1000);
 	check();
 }
 
 static void idle_check(void)
 {
-	dump();
-	checkit();
+	int freqs[NUMFREQ];
+
+	read_freqs(freqs);
+	dump(freqs);
+	checkit(freqs);
 }
 
 static void pm_rps_exit_handler(int sig)
 {
-	if (origmin > fmax) {
-		writeval(stuff[MAX].filp, origmax);
-		writeval(stuff[MIN].filp, origmin);
+	if (origfreqs[MIN] > readval(stuff[MAX].filp)) {
+		writeval(stuff[MAX].filp, origfreqs[MAX]);
+		writeval(stuff[MIN].filp, origfreqs[MIN]);
 	} else {
-		writeval(stuff[MIN].filp, origmin);
-		writeval(stuff[MAX].filp, origmax);
+		writeval(stuff[MIN].filp, origfreqs[MIN]);
+		writeval(stuff[MAX].filp, origfreqs[MAX]);
 	}
 }
 
@@ -279,8 +284,7 @@ int main(int argc, char **argv)
 			junk++;
 		} while(junk->name != NULL);
 
-		origmin = fmin;
-		origmax = fmax;
+		read_freqs(origfreqs);
 
 		igt_install_exit_handler(pm_rps_exit_handler);
 	}
-- 
1.8.5.2

[PATCH 3/4] pm_rps: Make frequency logging more compact

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
---
 tests/pm_rps.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/tests/pm_rps.c b/tests/pm_rps.c
index 37f7020..7ae0438 100644
--- a/tests/pm_rps.c
+++ b/tests/pm_rps.c
@@ -128,8 +128,9 @@ static void dumpit(const int *freqs)
 {
 	int i;
 
+	printf("gt freq (MHz):");
 	for (i = 0; i < NUMFREQ; i++)
-		printf("gt frequency %s (MHz):  %d\n", stuff[i].name, freqs[i]);
+		printf("  %s=%d", stuff[i].name, freqs[i]);
 
 	printf("\n");
 }
@@ -140,67 +141,67 @@ static void min_max_config(void (*check)(void))
 {
 	int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2;
 
-	log("Check original min and max...\n");
+	log("\nCheck original min and max...\n");
 	check();
 
-	log("Set min=RPn and max=RP0...\n");
+	log("\nSet min=RPn and max=RP0...\n");
 	writeval(stuff[MIN].filp, origfreqs[RPn]);
 	writeval(stuff[MAX].filp, origfreqs[RP0]);
 	check();
 
-	log("Increase min to midpoint...\n");
+	log("\nIncrease min to midpoint...\n");
 	writeval(stuff[MIN].filp, fmid);
 	check();
 
-	log("Increase min to RP0...\n");
+	log("\nIncrease min to RP0...\n");
 	writeval(stuff[MIN].filp, origfreqs[RP0]);
 	check();
 
-	log("Increase min above RP0 (invalid)...\n");
+	log("\nIncrease min above RP0 (invalid)...\n");
 	writeval_inval(stuff[MIN].filp, origfreqs[RP0] + 1000);
 	check();
 
-	log("Decrease max to RPn (invalid)...\n");
+	log("\nDecrease max to RPn (invalid)...\n");
 	writeval_inval(stuff[MAX].filp, origfreqs[RPn]);
 	check();
 
-	log("Decrease min to midpoint...\n");
+	log("\nDecrease min to midpoint...\n");
 	writeval(stuff[MIN].filp, fmid);
 	check();
 
-	log("Decrease min to RPn...\n");
+	log("\nDecrease min to RPn...\n");
 	writeval(stuff[MIN].filp, origfreqs[RPn]);
 	check();
 
-	log("Decrease min below RPn (invalid)...\n");
+	log("\nDecrease min below RPn (invalid)...\n");
 	writeval_inval(stuff[MIN].filp, 0);
 	check();
 
-	log("Decrease max to midpoint...\n");
+	log("\nDecrease max to midpoint...\n");
 	writeval(stuff[MAX].filp, fmid);
 	check();
 
-	log("Decrease max to RPn...\n");
+	log("\nDecrease max to RPn...\n");
 	writeval(stuff[MAX].filp, origfreqs[RPn]);
 	check();
 
-	log("Decrease max below RPn (invalid)...\n");
+	log("\nDecrease max below RPn (invalid)...\n");
 	writeval_inval(stuff[MAX].filp, 0);
 	check();
 
-	log("Increase min to RP0 (invalid)...\n");
+	log("\nIncrease min to RP0 (invalid)...\n");
 	writeval_inval(stuff[MIN].filp, origfreqs[RP0]);
 	check();
 
-	log("Increase max to midpoint...\n");
+	log("\nIncrease max to midpoint...\n");
 	writeval(stuff[MAX].filp, fmid);
 	check();
 
-	log("Increase max to RP0...\n");
+	log("\nIncrease max to RP0...\n");
 	writeval(stuff[MAX].filp, origfreqs[RP0]);
 	check();
 
-	log("Increase max above RP0 (invalid)...\n");
+	log("\nIncrease max above RP0 (invalid)...\n");
 	writeval_inval(stuff[MAX].filp, origfreqs[RP0] + 1000);
 	check();
 }
-- 
1.8.5.2

[PATCH 4/4] pm_rps: Require that cur reaches min at idle

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

The current frequency should reach the minimum frequency within a
reasonable time during idle. We hold forcewake to prevent interference
from sleep states.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
---
 tests/pm_rps.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/tests/pm_rps.c b/tests/pm_rps.c
index 7ae0438..50c66ee 100644
--- a/tests/pm_rps.c
+++ b/tests/pm_rps.c
@@ -33,6 +33,7 @@
 #include <unistd.h>
 #include <getopt.h>
 #include "drmtest.h"
+#include "igt_debugfs.h"
 
 static bool verbose = false;
 
@@ -57,6 +58,9 @@ struct junk {
 	{ "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { NULL, NULL, NULL }
 };
 
+static igt_debugfs_t dfs;
+static FILE *forcewake;
+
 static int readval(FILE *filp)
 {
 	int val;
@@ -206,17 +210,33 @@ static void min_max_config(void (*check)(void))
 	check();
 }
 
+#define IDLE_WAIT_TIMESTEP_MSEC 100
+#define IDLE_WAIT_TIMEOUT_MSEC 3000
 static void idle_check(void)
 {
 	int freqs[NUMFREQ];
-
-	read_freqs(freqs);
-	dump(freqs);
-	checkit(freqs);
+	int wait = 0;
+
+	/* Monitor frequencies until cur settles down to min, which should
+	 * happen within the allotted time */
+	do {
+		read_freqs(freqs);
+		dump(freqs);
+		checkit(freqs);
+		if (freqs[CUR] == freqs[MIN])
+			break;
+		usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC);
+		wait += IDLE_WAIT_TIMESTEP_MSEC;
+	} while (wait < IDLE_WAIT_TIMEOUT_MSEC);
+
+	igt_assert(freqs[CUR] == freqs[MIN]);
+	log("Required %d msec to reach cur=min\n", wait);
 }
 
 static void pm_rps_exit_handler(int sig)
 {
+	fclose(forcewake);
+
 	if (origfreqs[MIN] > readval(stuff[MAX].filp)) {
 		writeval(stuff[MAX].filp, origfreqs[MAX]);
 		writeval(stuff[MIN].filp, origfreqs[MIN]);
@@ -287,6 +307,12 @@ int main(int argc, char **argv)
 
 		read_freqs(origfreqs);
 
+		/* Hold forcewake throughout test to prevent sleep states from
+		 * interfering with evaluation of performance state management */
+		igt_require(igt_debugfs_init(&dfs) == 0);
+		forcewake = igt_debugfs_fopen(&dfs, "i915_forcewake_user", "r");
+		igt_require(forcewake);
+
 		igt_install_exit_handler(pm_rps_exit_handler);
 	}
 
-- 
1.8.5.2

Re: [PATCH 4/4] pm_rps: Require that cur reaches min at idle

[Daniel Vetter]

On Tue, Jan 21, 2014 at 05:14:34PM -0600, jeff.mcgee@intel.com wrote:
> From: Jeff McGee <jeff.mcgee@intel.com>
> 
> The current frequency should reach the minimum frequency within a
> reasonable time during idle. We hold forcewake to prevent interference
> from sleep states.
> 
> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> ---
>  tests/pm_rps.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> index 7ae0438..50c66ee 100644
> --- a/tests/pm_rps.c
> +++ b/tests/pm_rps.c
> @@ -33,6 +33,7 @@
>  #include <unistd.h>
>  #include <getopt.h>
>  #include "drmtest.h"
> +#include "igt_debugfs.h"
>  
>  static bool verbose = false;
>  
> @@ -57,6 +58,9 @@ struct junk {
>  	{ "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { NULL, NULL, NULL }
>  };
>  
> +static igt_debugfs_t dfs;
> +static FILE *forcewake;
> +
>  static int readval(FILE *filp)
>  {
>  	int val;
> @@ -206,17 +210,33 @@ static void min_max_config(void (*check)(void))
>  	check();
>  }
>  
> +#define IDLE_WAIT_TIMESTEP_MSEC 100
> +#define IDLE_WAIT_TIMEOUT_MSEC 3000
>  static void idle_check(void)
>  {
>  	int freqs[NUMFREQ];
> -
> -	read_freqs(freqs);
> -	dump(freqs);
> -	checkit(freqs);
> +	int wait = 0;
> +
> +	/* Monitor frequencies until cur settles down to min, which should
> +	 * happen within the allotted time */
> +	do {
> +		read_freqs(freqs);
> +		dump(freqs);
> +		checkit(freqs);
> +		if (freqs[CUR] == freqs[MIN])
> +			break;
> +		usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC);
> +		wait += IDLE_WAIT_TIMESTEP_MSEC;
> +	} while (wait < IDLE_WAIT_TIMEOUT_MSEC);
> +
> +	igt_assert(freqs[CUR] == freqs[MIN]);
> +	log("Required %d msec to reach cur=min\n", wait);
>  }
>  
>  static void pm_rps_exit_handler(int sig)
>  {
> +	fclose(forcewake);
> +
>  	if (origfreqs[MIN] > readval(stuff[MAX].filp)) {
>  		writeval(stuff[MAX].filp, origfreqs[MAX]);
>  		writeval(stuff[MIN].filp, origfreqs[MIN]);
> @@ -287,6 +307,12 @@ int main(int argc, char **argv)
>  
>  		read_freqs(origfreqs);
>  
> +		/* Hold forcewake throughout test to prevent sleep states from
> +		 * interfering with evaluation of performance state management */
> +		igt_require(igt_debugfs_init(&dfs) == 0);
> +		forcewake = igt_debugfs_fopen(&dfs, "i915_forcewake_user", "r");
> +		igt_require(forcewake);

That smells like a hack to work around broken kernels ... Why exactly do
we need this? Also, recent upstream should auto-deboost to the lowest freq
on idle systems to avoid the gpu ending up stuck at higher frequencies.
-Daniel

> +
>  		igt_install_exit_handler(pm_rps_exit_handler);
>  	}
>  
> -- 
> 1.8.5.2
> 
> _______________________________________________
> 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 4/4] pm_rps: Require that cur reaches min at idle

[Jeff McGee]

On Thu, Jan 23, 2014 at 11:40:18AM +0100, Daniel Vetter wrote:
> On Tue, Jan 21, 2014 at 05:14:34PM -0600, jeff.mcgee@intel.com wrote:
> > From: Jeff McGee <jeff.mcgee@intel.com>
> > 
> > The current frequency should reach the minimum frequency within a
> > reasonable time during idle. We hold forcewake to prevent interference
> > from sleep states.
> > 
> > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> > ---
> >  tests/pm_rps.c | 34 ++++++++++++++++++++++++++++++----
> >  1 file changed, 30 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> > index 7ae0438..50c66ee 100644
> > --- a/tests/pm_rps.c
> > +++ b/tests/pm_rps.c
> > @@ -33,6 +33,7 @@
> >  #include <unistd.h>
> >  #include <getopt.h>
> >  #include "drmtest.h"
> > +#include "igt_debugfs.h"
> >  
> >  static bool verbose = false;
> >  
> > @@ -57,6 +58,9 @@ struct junk {
> >  	{ "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { NULL, NULL, NULL }
> >  };
> >  
> > +static igt_debugfs_t dfs;
> > +static FILE *forcewake;
> > +
> >  static int readval(FILE *filp)
> >  {
> >  	int val;
> > @@ -206,17 +210,33 @@ static void min_max_config(void (*check)(void))
> >  	check();
> >  }
> >  
> > +#define IDLE_WAIT_TIMESTEP_MSEC 100
> > +#define IDLE_WAIT_TIMEOUT_MSEC 3000
> >  static void idle_check(void)
> >  {
> >  	int freqs[NUMFREQ];
> > -
> > -	read_freqs(freqs);
> > -	dump(freqs);
> > -	checkit(freqs);
> > +	int wait = 0;
> > +
> > +	/* Monitor frequencies until cur settles down to min, which should
> > +	 * happen within the allotted time */
> > +	do {
> > +		read_freqs(freqs);
> > +		dump(freqs);
> > +		checkit(freqs);
> > +		if (freqs[CUR] == freqs[MIN])
> > +			break;
> > +		usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC);
> > +		wait += IDLE_WAIT_TIMESTEP_MSEC;
> > +	} while (wait < IDLE_WAIT_TIMEOUT_MSEC);
> > +
> > +	igt_assert(freqs[CUR] == freqs[MIN]);
> > +	log("Required %d msec to reach cur=min\n", wait);
> >  }
> >  
> >  static void pm_rps_exit_handler(int sig)
> >  {
> > +	fclose(forcewake);
> > +
> >  	if (origfreqs[MIN] > readval(stuff[MAX].filp)) {
> >  		writeval(stuff[MAX].filp, origfreqs[MAX]);
> >  		writeval(stuff[MIN].filp, origfreqs[MIN]);
> > @@ -287,6 +307,12 @@ int main(int argc, char **argv)
> >  
> >  		read_freqs(origfreqs);
> >  
> > +		/* Hold forcewake throughout test to prevent sleep states from
> > +		 * interfering with evaluation of performance state management */
> > +		igt_require(igt_debugfs_init(&dfs) == 0);
> > +		forcewake = igt_debugfs_fopen(&dfs, "i915_forcewake_user", "r");
> > +		igt_require(forcewake);
> 
> That smells like a hack to work around broken kernels ... Why exactly do
> we need this? Also, recent upstream should auto-deboost to the lowest freq
> on idle systems to avoid the gpu ending up stuck at higher frequencies.
> -Daniel

I guess I'm a little unclear on the policy here. My understanding is that it
is acceptable for the requested frequency to remain above min if we are in
RC6, because the actual frequency is 0 MHz in that state and so we are
getting the most power savings anyway. With this in mind, I didn't want to
fail a system in which that occurred. Taking the forcewake allows us to
verify that turbo hardware is working correctly on its own merits
(particularly the interrupts). If the policy is to require that requested
frequency always go to min at idle (RC6 or not), then I will remove the
forcewake.
-Jeff
> 
> > +
> >  		igt_install_exit_handler(pm_rps_exit_handler);
> >  	}
> >  
> > -- 
> > 1.8.5.2
> > 
> > _______________________________________________
> > 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 4/4] pm_rps: Require that cur reaches min at idle

[Daniel Vetter]

On Thu, Jan 23, 2014 at 11:15:42AM -0600, Jeff McGee wrote:
> On Thu, Jan 23, 2014 at 11:40:18AM +0100, Daniel Vetter wrote:
> > On Tue, Jan 21, 2014 at 05:14:34PM -0600, jeff.mcgee@intel.com wrote:
> > > From: Jeff McGee <jeff.mcgee@intel.com>
> > > 
> > > The current frequency should reach the minimum frequency within a
> > > reasonable time during idle. We hold forcewake to prevent interference
> > > from sleep states.
> > > 
> > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> > > ---
> > >  tests/pm_rps.c | 34 ++++++++++++++++++++++++++++++----
> > >  1 file changed, 30 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> > > index 7ae0438..50c66ee 100644
> > > --- a/tests/pm_rps.c
> > > +++ b/tests/pm_rps.c
> > > @@ -33,6 +33,7 @@
> > >  #include <unistd.h>
> > >  #include <getopt.h>
> > >  #include "drmtest.h"
> > > +#include "igt_debugfs.h"
> > >  
> > >  static bool verbose = false;
> > >  
> > > @@ -57,6 +58,9 @@ struct junk {
> > >  	{ "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { NULL, NULL, NULL }
> > >  };
> > >  
> > > +static igt_debugfs_t dfs;
> > > +static FILE *forcewake;
> > > +
> > >  static int readval(FILE *filp)
> > >  {
> > >  	int val;
> > > @@ -206,17 +210,33 @@ static void min_max_config(void (*check)(void))
> > >  	check();
> > >  }
> > >  
> > > +#define IDLE_WAIT_TIMESTEP_MSEC 100
> > > +#define IDLE_WAIT_TIMEOUT_MSEC 3000
> > >  static void idle_check(void)
> > >  {
> > >  	int freqs[NUMFREQ];
> > > -
> > > -	read_freqs(freqs);
> > > -	dump(freqs);
> > > -	checkit(freqs);
> > > +	int wait = 0;
> > > +
> > > +	/* Monitor frequencies until cur settles down to min, which should
> > > +	 * happen within the allotted time */
> > > +	do {
> > > +		read_freqs(freqs);
> > > +		dump(freqs);
> > > +		checkit(freqs);
> > > +		if (freqs[CUR] == freqs[MIN])
> > > +			break;
> > > +		usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC);
> > > +		wait += IDLE_WAIT_TIMESTEP_MSEC;
> > > +	} while (wait < IDLE_WAIT_TIMEOUT_MSEC);
> > > +
> > > +	igt_assert(freqs[CUR] == freqs[MIN]);
> > > +	log("Required %d msec to reach cur=min\n", wait);
> > >  }
> > >  
> > >  static void pm_rps_exit_handler(int sig)
> > >  {
> > > +	fclose(forcewake);
> > > +
> > >  	if (origfreqs[MIN] > readval(stuff[MAX].filp)) {
> > >  		writeval(stuff[MAX].filp, origfreqs[MAX]);
> > >  		writeval(stuff[MIN].filp, origfreqs[MIN]);
> > > @@ -287,6 +307,12 @@ int main(int argc, char **argv)
> > >  
> > >  		read_freqs(origfreqs);
> > >  
> > > +		/* Hold forcewake throughout test to prevent sleep states from
> > > +		 * interfering with evaluation of performance state management */
> > > +		igt_require(igt_debugfs_init(&dfs) == 0);
> > > +		forcewake = igt_debugfs_fopen(&dfs, "i915_forcewake_user", "r");
> > > +		igt_require(forcewake);
> > 
> > That smells like a hack to work around broken kernels ... Why exactly do
> > we need this? Also, recent upstream should auto-deboost to the lowest freq
> > on idle systems to avoid the gpu ending up stuck at higher frequencies.
> > -Daniel
> 
> I guess I'm a little unclear on the policy here. My understanding is that it
> is acceptable for the requested frequency to remain above min if we are in
> RC6, because the actual frequency is 0 MHz in that state and so we are
> getting the most power savings anyway. With this in mind, I didn't want to
> fail a system in which that occurred. Taking the forcewake allows us to
> verify that turbo hardware is working correctly on its own merits
> (particularly the interrupts). If the policy is to require that requested
> frequency always go to min at idle (RC6 or not), then I will remove the
> forcewake.

We've learned the hard way that the hardware can get stuck, so having such
a testcase (maybe as a separate subtest, you already add tons of other
interface checks in your series here) would be rather useful. It's not a
hard requirement but imo a good sanity check on our code (and on recent
kernels we should force the gt to the lowest frequency already when idle).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] pm_rps: Require that cur reaches min at idle

[Jeff McGee]

On Thu, Jan 23, 2014 at 07:49:20PM +0100, Daniel Vetter wrote:
> On Thu, Jan 23, 2014 at 11:15:42AM -0600, Jeff McGee wrote:
> > On Thu, Jan 23, 2014 at 11:40:18AM +0100, Daniel Vetter wrote:
> > > On Tue, Jan 21, 2014 at 05:14:34PM -0600, jeff.mcgee@intel.com wrote:
> > > > From: Jeff McGee <jeff.mcgee@intel.com>
> > > > 
> > > > The current frequency should reach the minimum frequency within a
> > > > reasonable time during idle. We hold forcewake to prevent interference
> > > > from sleep states.
> > > > 
> > > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> > > > ---
> > > >  tests/pm_rps.c | 34 ++++++++++++++++++++++++++++++----
> > > >  1 file changed, 30 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> > > > index 7ae0438..50c66ee 100644
> > > > --- a/tests/pm_rps.c
> > > > +++ b/tests/pm_rps.c
> > > > @@ -33,6 +33,7 @@
> > > >  #include <unistd.h>
> > > >  #include <getopt.h>
> > > >  #include "drmtest.h"
> > > > +#include "igt_debugfs.h"
> > > >  
> > > >  static bool verbose = false;
> > > >  
> > > > @@ -57,6 +58,9 @@ struct junk {
> > > >  	{ "cur", "r", NULL }, { "min", "rb+", NULL }, { "max", "rb+", NULL }, { "RP0", "r", NULL }, { "RP1", "r", NULL }, { "RPn", "r", NULL }, { NULL, NULL, NULL }
> > > >  };
> > > >  
> > > > +static igt_debugfs_t dfs;
> > > > +static FILE *forcewake;
> > > > +
> > > >  static int readval(FILE *filp)
> > > >  {
> > > >  	int val;
> > > > @@ -206,17 +210,33 @@ static void min_max_config(void (*check)(void))
> > > >  	check();
> > > >  }
> > > >  
> > > > +#define IDLE_WAIT_TIMESTEP_MSEC 100
> > > > +#define IDLE_WAIT_TIMEOUT_MSEC 3000
> > > >  static void idle_check(void)
> > > >  {
> > > >  	int freqs[NUMFREQ];
> > > > -
> > > > -	read_freqs(freqs);
> > > > -	dump(freqs);
> > > > -	checkit(freqs);
> > > > +	int wait = 0;
> > > > +
> > > > +	/* Monitor frequencies until cur settles down to min, which should
> > > > +	 * happen within the allotted time */
> > > > +	do {
> > > > +		read_freqs(freqs);
> > > > +		dump(freqs);
> > > > +		checkit(freqs);
> > > > +		if (freqs[CUR] == freqs[MIN])
> > > > +			break;
> > > > +		usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC);
> > > > +		wait += IDLE_WAIT_TIMESTEP_MSEC;
> > > > +	} while (wait < IDLE_WAIT_TIMEOUT_MSEC);
> > > > +
> > > > +	igt_assert(freqs[CUR] == freqs[MIN]);
> > > > +	log("Required %d msec to reach cur=min\n", wait);
> > > >  }
> > > >  
> > > >  static void pm_rps_exit_handler(int sig)
> > > >  {
> > > > +	fclose(forcewake);
> > > > +
> > > >  	if (origfreqs[MIN] > readval(stuff[MAX].filp)) {
> > > >  		writeval(stuff[MAX].filp, origfreqs[MAX]);
> > > >  		writeval(stuff[MIN].filp, origfreqs[MIN]);
> > > > @@ -287,6 +307,12 @@ int main(int argc, char **argv)
> > > >  
> > > >  		read_freqs(origfreqs);
> > > >  
> > > > +		/* Hold forcewake throughout test to prevent sleep states from
> > > > +		 * interfering with evaluation of performance state management */
> > > > +		igt_require(igt_debugfs_init(&dfs) == 0);
> > > > +		forcewake = igt_debugfs_fopen(&dfs, "i915_forcewake_user", "r");
> > > > +		igt_require(forcewake);
> > > 
> > > That smells like a hack to work around broken kernels ... Why exactly do
> > > we need this? Also, recent upstream should auto-deboost to the lowest freq
> > > on idle systems to avoid the gpu ending up stuck at higher frequencies.
> > > -Daniel
> > 
> > I guess I'm a little unclear on the policy here. My understanding is that it
> > is acceptable for the requested frequency to remain above min if we are in
> > RC6, because the actual frequency is 0 MHz in that state and so we are
> > getting the most power savings anyway. With this in mind, I didn't want to
> > fail a system in which that occurred. Taking the forcewake allows us to
> > verify that turbo hardware is working correctly on its own merits
> > (particularly the interrupts). If the policy is to require that requested
> > frequency always go to min at idle (RC6 or not), then I will remove the
> > forcewake.
> 
> We've learned the hard way that the hardware can get stuck, so having such
> a testcase (maybe as a separate subtest, you already add tons of other
> interface checks in your series here) would be rather useful. It's not a
> hard requirement but imo a good sanity check on our code (and on recent
> kernels we should force the gt to the lowest frequency already when idle).
> -Daniel

OK. I will remove the forcewake from this patch, making this subtest check
overall ability to reach min freq at idle. I'll follow-up with patches for a
subtest to include the forcewake as a variant.
-Jeff

[PATCH 4/4 v2] pm_rps: Require that cur reaches min at idle

[jeff.mcgee]

From: Jeff McGee <jeff.mcgee@intel.com>

The current frequency should reach the minimum frequency within a
reasonable time during idle.

v2: Not using forcewake for this particular subtest per Daniel's
    suggestion.

Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
---
 tests/pm_rps.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/tests/pm_rps.c b/tests/pm_rps.c
index 7ae0438..24a1ad6 100644
--- a/tests/pm_rps.c
+++ b/tests/pm_rps.c
@@ -206,13 +206,27 @@ static void min_max_config(void (*check)(void))
 	check();
 }
 
+#define IDLE_WAIT_TIMESTEP_MSEC 100
+#define IDLE_WAIT_TIMEOUT_MSEC 3000
 static void idle_check(void)
 {
 	int freqs[NUMFREQ];
-
-	read_freqs(freqs);
-	dump(freqs);
-	checkit(freqs);
+	int wait = 0;
+
+	/* Monitor frequencies until cur settles down to min, which should
+	 * happen within the allotted time */
+	do {
+		read_freqs(freqs);
+		dump(freqs);
+		checkit(freqs);
+		if (freqs[CUR] == freqs[MIN])
+			break;
+		usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC);
+		wait += IDLE_WAIT_TIMESTEP_MSEC;
+	} while (wait < IDLE_WAIT_TIMEOUT_MSEC);
+
+	igt_assert(freqs[CUR] == freqs[MIN]);
+	log("Required %d msec to reach cur=min\n", wait);
 }
 
 static void pm_rps_exit_handler(int sig)
-- 
1.8.5.2

Re: [PATCH 4/4 v2] pm_rps: Require that cur reaches min at idle

[Daniel Vetter]

On Thu, Jan 23, 2014 at 03:54:50PM -0600, jeff.mcgee@intel.com wrote:
> From: Jeff McGee <jeff.mcgee@intel.com>
> 
> The current frequency should reach the minimum frequency within a
> reasonable time during idle.
> 
> v2: Not using forcewake for this particular subtest per Daniel's
>     suggestion.
> 
> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>

Hm, I guess I wasn't clear enough: I've thought about adding a new subtest
which just does the check that the actual frequency reaches the lowest
level on idle a new subtest, not extend the existing one.

Same somewhat holds for your first patch, atm the testcase is a very basic
test of the kernel error checking.

But even ignoring that I'm not really sure what you're aiming at. Imo the
current coverage is good enough since it makes sure that we have at least
a bit of error checking in place. Any extensions to this test should imo
only be done when we add new features or to exercise bugs (or classes of
bugs) that actually happened. Iirc (and I didn't check olds mails, so this
might be wrong) we have some corner-cases across suspend/resume and some
with the in-kernel code to adjust the requested frequency under load. So
imo that's what we should test for.

Reading through my test requirements write-up I've just noticed that I
didn't emphasis that there's also too much testing possible imho. I'm not
a big proponent of test driven developement and similar validate
everything approaches. I guess I need to write up my thoughts about too
much testing, too ;-)

Anyway I'm a bit confused about what's the overall goal here, so please
elaborate.

Cheers, Daniel

> --- tests/pm_rps.c | 22 ++++++++++++++++++---- 1 file changed, 18
> insertions(+), 4 deletions(-)
> 
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c index 7ae0438..24a1ad6
> 100644 --- a/tests/pm_rps.c +++ b/tests/pm_rps.c @@ -206,13 +206,27 @@
> static void min_max_config(void (*check)(void)) check(); }
>  
> +#define IDLE_WAIT_TIMESTEP_MSEC 100 +#define IDLE_WAIT_TIMEOUT_MSEC
> 3000 static void idle_check(void) { int freqs[NUMFREQ]; - -
> read_freqs(freqs); -	dump(freqs); -	checkit(freqs); +	int wait =
> 0; + +	/* Monitor frequencies until cur settles down to min,
> which should +	 * happen within the allotted time */ +	do { +
> read_freqs(freqs); +		dump(freqs); +		checkit(freqs); +
> if (freqs[CUR] == freqs[MIN]) +			break; +
> usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC); +		wait +=
> IDLE_WAIT_TIMESTEP_MSEC; +	} while (wait < IDLE_WAIT_TIMEOUT_MSEC); +
> +	igt_assert(freqs[CUR] == freqs[MIN]); +	log("Required %d msec to
> reach cur=min\n", wait); }
>  
>  static void pm_rps_exit_handler(int sig) -- 1.8.5.2
> 
> _______________________________________________ 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 4/4 v2] pm_rps: Require that cur reaches min at idle

[Jeff McGee]

On Sat, Jan 25, 2014 at 08:46:45PM +0100, Daniel Vetter wrote:
> On Thu, Jan 23, 2014 at 03:54:50PM -0600, jeff.mcgee@intel.com wrote:
> > From: Jeff McGee <jeff.mcgee@intel.com>
> > 
> > The current frequency should reach the minimum frequency within a
> > reasonable time during idle.
> > 
> > v2: Not using forcewake for this particular subtest per Daniel's
> >     suggestion.
> > 
> > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> 
> Hm, I guess I wasn't clear enough: I've thought about adding a new subtest
> which just does the check that the actual frequency reaches the lowest
> level on idle a new subtest, not extend the existing one.
> 
> Same somewhat holds for your first patch, atm the testcase is a very basic
> test of the kernel error checking.
> 
> But even ignoring that I'm not really sure what you're aiming at. Imo the
> current coverage is good enough since it makes sure that we have at least
> a bit of error checking in place. Any extensions to this test should imo
> only be done when we add new features or to exercise bugs (or classes of
> bugs) that actually happened. Iirc (and I didn't check olds mails, so this
> might be wrong) we have some corner-cases across suspend/resume and some
> with the in-kernel code to adjust the requested frequency under load. So
> imo that's what we should test for.
> 
> Reading through my test requirements write-up I've just noticed that I
> didn't emphasis that there's also too much testing possible imho. I'm not
> a big proponent of test driven developement and similar validate
> everything approaches. I guess I need to write up my thoughts about too
> much testing, too ;-)
> 
> Anyway I'm a bit confused about what's the overall goal here, so please
> elaborate.
> 
> Cheers, Daniel
> 
Hi Daniel. I guess it would have helped if I described my overall goals
with this test development in the beginning. I have two rps related
driver patches on hold from a few months ago because you felt the igt
testing wasn't adequate to accept these. They are:

"Update rps interrupt limits"
"Restore rps/rc6 on reset"

It took me some time to get around to this, but now I am intent on
expanding pm_rps with the proper subtests to expose the issues. An outline
of the subtests I have in mind:

min-max-config-at-idle
- Manipulate min and max through sysfs and make sure driver does the right
  thing. Right thing includes accepting valid values, rejecting invalid
  values, ensuring that cur remains between min and max, and (for idle)
  ensuring that cur reaches min after such changes. This last requirement
  is not being met in part because interrupt limits are not being updated
  properly when min and max are changed. That will be justification for
  patch "Update rps interrupt limits".
- I have been forming this subtest as an expansion of Ben's original test.
  His cases for min/max checking are a subset of this.

min-max-config-at-load
- Manipulate min and max as above, but do so during load. Check for the
  same basic requirements. When heavily loaded, cur should reach max. There
  is also potentially some problem with this scenario due to interrupt
  limits not being updated reliably. If cur is at max, and max is then
  increased, cur may stay at old max.
- This will be a new subtest that re-uses the min/max manipulation function
  but just do so under load.

restore-on-reset
- Trigger gpu reset, then test that user-set (non-default) min/max settings
  were retained and that turbo functions correctly when load is applied and
  removed. This scenario will fail today and is the point of "Restore rps/rc6
  on reset".
- This will be another new subtest not yet submitted,

So the subtests are focused on known issues. I'm open to any suggestions on
their design or implementation. Thanks.

Jeff

Re: [PATCH 4/4 v2] pm_rps: Require that cur reaches min at idle

[Daniel Vetter]

On Mon, Jan 27, 2014 at 10:24:53AM -0600, Jeff McGee wrote:
> On Sat, Jan 25, 2014 at 08:46:45PM +0100, Daniel Vetter wrote:
> > On Thu, Jan 23, 2014 at 03:54:50PM -0600, jeff.mcgee@intel.com wrote:
> > > From: Jeff McGee <jeff.mcgee@intel.com>
> > > 
> > > The current frequency should reach the minimum frequency within a
> > > reasonable time during idle.
> > > 
> > > v2: Not using forcewake for this particular subtest per Daniel's
> > >     suggestion.
> > > 
> > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> > 
> > Hm, I guess I wasn't clear enough: I've thought about adding a new subtest
> > which just does the check that the actual frequency reaches the lowest
> > level on idle a new subtest, not extend the existing one.
> > 
> > Same somewhat holds for your first patch, atm the testcase is a very basic
> > test of the kernel error checking.
> > 
> > But even ignoring that I'm not really sure what you're aiming at. Imo the
> > current coverage is good enough since it makes sure that we have at least
> > a bit of error checking in place. Any extensions to this test should imo
> > only be done when we add new features or to exercise bugs (or classes of
> > bugs) that actually happened. Iirc (and I didn't check olds mails, so this
> > might be wrong) we have some corner-cases across suspend/resume and some
> > with the in-kernel code to adjust the requested frequency under load. So
> > imo that's what we should test for.
> > 
> > Reading through my test requirements write-up I've just noticed that I
> > didn't emphasis that there's also too much testing possible imho. I'm not
> > a big proponent of test driven developement and similar validate
> > everything approaches. I guess I need to write up my thoughts about too
> > much testing, too ;-)
> > 
> > Anyway I'm a bit confused about what's the overall goal here, so please
> > elaborate.
> > 
> > Cheers, Daniel
> > 
> Hi Daniel. I guess it would have helped if I described my overall goals
> with this test development in the beginning. I have two rps related
> driver patches on hold from a few months ago because you felt the igt
> testing wasn't adequate to accept these. They are:
> 
> "Update rps interrupt limits"
> "Restore rps/rc6 on reset"
> 
> It took me some time to get around to this, but now I am intent on
> expanding pm_rps with the proper subtests to expose the issues. An outline
> of the subtests I have in mind:
> 
> min-max-config-at-idle
> - Manipulate min and max through sysfs and make sure driver does the right
>   thing. Right thing includes accepting valid values, rejecting invalid
>   values, ensuring that cur remains between min and max, and (for idle)
>   ensuring that cur reaches min after such changes. This last requirement
>   is not being met in part because interrupt limits are not being updated
>   properly when min and max are changed. That will be justification for
>   patch "Update rps interrupt limits".
> - I have been forming this subtest as an expansion of Ben's original test.
>   His cases for min/max checking are a subset of this.

Oh, I didn't realize that the idle limits are currently broken and that
your first patches fixes that. I think I now also understand why do you
the idle-check at each step, so that this actually gets properly
exercised.

Problem with your approach here is that this will cause a spurious
regression report from our QA since the current min-max-config-at-idle
will suddenly starts failing (since it will test more and currently broken
things with your patches applied). And that might shadow other basic
issues (yeah, unlikely but still).

So I think the right approach here is to keep the current subtest as-is
(maybe rename it to "basic-api" since that's pretty much the only thing it
does), and then add a completely new set of subtests like you've laid out
here. So just leave the existing code as-is and copypaste the code for all
your new tests (where you make changes at least).

It's probably simplest if you do this shuffling to avoid a rebase mess
with your existing patches.

> min-max-config-at-load
> - Manipulate min and max as above, but do so during load. Check for the
>   same basic requirements. When heavily loaded, cur should reach max. There
>   is also potentially some problem with this scenario due to interrupt
>   limits not being updated reliably. If cur is at max, and max is then
>   increased, cur may stay at old max.
> - This will be a new subtest that re-uses the min/max manipulation function
>   but just do so under load.
> 
> restore-on-reset
> - Trigger gpu reset, then test that user-set (non-default) min/max settings
>   were retained and that turbo functions correctly when load is applied and
>   removed. This scenario will fail today and is the point of "Restore rps/rc6
>   on reset".
> - This will be another new subtest not yet submitted,
> 
> So the subtests are focused on known issues. I'm open to any suggestions on
> their design or implementation. Thanks.

Excellent test plan imo and thanks a lot for unconfusing me.

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

Re: [PATCH 4/4 v2] pm_rps: Require that cur reaches min at idle

[Jeff McGee]

On Mon, Jan 27, 2014 at 05:50:04PM +0100, Daniel Vetter wrote:
> On Mon, Jan 27, 2014 at 10:24:53AM -0600, Jeff McGee wrote:
> > On Sat, Jan 25, 2014 at 08:46:45PM +0100, Daniel Vetter wrote:
> > > On Thu, Jan 23, 2014 at 03:54:50PM -0600, jeff.mcgee@intel.com wrote:
> > > > From: Jeff McGee <jeff.mcgee@intel.com>
> > > > 
> > > > The current frequency should reach the minimum frequency within a
> > > > reasonable time during idle.
> > > > 
> > > > v2: Not using forcewake for this particular subtest per Daniel's
> > > >     suggestion.
> > > > 
> > > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> > > 
> > > Hm, I guess I wasn't clear enough: I've thought about adding a new subtest
> > > which just does the check that the actual frequency reaches the lowest
> > > level on idle a new subtest, not extend the existing one.
> > > 
> > > Same somewhat holds for your first patch, atm the testcase is a very basic
> > > test of the kernel error checking.
> > > 
> > > But even ignoring that I'm not really sure what you're aiming at. Imo the
> > > current coverage is good enough since it makes sure that we have at least
> > > a bit of error checking in place. Any extensions to this test should imo
> > > only be done when we add new features or to exercise bugs (or classes of
> > > bugs) that actually happened. Iirc (and I didn't check olds mails, so this
> > > might be wrong) we have some corner-cases across suspend/resume and some
> > > with the in-kernel code to adjust the requested frequency under load. So
> > > imo that's what we should test for.
> > > 
> > > Reading through my test requirements write-up I've just noticed that I
> > > didn't emphasis that there's also too much testing possible imho. I'm not
> > > a big proponent of test driven developement and similar validate
> > > everything approaches. I guess I need to write up my thoughts about too
> > > much testing, too ;-)
> > > 
> > > Anyway I'm a bit confused about what's the overall goal here, so please
> > > elaborate.
> > > 
> > > Cheers, Daniel
> > > 
> > Hi Daniel. I guess it would have helped if I described my overall goals
> > with this test development in the beginning. I have two rps related
> > driver patches on hold from a few months ago because you felt the igt
> > testing wasn't adequate to accept these. They are:
> > 
> > "Update rps interrupt limits"
> > "Restore rps/rc6 on reset"
> > 
> > It took me some time to get around to this, but now I am intent on
> > expanding pm_rps with the proper subtests to expose the issues. An outline
> > of the subtests I have in mind:
> > 
> > min-max-config-at-idle
> > - Manipulate min and max through sysfs and make sure driver does the right
> >   thing. Right thing includes accepting valid values, rejecting invalid
> >   values, ensuring that cur remains between min and max, and (for idle)
> >   ensuring that cur reaches min after such changes. This last requirement
> >   is not being met in part because interrupt limits are not being updated
> >   properly when min and max are changed. That will be justification for
> >   patch "Update rps interrupt limits".
> > - I have been forming this subtest as an expansion of Ben's original test.
> >   His cases for min/max checking are a subset of this.
> 
> Oh, I didn't realize that the idle limits are currently broken and that
> your first patches fixes that. I think I now also understand why do you
> the idle-check at each step, so that this actually gets properly
> exercised.
> 
> Problem with your approach here is that this will cause a spurious
> regression report from our QA since the current min-max-config-at-idle
> will suddenly starts failing (since it will test more and currently broken
> things with your patches applied). And that might shadow other basic
> issues (yeah, unlikely but still).
> 
> So I think the right approach here is to keep the current subtest as-is
> (maybe rename it to "basic-api" since that's pretty much the only thing it
> does), and then add a completely new set of subtests like you've laid out
> here. So just leave the existing code as-is and copypaste the code for all
> your new tests (where you make changes at least).
> 
Ah yes, expanding the subtest to the point that it uncovers an old problem
will appear as a regression from recent driver updates, which it is not. I
didn't consider this. I agree with your approach to have the current subtest
cover only basic min/max parameter validation. But I would recommend taking
patches 1-3 of this set - they work within that scope and will not cause
failures. Can we just drop patch 4? I will re-introduce it as part of adding
the new subtests.
-Jeff

> It's probably simplest if you do this shuffling to avoid a rebase mess
> with your existing patches.
> 
> > min-max-config-at-load
> > - Manipulate min and max as above, but do so during load. Check for the
> >   same basic requirements. When heavily loaded, cur should reach max. There
> >   is also potentially some problem with this scenario due to interrupt
> >   limits not being updated reliably. If cur is at max, and max is then
> >   increased, cur may stay at old max.
> > - This will be a new subtest that re-uses the min/max manipulation function
> >   but just do so under load.
> > 
> > restore-on-reset
> > - Trigger gpu reset, then test that user-set (non-default) min/max settings
> >   were retained and that turbo functions correctly when load is applied and
> >   removed. This scenario will fail today and is the point of "Restore rps/rc6
> >   on reset".
> > - This will be another new subtest not yet submitted,
> > 
> > So the subtests are focused on known issues. I'm open to any suggestions on
> > their design or implementation. Thanks.
> 
> Excellent test plan imo and thanks a lot for unconfusing me.
> 
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4 v2] pm_rps: Require that cur reaches min at idle

[Daniel Vetter]

On Mon, Jan 27, 2014 at 04:23:26PM -0600, Jeff McGee wrote:
> On Mon, Jan 27, 2014 at 05:50:04PM +0100, Daniel Vetter wrote:
> > On Mon, Jan 27, 2014 at 10:24:53AM -0600, Jeff McGee wrote:
> > > On Sat, Jan 25, 2014 at 08:46:45PM +0100, Daniel Vetter wrote:
> > > > On Thu, Jan 23, 2014 at 03:54:50PM -0600, jeff.mcgee@intel.com wrote:
> > > > > From: Jeff McGee <jeff.mcgee@intel.com>
> > > > > 
> > > > > The current frequency should reach the minimum frequency within a
> > > > > reasonable time during idle.
> > > > > 
> > > > > v2: Not using forcewake for this particular subtest per Daniel's
> > > > >     suggestion.
> > > > > 
> > > > > Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>
> > > > 
> > > > Hm, I guess I wasn't clear enough: I've thought about adding a new subtest
> > > > which just does the check that the actual frequency reaches the lowest
> > > > level on idle a new subtest, not extend the existing one.
> > > > 
> > > > Same somewhat holds for your first patch, atm the testcase is a very basic
> > > > test of the kernel error checking.
> > > > 
> > > > But even ignoring that I'm not really sure what you're aiming at. Imo the
> > > > current coverage is good enough since it makes sure that we have at least
> > > > a bit of error checking in place. Any extensions to this test should imo
> > > > only be done when we add new features or to exercise bugs (or classes of
> > > > bugs) that actually happened. Iirc (and I didn't check olds mails, so this
> > > > might be wrong) we have some corner-cases across suspend/resume and some
> > > > with the in-kernel code to adjust the requested frequency under load. So
> > > > imo that's what we should test for.
> > > > 
> > > > Reading through my test requirements write-up I've just noticed that I
> > > > didn't emphasis that there's also too much testing possible imho. I'm not
> > > > a big proponent of test driven developement and similar validate
> > > > everything approaches. I guess I need to write up my thoughts about too
> > > > much testing, too ;-)
> > > > 
> > > > Anyway I'm a bit confused about what's the overall goal here, so please
> > > > elaborate.
> > > > 
> > > > Cheers, Daniel
> > > > 
> > > Hi Daniel. I guess it would have helped if I described my overall goals
> > > with this test development in the beginning. I have two rps related
> > > driver patches on hold from a few months ago because you felt the igt
> > > testing wasn't adequate to accept these. They are:
> > > 
> > > "Update rps interrupt limits"
> > > "Restore rps/rc6 on reset"
> > > 
> > > It took me some time to get around to this, but now I am intent on
> > > expanding pm_rps with the proper subtests to expose the issues. An outline
> > > of the subtests I have in mind:
> > > 
> > > min-max-config-at-idle
> > > - Manipulate min and max through sysfs and make sure driver does the right
> > >   thing. Right thing includes accepting valid values, rejecting invalid
> > >   values, ensuring that cur remains between min and max, and (for idle)
> > >   ensuring that cur reaches min after such changes. This last requirement
> > >   is not being met in part because interrupt limits are not being updated
> > >   properly when min and max are changed. That will be justification for
> > >   patch "Update rps interrupt limits".
> > > - I have been forming this subtest as an expansion of Ben's original test.
> > >   His cases for min/max checking are a subset of this.
> > 
> > Oh, I didn't realize that the idle limits are currently broken and that
> > your first patches fixes that. I think I now also understand why do you
> > the idle-check at each step, so that this actually gets properly
> > exercised.
> > 
> > Problem with your approach here is that this will cause a spurious
> > regression report from our QA since the current min-max-config-at-idle
> > will suddenly starts failing (since it will test more and currently broken
> > things with your patches applied). And that might shadow other basic
> > issues (yeah, unlikely but still).
> > 
> > So I think the right approach here is to keep the current subtest as-is
> > (maybe rename it to "basic-api" since that's pretty much the only thing it
> > does), and then add a completely new set of subtests like you've laid out
> > here. So just leave the existing code as-is and copypaste the code for all
> > your new tests (where you make changes at least).
> > 
> Ah yes, expanding the subtest to the point that it uncovers an old problem
> will appear as a regression from recent driver updates, which it is not. I
> didn't consider this. I agree with your approach to have the current subtest
> cover only basic min/max parameter validation. But I would recommend taking
> patches 1-3 of this set - they work within that scope and will not cause
> failures. Can we just drop patch 4? I will re-introduce it as part of adding
> the new subtests.

Works for me, too. I've also mashed the rename to "basic-api" on top while
at it. Patches merged to i-g-t, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch