Re: [PATCH i-g-t v2] tests/intel/xe_pmu: Do not enable VFs in unrelated subtests

[Gustavo Sousa <gustavo.sousa@intel.com>]

"Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com> writes:

> On 10/6/2026 4:11 PM, Gustavo Sousa wrote:
>> The test xe_pmu has a subtest group that enables SRIOV VFs as part of
>> its fixture.  Even though the fixture is part of a subtest group, the
>> enabling/disabling will happen regardless of whether those subests are
>> selected or not (via --run-subtest).
>>
>> As most subtests in xe_pmu don't need that, avoid doing that as a
>> fixture and rather incorporate the enabling/disabling as part of a new
>> macro called test_each_engine_with_vfs().
>>
>> v2:
>>    - Add "intel" to commit subject prefix. (Kamil)
>>    - Fix argument name of test_each_engine_with_vfs():
>>      s/num_vfs/num_fns/. (Kamil)
>>    - Enable/disable VFs only once per macro call by moving the call to
>>      xe_for_each_engine() inside the for-loop. (Marcin)
>>
>> Cc: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>   tests/intel/xe_pmu.c | 21 ++++++++++++---------
>>   1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/intel/xe_pmu.c b/tests/intel/xe_pmu.c
>> index d41ebba1faef..eda56c078653 100644
>> --- a/tests/intel/xe_pmu.c
>> +++ b/tests/intel/xe_pmu.c
>> @@ -124,6 +124,15 @@ static bool has_engine_active_ticks;
>>   			igt_dynamic_f("engine-%s%d", xe_engine_class_string(hwe->engine_class), \
>>   				      hwe->engine_instance)
>>   
>> +#define test_each_engine_with_vfs(test, fd, hwe, num_fns) \
>> +	igt_subtest_with_dynamic(test) \
>> +		for (num_fns = enable_and_provision_vfs(fd) + 1; \
>> +		     num_fns; \
>> +		     unprovision_and_disable_vfs(fd), num_fns = 0) \
>> +			xe_for_each_engine(fd, hwe) \
>> +				igt_dynamic_f("engine-%s%d", xe_engine_class_string(hwe->engine_class), \
>> +					      hwe->engine_instance)
>> +
>
> igt_assert (from enable_and_provision_vfs) before entering igt_dynamic 
> can hit an internal assertion and abort.

Oh... Right. I haven't considered the setup of VFs itself asserting.

>
> Could we move VF setup into the dynamic case and reuse the enabled VFs 
> across engines, like xe_sriov_scheduling does?

Well, this sounds like a good suggestion, but I'd argue that it is a bit
outside of the scope of this patch. It could be done as a follow-up at
some point though.

As a short-term solution, I think we could follow Kamil's suggestion of
adding a fixture to disable VFs if they were enabled.  We could do that
after each call to test_each_engine_with_vfs(). Not a big fan of this
(ideally that macro should be self-contained), but it should resolve the
problem?

Furthermore, for that to work properly, I believe we would need to also
update enable_and_provision_vfs() to either receive num_fns by reference
or make it a global variable.  The reason for that is that
enable_and_provision_vfs() will not return and, consequently, num_fns
will not get set if a long jump happens.

>
> Unfortunately it complicates the code, adds state and cleanup logic, but 
> I don’t see a simpler way to keep setup assertions safe.

One thing that comes to mind is to provide some infrastructure for
setting up cleanup functions that are called at the end of a
test/subtest/dyn-subtest.  We could create a macro igt_add_cleanup()
that would receive the function responsible for the cleanup and the
arguments.

For example, inside enable_and_provision_vfs(), we could have something
like:

    (...)
    igt_sriov_enable_vfs(fd, num_vfs);
    igt_add_cleanup(unprovision_and_disable_vfs, fd);
    (...)

We would still need to figure out how to allow passing the "fd" argument
to the macro like above.  This would likely require some preparatory macro like
igt_define_cleanup().

--
Gustavo Sousa

>
>>   static bool has_event(const char *device, const char *event)
>>   {
>>   	char buf[512];
>> @@ -1192,26 +1201,20 @@ int igt_main()
>>   	igt_subtest_group() {
>>   		unsigned int num_fns;
>>   
>> -		igt_fixture()
>> -			num_fns = enable_and_provision_vfs(fd) + 1;
>> -
>>   		igt_describe("Validate engine activity on all functions");
>> -		test_each_engine("all-fn-engine-activity-load", fd, eci)
>> +		test_each_engine_with_vfs("all-fn-engine-activity-load", fd, eci, num_fns)
>>   			engine_activity_all_fn(fd, eci, num_fns);
>>   
>>   		igt_describe("Validate per-function engine activity");
>> -		test_each_engine("fn-engine-activity-load", fd, eci)
>> +		test_each_engine_with_vfs("fn-engine-activity-load", fd, eci, num_fns)
>>   			for (int fn = 0; fn < num_fns; fn++)
>>   				engine_activity_fn(fd, eci, fn, XE_SRIOV_SCHED_PRIORITY_LOW);
>>   
>>   		igt_describe("Validate per-function engine activity when sched-if-idle is set");
>> -		test_each_engine("fn-engine-activity-sched-if-idle", fd, eci) {
>> +		test_each_engine_with_vfs("fn-engine-activity-sched-if-idle", fd, eci, num_fns) {
>>   			for (int fn = 0; fn < num_fns; fn++)
>>   				engine_activity_fn(fd, eci, fn, XE_SRIOV_SCHED_PRIORITY_NORMAL);
>>   		}
>> -
>> -		igt_fixture()
>> -			unprovision_and_disable_vfs(fd);
>>   	}
>>   
>>   	igt_subtest_group() {
>>
>> ---
>> base-commit: faad2cd0e3e8eb2e18111c2358823a7bdfb56d94
>> change-id: 20261005-xe_pmu-do-not-enable-vfs-in-unrelated-subtests-e24c1cd95d71
>>
>> Best regards,
>> --
>> Gustavo Sousa <gustavo.sousa@intel.com>