[PATCH i-g-t v5] tests/intel/gem_exec_capture: Fix many-* subtests

[Peter Senna Tschudin]

Hi Kamil,

Thanks again!

On Thu, Mar 28, 2024 at 4:49 PM Kamil Konieczny
<kamil.konieczny at linux.intel.com> wrote:
>
> Hi Peter,
> On 2024-03-28 at 13:42:51 +0100, Peter Senna Tschudin wrote:
> > This patch fixes the tests many-4k-incremental and many-4k-zero from
> > gem_exec_capture that are currently failing with an invalid file
> > descriptor error.
> >
> > tests/intel/gem_exec_capture.c
> >     many(), userptr(), capture_invisible()
>
> Please cite error log, delete two above lines and write that problem
> is with macro find_first_available_engine() (you later describe in datails).
>
> >         find_first_available_engine()
> >             for_each_ctx_engine()
> >                 for_each_ctx_cfg_engine()
> >
> > find_first_available_engine() expects a struct intel_execution_engine2
> > *e as parameter. for_each_ctx_cfg_engine() allocates a struct
> > intel_engine_data ed inside a for loop and then update e to point to an
> > element of ed.
> >
> > The problem is that ed has the block scope of the for loop, and trying
> > to access its content through e after the for loop has ended creates
> > undefined behavior.
> >
> > This patch updates e to point to the same content at a different memory
> > region, to avoid the block scope of ed. `e = &saved.engine;` updates e
> > to point to the same content but without scope restrictions as both
> > saved and e have the scope of the calling function.
> >
> > v5: Update subject and commit message.
> >
> > Signed-off-by: Peter Senna Tschudin <me at petersenna.com>
> > ---
> >  tests/intel/gem_exec_capture.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> > index 57b178f3e..f7b10bdd4 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); \
> >               for_each_ctx_engine(fd, ctx, e) \
> > -                     for_each_if(gem_class_can_store_dword(fd, e->class)) \
> > +                     for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> > +                             saved = configure_hangs(fd, e, ctx->id); \
> >                               break; \
> > +                     }  \
> > +             e = &saved.engine; \
>
> Ok, we need 'e' later on, so it is ok.
>
> >               igt_assert(e); \
>
> But this assert do no work, it will always succeed. Imho here
> code should check if it actually saved anything, so what about:

That was my bad, sorry. igt_assert(e) should be before the call to
configure_hangs() because 'e' is set by intel_get_current_engine()
which can return null.

>
> #define find_first_available_engine(fd, ctx, e, saved) \
>     do { \
>         bool no_dw_store_engine = true; \
>         \
>         ctx = intel_ctx_create_all_physical(fd); \
>         igt_assert(ctx); \
>         for_each_ctx_engine(fd, ctx, e) \
>             for_each_if(gem_class_can_store_dword(fd, e->class)) { \
>                 saved = configure_hangs(fd, e, ctx->id); \
>                 no_dw_store_engine = false; \
>                 break; \
>             }  \
>         e = &saved.engine; \
>         igt_skip_on(no_dw_store_engine); \
>     } while(0)

Yes! I sent a slightly different version skipping the test when
gem_class_can_store_dword() returns false. I sent a second patch for
this, in the hopes that having two patches is the "correct" way.

[...]