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

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

[Michel Thierry]

From: Chris Wilson <chris@chris-wilson.co.uk>

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>
Link: http://patchwork.freedesktop.org/patch/msgid/20170605121314.21135-1-chris@chris-wilson.co.uk
Reviewed-by: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: 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 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 */));
+
 	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)));
 
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 2/3] lib: Add reset-type helper in ioctl_wrappers

[Michel Thierry]

Soon we will have tests that are only for platforms with reset-engine
(GEN8+), so add a helper to query the has_gpu_reset via the getparam ioctl.

Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 lib/ioctl_wrappers.c | 22 ++++++++++++++++++++++
 lib/ioctl_wrappers.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 0816a7b6..958b7d03 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -1199,6 +1199,28 @@ bool gem_uses_full_ppgtt(int fd)
 }
 
 /**
+ * gem_gpu_reset_type:
+ * @fd: open i915 drm file descriptor
+ *
+ * Query whether reset-engine (2), global-reset (1) or reset-disable (0)
+ * is available.
+ *
+ * Returns: GPU reset type available
+ */
+int gem_gpu_reset_type(int fd)
+{
+	struct drm_i915_getparam gp;
+	int gpu_reset_type = -1;
+
+	memset(&gp, 0, sizeof(gp));
+	gp.param = I915_PARAM_HAS_GPU_RESET;
+	gp.value = &gpu_reset_type;
+	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+
+	return gpu_reset_type;
+}
+
+/**
  * gem_available_fences:
  * @fd: open i915 drm file descriptor
  *
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index e1279d94..0fd4e8e4 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -164,6 +164,7 @@ bool gem_has_blt(int fd);
 bool gem_has_vebox(int fd);
 bool gem_has_bsd2(int fd);
 int gem_gtt_type(int fd);
+int gem_gpu_reset_type(int fd);
 bool gem_uses_ppgtt(int fd);
 bool gem_uses_full_ppgtt(int fd);
 int gem_available_fences(int fd);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 3/3] tests/gem_reset_stats: Enforce full chip reset mode before run

[Michel Thierry]

Platforms with per-engine reset enabled (i915.reset=2) are unlikely to
perform a full chip reset, keeping the reset_count unmodified. In order
to keep the expectations of this test, enforce that full GPU reset is
enabled (i915.reset=1).

Later on, we can expand the reset_stats ioctl to also return the number
of per-engine resets and use reset_count + reset_engine_count when
checking for the updated reset count.

Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 tests/gem_reset_stats.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c
index 73afeeb2..6777868f 100644
--- a/tests/gem_reset_stats.c
+++ b/tests/gem_reset_stats.c
@@ -27,6 +27,8 @@
 
 #define _GNU_SOURCE
 #include "igt.h"
+#include "igt_sysfs.h"
+#include <limits.h>
 #include <stdbool.h>
 #include <unistd.h>
 #include <stdlib.h>
@@ -777,15 +779,23 @@ igt_main
 		int fd;
 
 		bool has_reset_stats;
+		bool using_full_reset;
 		fd = drm_open_driver(DRIVER_INTEL);
 		devid = intel_get_drm_devid(fd);
 
 		has_reset_stats = gem_has_reset_stats(fd);
 
+		igt_assert(igt_sysfs_set_parameter
+			   (fd, "reset", "%d", 1 /* only global reset */));
+
+		using_full_reset = (gem_gpu_reset_type(fd) == 1);
+
 		close(fd);
 
 		igt_require_f(has_reset_stats,
 			      "No reset stats ioctl support. Too old kernel?\n");
+		igt_require_f(using_full_reset,
+			      "Full GPU reset is not enabled. Is enable_hangcheck set?\n");
 	}
 
 	igt_subtest("params")
@@ -831,4 +841,13 @@ igt_main
 		igt_subtest_f("defer-hangcheck-%s", e->name)
 			RUN_TEST(defer_hangcheck(e));
 	}
+
+	igt_fixture {
+		int fd;
+
+		fd = drm_open_driver(DRIVER_INTEL);
+		igt_assert(igt_sysfs_set_parameter
+			   (fd, "reset", "%d", INT_MAX /* any reset method */));
+		close(fd);
+	}
 }
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Antonio Argenziano]

On 20/06/17 11:25, Michel Thierry wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> 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>
> Link: http://patchwork.freedesktop.org/patch/msgid/20170605121314.21135-1-chris@chris-wilson.co.uk
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: 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 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/3] lib: Add reset-type helper in ioctl_wrappers

[Arkadiusz Hiler]

On Tue, Jun 20, 2017 at 11:25:01AM -0700, Michel Thierry wrote:
> Soon we will have tests that are only for platforms with reset-engine
> (GEN8+), so add a helper to query the has_gpu_reset via the getparam ioctl.
> 
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>  lib/ioctl_wrappers.c | 22 ++++++++++++++++++++++
>  lib/ioctl_wrappers.h |  1 +
>  2 files changed, 23 insertions(+)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 0816a7b6..958b7d03 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -1199,6 +1199,28 @@ bool gem_uses_full_ppgtt(int fd)
>  }
>  
>  /**
> + * gem_gpu_reset_type:
> + * @fd: open i915 drm file descriptor
> + *
> + * Query whether reset-engine (2), global-reset (1) or reset-disable (0)

What about using an enum to save us commenting on the magic numbers
later on?

-- 
Cheers,
Arek

> + * is available.
> + *
> + * Returns: GPU reset type available
> + */
> +int gem_gpu_reset_type(int fd)
> +{
> +	struct drm_i915_getparam gp;
> +	int gpu_reset_type = -1;
> +
> +	memset(&gp, 0, sizeof(gp));
> +	gp.param = I915_PARAM_HAS_GPU_RESET;
> +	gp.value = &gpu_reset_type;
> +	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +
> +	return gpu_reset_type;
> +}
> +
> +/**
>   * gem_available_fences:
>   * @fd: open i915 drm file descriptor
>   *
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index e1279d94..0fd4e8e4 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -164,6 +164,7 @@ bool gem_has_blt(int fd);
>  bool gem_has_vebox(int fd);
>  bool gem_has_bsd2(int fd);
>  int gem_gtt_type(int fd);
> +int gem_gpu_reset_type(int fd);
>  bool gem_uses_ppgtt(int fd);
>  bool gem_uses_full_ppgtt(int fd);
>  int gem_available_fences(int fd);
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/3] lib: Add reset-type helper in ioctl_wrappers

[Michel Thierry]

On 6/28/2017 7:10 AM, Arkadiusz Hiler wrote:
> On Tue, Jun 20, 2017 at 11:25:01AM -0700, Michel Thierry wrote:
>> Soon we will have tests that are only for platforms with reset-engine
>> (GEN8+), so add a helper to query the has_gpu_reset via the getparam ioctl.
>>
>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>> ---
>>   lib/ioctl_wrappers.c | 22 ++++++++++++++++++++++
>>   lib/ioctl_wrappers.h |  1 +
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
>> index 0816a7b6..958b7d03 100644
>> --- a/lib/ioctl_wrappers.c
>> +++ b/lib/ioctl_wrappers.c
>> @@ -1199,6 +1199,28 @@ bool gem_uses_full_ppgtt(int fd)
>>   }
>>   
>>   /**
>> + * gem_gpu_reset_type:
>> + * @fd: open i915 drm file descriptor
>> + *
>> + * Query whether reset-engine (2), global-reset (1) or reset-disable (0)
> 
> What about using an enum to save us commenting on the magic numbers
> later on?
> 

The problem with enum is that we will forget to keep them updated.
But I can do something like we have for gem_gtt_type(); instead of tests 
calling gem_gpu_reset_type directly, I add these helpers:

- gem_reset_enabled {return gem_gpu_reset_type > 0}
- gem_reset_engine_enabled {return gem_gpu_reset_type > 1}

-Michel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2 1/2] lib: Add reset-type helper in ioctl_wrappers

[Michel Thierry]

Soon we will have tests that are only for platforms with reset-engine
(GEN8+), so add a helper to query the has_gpu_reset via the getparam ioctl.

v2: Add more helper functions to avoid using magic numbers in tests (Arek).

Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 lib/ioctl_wrappers.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/ioctl_wrappers.h |  3 +++
 2 files changed, 54 insertions(+)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 2bbaed54..51000bac 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -1193,6 +1193,57 @@ bool gem_uses_full_ppgtt(int fd)
 }
 
 /**
+ * gem_gpu_reset_type:
+ * @fd: open i915 drm file descriptor
+ *
+ * Query whether reset-engine (2), global-reset (1) or reset-disable (0)
+ * is available.
+ *
+ * Returns: GPU reset type available
+ */
+int gem_gpu_reset_type(int fd)
+{
+	struct drm_i915_getparam gp;
+	int gpu_reset_type = -1;
+
+	memset(&gp, 0, sizeof(gp));
+	gp.param = I915_PARAM_HAS_GPU_RESET;
+	gp.value = &gpu_reset_type;
+	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+
+	return gpu_reset_type;
+}
+
+/**
+ * gem_gpu_reset_enabled:
+ * @fd: open i915 drm file descriptor
+ *
+ * Feature test macro to check whether the kernel internally uses hangchecks
+ * and can reset the GPU upon hang detection. Note that this is also true when
+ * reset-engine (the lightweight, single engine reset) is available.
+ *
+ * Returns: Whether the driver will detect hangs and perform a reset.
+ */
+bool gem_gpu_reset_enabled(int fd)
+{
+	return gem_gpu_reset_type(fd) > 0;
+}
+
+/**
+ * gem_engine_reset_enabled:
+ * @fd: open i915 drm file descriptor
+ *
+ * Feature test macro to check whether the kernel internally uses hangchecks
+ * and can reset individual engines upon hang detection.
+ *
+ * Returns: Whether the driver will detect hangs and perform an engine reset.
+ */
+bool gem_engine_reset_enabled(int fd)
+{
+	return gem_gpu_reset_type(fd) > 1;
+}
+
+/**
  * gem_available_fences:
  * @fd: open i915 drm file descriptor
  *
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index e1279d94..8915edca 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -166,6 +166,9 @@ bool gem_has_bsd2(int fd);
 int gem_gtt_type(int fd);
 bool gem_uses_ppgtt(int fd);
 bool gem_uses_full_ppgtt(int fd);
+int gem_gpu_reset_type(int fd);
+bool gem_gpu_reset_enabled(int fd);
+bool gem_engine_reset_enabled(int fd);
 int gem_available_fences(int fd);
 uint64_t gem_total_mappable_size(int fd);
 uint64_t gem_total_stolen_size(int fd);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2 2/2] tests/gem_reset_stats: Enforce full chip reset mode before run

[Michel Thierry]

Platforms with per-engine reset enabled (i915.reset=2) are unlikely to
perform a full chip reset, keeping the reset_count unmodified. In order
to keep the expectations of this test, enforce that full GPU reset is
enabled (i915.reset=1).

Later on, we can expand the reset_stats ioctl to also return the number
of per-engine resets and use reset_count + reset_engine_count when
checking for the updated reset count.

v2: Rebase, don't use gem_gpu_reset_type directly, since we now have
additional helpers.

Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 tests/gem_reset_stats.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c
index 73afeeb2..9ac08aab 100644
--- a/tests/gem_reset_stats.c
+++ b/tests/gem_reset_stats.c
@@ -27,6 +27,8 @@
 
 #define _GNU_SOURCE
 #include "igt.h"
+#include "igt_sysfs.h"
+#include <limits.h>
 #include <stdbool.h>
 #include <unistd.h>
 #include <stdlib.h>
@@ -777,15 +779,24 @@ igt_main
 		int fd;
 
 		bool has_reset_stats;
+		bool using_full_reset;
 		fd = drm_open_driver(DRIVER_INTEL);
 		devid = intel_get_drm_devid(fd);
 
 		has_reset_stats = gem_has_reset_stats(fd);
 
+		igt_assert(igt_sysfs_set_parameter
+			   (fd, "reset", "%d", 1 /* only global reset */));
+
+		using_full_reset = !gem_engine_reset_enabled(fd) &&
+				   gem_gpu_reset_enabled(fd);
+
 		close(fd);
 
 		igt_require_f(has_reset_stats,
 			      "No reset stats ioctl support. Too old kernel?\n");
+		igt_require_f(using_full_reset,
+			      "Full GPU reset is not enabled. Is enable_hangcheck set?\n");
 	}
 
 	igt_subtest("params")
@@ -831,4 +842,13 @@ igt_main
 		igt_subtest_f("defer-hangcheck-%s", e->name)
 			RUN_TEST(defer_hangcheck(e));
 	}
+
+	igt_fixture {
+		int fd;
+
+		fd = drm_open_driver(DRIVER_INTEL);
+		igt_assert(igt_sysfs_set_parameter
+			   (fd, "reset", "%d", INT_MAX /* any reset method */));
+		close(fd);
+	}
 }
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 3/3] tests/gem_reset_stats: Enforce full chip reset mode before run

[Arkadiusz Hiler]

On Tue, Jun 20, 2017 at 11:25:02AM -0700, Michel Thierry wrote:
> Platforms with per-engine reset enabled (i915.reset=2) are unlikely to
> perform a full chip reset, keeping the reset_count unmodified. In order
> to keep the expectations of this test, enforce that full GPU reset is
> enabled (i915.reset=1).
> 
> Later on, we can expand the reset_stats ioctl to also return the number
> of per-engine resets and use reset_count + reset_engine_count when
> checking for the updated reset count.
> 
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>

This no longer applies due to changes in the context. It would be nice
if you would send rebased version as well :-)

@Antonio: any optionion on the patches? They LGTM, but an opinion of
someone more informed wouldn't hurt.

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 3/3] tests/gem_reset_stats: Enforce full chip reset mode before run

[Michel Thierry]

On 06/07/17 04:12, Arkadiusz Hiler wrote:
> On Tue, Jun 20, 2017 at 11:25:02AM -0700, Michel Thierry wrote:
>> Platforms with per-engine reset enabled (i915.reset=2) are unlikely to
>> perform a full chip reset, keeping the reset_count unmodified. In order
>> to keep the expectations of this test, enforce that full GPU reset is
>> enabled (i915.reset=1).
>>
>> Later on, we can expand the reset_stats ioctl to also return the number
>> of per-engine resets and use reset_count + reset_engine_count when
>> checking for the updated reset count.
>>
>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> 
> This no longer applies due to changes in the context. It would be nice
> if you would send rebased version as well :-)

Hi Arek,

I think the v2 of these patches still apply cleanly,

https://patchwork.freedesktop.org/patch/164248/ and
https://patchwork.freedesktop.org/patch/164249/

> 
> @Antonio: any optionion on the patches? They LGTM, but an opinion of
> someone more informed wouldn't hurt.
> 

Indeed.

-Michel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v2 2/2] tests/gem_reset_stats: Enforce full chip reset mode before run

[Antonio Argenziano]

On 28/06/17 11:37, Michel Thierry wrote:
> Platforms with per-engine reset enabled (i915.reset=2) are unlikely to
> perform a full chip reset, keeping the reset_count unmodified. In order
> to keep the expectations of this test, enforce that full GPU reset is
> enabled (i915.reset=1).
> 
> Later on, we can expand the reset_stats ioctl to also return the number
> of per-engine resets and use reset_count + reset_engine_count when
> checking for the updated reset count.
> 
> v2: Rebase, don't use gem_gpu_reset_type directly, since we now have
> additional helpers.
> 
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>   tests/gem_reset_stats.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c
> index 73afeeb2..9ac08aab 100644
> --- a/tests/gem_reset_stats.c
> +++ b/tests/gem_reset_stats.c
> @@ -27,6 +27,8 @@
>   
>   #define _GNU_SOURCE
>   #include "igt.h"
> +#include "igt_sysfs.h"
> +#include <limits.h>
>   #include <stdbool.h>
>   #include <unistd.h>
>   #include <stdlib.h>
> @@ -777,15 +779,24 @@ igt_main
>   		int fd;
>   
>   		bool has_reset_stats;
> +		bool using_full_reset;
>   		fd = drm_open_driver(DRIVER_INTEL);
>   		devid = intel_get_drm_devid(fd);
>   
>   		has_reset_stats = gem_has_reset_stats(fd);
>   
> +		igt_assert(igt_sysfs_set_parameter
> +			   (fd, "reset", "%d", 1 /* only global reset */));
> +
> +		using_full_reset = !gem_engine_reset_enabled(fd) &&
> +				   gem_gpu_reset_enabled(fd);
> +
>   		close(fd);
>   
>   		igt_require_f(has_reset_stats,
>   			      "No reset stats ioctl support. Too old kernel?\n");
> +		igt_require_f(using_full_reset,
> +			      "Full GPU reset is not enabled. Is enable_hangcheck set?\n");
>   	}
>   
>   	igt_subtest("params")
> @@ -831,4 +842,13 @@ igt_main
>   		igt_subtest_f("defer-hangcheck-%s", e->name)
>   			RUN_TEST(defer_hangcheck(e));
>   	}
> +
> +	igt_fixture {
> +		int fd;
> +
> +		fd = drm_open_driver(DRIVER_INTEL);
> +		igt_assert(igt_sysfs_set_parameter
> +			   (fd, "reset", "%d", INT_MAX /* any reset method */));

I would still suggest that we restore the reset value we had at the 
beginning of the test. I think that since we disabled single engine, 
this would actually set the parameter to an unsafe value.

Thanks,
Antonio

> +		close(fd);
> +	}
>   }
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v2 2/2] tests/gem_reset_stats: Enforce full chip reset mode before run

[Michel Thierry]

On 06/07/17 15:50, Antonio Argenziano wrote:
>> +
>> +    igt_fixture {
>> +        int fd;
>> +
>> +        fd = drm_open_driver(DRIVER_INTEL);
>> +        igt_assert(igt_sysfs_set_parameter
>> +               (fd, "reset", "%d", INT_MAX /* any reset method */));
> 
> I would still suggest that we restore the reset value we had at the 
> beginning of the test. I think that since we disabled single engine, 
> this would actually set the parameter to an unsafe value.

I talked with Antonio offline about it, but long story short, it doesn't 
matter much.

Writing INT_MAX to i915.reset is safe; a platform without reset-engine 
support will still only do (and most important, report via the get-param 
ioctl) that it can only do full-gpu-reset, even though i915.reset > 1.

-Michel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 3/3] tests/gem_reset_stats: Enforce full chip reset mode before run

[Arkadiusz Hiler]

On Thu, Jul 06, 2017 at 10:29:41AM -0700, Michel Thierry wrote:
> On 06/07/17 04:12, Arkadiusz Hiler wrote:
> > On Tue, Jun 20, 2017 at 11:25:02AM -0700, Michel Thierry wrote:
> > > Platforms with per-engine reset enabled (i915.reset=2) are unlikely to
> > > perform a full chip reset, keeping the reset_count unmodified. In order
> > > to keep the expectations of this test, enforce that full GPU reset is
> > > enabled (i915.reset=1).
> > > 
> > > Later on, we can expand the reset_stats ioctl to also return the number
> > > of per-engine resets and use reset_count + reset_engine_count when
> > > checking for the updated reset count.
> > > 
> > > Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> > 
> > This no longer applies due to changes in the context. It would be nice
> > if you would send rebased version as well :-)
> 
> Hi Arek,
> 
> I think the v2 of these patches still apply cleanly,
> 
> https://patchwork.freedesktop.org/patch/164248/ and
> https://patchwork.freedesktop.org/patch/164249/

Since you've said that you have a consensus with Antionio, and the
patches LGTM.

Acked-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

Pushed, thanks!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx