[igt-dev] [Intel-gfx] [PATCH i-g-t] i915: Improve static engine map for legacy
Chris Wilson
chris at chris-wilson.co.uk
Thu May 23 11:23:01 UTC 2019
Quoting Tvrtko Ursulin (2019-05-23 10:48:27)
>
> On 23/05/2019 09:06, Chris Wilson wrote:
> > We need to keep igt working on linus and dif, or Joonas gets very upset.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Cc: Andi Shyti <andi.shyti at intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > ---
> > lib/i915/gem_context.c | 2 +-
> > lib/i915/gem_engine_topology.c | 15 +++-------
> > lib/igt_gt.c | 54 ++++------------------------------
> > lib/igt_gt.h | 17 -----------
> > tests/i915/gem_ctx_isolation.c | 21 +++++--------
> > tools/intel_reg.c | 4 +--
> > 6 files changed, 19 insertions(+), 94 deletions(-)
> >
> > diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c
> > index f94d89cb4..07ab78174 100644
> > --- a/lib/i915/gem_context.c
> > +++ b/lib/i915/gem_context.c
> > @@ -290,7 +290,7 @@ bool gem_context_has_engine(int fd, uint32_t ctx, uint64_t engine)
> > * wouldn't produce any result.
> > */
> > if ((engine & ~(3<<13)) == I915_EXEC_BSD) {
> > - if (engine & (3 << 13) && !gem_has_bsd2(fd))
> > + if (engine & (2 << 13) && !gem_has_bsd2(fd))
> > return false;
> > }
> >
> > diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> > index d0c8bd5aa..fdd1b9516 100644
> > --- a/lib/i915/gem_engine_topology.c
> > +++ b/lib/i915/gem_engine_topology.c
> > @@ -223,22 +223,15 @@ struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id)
> > struct intel_execution_engine2 *__e2 =
> > &engine_data.engines[engine_data.nengines];
> >
> > - if (!igt_only_list_subtests()) {
> > - __e2->flags = gem_class_instance_to_eb_flags(fd,
> > - e2->class, e2->instance);
> > -
> > - if (!gem_has_ring(fd, __e2->flags))
> > - continue;
> > - } else {
> > - __e2->flags = -1; /* 0xfff... */
> > - }
> > -
> > __e2->name = e2->name;
> > __e2->instance = e2->instance;
> > __e2->class = e2->class;
> > + __e2->flags = e2->flags;
> > __e2->is_virtual = false;
> >
> > - engine_data.nengines++;
> > + if (igt_only_list_subtests() ||
> > + gem_has_ring(fd, e2->flags))
> > + engine_data.nengines++;
> > }
> > return engine_data;
> > }
> > diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> > index 6b7c037e6..78e3cd089 100644
> > --- a/lib/igt_gt.c
> > +++ b/lib/igt_gt.c
> > @@ -585,12 +585,12 @@ bool gem_can_store_dword(int fd, unsigned int engine)
> > }
> >
> > const struct intel_execution_engine2 intel_execution_engines2[] = {
> > - { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> > - { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> > - { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> > - { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> > - { "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
> > - { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> > + { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
> > + { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
> > + { "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },
> > + { "vcs1", I915_ENGINE_CLASS_VIDEO, 1, I915_EXEC_BSD | I915_EXEC_BSD_RING2 },
> > + { "vcs2", I915_ENGINE_CLASS_VIDEO, 2, -1 },
> > + { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0, I915_EXEC_VEBOX },
> > { }
> > };
> >
> > @@ -611,48 +611,6 @@ int gem_execbuf_flags_to_engine_class(unsigned int flags)
> > }
> > }
> >
> > -unsigned int
> > -gem_class_instance_to_eb_flags(int gem_fd,
> > - enum drm_i915_gem_engine_class class,
> > - unsigned int instance)
> > -{
> > - if (class != I915_ENGINE_CLASS_VIDEO)
> > - igt_assert(instance == 0);
> > - else
> > - igt_assert(instance >= 0 && instance <= 1);
> > -
> > - switch (class) {
> > - case I915_ENGINE_CLASS_RENDER:
> > - return I915_EXEC_RENDER;
> > - case I915_ENGINE_CLASS_COPY:
> > - return I915_EXEC_BLT;
> > - case I915_ENGINE_CLASS_VIDEO:
> > - if (instance == 0) {
> > - if (gem_has_bsd2(gem_fd))
> > - return I915_EXEC_BSD | I915_EXEC_BSD_RING1;
> > - else
> > - return I915_EXEC_BSD;
> > -
> > - } else {
> > - return I915_EXEC_BSD | I915_EXEC_BSD_RING2;
> > - }
> > - case I915_ENGINE_CLASS_VIDEO_ENHANCE:
> > - return I915_EXEC_VEBOX;
> > - case I915_ENGINE_CLASS_INVALID:
> > - default:
> > - igt_assert(0);
> > - };
> > -}
> > -
> > -bool gem_has_engine(int gem_fd,
> > - enum drm_i915_gem_engine_class class,
> > - unsigned int instance)
> > -{
> > - return gem_has_ring(gem_fd,
> > - gem_class_instance_to_eb_flags(gem_fd, class,
> > - instance));
> > -}
> > -
> > bool gem_ring_is_physical_engine(int fd, unsigned ring)
> > {
> > if (ring == I915_EXEC_DEFAULT)
> > diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> > index 77318e2a8..73b5002a0 100644
> > --- a/lib/igt_gt.h
> > +++ b/lib/igt_gt.h
> > @@ -102,21 +102,4 @@ extern const struct intel_execution_engine2 {
> >
> > int gem_execbuf_flags_to_engine_class(unsigned int flags);
> >
> > -unsigned int
> > -gem_class_instance_to_eb_flags(int gem_fd,
> > - enum drm_i915_gem_engine_class class,
> > - unsigned int instance);
> > -
> > -bool gem_has_engine(int gem_fd,
> > - enum drm_i915_gem_engine_class class,
> > - unsigned int instance);
> > -
> > -static inline
> > -void gem_require_engine(int gem_fd,
> > - enum drm_i915_gem_engine_class class,
> > - unsigned int instance)
> > -{
> > - igt_require(gem_has_engine(gem_fd, class, instance));
> > -}
> > -
> > #endif /* IGT_GT_H */
> > diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> > index bcd0f4812..5b054c81d 100644
> > --- a/tests/i915/gem_ctx_isolation.c
> > +++ b/tests/i915/gem_ctx_isolation.c
> > @@ -321,8 +321,7 @@ static uint32_t read_regs(int fd,
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 2;
> > - execbuf.flags =
> > - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> > + execbuf.flags = e->flags;
> > execbuf.rsvd1 = ctx;
> > gem_execbuf(fd, &execbuf);
> > gem_close(fd, obj[1].handle);
> > @@ -377,8 +376,7 @@ static void write_regs(int fd,
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffers_ptr = to_user_pointer(&obj);
> > execbuf.buffer_count = 1;
> > - execbuf.flags =
> > - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> > + execbuf.flags = e->flags;
> > execbuf.rsvd1 = ctx;
> > gem_execbuf(fd, &execbuf);
> > gem_close(fd, obj.handle);
> > @@ -448,8 +446,7 @@ static void restore_regs(int fd,
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 2;
> > - execbuf.flags =
> > - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> > + execbuf.flags = e->flags;
> > execbuf.rsvd1 = ctx;
> > gem_execbuf(fd, &execbuf);
> > gem_close(fd, obj[1].handle);
> > @@ -559,8 +556,7 @@ static void nonpriv(int fd,
> > 0x0505c0c0,
> > 0xdeadbeef
> > };
> > - unsigned int engine =
> > - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> > + unsigned int engine = e->flags;
> > unsigned int num_values = ARRAY_SIZE(values);
> >
> > /* Sigh -- hsw: we need cmdparser access to our own registers! */
> > @@ -616,9 +612,7 @@ static void isolation(int fd,
> > 0xaaaaaaaa,
> > 0xdeadbeef
> > };
> > - unsigned int engine = gem_class_instance_to_eb_flags(fd,
> > - e->class,
> > - e->instance);
> > + unsigned int engine = e->flags;
> > unsigned int num_values =
> > flags & (DIRTY1 | DIRTY2) ? ARRAY_SIZE(values) : 1;
> >
> > @@ -729,8 +723,7 @@ static void preservation(int fd,
> > 0xdeadbeef
> > };
> > const unsigned int num_values = ARRAY_SIZE(values);
> > - unsigned int engine =
> > - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> > + unsigned int engine = e->flags;
> > uint32_t ctx[num_values +1 ];
> > uint32_t regs[num_values + 1][2];
> > igt_spin_t *spin;
> > @@ -840,7 +833,7 @@ igt_main
> > igt_subtest_group {
> > igt_fixture {
> > igt_require(has_context_isolation & (1 << e->class));
> > - gem_require_engine(fd, e->class, e->instance);
> > + gem_require_ring(fd, e->flags);
> > igt_fork_hang_detector(fd);
> > }
> >
> > diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> > index 1247b70b0..e517956b8 100644
> > --- a/tools/intel_reg.c
> > +++ b/tools/intel_reg.c
> > @@ -329,9 +329,7 @@ static int register_srm(struct config *config, struct reg *reg,
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 2;
> > - execbuf.flags = gem_class_instance_to_eb_flags(fd,
> > - engine->class,
> > - engine->instance);
> > + execbuf.flags = engine->flags;
> > if (secure)
> > execbuf.flags |= I915_EXEC_SECURE;
> >
> >
>
> It still puzzles me at which point was I915_EXEC_BSD_RING1 not allowed
> on single vcs parts. Was it just a fail in IGT?!? The first hunk in this
> patch. Digging in history points to that..
I do recall making a decision to hide vcs1 if !vcs2 so that we didn't
duplicate (vcs, vcs1), and still have the plain randomly chosen vcs.
That is likely the additional source of confusion.
-Chris
More information about the igt-dev
mailing list