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

[Ramalingam C]

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 at 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