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

Tvrtko Ursulin
2019-03-21T11:39:37Z

On 21/03/2019 11:23, Andi Shyti wrote:
>>> +static void init_engine(struct intel_execution_engine2 *e2, const char *name,
>>> + uint16_t class, uint16_t instance, uint8_t flags)
>>
>> I'd probably use u64 for flags to match the structure.
>
> yes, flags it's u64, I used u8 because the flags we use is never
> supposed to be higher than than 3f. But sure, can make it u64.
>
>>> +{
>>> + static const char *unk_name = "unk";
>>> +
>>> + e2->class = class;
>>> + e2->instance = instance;
>>> + e2->flags = flags;
>>> +
>>> + if (name) {
>>> + e2->name = name;
>>
>> This path is used only for the legacy fall back mode so I am contemplating
>> whether is is even worth having the name passed in.
>
> yes, just wanted to be consistent. At the biginning the
> dup_engine had a bigger role, but then I demoted it to just doing
> this.
>
>> The if you find a virtual engine in the list (
>> I915_ENGINE_CLASS_INVALID/I915_ENGINE_CLASS_INVALID_VIRTUAL) you could set
>> the name to "virtual" or something.
>
> do we really need a name of the type "virtual-<engine>"?

Probably not. Plain "virtual" sounds ok.

>
>> Now listen to this.. how about we export the engine names via the query API?
>> Primarily I was thinking to distinguish difference instance of virtual, but
>> then it would also lessen the reliance on the static map. Thoughts?
>
> Do you mean that the name would be provided by the driver?
>
> Other than improving the debug information, is the name
> formatting giving any advantage if we can distinguish by
> class/instance/flags?
>
> We can't use it anyway for test creation.

Yeah, just need to skip it during for_each_physical_engine.

>
> [...]
>
>>> + uint8_t nengines = (param.size -
>>> + sizeof(struct i915_context_param_engines)) /
>>> + sizeof(engines->class_instance[0]);
>>
>> I'd probably just use unsigned int.
>
> Oh... I have set u32 in the intel_engine_data, I didn't reliase,
> I assume that nengines would never be higher than 64 (if that
> happens we can't handle it here).
>
> But Chris is considering the case we will have more tha 64
> engines, I can set it to u32, of course.
>
> [...]
>
>>> +#define for_each_engine_class_instance(fd__, ctx__, e__) \
>>> + for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
>>> + ((e__) = (i__.n < i__.nengines) ? &i__.engines[i__.n] : NULL); \
>>> + i__.n++)
>>
>> Do we want a context parameter in this helper, or even this helper at all? I
>> thought we can end up with only two, for_each_physical_engine and
>> for_each_context_engine - but I guess it is open for discussion.
>
> I don't know of possible use cases that do or don't need ctx
> outside the for_each...().
>
> If you don't see any use of the context index outside the
> for_each I can create the context inside the init_list function.
>
> But, I have a little concern about the destraction of that
> context. If the for_each... gets interrupted in the middle of the
> loop, we lose the context.

I am not following how we lose the context?

I was just discussing of our desired end game in therms of number and
signature for for_each_.. iterators.

For me for_each_physical_engine doesn't need the context since it is
about physical engines - not engine from the engine map. Maybe that one
should even have some asserts then to make sure someone hasn't
re-configured the default context.

And we have __for_each_physical_engine which uses the static table, for
subtest enumeration.

Then for_each_context_engine is the fully featured one, which has the
context id in parameters.

Can we solve all use cases with those three or we need mode?

Chris?

>
> [...]
>
>>> @@ -434,7 +434,7 @@ busy_check_all(int gem_fd, const struct intel_execution_engine2 *e,
>>> i = 0;
>>> fd[0] = -1;
>>> - for_each_engine_class_instance(gem_fd, e_) {
>>> + for_each_engine_class_instance(gem_fd, 0, e_) {
>>> if (e == e_)
>>> busy_idx = i;
>>> @@ -497,7 +497,7 @@ most_busy_check_all(int gem_fd, const struct intel_execution_engine2 *e,
>>> unsigned int idle_idx, i;
>>> i = 0;
>>> - for_each_engine_class_instance(gem_fd, e_) {
>>> + for_each_engine_class_instance(gem_fd, 0, e_) {
>>> if (e == e_)
>>> idle_idx = i;
>>> else if (spin)
>>> @@ -554,7 +554,7 @@ all_busy_check_all(int gem_fd, const unsigned int num_engines,
>>> unsigned int i;
>>> i = 0;
>>> - for_each_engine_class_instance(gem_fd, e) {
>>> + for_each_engine_class_instance(gem_fd, 0, e) {
>>> if (spin)
>>> __submit_spin_batch(gem_fd, spin, e, 64);
>>> else
>>> @@ -1683,7 +1683,7 @@ igt_main
>>> igt_require_gem(fd);
>>> igt_require(i915_type_id() > 0);
>>> - for_each_engine_class_instance(fd, e)
>>> + for_each_engine_class_instance(fd, 0, e)
>>> num_engines++;
>>> }
>>>
>>
>> Looks like this would work. Just the question of virtual engine, set of
>> chosen iterators, and maybe some nits.
>
> Yes, as we discussed, right after this patchset I will do the
> for_each_physical.
>
> What are the nits? I love nits :)

Just things like types and coding style. :) And some more details from
Chris' review.

Regards,

Tvrtko