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

Tvrtko Ursulin
2019-03-29T12:56:47Z

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