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

Andi Shyti
2019-04-11T23:31:10Z

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