| Message ID | 20180815092511.3838-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] igt/pm_rpm: Close local fd before trying to unload module | expand |
On Wed, Aug 15, 2018 at 10:25:11AM +0100, Chris Wilson wrote: > Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > tests/pm_rpm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c > index 65489bcdb..a4f9f783e 100644 > --- a/tests/pm_rpm.c > +++ b/tests/pm_rpm.c > @@ -2034,6 +2034,8 @@ int main(int argc, char *argv[]) > teardown_environment(); > > igt_subtest("module-reload") { > + teardown_environment(); > + Why isn't the teardown_environment(); under igt_fixture enough? > igt_debug("Reload w/o display\n"); > igt_i915_driver_unload(); > igt_assert_eq(igt_i915_driver_load("disable_display=1"), 0); > -- > 2.18.0 > > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev
Quoting Imre Deak (2018-08-15 12:01:31) > On Wed, Aug 15, 2018 at 10:25:11AM +0100, Chris Wilson wrote: > > Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > tests/pm_rpm.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c > > index 65489bcdb..a4f9f783e 100644 > > --- a/tests/pm_rpm.c > > +++ b/tests/pm_rpm.c > > @@ -2034,6 +2034,8 @@ int main(int argc, char *argv[]) > > teardown_environment(); > > > > igt_subtest("module-reload") { > > + teardown_environment(); > > + > > Why isn't the teardown_environment(); under igt_fixture enough? Hmm, looks like I thought about it earlier. I guess we leak an fd... -Chris
On 15/08/18 02:25, Chris Wilson wrote: > Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > tests/pm_rpm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c > index 65489bcdb..a4f9f783e 100644 > --- a/tests/pm_rpm.c > +++ b/tests/pm_rpm.c > @@ -2034,6 +2034,8 @@ int main(int argc, char *argv[]) > teardown_environment(); > > igt_subtest("module-reload") { > + teardown_environment(); There is already a fixture with a call to 'teardown_environment()' surrounding this test, is it missing a couple of subtests groups? Antonio > + > igt_debug("Reload w/o display\n"); > igt_i915_driver_unload(); > igt_assert_eq(igt_i915_driver_load("disable_display=1"), 0); >
Quoting Antonio Argenziano (2018-08-17 18:29:09) > > > On 15/08/18 02:25, Chris Wilson wrote: > > Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > tests/pm_rpm.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c > > index 65489bcdb..a4f9f783e 100644 > > --- a/tests/pm_rpm.c > > +++ b/tests/pm_rpm.c > > @@ -2034,6 +2034,8 @@ int main(int argc, char *argv[]) > > teardown_environment(); > > > > igt_subtest("module-reload") { > > + teardown_environment(); > > There is already a fixture with a call to 'teardown_environment()' > surrounding this test, is it missing a couple of subtests groups? It was intended for sequential execution (and I had forgotten that I had purposely placed it after the teardown_environment -- the confusion was caused by teardown_environment being incomplete). Adding a subtest group for the preceding bunch would have the debatable consequence of module- reload reporting a FAIL if something else caused a wakeref leak. My opinion is to prefer SKIP for external artefacts so that it is clear when a test failed of its own accord (and so be useful for debugging). It just so happens that this test was to reproduce the trigger for the external failures elsewhere and so detect the wakeref leak. -Chris
On 17/08/18 10:49, Chris Wilson wrote: > Quoting Antonio Argenziano (2018-08-17 18:29:09) >> >> >> On 15/08/18 02:25, Chris Wilson wrote: >>> Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload") >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >>> tests/pm_rpm.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c >>> index 65489bcdb..a4f9f783e 100644 >>> --- a/tests/pm_rpm.c >>> +++ b/tests/pm_rpm.c >>> @@ -2034,6 +2034,8 @@ int main(int argc, char *argv[]) >>> teardown_environment(); >>> >>> igt_subtest("module-reload") { >>> + teardown_environment(); >> >> There is already a fixture with a call to 'teardown_environment()' >> surrounding this test, is it missing a couple of subtests groups? > > It was intended for sequential execution (and I had forgotten that I had > purposely placed it after the teardown_environment -- the confusion was > caused by teardown_environment being incomplete). Adding a subtest group > for the preceding bunch would have the debatable consequence of module- > reload reporting a FAIL if something else caused a wakeref leak. My > opinion is to prefer SKIP for external artefacts so that it is clear when > a test failed of its own accord (and so be useful for debugging). It just > so happens that this test was to reproduce the trigger for the external > failures elsewhere and so detect the wakeref leak. Agreed. Maybe a comment would help, your choice. Reviewed-by: Antonio Argenziano <antonio.argenziano@intel.com> > -Chris >
diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c index 65489bcdb..a4f9f783e 100644 --- a/tests/pm_rpm.c +++ b/tests/pm_rpm.c @@ -2034,6 +2034,8 @@ int main(int argc, char *argv[]) teardown_environment(); igt_subtest("module-reload") { + teardown_environment(); + igt_debug("Reload w/o display\n"); igt_i915_driver_unload(); igt_assert_eq(igt_i915_driver_load("disable_display=1"), 0);
Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- tests/pm_rpm.c | 2 ++ 1 file changed, 2 insertions(+)