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

[Imre Deak <imre.deak@intel.com>]

On ti, 2015-12-01 at 09:22 -0800, Bob Paauwe wrote:
> 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.

It is the correct premise and if it doesn't hold then there is a real
bug either in the testcase or the kernel which needs to be addressed
differently. I haven't found anything concrete but one suspect is the
logic that waits for the GPU idle condition, maybe the timeout
value idle_check() or the hard-coded duration in do_load_gpu() is
incorrect. In any case let's not paper over this issue, the very reason
we have test cases is to uncover such bugs.

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

The only place we should check for freq==idle is in idle_check() and
that one is not called during the loaded subtest, it wouldn't even make
sense to do so. So I'm not sure how this subtest fails for you. 

> 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 point of the "check" argument of min_max_config() is to distinguish
between the idle and loaded cases. The check functions passed in know
already if they can expect the frequency to reach the idle frequency
or not.

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

I think we only need your first patch and if there is any failure after
that we have to root cause that and fix it properly.

--Imre

> 
> > 
> > > ---
> > >  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();
> > >  	}
> > >  
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx