[PATCH i-g-t v6 2/2] Skip the test when no engines are found

[Peter Senna Tschudin]

On Tue, Apr 2, 2024 at 6:17 PM Kamil Konieczny
<kamil.konieczny at linux.intel.com> wrote:
>
> Hi Peter,
> On 2024-03-30 at 15:19:32 +0100, Peter Senna Tschudin wrote:
> > This patch calls igt_skip() when no engines are found by
> > find_first_available_engine() preventing downstream code from crashing.
>
> I would prefer to have this change in one patch.

v7 on the way as a single patch.

>
> >
> > Signed-off-by: Peter Senna Tschudin <me at petersenna.com>
> > ---
> >  tests/intel/gem_exec_capture.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> > index a8348f21b..2afb84283 100644
> > --- a/tests/intel/gem_exec_capture.c
> > +++ b/tests/intel/gem_exec_capture.c
> > @@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
> >               ctx = intel_ctx_create_all_physical(fd); \
> >               igt_assert(ctx); \
>
> If 'e' is causing problems here, maybe introduce new var 'tmpe'?

Not trivial as we can only define a new variable inside a macro inside
a block of something like a for loop.

> So you can start with:
>
>         e = NULL;
>                 for_each_ctx_engine(fd, ctx, tmpe) \
>
> then use tmpe->engine and after for_each... loop you may add
> igt_skip_on_f(&saved.engine == NULL, "no capable engine found\n"); \
>
> > -                     for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> > +                     if(gem_class_can_store_dword(fd, e->class)) { \
>
> This looks much better, you replaced confusing macro with if()
>
> >                               igt_assert(e); \
> >                               saved = configure_hangs(fd, e, ctx->id); \
> >                               break; \
> > +                     } else { \
> > +                             igt_skip("find_first_available_engine(): No engine available\n"); \
>
> This do not solve a problem, you may have capable engine later on
> as there is for_each... loop.

Wow, thank you for catching that! I was confused by the 'break', my
reading of the code was wrong. Thank you!

>
> >                       } \
> >               e = &saved.engine; \
>
> If using 'tmpe' this should be before 'break;' line and here
> you should place igt_skip_on_f()

I guess we don't need another variable, saved is fine and will have a
valid engine or not when it is the case. What about initializing
`saved_engine.engine.name[0] = '\0';` in many() then:

#define find_first_available_engine(fd, ctx, e, saved) \
    do { \
        ctx = intel_ctx_create_all_physical(fd); \
        igt_assert(ctx); \
        for_each_ctx_engine(fd, ctx, e) { \
            if(gem_class_can_store_dword(fd, e->class)) { \
                saved = configure_hangs(fd, e, ctx->id); \
                break; \
            } \
        } \
        e = &saved.engine; \
        igt_skip_on_f(e->name[0] == '\0', "no capable engine found\n"); \
    } while(0)

Thank you Kamil!


>
> Regards,
> Kamil
>
> >       } while(0)
> > --
> > 2.44.0
> >