Re: [igt-dev] [PATCH v19 5/6] lib: igt_dummyload: use for_each_context_engine()

From: Andi Shyti <andi.shyti@intel.com>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: IGT dev <igt-dev@lists.freedesktop.org>, Andi Shyti <andi@etezian.org>
Subject: Re: [igt-dev] [PATCH v19 5/6] lib: igt_dummyload: use for_each_context_engine()
Date: Mon, 8 Apr 2019 19:45:48 +0300	[thread overview]
Message-ID: <20190408164548.GA2556@intel.intel> (raw)
In-Reply-To: <5bec9ae6-aaf4-4c65-b300-c51985b889b0@linux.intel.com>

Hi Tvrtko,

> > -		engines[nengine++] = opts->engine;
> > +		struct intel_execution_engine2 *e;
> > +		int class;
> > +
> > +		if (!gem_ctx_get_engine(fd, opts->engine, opts->ctx, e)) {
> > +			class = e->class;
> > +		} else {
> > +			gem_require_ring(fd, opts->engine);
> > +			class = gem_eb_to_class(opts->engine);
> > +		}
> > +
> > +		if (opts->flags & IGT_SPIN_POLL_RUN)
> > +			igt_require(gem_class_can_store_dword(fd, class));
> > +
> > +		flags[nengine++] = opts->engine;
> >   	}
> 
> Patch looks good but I'm afraid I made an oversight earlier in review when I
> asked you to move this else block in this function. I forgot that the
> differentiation between igt_spin_batch_new and __igt_spin_batch_new was that
> the latter should not do any execbuf calls apart from the spinner itself.
> Since gem_require_ring does one it could be a problem.
> 
> Would you mind moving just this else block back into the main constructor?

No problem, I'll move it back.

> With that you can have:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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