[igt-dev] [PATCH i-g-t] tests/i915: Use engine query interface for gem_ctx_isolation/persistence
Summers, Stuart
stuart.summers at intel.com
Thu Dec 5 22:07:30 UTC 2019
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 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_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 at 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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3270 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20191205/1bfc047d/attachment.bin>
More information about the igt-dev
mailing list