Re: [igt-dev] [PATCH i-g-t 24/25] gem_wsim: Discover engines

[Andi Shyti <andi.shyti@intel.com>]

On Fri, May 17, 2019 at 12:25:25PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Instead of hardcoding the VCS balancing engines, discover, both with the
> new engines query, or with the legacy get_param in the fallback case, so
> class based addressing always works.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  benchmarks/gem_wsim.c | 180 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 173 insertions(+), 7 deletions(-)
> 
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index d43e7c767801..539de243f6e8 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -365,34 +365,198 @@ static int str_to_engine(const char *str)
>  	return -1;
>  }
>  
> +static bool __engines_queried;
> +static unsigned int __num_engines;
> +static struct i915_engine_class_instance *__engines;
> +
> +static int
> +__i915_query(int i915, struct drm_i915_query *q)
> +{
> +	if (igt_ioctl(i915, DRM_IOCTL_I915_QUERY, q))
> +		return -errno;
> +	return 0;
> +}
> +
> +static int
> +__i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
> +{
> +	struct drm_i915_query q = {
> +		.num_items = n_items,
> +		.items_ptr = to_user_pointer(items),
> +	};
> +	return __i915_query(i915, &q);
> +}
> +
> +static void
> +i915_query_items(int i915, struct drm_i915_query_item *items, uint32_t n_items)
> +{
> +	igt_assert_eq(__i915_query_items(i915, items, n_items), 0);
> +}
> +
> +static bool has_query(int i915)
> +{
> +	struct drm_i915_query query = {};
> +
> +	return __i915_query(i915, &query) == 0;
> +}
> +
> +static bool has_engine_query(int i915)
> +{
> +	struct drm_i915_query_item item = {
> +		.query_id = DRM_I915_QUERY_ENGINE_INFO,
> +	};
> +
> +	return __i915_query_items(i915, &item, 1) == 0 && item.length > 0;
> +}
> +
> +static void query_engines(void)
> +{
> +	struct i915_engine_class_instance *engines;
> +	unsigned int num;
> +
> +	if (__engines_queried)
> +		return;
> +
> +	__engines_queried = true;
> +
> +	if (!has_query(fd) || !has_engine_query(fd)) {

One question, still. What is the real use of this check and
'has_query' that is used only here.

I mean... here you want to check whether the "ioctl is not
implemented" or "ioctl is not implemented and length  is 0".

Wouldn't in this case just '!has_engine_query()' be enough? or
have I missed any case?

> +		unsigned int num_bsd = gem_has_bsd(fd) + gem_has_bsd2(fd);
> +		unsigned int i = 0;
> +
> +		igt_assert(num);
> +
> +		num = 1 + num_bsd;

did you mean the above two lines swapped?

> +
> +		if (gem_has_blt(fd))
> +			num++;
> +
> +		if (gem_has_vebox(fd))
> +			num++;
> +
> +		engines = calloc(num,
> +				 sizeof(struct i915_engine_class_instance));
> +		igt_assert(engines);
> +
> +		engines[i].engine_class = I915_ENGINE_CLASS_RENDER;
> +		engines[i].engine_instance = 0;
> +		i++;
> +
> +		if (gem_has_blt(fd)) {
> +			engines[i].engine_class = I915_ENGINE_CLASS_COPY;
> +			engines[i].engine_instance = 0;
> +			i++;
> +		}
> +
> +		if (gem_has_bsd(fd)) {
> +			engines[i].engine_class = I915_ENGINE_CLASS_VIDEO;
> +			engines[i].engine_instance = 0;
> +			i++;
> +		}
> +
> +		if (gem_has_bsd2(fd)) {
> +			engines[i].engine_class = I915_ENGINE_CLASS_VIDEO;
> +			engines[i].engine_instance = 1;
> +			i++;
> +		}
> +
> +		if (gem_has_vebox(fd)) {
> +			engines[i].engine_class =
> +				I915_ENGINE_CLASS_VIDEO_ENHANCE;
> +			engines[i].engine_instance = 0;
> +			i++;
> +		}

mmhhh... isn't this the intel_execution_engine2[]? Yet another
way for having engine list... in the long run, updating here (as
well) won't be easy to remember.

Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev