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

[Daniel Vetter <daniel@ffwll.ch>]

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

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