[igt-dev] [PATCH v16 8/8] test: perf_pmu: use the gem_engine_topology library

[Tvrtko Ursulin]

On 29/03/2019 12:47, Andi Shyti wrote:
>>> -static igt_spin_t * __spin_poll(int fd, uint32_t ctx, unsigned long flags)
>>> +static igt_spin_t * __spin_poll(int fd, uint32_t ctx, const struct intel_execution_engine2 *e)
>>>    {
>>>    	struct igt_spin_factory opts = {
>>>    		.ctx = ctx,
>>> -		.engine = flags,
>>> +		.engine = e->flags,
>>>    	};
>>> -	if (gem_can_store_dword(fd, flags))
>>> +	if (gem_engine_can_store_dword(fd, e))
>>>    		opts.flags |= IGT_SPIN_POLL_RUN;
>>>    	return __igt_spin_batch_factory(fd, &opts);
>>> @@ -211,7 +211,16 @@ static unsigned long __spin_wait(int fd, igt_spin_t *spin)
>>>    static igt_spin_t * __spin_sync(int fd, uint32_t ctx, unsigned long flags)
>>
>> __spin_sync should also take engine instead of flags, like __spin_poll.
> 
> But if __spin_sync is called inside __for_each_physical_engine
> (which in this code never happens), engine doesn't have any flags
> set. right?
> 
>>>    {
>>> -	igt_spin_t *spin = __spin_poll(fd, ctx, flags);
>>> +	struct igt_spin_factory opts = {
>>> +		.ctx = ctx,
>>> +		.engine = flags,
>>> +	};
>>> +	igt_spin_t *spin;
>>> +
>>> +	if (gem_can_store_dword(fd, flags))
>>> +		opts.flags |= IGT_SPIN_POLL_RUN;
>>> +
>>> +	spin = __igt_spin_batch_factory(fd, &opts);
>>
>> And then you don't need this hunk at all, making the whole thing completely
>> ready for more engines.
>>
>> And e2ring will end up with no callers so can be removed.
> 
> Same reason as above, which is this one:
> 
>>> -		__for_each_engine_class_instance(e) {
>>> +		__for_each_static_engine(e) {
>>>    			igt_subtest_group {
>>>    				igt_fixture {
>>>    					gem_require_engine(render_fd,
>>>
>>
>> Yep, modulo __spin_sync change, this is what I had in mind.
> 
> at this point I don't have any flag set, any check whether the
> engine really exists, no context mapping (still flags) :/
> 
> Am I missing it?

No I missed it. How about this then...

We make __for_each_physical_engine call igt_only_list_subtests during 
setup and decide whether or not it is allowed to open the device.

Therefore in subtest enumeration it doesn't, uses the static list, and 
we don't need e->flags to be correct, just e->name.

In normal test runs it is allowed to do everything and engines are 
always properly initialized?

Regards,

Tvrtko