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

[Michel Thierry]

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

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 at intel.com>