[Intel-gfx] [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery

[Ramalingam C]

On 2019-07-31 at 07:12:35 +0100, Tvrtko Ursulin wrote:
> 
> On 30/07/2019 18:07, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-30 16:20:08)
> > > 
> > > On 30/07/2019 09:04, Ramalingam C wrote:
> > > > On 2019-07-30 at 13:05:27 +0100, Tvrtko Ursulin wrote:
> > > > > 
> > > > > On 30/07/2019 04:58, Ramalingam C wrote:
> > > > > > +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)
> > > > > 
> > > > > I think we try to avoid implied int but not sure in this case whether to suggest unsigned int, long or uint64_t. If we are suggesting in the function name that any flags can be passed in perhaps it should be uint64_t and then we filter on the engine bits (flags.. &= I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that would be more robust for a generic helper.
> > > > > 
> > > > > And add a doc blurb for this helper since it is non-obvious why we went for different and not same. My thinking was the name different would be clearer to express kind of tri-state nature of this check. (Flags may be different, but engines are not guaranteed to be different.) Have I over-complicated it? Do we need to make it clearer by naming it gem_eb_flags_are_guaranteed_different_engines? :)
> > > > For me current shape looks good enough :) I will use the uint64_t for
> > > > parameter types.
> > > 
> > > Okay but please add some documentation for the helper (we've been very
> > > bad in this work in this respect so far) and also filter out non-engine
> > > selection bits from the flags before doing the checks.
> > > 
> > > > > 
> > > > > > +{
> > > > > > +   if (eb_flag1 == eb_flag2)
> > > > > > +           return false;
> > > > > > +
> > > > > > +   /* DEFAULT stands for RENDER in legacy uAPI*/
> > > > > > +   if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
> > > > > > +        (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
> > > > > > +           return false;
> > > > > > +
> > > > > > +   /*
> > > > > > +    * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
> > > > > > +    * same engine.
> > > > > > +    */
> > > > > > +   if ((eb_flag1 == I915_EXEC_BSD) &&
> > > > > > +       (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> > > > > > +                   return false;
> > > > > > +
> > > > > > +   if ((eb_flag2 == I915_EXEC_BSD) &&
> > > > > > +       (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> > > > > > +                   return false;
> > > > > 
> > > > > I think this works.
> > > > > 
> > > > > I've also come up with something to merge the two checks, not 100% it's correct or more readable:
> > > > > 
> > > > > if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at least one is BSD
> > > > >       !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
> > > > >       (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
> > > > >       return false;
> > > > > 
> > > > > Would need feeding in some values and checking it works as expected. Probably not worth it since I doubt it is more readable.
> > > > readability perspective, we could stick to the original version. If we
> > > > want to go ahead we need to do below ops:
> > > 
> > > Stick with your version I think.
> > > 
> > > Chris is being quiet BTW. Either we are below his radar and he'll scream
> > > later, or we managed to approach something he finds passable. ;)
> > 
> > Just waiting until I have to use it in anger :)
> > 
> > Gut feeling is that I won't have to use it, in that if I have two
> > different timelines pointing to the same physical engine, do I really
> > care? The situations where I would have distinct engine mappings strike
> > me as being cases where I'm testing timelines; otherwise I would create
> > contexts with the same ctx->engines[] map.
> 
> I do dislike this complication of testing uapi flags but figuring out the
> physical engines under the covers. Do you think it would be clearer do drop
> this helper and instead use two contexts in this test? It would make it
> dependent on contexts though..
You mean different context for internal iteration of the engines?
Is it possible for all usecases? What if a test want to test a operation
in the same context on all other engines than a engine passed in?
Need to confirm if there is any such need though.

-Ram
> 
> Maybe we should altogether re-visit my plan to eliminate the legacy
> for_each_physical_engine iterator. I was hoping we don't have two. So wanted
> to make it explicit where we are testing the old eb ABI, and where we are
> testing new. However reality seems more complicated.. Would it instead work
> better to rename the old iterator to something like
> for_each_legacy_physical_engine?
> 
> Regards,
> 
> Tvrtko