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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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.

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.

So appears the spinner interface will always need a intel_ctx_t (meaning
more code changes).
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev