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

[Tvrtko Ursulin]

On 22/03/2019 07:59, Chris Wilson wrote:
> 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.

Yes I got confused.

>>> +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.

You are simply saying to move the "IS_PHYISICAL" test to init_engine 
here and store it in a flag per engine?

Regards,

Tvrtko