| Message ID | 20200213012840.31472-6-dale.b.stimson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmio_base via debugfs infrastructure + gem_ctx_isolation | expand |
On Wed, Feb 12, 2020 at 05:28:40PM -0800, Dale B Stimson wrote: > At the start of igt_main, failure of the initial tests for successful > initialization transfer control to the end of an igt_fixture block. > From there, execution of the main per-engine loop is attempted. > Instead, the test should be caused to exit. > > If initialization fails, exit. > > Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com> > --- > tests/i915/gem_ctx_isolation.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c > index 07ffbb84a..b11158dab 100644 > --- a/tests/i915/gem_ctx_isolation.c > +++ b/tests/i915/gem_ctx_isolation.c > @@ -898,10 +898,13 @@ igt_main > int fd = -1; > struct eng_mmio_base_table_s *mbp = NULL; > uint32_t mmio_base = 0; > + /* igt_fixture block is skipped if --list-subtests, so start with true. */ > + bool init_successful = true; > > igt_fixture { > int gen; > > + init_successful = false; > fd = drm_open_driver(DRIVER_INTEL); > igt_require_gem(fd); > igt_require(gem_has_contexts(fd)); > @@ -916,8 +919,20 @@ igt_main > igt_skip_on(gen > LAST_KNOWN_GEN); > > mbp = gem_engine_mmio_base_info_get(fd); > + init_successful = true; > } > > + if (!init_successful) { > + igt_exit_early(); > + } > + NAK. All this dancing around the infrastructure just makes changing the infrastructure later be awkward and produce weird errors. If something in the fixture failed, with this code you never enter the subtest, making the test result 'notrun' instead of the correct 'skip' or 'fail'. What is the problem this is trying to solve? Incorrect engine list used? If you have a subtest per static engine, all CI does is execute per static engine. Converting this test to use dynamic subtests for engines is the way forward.
On 2020-02-13 10:29:55, Petri Latvala wrote: > On Wed, Feb 12, 2020 at 05:28:40PM -0800, Dale B Stimson wrote: > > At the start of igt_main, failure of the initial tests for successful > > initialization transfer control to the end of an igt_fixture block. > > From there, execution of the main per-engine loop is attempted. > > Instead, the test should be caused to exit. > > > > If initialization fails, exit. > > > > Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com> > > --- > > tests/i915/gem_ctx_isolation.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c > > index 07ffbb84a..b11158dab 100644 > > --- a/tests/i915/gem_ctx_isolation.c > > +++ b/tests/i915/gem_ctx_isolation.c > > @@ -898,10 +898,13 @@ igt_main > > int fd = -1; > > struct eng_mmio_base_table_s *mbp = NULL; > > uint32_t mmio_base = 0; > > + /* igt_fixture block is skipped if --list-subtests, so start with true. */ > > + bool init_successful = true; > > > > igt_fixture { > > int gen; > > > > + init_successful = false; > > fd = drm_open_driver(DRIVER_INTEL); > > igt_require_gem(fd); > > igt_require(gem_has_contexts(fd)); > > @@ -916,8 +919,20 @@ igt_main > > igt_skip_on(gen > LAST_KNOWN_GEN); > > > > mbp = gem_engine_mmio_base_info_get(fd); > > + init_successful = true; > > } > > > > + if (!init_successful) { > > + igt_exit_early(); > > + } > > + > > NAK. All this dancing around the infrastructure just makes changing > the infrastructure later be awkward and produce weird errors. > > If something in the fixture failed, with this code you never enter the > subtest, making the test result 'notrun' instead of the correct 'skip' > or 'fail'. > > What is the problem this is trying to solve? Incorrect engine list > used? If you have a subtest per static engine, all CI does is execute > per static engine. Converting this test to use dynamic subtests for > engines is the way forward. > > > -- > Petri Latvala NAK understood and accepted. I will address this in a different way, targeting the underlying issues instead of the symptom. Please see my patch (just sent to ML): lib/i915/gem_engine_topology.c - intel_get_current_engine invalid result To answer to your question about what this was trying to solve: Briefly, and specifically addressing gem_ctx_isolation: As-is observed behavior when open (or debugfs open) fails: per-engine loop executes forever: Subtest vecs0-nonpriv: FAIL Subtest vecs0-nonpriv-switch: FAIL Subtest vecs0-clean: FAIL Subtest vecs0-dirty-create: FAIL Subtest vecs0-dirty-switch: FAIL Subtest vecs0-none: FAIL Subtest vecs0-S3: FAIL Subtest vecs0-S4: FAIL Subtest vecs0-reset: FAIL And repeat, ad infinitum for vecs0
On Thu, Feb 13, 2020 at 11:29:48AM -0800, Dale B Stimson wrote: > On 2020-02-13 10:29:55, Petri Latvala wrote: > > On Wed, Feb 12, 2020 at 05:28:40PM -0800, Dale B Stimson wrote: > > > At the start of igt_main, failure of the initial tests for successful > > > initialization transfer control to the end of an igt_fixture block. > > > From there, execution of the main per-engine loop is attempted. > > > Instead, the test should be caused to exit. > > > > > > If initialization fails, exit. > > > > > > Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com> > > > --- > > > tests/i915/gem_ctx_isolation.c | 15 +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c > > > index 07ffbb84a..b11158dab 100644 > > > --- a/tests/i915/gem_ctx_isolation.c > > > +++ b/tests/i915/gem_ctx_isolation.c > > > @@ -898,10 +898,13 @@ igt_main > > > int fd = -1; > > > struct eng_mmio_base_table_s *mbp = NULL; > > > uint32_t mmio_base = 0; > > > + /* igt_fixture block is skipped if --list-subtests, so start with true. */ > > > + bool init_successful = true; > > > > > > igt_fixture { > > > int gen; > > > > > > + init_successful = false; > > > fd = drm_open_driver(DRIVER_INTEL); > > > igt_require_gem(fd); > > > igt_require(gem_has_contexts(fd)); > > > @@ -916,8 +919,20 @@ igt_main > > > igt_skip_on(gen > LAST_KNOWN_GEN); > > > > > > mbp = gem_engine_mmio_base_info_get(fd); > > > + init_successful = true; > > > } > > > > > > + if (!init_successful) { > > > + igt_exit_early(); > > > + } > > > + > > > > NAK. All this dancing around the infrastructure just makes changing > > the infrastructure later be awkward and produce weird errors. > > > > If something in the fixture failed, with this code you never enter the > > subtest, making the test result 'notrun' instead of the correct 'skip' > > or 'fail'. > > > > What is the problem this is trying to solve? Incorrect engine list > > used? If you have a subtest per static engine, all CI does is execute > > per static engine. Converting this test to use dynamic subtests for > > engines is the way forward. > > > > > > -- > > Petri Latvala > > NAK understood and accepted. > > I will address this in a different way, targeting the underlying issues > instead of the symptom. Please see my patch (just sent to ML): > lib/i915/gem_engine_topology.c - intel_get_current_engine invalid result > > To answer to your question about what this was trying to solve: > > Briefly, and specifically addressing gem_ctx_isolation: > > As-is observed behavior when open (or debugfs open) fails: per-engine loop > executes forever: > Subtest vecs0-nonpriv: FAIL > Subtest vecs0-nonpriv-switch: FAIL > Subtest vecs0-clean: FAIL > Subtest vecs0-dirty-create: FAIL > Subtest vecs0-dirty-switch: FAIL > Subtest vecs0-none: FAIL > Subtest vecs0-S3: FAIL > Subtest vecs0-S4: FAIL > Subtest vecs0-reset: FAIL > And repeat, ad infinitum for vecs0 > Ah, the good old non-progressing engine loop. We already have fixes for two of the occurrences, you have found a third one. =(
diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c index 07ffbb84a..b11158dab 100644 --- a/tests/i915/gem_ctx_isolation.c +++ b/tests/i915/gem_ctx_isolation.c @@ -898,10 +898,13 @@ igt_main int fd = -1; struct eng_mmio_base_table_s *mbp = NULL; uint32_t mmio_base = 0; + /* igt_fixture block is skipped if --list-subtests, so start with true. */ + bool init_successful = true; igt_fixture { int gen; + init_successful = false; fd = drm_open_driver(DRIVER_INTEL); igt_require_gem(fd); igt_require(gem_has_contexts(fd)); @@ -916,8 +919,20 @@ igt_main igt_skip_on(gen > LAST_KNOWN_GEN); mbp = gem_engine_mmio_base_info_get(fd); + init_successful = true; } + if (!init_successful) { + igt_exit_early(); + } + + /** + * With --list-subtests the above igt_fixture block is skipped and so + * the device is not open. Because fd < 0, __for_each_physical_engine + * falls back to a static engine list, which will affect the output + * of --list-subtests. + */ + /* __for_each_physical_engine switches context to all engines. */ __for_each_physical_engine(fd, e) { igt_subtest_group {
At the start of igt_main, failure of the initial tests for successful initialization transfer control to the end of an igt_fixture block. From there, execution of the main per-engine loop is attempted. Instead, the test should be caused to exit. If initialization fails, exit. Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com> --- tests/i915/gem_ctx_isolation.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)