Re: [igt-dev] [PATCH i-g-t v2 4/5] test/i915: gem_exec_params: use the gem_engine_topology library

Ramalingam C
2019-05-30T16:10:44Z

On 2019-05-30 at 16:49:45 +0100, Chris Wilson wrote:
> Quoting Ramalingam C (2019-05-30 16:42:21)
> > Replace the legacy for_each_engine* defines with the ones implemented
> > in the gem_engine_topology library.
> >
> > v2:
> > has_ring() check is removed [tvertko]
> > Engine type is identified through CLASS [tvrtko]
> >
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > ---
> > tests/i915/gem_exec_params.c | 41 ++++++------------------------------
> > 1 file changed, 7 insertions(+), 34 deletions(-)
> >
> > diff --git a/tests/i915/gem_exec_params.c b/tests/i915/gem_exec_params.c
> > index 49c56a8d75e9..3bc5b0a9d531 100644
> > --- a/tests/i915/gem_exec_params.c
> > +++ b/tests/i915/gem_exec_params.c
> > @@ -52,30 +52,6 @@
> > #define LOCAL_I915_EXEC_BATCH_FIRST (1 << 18)
> > #define LOCAL_I915_EXEC_FENCE_ARRAY (1 << 19)
> >
> > -static bool has_ring(int fd, unsigned ring_exec_flags)
> > -{
> > - switch (ring_exec_flags & I915_EXEC_RING_MASK) {
> > - case 0:
> > - case I915_EXEC_RENDER:
> > - return true;
> > -
> > - case I915_EXEC_BSD:
> > - if (ring_exec_flags & LOCAL_I915_EXEC_BSD_MASK)
> > - return gem_has_bsd2(fd);
> > - else
> > - return gem_has_bsd(fd);
> > -
> > - case I915_EXEC_BLT:
> > - return gem_has_blt(fd);
> > -
> > - case I915_EXEC_VEBOX:
> > - return gem_has_vebox(fd);
> > - }
> > -
> > - igt_assert_f(0, "invalid exec flag 0x%x\n", ring_exec_flags);
> > - return false;
> > -}
> > -
> > static bool has_exec_batch_first(int fd)
> > {
> > int val = -1;
> > @@ -201,7 +177,7 @@ int fd;
> >
> > igt_main
> > {
> > - const struct intel_execution_engine *e;
> > + const struct intel_execution_engine2 *e;
> >
> > igt_fixture {
> > fd = drm_open_driver(DRIVER_INTEL);
> > @@ -235,11 +211,9 @@ igt_main
> > }
> >
> > igt_subtest("control") {
> > - for (e = intel_execution_engines; e->name; e++) {
> > - if (has_ring(fd, e->exec_id | e->flags)) {
> > - execbuf.flags = e->exec_id | e->flags;
> > - gem_execbuf(fd, &execbuf);
> > - }
> > + __for_each_physical_engine(fd, e) {
> > + execbuf.flags = e->flags;
> > + gem_execbuf(fd, &execbuf);
>
> This is silly. Previously it was cross-checking the GETPARAM against the
> EXECBUFFER uABI. Now it is asking the same question twice.
>
> This is the example that should not be using physical engines as it
> exercising ABI. Please be careful.

Still I am not able to understand this comment. If you dont mind could
you please elaborate?

As per my understanding, we iterate on physical engines and execute the
execbuf on those engines.

-Ram.
> -Chris