[Intel-gfx] [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Jul 30 15:20:08 UTC 2019
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:
>>> Legacy execbuf abi tests are prefixed with legacy. New test are added to
>>> run on physical engines accessed through engine discovery.
>>>
>>> So legacy tests run on the unconfigured (with engine map) context and
>>> use a new helper gem_eb_flags_to_engine to look up the engine from the
>>> intel_execution_engines2 static list. This is only to enable the core
>>> test code to be shared.
>>>
>>> Places where new contexts are created had to be updated to either
>>> equally configure the contexts or not.
>>>
>>> v2:
>>> retained the test as it is for legacy uapi testing and duplciated for
>>> new engine discovery [Tvrtko]
>>> v3:
>>> Few nits addressed [Tvrtko]
>>> v4:
>>> In legacy uAPI test path, iterate through for_each_engine [Tvrtko]
>>> v5:
>>> Function for exec_flag comparison [Tvrtko]
>>>
>>> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
>>> ---
>>> lib/igt_gt.c | 25 ++++++++++++++++++
>>> lib/igt_gt.h | 1 +
>>> tests/i915/gem_exec_async.c | 52 +++++++++++++++++++++++++++----------
>>> 3 files changed, 64 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
>>> index 78e3cd08925b..f594a46282bc 100644
>>> --- a/lib/igt_gt.c
>>> +++ b/lib/igt_gt.c
>>> @@ -633,3 +633,28 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
>>>
>>> return gem_has_ring(fd, ring);
>>> }
>>> +
>>> +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. ;)
Regards,
Tvrtko
>
> if (((flag1 | flag2) & I915_EXEC_BSD) && //Atleast one is BSD
> !((flag1 ^ flag2) & I915_EXEC_BSD) && //Both are BSD
> ((flag1 | flag2) & I915_EXEC_RING_MASK) != I915_EXEC_RING_MASK) //Not BSD0 and BSD1
> return false;
>>
>>> +
>>> + return true;
>>> +}
>>> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
>>> index 73b5002a04bd..760b8baefc48 100644
>>> --- a/lib/igt_gt.h
>>> +++ b/lib/igt_gt.h
>>> @@ -101,5 +101,6 @@ extern const struct intel_execution_engine2 {
>>> } intel_execution_engines2[];
>>>
>>> int gem_execbuf_flags_to_engine_class(unsigned int flags);
>>> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2);
>>>
>>> #endif /* IGT_GT_H */
>>> diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
>>> index 9a06af7e29cb..5fdce5ac5b77 100644
>>> --- a/tests/i915/gem_exec_async.c
>>> +++ b/tests/i915/gem_exec_async.c
>>> @@ -80,9 +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, const struct intel_execution_engine2 *e2, bool legacy)
>>> {
>>> const int gen = intel_gen(intel_get_drm_devid(fd));
>>> + const struct intel_execution_engine2 *other2;
>>> struct drm_i915_gem_exec_object2 obj[2];
>>> #define SCRATCH 0
>>> #define BATCH 1
>>> @@ -138,20 +139,33 @@ 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 = e2->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)
>>> - continue;
>>> + if (legacy) {
>>> + for_each_engine(fd, other) {
>>> + if (gem_eb_flags_are_different_engines(e2->flags, other))
>>
>> if (!different)
> Will fix it. Thanks.
>
> -Ram
>>
>> Regards,
>>
>> Tvrtko
>>
>>> + continue;
>>>
>>> - if (!gem_can_store_dword(fd, other))
>>> - continue;
>>> + if (!gem_can_store_dword(fd, other))
>>> + continue;
>>> +
>>> + store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
>>> + i++;
>>> + }
>>> + } else {
>>> + __for_each_physical_engine(fd, other2) {
>>> + if (gem_engine_is_equal(e2, other2))
>>> + continue;
>>>
>>> - store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
>>> - i++;
>>> + if (!gem_class_can_store_dword(fd, other2->class))
>>> + continue;
>>> +
>>> + store_dword(fd, other2->flags, obj[SCRATCH].handle, 4*i, i);
>>> + i++;
>>> + }
>>> }
>>>
>>> *batch = MI_BATCH_BUFFER_END;
>>> @@ -185,7 +199,9 @@ static bool has_async_execbuf(int fd)
>>>
>>> igt_main
>>> {
>>> + const struct intel_execution_engine2 *e2;
>>> const struct intel_execution_engine *e;
>>> + struct intel_execution_engine2 e2__;
>>> int fd = -1;
>>>
>>> igt_skip_on_simulation();
>>> @@ -200,14 +216,22 @@ igt_main
>>> }
>>>
>>> for (e = intel_execution_engines; e->name; e++) {
>>> - /* default exec-id is purely symbolic */
>>> - if (e->exec_id == 0)
>>> + e2__ = gem_eb_flags_to_engine(e->exec_id | e->flags);
>>> + if (e2__.flags == -1)
>>> continue;
>>> + e2 = &e2__;
>>>
>>> - igt_subtest_f("concurrent-writes-%s", e->name) {
>>> + igt_subtest_f("legacy-concurrent-writes-%s", e2->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);
>>> + igt_require(gem_class_can_store_dword(fd, e2->class));
>>> + one(fd, e2, true);
>>> + }
>>> + }
>>> +
>>> + __for_each_physical_engine(fd, e2) {
>>> + igt_subtest_f("concurrent-writes-%s", e2->name) {
>>> + igt_require(gem_class_can_store_dword(fd, e2->class));
>>> + one(fd, e2, false);
>>> }
>>> }
>>>
>>>
>
More information about the Intel-gfx
mailing list