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

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

Kamil Konieczny <kamil.konieczny@linux.intel.com> writes:

> Hi Gustavo,
> On 2026-10-06 at 11:11:19 -0300, 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)
>> +
>>  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);
>
> Hmm, maybe that last one should stay, test should do a cleanup,
> after it ends with igt_assert() or skip, for example:
>
> 		igt_fixture() {
> 			if (num_nfs)
> 				unprovision_and_disable_vfs(fd);
> 		}

The cleanup is already done in the for-loop generated by the macro. Do
you see a case where we really need this extra cleanup call?

--
Gustavo Sousa

> 	
>
> Regards,
> Kamil
>
>>  	}
>>  
>>  	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>
>>