Re: [igt-dev] [PATCH i-g-t v2 2/5] test/i915: gem_exec_async: use the gem_engine_topology library

Andi Shyti
2019-06-06T14:09:02Z

Hi Ram,

On Thu, May 30, 2019 at 09:12:19PM +0530, Ramalingam C wrote:
> Replace the legacy for_each_engine* defines with the ones
> implemented in the gem_engine_topology library.
>
> v2:
> class and instance are compared for engine identification [Andi]
>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>

this looks good to me,

Reviewed-by: Andi Shyti <andi.shyti@intel.com>

Thanks,
Andi

> ---
> tests/i915/gem_exec_async.c | 34 +++++++++++++++-------------------
> 1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
> index 9a06af7e29cb..0da97f1f0506 100644
> --- a/tests/i915/gem_exec_async.c
> +++ b/tests/i915/gem_exec_async.c
> @@ -28,7 +28,7 @@
>
> IGT_TEST_DESCRIPTION("Check that we can issue concurrent writes across the engines.");
>
> -static void store_dword(int fd, unsigned ring,
> +static void store_dword(int fd, const struct intel_execution_engine2 *e,
> uint32_t target, uint32_t offset, uint32_t value)
> {
> const int gen = intel_gen(intel_get_drm_devid(fd));
> @@ -41,7 +41,7 @@ static void store_dword(int fd, unsigned ring,
> memset(&execbuf, 0, sizeof(execbuf));
> execbuf.buffers_ptr = to_user_pointer(obj);
> execbuf.buffer_count = 2;
> - execbuf.flags = ring;
> + execbuf.flags = e->flags;
> if (gen < 6)
> execbuf.flags |= I915_EXEC_SECURE;
>
> @@ -80,15 +80,15 @@ 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 *e)
> {
> const int gen = intel_gen(intel_get_drm_devid(fd));
> struct drm_i915_gem_exec_object2 obj[2];
> + struct intel_execution_engine2 *engine;
> #define SCRATCH 0
> #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 +138,21 @@ 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 = e->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_each_physical_engine(fd, engine) {
> + if (engine->class == e->class &&
> + engine->instance == e->instance)
> continue;
>
> - if (!gem_can_store_dword(fd, other))
> + if (!gem_class_can_store_dword(fd, engine->class))
> continue;
>
> - store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
> + store_dword(fd, engine, obj[SCRATCH].handle, 4*i, i);
> i++;
> }
>
> @@ -185,7 +187,7 @@ static bool has_async_execbuf(int fd)
>
> igt_main
> {
> - const struct intel_execution_engine *e;
> + const struct intel_execution_engine2 *e;
> int fd = -1;
>
> igt_skip_on_simulation();
> @@ -199,17 +201,11 @@ 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;
> -
> + __for_each_physical_engine(fd, e)
> 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);
> + igt_require(gem_class_can_store_dword(fd, e->class));
> + one(fd, e);
> }
> - }
>
> igt_fixture {
> igt_stop_hang_detector();
> --
> 2.19.1