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..
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
