Re: [PATCH i-g-t 1/3] lib: Force global reset + uevents for hang detector

[Antonio Argenziano <antonio.argenziano@xxxxxxxxx>]

On 20/06/17 11:25, Michel Thierry wrote:
> From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>
> The hang detector relies on a uevent for notification and aborting the
> test. As proposed, fine-grained resets may not produce a global uevent
> and so this hang detection becomes void. As we don't expect any hang, we
> can just reduce the reset to only a global + uevent and so maintain
> functionality, and switch back to fine-grained resets afterwards.
>
> Note that any test that requires testing fine-grained resets should
> ensure that they are enabled first as igt may leave the global
> parameters in an inconsistent state.
>
> v2: Restore fine-grained resets for explict igt_allow_hang()
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
> Link: http://patchwork.freedesktop.org/patch/msgid/20170605121314.21135-1-chris@xxxxxxxxxxxxxxxxxx
> Reviewed-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> ---
>   lib/igt_aux.c   | 10 ++++++++
>   lib/igt_gt.c    |  4 ++++
>   lib/igt_sysfs.c | 72 ++++++++++++++++++++++++++++++++++++++++++---------------
>   lib/igt_sysfs.h |  6 +++++
>   4 files changed, 74 insertions(+), 18 deletions(-)
>
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index f76e55d5..eb563f72 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -58,6 +58,7 @@
>   #include "igt_debugfs.h"
>   #include "igt_gt.h"
>   #include "igt_rand.h"
> +#include "igt_sysfs.h"
>   #include "config.h"
>   #include "intel_reg.h"
>   #include "ioctl_wrappers.h"
> @@ -451,6 +452,15 @@ void igt_fork_hang_detector(int fd)
>   
>   	igt_assert(fstat(fd, &st) == 0);
>   
> +	/*
> +	 * Disable per-engine reset to force an error uevent. We don't
> +	 * expect to get any hangs whilst the detector is enabled (if we do
> +	 * they are a test failure!) and so the loss of per-engine reset
> +	 * functionality is not an issue.
> +	 */
> +	igt_assert(igt_sysfs_set_parameter
> +		   (fd, "reset", "%d", 1 /* only global reset */));
> +

I think we should restore this parameter to the original value it had 
before the test started when we exit, or at least when we exit cleanly 
(test didn't fail).  We are changing the behavior of the system without 
advertising it too much. The next test (which might not be IGT) could 
now have an unexpected outcome. I know there has been already a 
discussion about it here: 
"http://www.spinics.net/lists/intel-gfx/msg129851.html"; but I don't 
think there was a clear closure. As a general point I would say that if 
possible a test should always leave the system in the state that it had 
at the beginning of the test.

Antonio

>   	signal(SIGIO, sig_abort);
>   	igt_fork_helper(&hang_detector)
>   		hang_detector_process(getppid(), st.st_rdev);
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index be44fcae..6f7daa5e 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -21,6 +21,7 @@
>    * IN THE SOFTWARE.
>    */
>   
> +#include <limits.h>
>   #include <string.h>
>   #include <strings.h>
>   #include <signal.h>
> @@ -162,6 +163,9 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
>   	struct local_i915_gem_context_param param;
>   	unsigned ban;
>   
> +	igt_assert(igt_sysfs_set_parameter
> +		   (fd, "reset", "%d", INT_MAX /* any reset method */));
> +
>   	if (!igt_check_boolean_env_var("IGT_HANG", true))
>   		igt_skip("hang injection disabled by user");
>   	gem_context_require_bannable(fd);
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 8ebc5b4f..15ed34be 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -139,6 +139,35 @@ int igt_sysfs_open(int device, int *idx)
>   }
>   
>   /**
> + * igt_sysfs_set_parameters:
> + * @device: fd of the device (or -1 to default to Intel)
> + * @parameter: the name of the parameter to set
> + * @fmt: printf-esque format string
> + *
> + * Returns true on success
> + */
> +bool igt_sysfs_set_parameter(int device,
> +			     const char *parameter,
> +			     const char *fmt, ...)
> +{
> +	va_list ap;
> +	int dir;
> +	int ret;
> +
> +	dir = igt_sysfs_open_parameters(device);
> +	if (dir < 0)
> +		return false;
> +
> +	va_start(ap, fmt);
> +	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
> +	va_end(ap);
> +
> +	close(dir);
> +
> +	return ret > 0;
> +}
> +
> +/**
>    * igt_sysfs_open_parameters:
>    * @device: fd of the device (or -1 to default to Intel)
>    *
> @@ -336,19 +365,7 @@ int igt_sysfs_scanf(int dir, const char *attr, const char *fmt, ...)
>   	return ret;
>   }
>   
> -/**
> - * igt_sysfs_printf:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> - * @fmt: printf format string
> - * @...: Additional paramaters to store the scaned input values
> - *
> - * printf() wrapper for sysfs.
> - *
> - * Returns:
> - * Number of characters written, negative value on error.
> - */
> -int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
> +int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>   {
>   	FILE *file;
>   	int fd;
> @@ -360,12 +377,7 @@ int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
>   
>   	file = fdopen(fd, "w");
>   	if (file) {
> -		va_list ap;
> -
> -		va_start(ap, fmt);
>   		ret = vfprintf(file, fmt, ap);
> -		va_end(ap);
> -
>   		fclose(file);
>   	}
>   	close(fd);
> @@ -374,6 +386,30 @@ int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
>   }
>   
>   /**
> + * igt_sysfs_printf:
> + * @dir: directory for the device from igt_sysfs_open()
> + * @attr: name of the sysfs node to open
> + * @fmt: printf format string
> + * @...: Additional paramaters to store the scaned input values
> + *
> + * printf() wrapper for sysfs.
> + *
> + * Returns:
> + * Number of characters written, negative value on error.
> + */
> +int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
> +{
> +	va_list ap;
> +	int ret;
> +
> +	va_start(ap, fmt);
> +	ret = igt_sysfs_vprintf(dir, attr, fmt, ap);
> +	va_end(ap);
> +
> +	return ret;
> +}
> +
> +/**
>    * igt_sysfs_get_u32:
>    * @dir: directory for the device from igt_sysfs_open()
>    * @attr: name of the sysfs node to open
> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> index 4089535d..d666438a 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -29,6 +29,10 @@
>   
>   int igt_sysfs_open(int device, int *idx);
>   int igt_sysfs_open_parameters(int device);
> +bool igt_sysfs_set_parameter(int device,
> +			     const char *parameter,
> +			     const char *fmt, ...)
> +	__attribute__((format(printf,3,4)));
>   
>   int igt_sysfs_read(int dir, const char *attr, void *data, int len);
>   int igt_sysfs_write(int dir, const char *attr, const void *data, int len);
> @@ -38,6 +42,8 @@ char *igt_sysfs_get(int dir, const char *attr);
>   
>   int igt_sysfs_scanf(int dir, const char *attr, const char *fmt, ...)
>   	__attribute__((format(scanf,3,4)));
> +int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
> +	__attribute__((format(printf,3,0)));
>   int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
>   	__attribute__((format(printf,3,4)));
>   
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx