Quoting Tvrtko Ursulin (2019-07-31 07:12:35) > > 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.. Going back to look at the use case... tests/i915/gem_exec_async.c | 81 ++++++++++++++++++++++++++++--------- 1 file changed, 63 insertions(+), 18 deletions(-) diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c index 9a06af7e2..fafca98ad 100644 --- a/tests/i915/gem_exec_async.c +++ b/tests/i915/gem_exec_async.c @@ -80,7 +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, + unsigned int idx, + const struct intel_execution_engine2 *engines, + unsigned int num_engines) { const int gen = intel_gen(intel_get_drm_devid(fd)); struct drm_i915_gem_exec_object2 obj[2]; @@ -88,7 +91,6 @@ static void one(int fd, unsigned ring, uint32_t flags) #define BATCH 1 struct drm_i915_gem_relocation_entry reloc; struct drm_i915_gem_execbuffer2 execbuf; - unsigned int other; uint32_t *batch; int i; @@ -138,19 +140,17 @@ 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 = engines[idx].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) + for (unsigned int other = 0; other < num_engines; other++) { + if (other == idx) continue; - if (!gem_can_store_dword(fd, other)) - continue; - - store_dword(fd, other, obj[SCRATCH].handle, 4*i, i); + store_dword(fd, engines[other].flags, + obj[SCRATCH].handle, 4*i, i); i++; } @@ -185,7 +185,6 @@ static bool has_async_execbuf(int fd) igt_main { - const struct intel_execution_engine *e; int fd = -1; igt_skip_on_simulation(); @@ -199,15 +198,61 @@ igt_main igt_fork_hang_detector(fd); } - for (e = intel_execution_engines; e->name; e++) { - /* default exec-id is purely symbolic */ - if (e->exec_id == 0) - continue; + /* First up, check the legacy engine selector ABI for independence */ + igt_subtest_group { + struct intel_execution_engine2 engines[64]; + unsigned int num_engines = 0; + + igt_fixture { + const struct intel_execution_engine *e; + + for (e = intel_execution_engines; e->name; e++) { + if (!gem_ring_has_physical_engine(fd, e->exec_id | e->flags)) + continue; + + /* Must be unique, no unknowable BSD aliases! */ + engines[num_engines] = + gem_eb_flags_to_engine(e->exec_id | e->flags); + if (engines[num_engines].flags != -1) + continue; + + if (!gem_can_store_dword(fd, engines[num_engines].flags)) + continue; + + num_engines++; + if (num_engines == ARRAY_SIZE(engines)) + break; + } + } + + for (unsigned int n = 0; n < num_engines; n++) { + igt_subtest_f("legacy-concurrent-writes-%s", + engines[n].name) + one(fd, n, engines, num_engines); + } + } + + /* Same again, but using a ctx->engine[] map and indices */ + igt_subtest_group { + struct intel_execution_engine2 engines[64]; + unsigned int num_engines = 0; + + igt_fixture { + const struct intel_execution_engine2 *e; + + __for_each_physical_engine(fd, e) { + if (!gem_class_can_store_dword(fd, e->class)) + continue; + + engines[num_engines++] = *e; + if (num_engines == ARRAY_SIZE(engines)) + break; + } + } - igt_subtest_f("concurrent-writes-%s", e->name) { - igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags)); - igt_require(gem_can_store_dword(fd, e->exec_id | e->flags)); - one(fd, e->exec_id, e->flags); + for (unsigned int n = 0; n < num_engines; n++) { + igt_subtest_f("concurrent-writes-%s", engines[n].name) + one(fd, n, engines, num_engines); } } (Fill in the gaps for the new structs!) Is more of what I was expecting. (Eventually no one will notice the BSD aliasing bit rotting and we can remove it from the uABI.) -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx