[Intel-gfx] [PATCH igt v2] lib: Force global reset + uevents for hang detector

[Chris Wilson]

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 at chris-wilson.co.uk>
> > Cc: Michel Thierry <michel.thierry at 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