Re: [PATCH igt v2] lib: Force global reset + uevents for hang detector

[Michel Thierry <michel.thierry@intel.com>]

On 6/5/2017 5:13 AM, Chris Wilson wrote:
> 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@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> ---
>   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 1222806c..b1b63db5 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -60,6 +60,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"
> @@ -449,6 +450,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 */));
> +
>   	signal(SIGIO, sig_abort);
>   	igt_fork_helper(&hang_detector)
>   		hang_detector_process(getppid(), st.st_rdev);

I think the stop_hang_detector needs to restore the 'reset' value too, 
or subsequent tests (using igt_hang_ctx) would only have global reset;

@@ -457,6 +457,8 @@ void igt_fork_hang_detector(int fd)
  void igt_stop_hang_detector(void)
  {
  	igt_stop_helper(&hang_detector);
+	igt_assert(igt_sysfs_set_parameter
+		   (fd, "reset", "%d", INT_MAX /* any reset method */));
  }
  #else
  void igt_fork_hang_detector(int fd)


> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 11643edd..65743d07 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 <signal.h>
>   #include <errno.h>
> @@ -161,6 +162,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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx