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

[Tvrtko Ursulin]

On 05/04/2019 10:00, Andi Shyti wrote:
> Hi Tvrtko,
> 
>>> +	struct i915_context_param_engines *engines =
>>> +			(struct i915_context_param_engines*) param->value;
>>
>> Cosmetics only: Our coding style is "(type *)var".
>>
>>> +	for (typeof(engines->class_instance[0]) *p =
>>> +			&engines->class_instance[0];
>>> +						i < ed->nengines; i++, p++)
>>
>> More cosmetics: Indentation alignment looks wonky.
> 
> working at the edge of the 80 characters provides always
> cosmetic challenges, I'll fix the above.
> 
>>> +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;
>>> +}
>>> +
>>
>> Why can this just return ed->current_engine? I am confused by the need to
>> check against ed->nengines both in the getter and in the next helper. Just
>> need to init ed->current_engine to the first one in init_engine_list I
>> think.
> 
> In the previous patch there was an unrefined mistake:
> ed->current_engine was initialized in the
> 'intel_init_engine_list()' function.
> 
> But because there everything is allocated in stack, the
> '&ed->engines[0]' is also in stack that get freed when the
> function returns, with the result that I lose reference.
> 
> For this, I would need the pointer to be assigned somewhere that
> works with the addresses provided by the caller and this looks
> like the best place for it (indeed I removed the assignement from
> intel_init_engine_list()).

You are right, yep.

> 
>>> +struct intel_execution_engine2
>>> +*intel_get_current_physical_engine(struct intel_engine_data *ed)
>>> +{
>>> +	struct intel_execution_engine2 *e;
>>> +
>>> +	if (ed->n >= ed->nengines)
>>> +		return NULL;
>>
>> Can this be hit? intel_next_engine looks to be avoiding incrementing past
>> nengines - 1.
> 
> yes, you're right, I didn't want to hit the for loop for this
> case that can be checked here, it's redundant, but it looks more
> readable. I'll remove it.
> 
>>> +
>>> +	if (igt_only_list_subtests())
>>> +		intel_next_engine(ed);
>>
>> In subtest listing mode there cannot be virtual engines in the list so I
>> think this branch is not needed.
> 
> Yes, indeed, if we have the list of engines at this point, it's
> only physical. I'll remove it.
> 
>>> +			__e2->name       = e2->name;
>>> +			__e2->instance   = e2->instance;
>>> +			__e2->class      = e2->class;
>>> +			__e2->is_virtual = false;
>>
>> You could use init_engine here but granted it would have to do a redundant
>> name search so maybe not.
> 
> yes, this is how it is was done at first, I put it outside for
> the reason you mentioned :)

That's fine. Okay then, just one or two details to update.

Regards,

Tvrtko