[igt-dev] [PATCH i-g-t 54/77] lib/i915: Use for_each_physical_ring for submission tests

[Jason Ekstrand]

On Tue, Jun 15, 2021 at 11:56 PM Zbigniew Kempczyński
<zbigniew.kempczynski at intel.com> wrote:
>
> On Tue, Jun 15, 2021 at 01:23:19PM -0500, Jason Ekstrand wrote:
> > On Tue, Jun 15, 2021 at 3:44 AM Zbigniew Kempczyński
> > <zbigniew.kempczynski at intel.com> wrote:
> > >
> > > On Mon, Jun 14, 2021 at 11:38:39AM -0500, Jason Ekstrand wrote:
> > > > This does make the assumption that ctx0 has the default set of engines
> > > > but, now that we've converted everything to intel_ctx_t, this assumption
> > > > should be ok.
> > > >
> > > > Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> > > > ---
> > > >  lib/i915/gem_submission.c | 13 +++++--------
> > > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
> > > > index bd4bbb3ef..1abc73ec7 100644
> > > > --- a/lib/i915/gem_submission.c
> > > > +++ b/lib/i915/gem_submission.c
> > > > @@ -33,6 +33,7 @@
> > > >  #include "i915/gem.h"
> > > >  #include "i915/gem_create.h"
> > > >  #include "i915/gem_engine_topology.h"
> > > > +#include "i915/gem_ring.h"
> > > >  #include "i915/gem_submission.h"
> > > >
> > > >  #include "igt_core.h"
> > > > @@ -200,10 +201,8 @@ void gem_test_engine(int i915, unsigned int engine)
> > > >       gem_write(i915, obj.handle, 0, &bbe, sizeof(bbe));
> > > >
> > > >       if (engine == ALL_ENGINES) {
> > > > -             const struct intel_execution_engine2 *e2;
> > > > -
> > > > -             __for_each_physical_engine(i915, e2) {
> > > > -                     execbuf.flags = e2->flags;
> > > > +             for_each_physical_ring(e, i915) {
> > > > +                     execbuf.flags = eb_ring(e);
> > > >                       gem_execbuf(i915, &execbuf);
> > >
> > > Why not to use for_each_ctx_engine() with intel intel_ctx_0 here?
> >
> > We could.  I do kind-of like the fact that it's obvious and clear that
> > it's testing physical rings instead of going through the engines API.
> > It does say explicitly in the docs that it takes an I915_EXEC_RING
> > enum.  But I'd be happy to change if you think it'd be more clear that
> > way.
>
> Both interfaces returns same engine set with the respect of the order
> what shouldn't be the problem at all. SAs we started to be more familiar
> with your code for_each_ctx_engine(fd, &intel_ctx_0(), e) looks good
> for me and I know what to expect on that context. Please change to that
> - diff will be similar to others where you'd replaced
> __for_each_physical_engine to for_each_ctx_engine().

Ok, you've convinced me.  I typed it up and I actually really like it.
I've sent a v2.

--Jason