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

["Summers, Stuart" <stuart.summers@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6376 bytes --]

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 <tvrtko.ursulin@intel.com>

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

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev