Re: [igt-dev] [PATCH v19 2/6] lib/i915: add gem_engine_topology library and for_each loop definition

From: Andi Shyti <andi@etezian.org>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: IGT dev <igt-dev@lists.freedesktop.org>, Andi Shyti <andi@etezian.org>
Subject: Re: [igt-dev] [PATCH v19 2/6] lib/i915: add gem_engine_topology library and for_each loop definition
Date: Fri, 12 Apr 2019 02:23:09 +0300	[thread overview]
Message-ID: <20190411232309.GD24310@jack.zhora.eu> (raw)
In-Reply-To: <8a96c375-cf44-81bb-66a9-e00f53e01493@linux.intel.com>

On Thu, Apr 11, 2019 at 05:03:58PM +0100, Tvrtko Ursulin wrote:
> 
> On 08/04/2019 17:15, Andi Shyti wrote:
> > The gem_engine_topology library is a set of functions that
> > interface with the query and getparam/setparam ioctls.
> > 
> > The library's access point is the 'intel_init_engine_list()'
> > function that, everytime is called, generates the list of active
> > engines and returns them in a 'struct intel_engine_data'. The
> > structure contains only the engines that are actively present in
> > the GPU.
> > 
> > The function can work in both the cases that the query and
> > getparam ioctls are implemented or not by the running kernel. In
> > case they are implemented, a query is made to the driver to fetch
> > the list of active engines. In case they are not implemented, the
> > list is taken from the 'intel_execution_engines2' array and
> > stored only after checking their presence.
> > 
> > The gem_engine_topology library provides some iteration helpers:
> > 
> >   - intel_get_current_engine(): provides the current engine in the
> >     iteration.
> > 
> >   - intel_get_current_physical_engine(): provides the current
> >     physical engine, if the current engine is a virtual engine,
> >     it moves forward until it finds a physical engine.
> > 
> >   - intel_next_engine() it just increments the counter so that it
> >     points to the next engine.
> > 
> > Extend the 'for_each_engine_class_instance' so that it can loop
> > using the new 'intel_init_engine_list()' and rename it to
> > 'for_each_context_engine'.
> > 
> > Move '__for_each_engine_class_instance' to gem_engine_topology.h
> > and rename it to '__for_each_static_engine'.
> > 
> > Update accordingly tests/perf_pmu.c to use correctly the new
> > for_each loops.
> > 
> > Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> > ---
> >   lib/Makefile.sources           |   2 +
> >   lib/i915/gem_engine_topology.c | 281 +++++++++++++++++++++++++++++++++
> >   lib/i915/gem_engine_topology.h |  78 +++++++++
> >   lib/igt.h                      |   1 +
> >   lib/igt_gt.h                   |   2 +
> >   lib/meson.build                |   1 +
> >   6 files changed, 365 insertions(+)
> >   create mode 100644 lib/i915/gem_engine_topology.c
> >   create mode 100644 lib/i915/gem_engine_topology.h
> > 
> > diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> > index e00347f945c5..41331c2f2b80 100644
> > --- a/lib/Makefile.sources
> > +++ b/lib/Makefile.sources
> > @@ -13,6 +13,8 @@ lib_source_list =	 	\
> >   	i915/gem_ring.c	\
> >   	i915/gem_mman.c	\
> >   	i915/gem_mman.h	\
> > +	i915/gem_engine_topology.c	\
> > +	i915/gem_engine_topology.h	\
> >   	i915_3d.h		\
> >   	i915_reg.h		\
> >   	i915_pciids.h		\
> > diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> > new file mode 100644
> > index 000000000000..06f538372a4d
> > --- /dev/null
> > +++ b/lib/i915/gem_engine_topology.c
> > @@ -0,0 +1,281 @@
> > +/*
> > + * Copyright © 2019 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#include "drmtest.h"
> > +#include "ioctl_wrappers.h"
> > +
> > +#include "i915/gem_engine_topology.h"
> > +
> > +static int __gem_query(int fd, struct drm_i915_query *q)
> > +{
> > +	int err = 0;
> > +
> > +	if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
> > +		err = -errno;
> > +
> > +	errno = 0;
> > +	return err;
> > +}
> > +
> > +static void gem_query(int fd, struct drm_i915_query *q)
> > +{
> > +	igt_assert_eq(__gem_query(fd, q), 0);
> > +}
> > +
> > +static void query_engines(int fd,
> > +			  struct drm_i915_query_engine_info *query_engines,
> > +			  int length)
> > +{
> > +	struct drm_i915_query_item item = { };
> > +	struct drm_i915_query query = { };
> > +
> > +	item.query_id = DRM_I915_QUERY_ENGINE_INFO;
> > +	query.items_ptr = to_user_pointer(&item);
> > +	query.num_items = 1;
> > +	item.length = length;
> > +
> > +	item.data_ptr = to_user_pointer(query_engines);
> > +
> > +	gem_query(fd, &query);
> > +}
> > +
> > +static void ctx_map_engines(int fd, struct intel_engine_data *ed,
> > +			    struct drm_i915_gem_context_param *param)
> > +{
> > +	struct i915_context_param_engines *engines =
> > +			(struct i915_context_param_engines *) param->value;
> > +	int i = 0;
> > +
> > +	for (typeof(engines->class_instance[0]) *p =
> > +	     &engines->class_instance[0];
> > +	     i < ed->nengines; i++, p++) {
> > +		p->engine_class = ed->engines[i].class;
> > +		p->engine_instance = ed->engines[i].instance;
> > +	}
> > +
> > +	param->size = offsetof(typeof(*engines), class_instance[i]);
> > +	engines->extensions = 0;
> > +
> > +	gem_context_set_param(fd, param);
> > +}
> > +
> > +static void init_engine(struct intel_execution_engine2 *e2,
> > +			int class, int instance, uint64_t flags)
> > +{
> > +	const struct intel_execution_engine2 *__e2;
> > +	static const char *unknown_name = "unknown",
> > +			  *virtual_name = "virtual";
> > +
> > +	e2->class    = class;
> > +	e2->instance = instance;
> > +	e2->flags    = flags;
> > +
> > +	/* engine is a virtual engine */
> > +	if (class == I915_ENGINE_CLASS_INVALID) {
> > +		e2->name = virtual_name;
> > +		e2->is_virtual = true;
> > +		return;
> > +	}
> > +
> > +	__for_each_static_engine(__e2)
> > +		if (__e2->class == class && __e2->instance == instance)
> > +			break;
> > +
> > +	if (__e2->name) {
> > +		e2->name = __e2->name;
> > +	} else {
> > +		igt_warn("found unknown engine (%d, %d)", class, instance);
> > +		e2->name = unknown_name;
> > +	}
> > +
> > +	/* just to remark it */
> > +	e2->is_virtual = false;
> > +}
> > +
> > +static void query_engine_list(int fd, struct intel_engine_data *ed)
> > +{
> > +	uint8_t buff[SIZEOF_QUERY] = { };
> > +	struct drm_i915_query_engine_info *query_engine =
> > +			(struct drm_i915_query_engine_info *) buff;
> > +	int i;
> > +
> > +	query_engines(fd, query_engine, SIZEOF_QUERY);
> > +
> > +	for (i = 0; i < query_engine->num_engines; i++)
> > +		init_engine(&ed->engines[i],
> > +			    query_engine->engines[i].engine_class,
> > +			    query_engine->engines[i].engine_instance, i);
> > +
> > +	ed->nengines = query_engine->num_engines;
> > +}
> > +
> > +struct intel_execution_engine2
> > +*intel_get_current_engine(struct intel_engine_data *ed)
> > +{
> > +	if (!ed->n)
> > +		ed->current_engine = &ed->engines[0];
> > +	else if (ed->n >= ed->nengines)
> > +		ed->current_engine = NULL;
> > +
> > +	return ed->current_engine;
> > +}
> > +
> > +void intel_next_engine(struct intel_engine_data *ed)
> > +{
> > +	if (ed->n + 1 < ed->nengines) {
> > +		ed->n++;
> 
> This is broken for one engine. It should read:

true! I considered as a case but then I forgot to fix it.

> 	if (++ed->n < ed->nengines)

an uncontrolled access to intel_next_engine would increase 'n'
incontrollably as well.

> 		ed->current_engine = &ed->engines[ed->n];
> 	else
> 		ed->current_engine = NULL;

I should assign here "ed->n = ed->nengines".

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