* [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
* 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
* [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 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.