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

[Chris Wilson]

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.
-Chris