[igt-dev] [PATCH i-g-t 43/79] tests/i915/gem_ctx_persistence: Convert to intel_ctx_t

[Dixit, Ashutosh]

Some time ago, Dixit, Ashutosh wrote:
>
> On Tue, 22 Jun 2021 22:38:04 -0700, Jason Ekstrand wrote:
> >
> > On Mon, Jun 21, 2021 at 8:52 PM Dixit, Ashutosh
> > <ashutosh.dixit at intel.com> wrote:
> > >
> > > On Thu, 17 Jun 2021 12:12:50 -0700, Jason Ekstrand wrote:
> > > >
> > > > Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> > >
> > > A couple of questions below just in case, otherwise this is:
> > >
> > > Reviewed-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> >
> > Thanks!  I'll wait to apply that until you're satisfied with the
> > answers below.

I have a question below but please go ahead and apply.

> >
> > > > diff --git a/lib/intel_ctx.h b/lib/intel_ctx.h
> > > > index 054fecc4a..e34cefc14 100644
> > > > --- a/lib/intel_ctx.h
> > > > +++ b/lib/intel_ctx.h
> > > > @@ -16,6 +16,7 @@
> > > >   * intel_ctx_cfg_t:
> > > >   * @flags: Context create flags
> > > >   * @vm: VM to inherit or 0 for using a per-context VM
> > > > + * @nopersist: set I915_CONTEXT_PARAM_PERSISTENCE to 0
> > > >   * @num_engines: Number of client-specified engines or 0 for legacy mode
> > > >   * @engines: Client-specified engines
> > > >   *
> > > > @@ -42,6 +43,7 @@
> > > >  typedef struct intel_ctx_cfg {
> > > >       uint32_t flags;
> > > >       uint32_t vm;
> > > > +     bool nopersist;
> > >
> > > To avoid the negative, wondering if we could have had a 'persist' field
> > > here rather than 'nopersist'? For regular contexts 'persist' seems
> > > fine. When this field is introduced we would default it to true but call
> > > the context setparam ioctl only if 'persist' were false.
> >
> > We don't have a good way to "default" things, at least not yet.  We
> > could have a #define INTEL_CTX_CFG_DEFAULT or something like that if
> > we wanted to have non-zero values be defaults, I suppose.  Then we
> > could default persist=true.
> >
> > > I do understand that contexts are persistent by default so 'nopersist' is
> > > really where something extra needs to be done. Is this why 'nopersist' was
> > > chosen here?
> >
> > Roughly, yes...

'nopersist' is fine for now, we can revisit if needed later.

> >
> > > Also, how are legacy contexts handled (since they really don't have a cfg)?
> > > Are they always persistent (or can they ever be non-persistent)? That would
> > > be another reason for having 'nopersist' I guess.
> >
> > I've tried to make it so that a legacy context (regular old
> > gem_context_create(fd) or intel_ctx_create(fd, NULL)) is equivalent to
> > using a zero-initialized context.  Legacy contexts are persistent by
> > default.

The question I had was can a legacy context ever be non-persistent? Is the
'nopersist' field valid for a legacy context and can it have a 'true' value?

> >
> > > > @@ -460,14 +467,16 @@ static void test_noheartbeat_many(int i915, int count, unsigned int flags)
> > > >               igt_assert(set_heartbeat(i915, e->full_name, 500));
> > > >
> > > >               for (int n = 0; n < ARRAY_SIZE(spin); n++) {
> > > > -                     uint32_t ctx;
> > > > +                     const intel_ctx_t *ctx;
> > > > +
> > > > +                     ctx = intel_ctx_create(i915, NULL);
> > > >
> > > > -                     ctx = gem_context_create(i915);
> > > > -                     spin[n] = igt_spin_new(i915, ctx, .engine = eb_ring(e),
> > > > +                     spin[n] = igt_spin_new(i915, .ctx = ctx,
> > > > +                                            .engine = eb_ring(e),
> > > >                                              .flags = (IGT_SPIN_FENCE_OUT |
> > > >                                                        IGT_SPIN_POLL_RUN |
> > > >                                                        flags));
> > > > -                     gem_context_destroy(i915, ctx);
> > > > +                     intel_ctx_destroy(i915, ctx);
> > >
> > > Any particular reason why we are creating legacy intel_ctx_t here (and also
> > > in test_noheartbeat_close())? There are other places in this file where we
> > > have not changed previous gem contexts created with gem_context_create() so
> > > just wondering.
> >
> > Most likely for IGT_SPIN_POLL_RUN.

Got it, thanks!

> >
> > --Jason
> >
> > > > @@ -772,13 +783,10 @@ static void test_process_mixed(int pfd, unsigned int engine)
> > > >
> > > >               for (int persists = 0; persists <= 1; persists++) {
> > > >                       igt_spin_t *spin;
> > > > -                     uint32_t ctx;
> > > > -
> > > > -                     ctx = gem_context_create(i915);
> > > > -                     gem_context_copy_engines(pfd, 0, i915, ctx);
> > > > -                     gem_context_set_persistence(i915, ctx, persists);
> > > > +                     const intel_ctx_t *ctx;
> > > >
> > > > -                     spin = igt_spin_new(i915, ctx,
> > > > +                     ctx = ctx_create_persistence(i915, cfg, persists);
> > > > +                     spin = igt_spin_new(i915, .ctx = ctx,
> > > >                                           .engine = engine,
> > > >                                           .flags = IGT_SPIN_FENCE_OUT);
> > >
> > > No intel_ctx_destroy() here, but neither gem_context_destroy() so that
> > > seems to be the design...