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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jan 23, 2014 at 03:54:50PM -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.
> 
> v2: Not using forcewake for this particular subtest per Daniel's
>     suggestion.
> 
> Signed-off-by: Jeff McGee <jeff.mcgee@intel.com>

Hm, I guess I wasn't clear enough: I've thought about adding a new subtest
which just does the check that the actual frequency reaches the lowest
level on idle a new subtest, not extend the existing one.

Same somewhat holds for your first patch, atm the testcase is a very basic
test of the kernel error checking.

But even ignoring that I'm not really sure what you're aiming at. Imo the
current coverage is good enough since it makes sure that we have at least
a bit of error checking in place. Any extensions to this test should imo
only be done when we add new features or to exercise bugs (or classes of
bugs) that actually happened. Iirc (and I didn't check olds mails, so this
might be wrong) we have some corner-cases across suspend/resume and some
with the in-kernel code to adjust the requested frequency under load. So
imo that's what we should test for.

Reading through my test requirements write-up I've just noticed that I
didn't emphasis that there's also too much testing possible imho. I'm not
a big proponent of test driven developement and similar validate
everything approaches. I guess I need to write up my thoughts about too
much testing, too ;-)

Anyway I'm a bit confused about what's the overall goal here, so please
elaborate.

Cheers, Daniel

> --- tests/pm_rps.c | 22 ++++++++++++++++++---- 1 file changed, 18
> insertions(+), 4 deletions(-)
> 
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c index 7ae0438..24a1ad6
> 100644 --- a/tests/pm_rps.c +++ b/tests/pm_rps.c @@ -206,13 +206,27 @@
> 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) -- 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