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 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