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

[Jeff McGee <jeff.mcgee@intel.com>]

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