Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] i915: Improve static engine map for legacy

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 23/05/2019 09:06, Chris Wilson wrote:
> We need to keep igt working on linus and dif, or Joonas gets very upset.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   lib/i915/gem_context.c         |  2 +-
>   lib/i915/gem_engine_topology.c | 15 +++-------
>   lib/igt_gt.c                   | 54 ++++------------------------------
>   lib/igt_gt.h                   | 17 -----------
>   tests/i915/gem_ctx_isolation.c | 21 +++++--------
>   tools/intel_reg.c              |  4 +--
>   6 files changed, 19 insertions(+), 94 deletions(-)
> 
> diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c
> index f94d89cb4..07ab78174 100644
> --- a/lib/i915/gem_context.c
> +++ b/lib/i915/gem_context.c
> @@ -290,7 +290,7 @@ bool gem_context_has_engine(int fd, uint32_t ctx, uint64_t engine)
>   	 * wouldn't produce any result.
>   	 */
>   	if ((engine & ~(3<<13)) == I915_EXEC_BSD) {
> -		if (engine & (3 << 13) && !gem_has_bsd2(fd))
> +		if (engine & (2 << 13) && !gem_has_bsd2(fd))
>   			return false;
>   	}
>   
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index d0c8bd5aa..fdd1b9516 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -223,22 +223,15 @@ struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id)
>   			struct intel_execution_engine2 *__e2 =
>   				&engine_data.engines[engine_data.nengines];
>   
> -			if (!igt_only_list_subtests()) {
> -				__e2->flags = gem_class_instance_to_eb_flags(fd,
> -						e2->class, e2->instance);
> -
> -				if (!gem_has_ring(fd, __e2->flags))
> -					continue;
> -			} else {
> -				__e2->flags = -1; /* 0xfff... */
> -			}
> -
>   			__e2->name       = e2->name;
>   			__e2->instance   = e2->instance;
>   			__e2->class      = e2->class;
> +			__e2->flags      = e2->flags;
>   			__e2->is_virtual = false;
>   
> -			engine_data.nengines++;
> +			if (igt_only_list_subtests() ||
> +			    gem_has_ring(fd, e2->flags))
> +				engine_data.nengines++;
>   		}
>   		return engine_data;
>   	}
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 6b7c037e6..78e3cd089 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -585,12 +585,12 @@ bool gem_can_store_dword(int fd, unsigned int engine)
>   }
>   
>   const struct intel_execution_engine2 intel_execution_engines2[] = {
> -	{ "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> -	{ "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> -	{ "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> -	{ "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> -	{ "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
> -	{ "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> +	{ "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
> +	{ "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
> +	{ "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },

execbuf will reject this on single vcs parts. :( Am I not seeing some 
place where you fudge it into compliance?

Regards,

Tvrtko

> +	{ "vcs1", I915_ENGINE_CLASS_VIDEO, 1, I915_EXEC_BSD | I915_EXEC_BSD_RING2 },
> +	{ "vcs2", I915_ENGINE_CLASS_VIDEO, 2, -1 },
> +	{ "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0, I915_EXEC_VEBOX },
>   	{ }
>   };
>   
> @@ -611,48 +611,6 @@ int gem_execbuf_flags_to_engine_class(unsigned int flags)
>   	}
>   }
>   
> -unsigned int
> -gem_class_instance_to_eb_flags(int gem_fd,
> -			       enum drm_i915_gem_engine_class class,
> -			       unsigned int instance)
> -{
> -	if (class != I915_ENGINE_CLASS_VIDEO)
> -		igt_assert(instance == 0);
> -	else
> -		igt_assert(instance >= 0 && instance <= 1);
> -
> -	switch (class) {
> -	case I915_ENGINE_CLASS_RENDER:
> -		return I915_EXEC_RENDER;
> -	case I915_ENGINE_CLASS_COPY:
> -		return I915_EXEC_BLT;
> -	case I915_ENGINE_CLASS_VIDEO:
> -		if (instance == 0) {
> -			if (gem_has_bsd2(gem_fd))
> -				return I915_EXEC_BSD | I915_EXEC_BSD_RING1;
> -			else
> -				return I915_EXEC_BSD;
> -
> -		} else {
> -			return I915_EXEC_BSD | I915_EXEC_BSD_RING2;
> -		}
> -	case I915_ENGINE_CLASS_VIDEO_ENHANCE:
> -		return I915_EXEC_VEBOX;
> -	case I915_ENGINE_CLASS_INVALID:
> -	default:
> -		igt_assert(0);
> -	};
> -}
> -
> -bool gem_has_engine(int gem_fd,
> -		    enum drm_i915_gem_engine_class class,
> -		    unsigned int instance)
> -{
> -	return gem_has_ring(gem_fd,
> -			    gem_class_instance_to_eb_flags(gem_fd, class,
> -							   instance));
> -}
> -
>   bool gem_ring_is_physical_engine(int fd, unsigned ring)
>   {
>   	if (ring == I915_EXEC_DEFAULT)
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 77318e2a8..73b5002a0 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -102,21 +102,4 @@ extern const struct intel_execution_engine2 {
>   
>   int gem_execbuf_flags_to_engine_class(unsigned int flags);
>   
> -unsigned int
> -gem_class_instance_to_eb_flags(int gem_fd,
> -			       enum drm_i915_gem_engine_class class,
> -			       unsigned int instance);
> -
> -bool gem_has_engine(int gem_fd,
> -		    enum drm_i915_gem_engine_class class,
> -		    unsigned int instance);
> -
> -static inline
> -void gem_require_engine(int gem_fd,
> -			enum drm_i915_gem_engine_class class,
> -			unsigned int instance)
> -{
> -	igt_require(gem_has_engine(gem_fd, class, instance));
> -}
> -
>   #endif /* IGT_GT_H */
> diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> index bcd0f4812..5b054c81d 100644
> --- a/tests/i915/gem_ctx_isolation.c
> +++ b/tests/i915/gem_ctx_isolation.c
> @@ -321,8 +321,7 @@ static uint32_t read_regs(int fd,
>   	memset(&execbuf, 0, sizeof(execbuf));
>   	execbuf.buffers_ptr = to_user_pointer(obj);
>   	execbuf.buffer_count = 2;
> -	execbuf.flags =
> -		gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> +	execbuf.flags = e->flags;
>   	execbuf.rsvd1 = ctx;
>   	gem_execbuf(fd, &execbuf);
>   	gem_close(fd, obj[1].handle);
> @@ -377,8 +376,7 @@ static void write_regs(int fd,
>   	memset(&execbuf, 0, sizeof(execbuf));
>   	execbuf.buffers_ptr = to_user_pointer(&obj);
>   	execbuf.buffer_count = 1;
> -	execbuf.flags =
> -		gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> +	execbuf.flags = e->flags;
>   	execbuf.rsvd1 = ctx;
>   	gem_execbuf(fd, &execbuf);
>   	gem_close(fd, obj.handle);
> @@ -448,8 +446,7 @@ static void restore_regs(int fd,
>   	memset(&execbuf, 0, sizeof(execbuf));
>   	execbuf.buffers_ptr = to_user_pointer(obj);
>   	execbuf.buffer_count = 2;
> -	execbuf.flags =
> -		gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> +	execbuf.flags = e->flags;
>   	execbuf.rsvd1 = ctx;
>   	gem_execbuf(fd, &execbuf);
>   	gem_close(fd, obj[1].handle);
> @@ -559,8 +556,7 @@ static void nonpriv(int fd,
>   		0x0505c0c0,
>   		0xdeadbeef
>   	};
> -	unsigned int engine =
> -		gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> +	unsigned int engine = e->flags;
>   	unsigned int num_values = ARRAY_SIZE(values);
>   
>   	/* Sigh -- hsw: we need cmdparser access to our own registers! */
> @@ -616,9 +612,7 @@ static void isolation(int fd,
>   		0xaaaaaaaa,
>   		0xdeadbeef
>   	};
> -	unsigned int engine = gem_class_instance_to_eb_flags(fd,
> -							     e->class,
> -							     e->instance);
> +	unsigned int engine = e->flags;
>   	unsigned int num_values =
>   		flags & (DIRTY1 | DIRTY2) ? ARRAY_SIZE(values) : 1;
>   
> @@ -729,8 +723,7 @@ static void preservation(int fd,
>   		0xdeadbeef
>   	};
>   	const unsigned int num_values = ARRAY_SIZE(values);
> -	unsigned int engine =
> -		gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> +	unsigned int engine = e->flags;
>   	uint32_t ctx[num_values +1 ];
>   	uint32_t regs[num_values + 1][2];
>   	igt_spin_t *spin;
> @@ -840,7 +833,7 @@ igt_main
>   		igt_subtest_group {
>   			igt_fixture {
>   				igt_require(has_context_isolation & (1 << e->class));
> -				gem_require_engine(fd, e->class, e->instance);
> +				gem_require_ring(fd, e->flags);
>   				igt_fork_hang_detector(fd);
>   			}
>   
> diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> index 1247b70b0..e517956b8 100644
> --- a/tools/intel_reg.c
> +++ b/tools/intel_reg.c
> @@ -329,9 +329,7 @@ static int register_srm(struct config *config, struct reg *reg,
>   	memset(&execbuf, 0, sizeof(execbuf));
>   	execbuf.buffers_ptr = to_user_pointer(obj);
>   	execbuf.buffer_count = 2;
> -	execbuf.flags = gem_class_instance_to_eb_flags(fd,
> -						       engine->class,
> -						       engine->instance);
> +	execbuf.flags = engine->flags;
>   	if (secure)
>   		execbuf.flags |= I915_EXEC_SECURE;
>   
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev