Re: [igt-dev] [PATCH v16 8/8] test: perf_pmu: use the gem_engine_topology library

Andi Shyti
2019-03-29T12:47:29Z

> > -static igt_spin_t * __spin_poll(int fd, uint32_t ctx, unsigned long flags)
> > +static igt_spin_t * __spin_poll(int fd, uint32_t ctx, const struct intel_execution_engine2 *e)
> > {
> > struct igt_spin_factory opts = {
> > .ctx = ctx,
> > - .engine = flags,
> > + .engine = e->flags,
> > };
> > - if (gem_can_store_dword(fd, flags))
> > + if (gem_engine_can_store_dword(fd, e))
> > opts.flags |= IGT_SPIN_POLL_RUN;
> > return __igt_spin_batch_factory(fd, &opts);
> > @@ -211,7 +211,16 @@ static unsigned long __spin_wait(int fd, igt_spin_t *spin)
> > static igt_spin_t * __spin_sync(int fd, uint32_t ctx, unsigned long flags)
>
> __spin_sync should also take engine instead of flags, like __spin_poll.

But if __spin_sync is called inside __for_each_physical_engine
(which in this code never happens), engine doesn't have any flags
set. right?

> > {
> > - igt_spin_t *spin = __spin_poll(fd, ctx, flags);
> > + struct igt_spin_factory opts = {
> > + .ctx = ctx,
> > + .engine = flags,
> > + };
> > + igt_spin_t *spin;
> > +
> > + if (gem_can_store_dword(fd, flags))
> > + opts.flags |= IGT_SPIN_POLL_RUN;
> > +
> > + spin = __igt_spin_batch_factory(fd, &opts);
>
> And then you don't need this hunk at all, making the whole thing completely
> ready for more engines.
>
> And e2ring will end up with no callers so can be removed.

Same reason as above, which is this one:

> > - __for_each_engine_class_instance(e) {
> > + __for_each_static_engine(e) {
> > igt_subtest_group {
> > igt_fixture {
> > gem_require_engine(render_fd,
> >
>
> Yep, modulo __spin_sync change, this is what I had in mind.

at this point I don't have any flag set, any check whether the
engine really exists, no context mapping (still flags) :/

Am I missing it?

Andi