On Thu, 2019-12-05 at 10:23 +0000, Tvrtko Ursulin wrote: > On 05/12/2019 10:09, Petri Latvala wrote: > > On Thu, Dec 05, 2019 at 09:54:25AM +0000, Tvrtko Ursulin wrote: > > > > > > On 05/12/2019 09:47, Andi Shyti wrote: > > > > Hi, > > > > > > > > On Thu, Dec 05, 2019 at 09:22:56AM +0000, Tvrtko Ursulin wrote: > > > > > On 05/12/2019 09:01, Petri Latvala wrote: > > > > > > On Wed, Dec 04, 2019 at 07:05:12PM +0000, Summers, Stuart > > > > > > wrote: > > > > > > > On Wed, 2019-12-04 at 16:53 +0200, Petri Latvala wrote: > > > > > > > > On Wed, Dec 04, 2019 at 09:36:11AM +0000, Tvrtko > > > > > > > > Ursulin wrote: > > > > > > > > > > > > > > > > > > On 03/12/2019 05:11, Stuart Summers wrote: > > > > > > > > > > Align with gem_exec_basic and other tests using the > > > > > > > > > > newer > > > > > > > > > > engine query interface into i915 to enumerate > > > > > > > > > > active engines. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Stuart Summers < > > > > > > > > > > stuart.summers@intel.com> > > > > > > > > > > --- > > > > > > > > > > tests/i915/gem_ctx_isolation.c | 4 ++-- > > > > > > > > > > tests/i915/gem_ctx_persistence.c | 2 +- > > > > > > > > > > 2 files changed, 3 insertions(+), 3 > > > > > > > > > > deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/tests/i915/gem_ctx_isolation.c > > > > > > > > > > b/tests/i915/gem_ctx_isolation.c > > > > > > > > > > index 6aa27133..9435209e 100644 > > > > > > > > > > --- a/tests/i915/gem_ctx_isolation.c > > > > > > > > > > +++ b/tests/i915/gem_ctx_isolation.c > > > > > > > > > > @@ -856,6 +856,7 @@ static unsigned int > > > > > > > > > > __has_context_isolation(int fd) > > > > > > > > > > igt_main > > > > > > > > > > { > > > > > > > > > > + struct intel_execution_engine2 *e; > > > > > > > > > > unsigned int has_context_isolation = 0; > > > > > > > > > > int fd = -1; > > > > > > > > > > @@ -876,8 +877,7 @@ igt_main > > > > > > > > > > igt_skip_on(gen > > > > > > > > > > > LAST_KNOWN_GEN); > > > > > > > > > > } > > > > > > > > > > - for (const struct intel_execution_engine2 *e = > > > > > > > > > > intel_execution_engines2; > > > > > > > > > > - e->name; e++) { > > > > > > > > > > + __for_each_physical_engine(fd, e) { > > > > > > > > > > igt_subtest_group { > > > > > > > > > > igt_fixture { > > > > > > > > > > igt_require(has > > > > > > > > > > _context_isolati > > > > > > > > > > on & (1 << e->class)); > > > > > > > > > > diff --git a/tests/i915/gem_ctx_persistence.c > > > > > > > > > > b/tests/i915/gem_ctx_persistence.c > > > > > > > > > > index d68431ae..30772159 100644 > > > > > > > > > > --- a/tests/i915/gem_ctx_persistence.c > > > > > > > > > > +++ b/tests/i915/gem_ctx_persistence.c > > > > > > > > > > @@ -727,7 +727,7 @@ igt_main > > > > > > > > > > igt_subtest("hangcheck") > > > > > > > > > > test_nohangcheck_hostile(i915); > > > > > > > > > > - __for_each_static_engine(e) { > > > > > > > > > > + __for_each_physical_engine(i915, e) { > > > > > > > > > > igt_subtest_group { > > > > > > > > > > igt_fixture { > > > > > > > > > > gem_require_rin > > > > > > > > > > g(i915, e- > > > > > > > > > > > flags); > > > > > > > > > > > > > > > > > > __for_each_static_engine is correct, at least if you > > > > > > > > > don't want CI > > > > > > > > > folks go > > > > > > > > > look for their pitchforks. :) Same for the first > > > > > > > > > hunk, everything > > > > > > > > > that > > > > > > > > > enumerates subtests needs to be static. > > > > > > > > > > > > > > > > > > Option 2, the preferred one - convert the tests to > > > > > > > > > igt_subtest_with_dynamic > > > > > > > > > and then you can use __for_each_physical_engine. > > > > > > > > > > > > > > > > Doesn't __for_each_physical_engine anyway have a hack > > > > > > > > for being > > > > > > > > called in that context? > > > > > > > > > > > > > > > > Btw, option 2 is > > > > > > > > https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/44 > > > > > > > > > > > > > > > > Currently blocking that effort is getting > > > > > > > > https://patchwork.freedesktop.org/series/70286/ into > > > > > > > > shape. > > > > > > > > > > > > > > Wait I'm a little confused, sorry for the naivete here. > > > > > > > Does this mean > > > > > > > these kinds of changes are blocked on the above series? > > > > > > > Or are you > > > > > > > saying I should go ahead and convert this to the dynamic > > > > > > > subtests? Or > > > > > > > that we can move forward with the current approach and > > > > > > > convert at a > > > > > > > later time? > > > > > > > > > > > > Option 2 is blocked by series 70286, sorry for the > > > > > > confusion. I don't > > > > > > mind slapping your patch in while waiting for that, since > > > > > > it's $RAND > > > > > > days away still to get there, if it fixes a problem you're > > > > > > having now. > > > > > > > > > > I think hack for static enumeration if in --list-subtest mode > > > > > did not work > > > > > in practice. Andi, do you remember the details? > > > > > > > > __for_each_physical_engine checks for igt_only_list_subtests(), > > > > if so it behaves like __for_each_static_engine(). > > > > > > > > Was this what you asked? > > > > > > Yes. Am I mis-remembering that there was a problem with this > > > mandating to > > > keep using __for_each_static_engine directly for subtest > > > enumeration? Maybe > > > I am.. > > > > __physical is used in a couple of tests to enumerate subtests > > already, > > and there are still issues caused by that. Not in the CI sense, the > > enumeration works, but can cause havoc when executing the tests. > > Don't > > have links available on hand now, sorry. > > > > In a nutshell: It's ok for now. I don't like it, but it's used > > already > > and it's going away throughout soon. Feel free to use it until said > > otherwise. > > Okay, sorry for the confusion: > > Reviewed-by: Tvrtko Ursulin Thanks for the review here and the discussion. Not sure I have push rights here yet. Any chance for some help getting this merged? Thanks, Stuart > > Regards, > > Tvrtko > >