Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

2018-08-17 Thread Antonio Argenziano



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


-Chris


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

2018-08-17 Thread Chris Wilson
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 
> > ---
> >   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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

2018-08-17 Thread Antonio Argenziano



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 
---
  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);


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

2018-08-15 Thread Chris Wilson
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 
> > ---
> >  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

2018-08-15 Thread Imre Deak
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 
> ---
>  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-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx