[igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence

[Petri Latvala]

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 at 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_ring(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.


-- 
Petri Latvala