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

[Andi Shyti]

Hi Tvrtko,

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

we added __for_each_static_engine() for simplicity and used when
the user does want to loop through static engines.

But if a user wants static engines only if the API is not
supported, then for_each_physical_engine() works, as well.

Indeed, the code says:

if (get_param()) { /* we check here if the API fails */

	Loop through the static list if igt_only_list_subtests
	otherwise loop through exising engines (as it was done
	before by executing a batch buffer).

} else if (!param.size) {

	query and map engines

} else {

	get the engines from the previously contructed list

}

Andi