Re: [PATCH i-g-t] tests/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-05 at 19:40:45 -0300, Gustavo Sousa wrote:
>
> I did give you r-b but after compilling this I have few nits,
> first about subject, please add 'intel/' in prefix in subject, so
> better
>
> [PATCH i-g-t] tests/intel/xe_pmu: Do not enable VFs in unrelated subtests

Sure.

>
>> 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().
>> 
>> 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..79d466093d1d 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_vfs) \
>
> num_vfs is unused, either use it or drop it.

Hm. Actuallly num_vfs should have been called num_fns. Good catch.

I believe this worked by luck: the users of test_each_engine_with_vfs()
have the variable num_fns defined in their scope so the macro ended up
working even with the wrong signature.

I'll fix this in v2 to use num_fns as argument name.

Thanks!

--
Gustavo Sousa

>
> Regards,
> Kamil
>
>> +	igt_subtest_with_dynamic(test) \
>> +		xe_for_each_engine(fd, hwe) \
>> +			for (num_fns = enable_and_provision_vfs(fd) + 1; \
>> +			     num_fns; \
>> +			     unprovision_and_disable_vfs(fd), num_fns = 0) \
>> +				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);
>>  	}
>>  
>>  	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>
>>