[igt-dev] [PATCH i-g-t] tests/i915/gem_exec_async: Update with engine discovery

[Ramalingam C]

On 2019-07-24 at 11:28:19 +0100, Chris Wilson wrote:
> Quoting Ramalingam C (2019-07-24 03:21:18)
> > Legacy execbuf abi tests are prefixed with legacy. New test are added to
> > run on physical engines accessed through engine discovery.
> > 
> > So legacy tests run on the unconfigured (with engine map) context and
> > use a new helper gem_eb_flags_to_engine to look up the engine from the
> > intel_execution_engines2 static list. This is only to enable the core
> > test code to be shared.
> > 
> > Places where new contexts are created had to be updated to either
> > equally configure the contexts or not.
> > 
> > v2:
> >   retained the test as it is for legacy uapi testing and duplciated for
> >         new engine discovery [Tvrtko]
> > v3:
> >   Few nits addressed [Tvrtko]
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> > ---
> >  tests/i915/gem_exec_async.c | 55 +++++++++++++++++++++++++++----------
> >  1 file changed, 41 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
> > index 9a06af7e29cb..de3652da452a 100644
> > --- a/tests/i915/gem_exec_async.c
> > +++ b/tests/i915/gem_exec_async.c
> > @@ -80,9 +80,10 @@ static void store_dword(int fd, unsigned ring,
> >         gem_close(fd, obj[1].handle);
> >  }
> >  
> > -static void one(int fd, unsigned ring, uint32_t flags)
> > +static void one(int fd, const struct intel_execution_engine2 *e2, bool legacy)
> >  {
> >         const int gen = intel_gen(intel_get_drm_devid(fd));
> > +       const struct intel_execution_engine2 *other2;
> >         struct drm_i915_gem_exec_object2 obj[2];
> >  #define SCRATCH 0
> >  #define BATCH 1
> > @@ -138,20 +139,36 @@ static void one(int fd, unsigned ring, uint32_t flags)
> >         memset(&execbuf, 0, sizeof(execbuf));
> >         execbuf.buffers_ptr = to_user_pointer(obj);
> >         execbuf.buffer_count = 2;
> > -       execbuf.flags = ring | flags;
> > +       execbuf.flags = e2->flags;
> >         igt_require(__gem_execbuf(fd, &execbuf) == 0);
> >         gem_close(fd, obj[BATCH].handle);
> >  
> >         i = 0;
> > -       for_each_physical_engine(fd, other) {
> > -               if (other == ring)
> > -                       continue;
> > +       if (legacy) {
> > +               for_each_engine(fd, other) {
> > +                       if (!gem_ring_has_physical_engine(fd, other))
> > +                               continue;
> >  
> > -               if (!gem_can_store_dword(fd, other))
> > -                       continue;
> > +                       if (e2->flags == other)
> > +                               continue;
> > +
> > +                       if (!gem_can_store_dword(fd, other))
> > +                               continue;
> > +
> > +                       store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
> > +                       i++;
> > +               }
> > +       } else {
> > +               __for_each_physical_engine(fd, other2) {
> > +                       if (gem_engine_is_equal(e2, other2))
> > +                               continue;
> >  
> > -               store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
> > -               i++;
> > +                       if (!gem_class_can_store_dword(fd, other2->class))
> > +                               continue;
> > +
> > +                       store_dword(fd, other2->flags, obj[SCRATCH].handle, 4*i, i);
> > +                       i++;
> > +               }
> >         }
> 
> Does this look neater if we pass in the array of engines and just skip
> if others[] == this_engine. The array can be constructed once during
> each subgroup fixture, where legacy/non-legacy mode is know. That array
> generation is also fairly common so perhaps having a
> gem_engine_topology.c function return the set of physical engines with
> ALL_ENGINES_STORE_DWORD | ALL_ENGINES_LEGACY? Bonus points for a reverse
> lookup to string.
Chris,

Sure We could do this. Even after this we might need an explicit call for
engine list for different context than default(0). But it is needed in
special cases.

BR,
-Ram.
> -Chris