Re: [igt-dev] [PATCH v15 4/5] lib/i915: add gem_engine_topology library and for_each loop definition

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Andi Shyti <andi.shyti@intel.com>,
	IGT dev <igt-dev@lists.freedesktop.org>,
	Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Andi Shyti <andi@etezian.org>
Subject: Re: [igt-dev] [PATCH v15 4/5] lib/i915: add gem_engine_topology library and for_each loop definition
Date: Fri, 22 Mar 2019 07:59:18 +0000	[thread overview]
Message-ID: <155324155883.26447.7921522281863006035@skylake-alporthouse-com> (raw)
In-Reply-To: <6b531677-8e74-c51c-535a-bb5e1c1f2ac7@linux.intel.com>

Quoting Tvrtko Ursulin (2019-03-22 07:47:02)
> 
> On 21/03/2019 16:05, Andi Shyti wrote:
> > +{
> > +     static const char *unknown_name = "unknown",
> > +                       *virtual_name = "virtual";
> 
> Unusual style but it is actually readable so I think I like it.

Bah, if I can't find a cino= setting, I'm not adopting it ;)

> > +
> > +     e2->class    = class;
> > +     e2->instance = instance;
> > +     e2->flags    = flags;
> > +
> > +     if (class < 0 && instance < 0) {
> > +             e2->name = virtual_name;
> > +     } else {
> > +             const struct intel_execution_engine2 *__e2;
> > +
> > +             __for_each_static_engine(__e2)
> > +                     if (__e2->class == class && __e2->instance == instance)
> > +                             break;
> > +
> > +             e2->name = __e2->name ? __e2->name : unknown_name;
> 
> I've now started to worry about how will CI/buglog handle us forgetting 
> to expand the static list. (More than one subtest of a same name for 
> "test-$engine_name" ones?) Do we want and igt_warn on unknown engines to 
> make it more visible? Or even just crash?

Set flags to -1ull. That should cause EINVAL forever one hopes.

We shouldn't get any test (atm) with unknown as we only use the static
table for test generation. For runtime test discovery, we can apply the
filter of does this engine actually exist.

> > +void intel_next_engine(struct intel_engine_data *ed);
> > +
> > +#define IS_PHYSICAL_ENGINE(e2) ((e2->class >= 0) && (e2->instance >= 0))
> 
> Chris, do you think this will be future proof enough?

At the moment, we've reserved just the one identifier for placeholders
(class == I915_ENGINE_CLASS_INVALID). And I feel confident that should
be enough.

The problem is if something else gave us multiple instances of a logical
engine for which we have no means to determine the physical mapping,
which is vvv

> I remembered how at one point I had "IS_PHYSICAL" as a flag in engine query.
> 
> Or we make this here more explicit by being "IS_VIRTUAL" and invert the 
> test in the caller?

Aye. I think you are right here, and we need to put a caps field into
the engine_data (filled in by i915_query for valid classes and default
to !phys for invalid slots). A lot of the for_each_physical_engine()
tests do not make sense if there is automagic engine mapping going on
behind the scenes.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev