[igt-dev] [PATCH i-g-t v4] tests/i915/gem_exec_async: Update with engine discovery

[Ramalingam C]

On 2019-07-29 at 15:26:54 +0100, Tvrtko Ursulin wrote:
> 
> On 25/07/2019 09:45, 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]
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> > ---
> >   tests/i915/gem_exec_async.c | 52 +++++++++++++++++++++++++++----------
> >   1 file changed, 38 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
> > index 9a06af7e29cb..93cfc3d28541 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 (e2->flags == other)
> > +				continue;
> 
> Have you checked if we get I915_EXEC_DEFAULT and I915_EXEC_RENDER here, and
> what happens in that case, and what happens with I915_EXEC_BSD and
> I915_EXEC_BSD1/2?
Sorry I remember you mentioned to fix this in previous version. I guess
below implementation will help in this regard

bool gem_legacy_engine_is_same(unsigned eb_flag1, unsigned eb_flag2)
{
        if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
             (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
                return true;

        if (eb_flag1 & ~(3<<13) == eb_flag2 & ~(3<<13))
                return true;

        if (eb_flag1 == eb_flag2)
                return true;

        return false;
}

or in the above function we could change the eb_flag as below

if (eb_flag1 == I915_EXEC_DEFAULT)
	eb_flag1 = I915_EXEC_RENDER;

sounds good?

-Ram
> 
> Regards,
> 
> Tvrtko
> 
> > -		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);
> >   		}
> >   	}
> >