diff mbox

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

Message ID 1390346074-15774-5-git-send-email-jeff.mcgee@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

jeff.mcgee@intel.com Jan. 21, 2014, 11:14 p.m. UTC 
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(-)

Comments

Daniel Vetter Jan. 23, 2014, 10:40 a.m. UTC | #1 
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
jeff.mcgee@intel.com Jan. 23, 2014, 5:15 p.m. UTC | #2 
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
Daniel Vetter Jan. 23, 2014, 6:49 p.m. UTC | #3 
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
jeff.mcgee@intel.com Jan. 23, 2014, 8:24 p.m. UTC | #4 
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
diff mbox

Patch

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);
 	}