Re: [PATCH 2/2] igt/pm_rps: Add checks for freq = idle (RPn) in specific cases.

From: Bob Paauwe <bob.j.paauwe@intel.com>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 2/2] igt/pm_rps: Add checks for freq = idle (RPn) in specific cases.
Date: Tue, 1 Dec 2015 09:22:08 -0800	[thread overview]
Message-ID: <20151201092208.0fc6724a@bpaauwe-desk.fm.intel.com> (raw)
In-Reply-To: <1448978215.14710.30.camel@intel.com>

On Tue, 1 Dec 2015 15:56:55 +0200
Imre Deak <imre.deak@intel.com> wrote:

> On ma, 2015-11-30 at 16:23 -0800, Bob Paauwe wrote:
> > Now that the frequency can drop below the user specified minimum when
> > the gpu is idle, add some checking to verify that it really does drop
> > down to the RPn frequency in specific cases.
> > 
> > To do this, modify the main test flow to take two 'check' routines
> > instead of one. When running the config-min-max-idle subtest make
> > use of the second check routine to verify that the frequency drops
> > to RPn instead of simply <= user min frequency.  For all other
> > subtests, use the original check routines for both checks.
> > 
> > Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> 
> I don't see the point. The frequency should always drop to the idle
> frequency if the GPU is idle, it's not enough that it's below MIN-freq. 
> So why do we need separate CUR-freq<=MIN-freq and CUR-freq==idle-freq
> checks?

I started from the premise that it should always drop to idle but
that's just not the case.  The min_max_config() function is used for
both the idle and loaded subtests.  If you try to check for freq=idle
when doing the loaded subtest it will fail since it never goes idle.
Even in the idle subtest there are cases where it doesn't drop down to
idle. 

I suppose I could duplicate min_max_config and have a
min_max_config_idle() and min_max_conifg_not_idle() for use by the
different subtests. 

The real problem is that the test was not designed to handle the case
where the freq could drop below min and probably needs to be
re-designed.  I've been trying to find a quick fix vs. a complete
overhaul as this isn't really a priority for me.

> 
> > ---
> >  tests/pm_rps.c | 47 ++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 34 insertions(+), 13 deletions(-)
> > 
> > diff --git a/tests/pm_rps.c b/tests/pm_rps.c
> > index f919625..96225ce 100644
> > --- a/tests/pm_rps.c
> > +++ b/tests/pm_rps.c
> > @@ -364,7 +364,7 @@ static int get_hw_rounded_freq(int target)
> >  	return ret;
> >  }
> >  
> > -static void min_max_config(void (*check)(void), bool load_gpu)
> > +static void min_max_config(void (*check)(void), void
> > (*check2)(void), bool load_gpu)
> >  {
> >  	int fmid = (origfreqs[RPn] + origfreqs[RP0]) / 2;
> >  
> > @@ -384,7 +384,7 @@ static void min_max_config(void (*check)(void),
> > bool load_gpu)
> >  	writeval(stuff[MAX].filp, origfreqs[RP0]);
> >  	if (load_gpu)
> >  		do_load_gpu();
> > -	check();
> > +	check2();
> >  
> >  	igt_debug("\nIncrease min to midpoint...\n");
> >  	writeval(stuff[MIN].filp, fmid);
> > @@ -412,7 +412,7 @@ static void min_max_config(void (*check)(void),
> > bool load_gpu)
> >  	writeval(stuff[MIN].filp, origfreqs[RPn]);
> >  	if (load_gpu)
> >  		do_load_gpu();
> > -	check();
> > +	check2();
> >  
> >  	igt_debug("\nDecrease min below RPn (invalid)...\n");
> >  	writeval_inval(stuff[MIN].filp, 0);
> > @@ -420,11 +420,11 @@ static void min_max_config(void (*check)(void),
> > bool load_gpu)
> >  
> >  	igt_debug("\nDecrease max to midpoint...\n");
> >  	writeval(stuff[MAX].filp, fmid);
> > -	check();
> > +	check2();
> >  
> >  	igt_debug("\nDecrease max to RPn...\n");
> >  	writeval(stuff[MAX].filp, origfreqs[RPn]);
> > -	check();
> > +	check2();
> >  
> >  	igt_debug("\nDecrease max below RPn (invalid)...\n");
> >  	writeval_inval(stuff[MAX].filp, 0);
> > @@ -436,11 +436,11 @@ static void min_max_config(void (*check)(void),
> > bool load_gpu)
> >  
> >  	igt_debug("\nIncrease max to midpoint...\n");
> >  	writeval(stuff[MAX].filp, fmid);
> > -	check();
> > +	check2();
> >  
> >  	igt_debug("\nIncrease max to RP0...\n");
> >  	writeval(stuff[MAX].filp, origfreqs[RP0]);
> > -	check();
> > +	check2();
> >  
> >  	igt_debug("\nIncrease max above RP0 (invalid)...\n");
> >  	writeval_inval(stuff[MAX].filp, origfreqs[RP0] + 1000);
> > @@ -461,7 +461,7 @@ static void basic_check(void)
> >  
> >  #define IDLE_WAIT_TIMESTEP_MSEC 100
> >  #define IDLE_WAIT_TIMEOUT_MSEC 10000
> > -static void idle_check(void)
> > +static void idle_check_min(void)
> >  {
> >  	int freqs[NUMFREQ];
> >  	int wait = 0;
> > @@ -482,6 +482,27 @@ static void idle_check(void)
> >  	igt_debug("Required %d msec to reach cur<=min\n", wait);
> >  }
> >  
> > +static void idle_check_idle(void)
> > +{
> > +	int freqs[NUMFREQ];
> > +	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[RPn])
> > +			break;
> > +		usleep(1000 * IDLE_WAIT_TIMESTEP_MSEC);
> > +		wait += IDLE_WAIT_TIMESTEP_MSEC;
> > +	} while (wait < IDLE_WAIT_TIMEOUT_MSEC);
> > +
> > +	igt_assert_eq(freqs[CUR], freqs[RPn]);
> > +	igt_debug("Required %d msec to reach cur=idle\n", wait);
> > +}
> > +
> >  #define LOADED_WAIT_TIMESTEP_MSEC 100
> >  #define LOADED_WAIT_TIMEOUT_MSEC 3000
> >  static void loaded_check(void)
> > @@ -577,7 +598,7 @@ static void reset(void)
> >  
> >  	igt_debug("Removing load...\n");
> >  	load_helper_stop();
> > -	idle_check();
> > +	idle_check_min();
> >  }
> >  
> >  /*
> > @@ -635,7 +656,7 @@ static void blocking(void)
> >  	matchit(pre_freqs, post_freqs);
> >  
> >  	igt_debug("Removing load...\n");
> > -	idle_check();
> > +	idle_check_min();
> >  }
> >  
> >  static void pm_rps_exit_handler(int sig)
> > @@ -686,14 +707,14 @@ igt_main
> >  	}
> >  
> >  	igt_subtest("basic-api")
> > -		min_max_config(basic_check, false);
> > +		min_max_config(basic_check, basic_check, false);
> >  
> >  	igt_subtest("min-max-config-idle")
> > -		min_max_config(idle_check, true);
> > +		min_max_config(idle_check_min, idle_check_idle,
> > true);
> >  
> >  	igt_subtest("min-max-config-loaded") {
> >  		load_helper_run(HIGH);
> > -		min_max_config(loaded_check, false);
> > +		min_max_config(loaded_check, loaded_check, false);
> >  		load_helper_stop();
> >  	}
> >  

-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx