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:29:12 -0300, Gustavo Sousa wrote:
>> 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?
>
> When test fails due to igt_assert(), code execution jumps to
> next igt_fixture, so also no cleanup will happen in for-loop.

I think skip and assertion failures actually make the program jump
back to where the igt_dynamic_f() was called, no?

Here is an example code (saved as tests/foo.c):

    #include "igt_core.h"
    
    static void run_dynamic_test_logic(int index)
    {
    	igt_info("Running dynamic work for iteration %d...\n", index + 1);
    	igt_assert_f(index != 1, "Intentional IGT assertion failure on the 2nd dynamic subtest!\n");
    	igt_skip_on_f(index == 2, "Intentional IGT skip on the 3rd dynamic subtest!\n");
    	igt_info("Dynamic work for iteration %d completed successfully.\n", index + 1);
    }
    
    int igt_main()
    {
    	igt_subtest_with_dynamic("container-subtest") {
    		for (int i = 0; i < 3; i++) {
    			igt_info("\nBefore calling igt_dynamic_f().\n");
    			igt_dynamic_f("dynamic-subtest-%d", i) {
    				run_dynamic_test_logic(i);
    			}
    			igt_info("After calling igt_dynamic_f().\n\n");
    		}
    	}
    }
    

, and here is the output of compiling and running it:

    $ ninja -C build tests/foo && ./build/tests/foo
    ninja: Entering directory `build'
    [1/9] Compiling C object tests/foo.p/foo.c.o
    [2/9] Generating lib/version.h with a custom command
    [3/3] Linking target tests/foo
    IGT-Version: 2.6-g095481e279a9 (x86_64) (Linux: 7.2.7-200.fc44.x86_64 x86_64)
    Using IGT_SRANDOM=1791302421 for randomisation
    Starting subtest: container-subtest
    
    Before calling igt_dynamic_f().
    Starting dynamic subtest: dynamic-subtest-0
    Running dynamic work for iteration 1...
    Dynamic work for iteration 1 completed successfully.
    Dynamic subtest dynamic-subtest-0: SUCCESS (0.000s)
    After calling igt_dynamic_f().
    
    
    Before calling igt_dynamic_f().
    Starting dynamic subtest: dynamic-subtest-1
    Running dynamic work for iteration 2...
    [61375.969517] (foo:1196168) CRITICAL: Test assertion failure function run_dynamic_test_logic, file ../tests/foo.c:6:
    [61375.969520] (foo:1196168) CRITICAL: Failed assertion: index != 1
    [61375.969527] (foo:1196168) CRITICAL: Last errno: 13, Permission denied
    [61375.969528] (foo:1196168) CRITICAL: Intentional IGT assertion failure on the 2nd dynamic subtest!
    Stack trace: not implemented
    Dynamic subtest dynamic-subtest-1 failed.
    **** DEBUG ****
    [61375.969512] (foo:1196168) INFO: Running dynamic work for iteration 2...
    [61375.969517] (foo:1196168) CRITICAL: Test assertion failure function run_dynamic_test_logic, file ../tests/foo.c:6:
    [61375.969520] (foo:1196168) CRITICAL: Failed assertion: index != 1
    [61375.969527] (foo:1196168) CRITICAL: Last errno: 13, Permission denied
    [61375.969528] (foo:1196168) CRITICAL: Intentional IGT assertion failure on the 2nd dynamic subtest!
    [61375.969530] (foo:1196168) igt_core-INFO: Stack trace: not implemented
    ****  END  ****
    Dynamic subtest dynamic-subtest-1: FAIL (0.000s)
    After calling igt_dynamic_f().
    
    
    Before calling igt_dynamic_f().
    Starting dynamic subtest: dynamic-subtest-2
    Running dynamic work for iteration 3...
    Test requirement not met in function run_dynamic_test_logic, file ../tests/foo.c:7:
    Test requirement: !(index == 2)
    Intentional IGT skip on the 3rd dynamic subtest!
    Last errno: 13, Permission denied
    Dynamic subtest dynamic-subtest-2: SKIP (0.000s)
    After calling igt_dynamic_f().
    
    Subtest container-subtest: FAIL (0.000s)

--
Gustavo Sousa
>
> Regards,
> Kamil
>
>> 
>> --
>> 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>
>> >>