Re: [igt-dev] [PATCH i-g-t 68/77] igt/dummyload: Require an intel_ctx_t for POLL_RUN and !ALL_ENGINES

[Jason Ekstrand <jason@jlekstrand.net>]

On Tue, Jun 15, 2021 at 9:49 PM Dixit, Ashutosh
<ashutosh.dixit@intel.com> wrote:
>
> On Tue, 15 Jun 2021 19:25:01 -0700, Dixit, Ashutosh wrote:
> >
> > On Mon, 14 Jun 2021 09:38:53 -0700, Jason Ekstrand wrote:
> > >
> > > diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
> > > index b0a7b361c..25e49de47 100644
> > > --- a/lib/igt_dummyload.c
> > > +++ b/lib/igt_dummyload.c
> > > @@ -421,22 +421,12 @@ igt_spin_factory(int fd, const struct igt_spin_factory *opts)
> > >  {
> > >     igt_spin_t *spin;
> > >
> > > -   if (opts->engine != ALL_ENGINES) {
> > > -           struct intel_execution_engine2 e;
> > > -           int class;
> > > -
> > > -           if (opts->ctx) {
> > > -                   class = opts->ctx->cfg.engines[opts->engine].engine_class;
> > > -           } else if (!gem_context_lookup_engine(fd, opts->engine,
> > > -                                                 opts->ctx_id, &e)) {
> > > -                   class = e.class;
> > > -           } else {
> > > -                   gem_require_ring(fd, opts->engine);
> > > -                   class = gem_execbuf_flags_to_engine_class(opts->engine);
> > > -           }
> > > +   if ((opts->flags & IGT_SPIN_POLL_RUN) && opts->engine != ALL_ENGINES) {
> > > +           unsigned int class;
> > >
> > > -           if (opts->flags & IGT_SPIN_POLL_RUN)
> > > -                   igt_require(gem_class_can_store_dword(fd, class));
> > > +           igt_assert(opts->ctx);
> > > +           class = intel_ctx_engine_class(opts->ctx, opts->engine);
> > > +           igt_require(gem_class_can_store_dword(fd, class));
> > >     }
> >
> > I am not sure if this is correct. If I straightforwardly enforce opts->ctx
> > and tranform the original logic I get this:
> >
> >         if (opts->engine != ALL_ENGINES) {
> >                 int class;
> >
> >                 igt_assert(opts->ctx);
> >                 class = intel_ctx_engine_class(opts->ctx, opts->engine);
> >                 if (opts->flags & IGT_SPIN_POLL_RUN)
> >                         igt_require(gem_class_can_store_dword(fd, class));
> >         }
> >
> > or even
> >
> >         if (opts->engine != ALL_ENGINES) {
> >                 igt_assert(opts->ctx);
> >                 if (opts->flags & IGT_SPIN_POLL_RUN) {
> >                       int class = intel_ctx_engine_class(opts->ctx, opts->engine);
> >                         igt_require(gem_class_can_store_dword(fd, class));
> >               }
> >         }
> >
> > Importantly, "igt_assert(opts->ctx)" is invoked independent of opts->flags,
> > unlike the code in the patch.

Yes, that's correct.  However, if you look at the original, the only
side-effect of any of the code in the outer `if (opts->engine !=
ALL_ENGINES)` block is the `igt_require(gem_class_can_store_dword()`.
All of the rest of it exists simply to get the class.  My original
intention was to add the `igt_assert(opts->ctx)` as conservatively as
possible.

But, now that I look deeper... we have this in emit_recursive_batch()
which is called unconditionally from igt_spin_factory():

    nengine = 0;
    if (opts->engine == ALL_ENGINES) {
        struct intel_execution_engine2 *engine;

        igt_assert(opts->ctx);
        for_each_ctx_engine(fd, opts->ctx, engine) {
            if (opts->flags & IGT_SPIN_POLL_RUN &&
                !gem_class_can_store_dword(fd, engine->class))
                continue;

            flags[nengine++] = engine->flags;
        }
    } else {
        flags[nengine++] = opts->engine;
    }
    igt_require(nengine);

so I think I was being conservative for no good reason.  I'll
restructure like you suggested.

> I have another related question/observation. If what I am saying above is
> indeed true, is the "ctx_id" field in "struct igt_spin_factory" ever
> really used? If not we should delete it from the struct and we can get rid
> of these asserts.
>
> This is because I am saying above that we have an igt_assert(opts->ctx)
> above in igt_spin_factory() when "opts->engine != ALL_ENGINES" and then we
> have another igt_assert(opts->ctx) in emit_recursive_batch() when
> "opts->engine == ALL_ENGINES". So it appears we must always have
> intel_ctx_t and can never use the "ctx_id" field in "struct
> igt_spin_factory" so it should be removed.

It is gone by the end of the series.

> So appears the spinner interface will always need a intel_ctx_t (meaning
> more code changes).

At the end, it does still work with ctx0 if you use I915_EXEC_RENDER
or similar.  There are a few users of this which just want the GPU
busy for some reason.  However, the vast majority of cases use an
intel_ctx_t.

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