Re: [PATCH] lib/igt_aux: Introduce igt_interactive_debug_manual_check.

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Mar 16, 2015 at 06:44:05PM -0400, Rodrigo Vivi wrote:
> This is an extention of igt_debug_wait_for_keypress that also can have
> customized message and return key pressed.
> 
> v2: This is actualy a v2. V1 was an extension of original
>     igt_debug_wait_for_keypress but it was nacked.
> 
> v3: Make [Y/n] check inside aux function as suggested by Daniel.
>     Also renaming and adding first use case along with the axu function.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  lib/igt_aux.c            | 57 ++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_aux.h            |  1 +
>  tests/kms_psr_sink_crc.c | 11 ++++------
>  3 files changed, 62 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index 131ff4b..8b08678 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -448,6 +448,63 @@ void igt_debug_wait_for_keypress(const char *var)
>  	tcsetattr ( STDIN_FILENO, TCSANOW, &oldt );
>  }
>  
> +/**
> + * igt_interactive_debug_manual_check:
> + * @var: var lookup to to enable this wait
> + * @expected: message to be printed as expected behaviour before wait for keys Y/n
> + *
> + * Waits for a key press when run interactively and when the corresponding debug
> + * var is set in the --interactive-debug=<var> variable. Multiple vars
> + * can be specified as a comma-separated list or alternatively "all" if a wait
> + * should happen for all cases.
> + *
> + * This is useful for display tests where under certain situation manual
> + * inspection of the display is useful. Or when running a testcase in the
> + * background.
> + *
> + * When not connected to a terminal interactive_debug is ignored
> + * and execution immediately continues. For this reason by default this function
> + * returns true. It returns false only when N/n is pressed indicating the
> + * user ins't seeing what was expected.
> + *
> + * Returns: False on N/n is pressed. True otherwise.
> + */
> +bool igt_interactive_debug_manual_check(const char *var, const char *expected)

See my other mail, but for consistency I think we should call this
igt_debug_manual_check. That way we match the prefix of
igt_debug_wait_for_keypress.

> +{
> +	struct termios oldt, newt;
> +	char key;
> +
> +	if (!isatty(STDIN_FILENO))
> +		return true;
> +
> +	if (!igt_interactive_debug)
> +		return true;
> +
> +	if (!strstr(igt_interactive_debug, var) &&
> +	    !strstr(igt_interactive_debug, "all"))
> +		return true;
> +
> +	igt_info("Is %s [Y/n]", expected);
> +
> +	tcgetattr ( STDIN_FILENO, &oldt );
> +	newt = oldt;
> +	newt.c_lflag &= ~ICANON;
> +	tcsetattr ( STDIN_FILENO, TCSANOW, &newt );
> +	key = getchar();
> +	tcsetattr ( STDIN_FILENO, TCSANOW, &oldt );
> +
> +
> +	if (key) {
> +		if (key == 'n' || key == 'N') {
> +			igt_info("\n");
> +			return false;
> +		} else
> +			igt_info("\n");
> +	}
> +
> +	return true;
> +}
> +
>  #define POWER_DIR "/sys/devices/pci0000:00/0000:00:02.0/power"
>  /* We just leak this on exit ... */
>  int pm_status_fd = -1;
> diff --git a/lib/igt_aux.h b/lib/igt_aux.h
> index 0c361f2..de909a1 100644
> --- a/lib/igt_aux.h
> +++ b/lib/igt_aux.h
> @@ -63,6 +63,7 @@ void igt_system_hibernate_autoresume(void);
>  void igt_drop_root(void);
>  
>  void igt_debug_wait_for_keypress(const char *var);
> +bool igt_interactive_debug_manual_check(const char *var, const char *expected);
>  
>  enum igt_runtime_pm_status {
>  	IGT_RUNTIME_PM_STATUS_ACTIVE,
> diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
> index 24f5ca8..fcf9c66 100644
> --- a/tests/kms_psr_sink_crc.c
> +++ b/tests/kms_psr_sink_crc.c
> @@ -266,7 +266,7 @@ static void get_sink_crc(data_t *data, char *crc) {
>  	igt_require(file);
>  
>  	ret = fscanf(file, "%s\n", crc);
> -	igt_require_f(ret > 0, "Sink CRC is unreliable on this machine. Try manual debug with --interactive-debug=manual\n");
> +	igt_require_f(ret > 0, "Sink CRC is unreliable on this machine. Try manual debug with --interactive-debug=no-crc\n");
>  
>  	fclose(file);
>  
> @@ -316,12 +316,9 @@ static bool is_green(char *crc)
>  
>  static void assert_or_manual(bool condition, const char *expected)
>  {
> -	if (igt_interactive_debug)
> -		igt_info("Is %s?\n", expected);
> -	else
> -		igt_debug("%s\n", expected);
> -	igt_debug_wait_for_keypress("manual");
> -	igt_assert(igt_interactive_debug || condition);
> +	int ret;
> +	ret = igt_interactive_debug_manual_check("no-crc", expected);
> +	igt_assert(ret || condition);

Imo the igt assert should be in the helper too, and the helper just has a
return type of void.
-Daniel

>  }
>  
>  static void test_crc(data_t *data)
> -- 
> 1.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx