* [PATCH igt] lib: Force global reset + uevents for hang detector
@ 2017-06-05 11:05 Chris Wilson
2017-06-05 11:15 ` Mika Kuoppala
2017-06-05 12:13 ` [PATCH igt v2] " Chris Wilson
0 siblings, 2 replies; 8+ messages in thread
From: Chris Wilson @ 2017-06-05 11:05 UTC (permalink / raw)
To: intel-gfx
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.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
cc: Michel Thierr <michel.thierry@intel.com>
---
lib/igt_aux.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 1222806c..ca2feac3 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"
@@ -443,12 +444,34 @@ static void sig_abort(int sig)
igt_assert(!"GPU hung");
}
+static bool set_parameter(int fd, const char *parameter, int value)
+{
+ int dir;
+
+ dir = igt_sysfs_open_parameters(fd);
+ if (dir < 0)
+ return false;
+
+ igt_sysfs_printf(dir, parameter, "%d", value);
+ close(dir);
+
+ return true;
+}
+
void igt_fork_hang_detector(int fd)
{
struct stat st;
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(set_parameter(fd, "reset", 1 /* global reset only */));
+
signal(SIGIO, sig_abort);
igt_fork_helper(&hang_detector)
hang_detector_process(getppid(), st.st_rdev);
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH igt] lib: Force global reset + uevents for hang detector
2017-06-05 11:05 [PATCH igt] lib: Force global reset + uevents for hang detector Chris Wilson
@ 2017-06-05 11:15 ` Mika Kuoppala
2017-06-05 12:00 ` Chris Wilson
2017-06-05 12:23 ` Chris Wilson
2017-06-05 12:13 ` [PATCH igt v2] " Chris Wilson
1 sibling, 2 replies; 8+ messages in thread
From: Mika Kuoppala @ 2017-06-05 11:15 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> 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.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> cc: Michel Thierr <michel.thierry@intel.com>
+y
> ---
> lib/igt_aux.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index 1222806c..ca2feac3 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"
> @@ -443,12 +444,34 @@ static void sig_abort(int sig)
> igt_assert(!"GPU hung");
> }
>
> +static bool set_parameter(int fd, const char *parameter, int value)
> +{
> + int dir;
> +
> + dir = igt_sysfs_open_parameters(fd);
> + if (dir < 0)
> + return false;
> +
> + igt_sysfs_printf(dir, parameter, "%d", value);
> + close(dir);
> +
> + return true;
> +}
> +
> void igt_fork_hang_detector(int fd)
> {
> struct stat st;
>
> 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.
> + */
Makes sense. Tho I only looked one test which disables
hang detector before starting the more fine grained hang tests.
-Mika
> + igt_assert(set_parameter(fd, "reset", 1 /* global reset only */));
> +
> signal(SIGIO, sig_abort);
> igt_fork_helper(&hang_detector)
> hang_detector_process(getppid(), st.st_rdev);
> --
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH igt] lib: Force global reset + uevents for hang detector
2017-06-05 11:15 ` Mika Kuoppala
@ 2017-06-05 12:00 ` Chris Wilson
2017-06-05 12:23 ` Chris Wilson
1 sibling, 0 replies; 8+ messages in thread
From: Chris Wilson @ 2017-06-05 12:00 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx
Quoting Mika Kuoppala (2017-06-05 12:15:15)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > 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.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > cc: Michel Thierr <michel.thierry@intel.com>
> +y
>
> > ---
> > lib/igt_aux.c | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> > index 1222806c..ca2feac3 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"
> > @@ -443,12 +444,34 @@ static void sig_abort(int sig)
> > igt_assert(!"GPU hung");
> > }
> >
> > +static bool set_parameter(int fd, const char *parameter, int value)
> > +{
> > + int dir;
> > +
> > + dir = igt_sysfs_open_parameters(fd);
> > + if (dir < 0)
> > + return false;
> > +
> > + igt_sysfs_printf(dir, parameter, "%d", value);
> > + close(dir);
> > +
> > + return true;
> > +}
> > +
> > void igt_fork_hang_detector(int fd)
> > {
> > struct stat st;
> >
> > 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.
> > + */
>
> Makes sense. Tho I only looked one test which disables
> hang detector before starting the more fine grained hang tests.
I did at first do a set_parameter("reset", INT_MAX) on
stop_hang_detector, but lacked the fd and didn't face wiring it up. So I
decided that any test that actually wants to test per-engine resets
should take of enabling per-engine resets first.
I guess igt_allow_hang() should do the inverse. That makes sense to me.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH igt v2] lib: Force global reset + uevents for hang detector
2017-06-05 11:05 [PATCH igt] lib: Force global reset + uevents for hang detector Chris Wilson
2017-06-05 11:15 ` Mika Kuoppala
@ 2017-06-05 12:13 ` Chris Wilson
2017-06-05 21:21 ` Michel Thierry
1 sibling, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2017-06-05 12:13 UTC (permalink / raw)
To: intel-gfx
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);
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)));
--
2.11.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH igt] lib: Force global reset + uevents for hang detector
2017-06-05 11:15 ` Mika Kuoppala
2017-06-05 12:00 ` Chris Wilson
@ 2017-06-05 12:23 ` Chris Wilson
1 sibling, 0 replies; 8+ messages in thread
From: Chris Wilson @ 2017-06-05 12:23 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx
Quoting Mika Kuoppala (2017-06-05 12:15:15)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> > + /*
> > + * 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.
> > + */
>
> Makes sense. Tho I only looked one test which disables
> hang detector before starting the more fine grained hang tests.
The alternative is that we send an error uevent for per-engine resets.
But I think that's overkill for TDR, and we want to keep as much as
possible generic. :|
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH igt v2] lib: Force global reset + uevents for hang detector
2017-06-05 12:13 ` [PATCH igt v2] " Chris Wilson
@ 2017-06-05 21:21 ` Michel Thierry
2017-06-05 22:57 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Michel Thierry @ 2017-06-05 21:21 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH igt v2] lib: Force global reset + uevents for hang detector
2017-06-05 21:21 ` Michel Thierry
@ 2017-06-05 22:57 ` Chris Wilson
2017-06-05 23:14 ` Michel Thierry
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2017-06-05 22:57 UTC (permalink / raw)
To: Michel Thierry, intel-gfx
Quoting Michel Thierry (2017-06-05 22:21:48)
> 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;
There's nothing fundamentally wrong with that... The problem is that we
are introducing a second reset path, and so anything that is testing
resets should ideally exercise both. :| That's where I was going with
my comments about this leaving igt in an inconsistent state and that we
should specify which path we want to test when we do the tests.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH igt v2] lib: Force global reset + uevents for hang detector
2017-06-05 22:57 ` Chris Wilson
@ 2017-06-05 23:14 ` Michel Thierry
0 siblings, 0 replies; 8+ messages in thread
From: Michel Thierry @ 2017-06-05 23:14 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 6/5/2017 3:57 PM, Chris Wilson wrote:
> Quoting Michel Thierry (2017-06-05 22:21:48)
>> 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;
>
> There's nothing fundamentally wrong with that... The problem is that we
> are introducing a second reset path, and so anything that is testing
> resets should ideally exercise both. :| That's where I was going with
> my comments about this leaving igt in an inconsistent state and that we
> should specify which path we want to test when we do the tests.
Ok, I agree "reset tests" should either exercise both or explicitly set
if they want engine or global reset. Other tests that just require a
hang subtest can use either one.
The caveat would be display's hung subtests, but I noticed they write -1
to i915_wedged, and that will trigger a global reset anyway.
Reviewed-by: Michel Thierry <michel.thierry@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-05 23:14 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-05 11:05 [PATCH igt] lib: Force global reset + uevents for hang detector Chris Wilson
2017-06-05 11:15 ` Mika Kuoppala
2017-06-05 12:00 ` Chris Wilson
2017-06-05 12:23 ` Chris Wilson
2017-06-05 12:13 ` [PATCH igt v2] " Chris Wilson
2017-06-05 21:21 ` Michel Thierry
2017-06-05 22:57 ` Chris Wilson
2017-06-05 23:14 ` Michel Thierry
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.