Re: [PATCH i-g-t] tests/intel/intel_sysfs_debugfs: Remove xe-gt

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

On 4/10/2025 8:11 PM, Kamil Konieczny wrote:
> Hi Peter,
> On 2025-04-10 at 11:03:15 +0200, Peter Senna Tschudin wrote:
>> The intel_sysfs_debugfs test now includes functionality to read all Xe
>> debugfs files, making the older xe-gt test redundant. Additionally,
>> xe-gt causes issues when testing Virtual Functions (VFs) in SR-IOV
>> setups, as some of the debugfs files it expects are not present for VFs.
>>
>> Rather than extending the overlapping and problematic xe-gt, this commit
>> removes it entirely.
> 
> I agree on removing reading part but not on existence checks.
> Imho start with small refactor:
> 
> - remove readings
> - change existing checks so they will print _all_ missing
>    debugfs nodes and only after that test will fail if there
>    were misses
Isn’t that what validate_entries() is supposed to handle? (Although the 
warnings are currently hidden unless you use the --warn-not-hit flag - 
and that only works properly after removing the leading -- in 
long_options to make it behave naturally 😉)

There are already missing nodes like: gtX/tunings, gtX/stats, 
gtX/uc/gsc_info, gtX/uc/guc_ctb, plus additional ones if certain 
features are enabled.

The main problems I see with this approach are:

1. Maintenance cost – the list is already outdated and it's unclear what 
criteria should be used to decide which attributes to include

2. Platform/feature-specific nodes – we’d need to replicate kernel logic 
to avoid false positives, which adds further maintenance overhead. Those 
specific feature checks are probably better handled by dedicated tests 
that directly use those attributes. Being a VF is just one specific case 
of this.
	
3. VM scenarios – we might be using an older kernel without some nodes. 
In IGT, we typically skip tests if the required feature isn’t available.

4. Private developer attributes – if a developer adds their own 
"non-public" attributes, the test would fail (though maybe that's not a 
major issue?).

--
marcin>
> Final step should be:
> - adding a flag for those nodes which should not exist on VF,
>    detect that and ignore this on VFs.
> 
> These could be done in one or in two patches.
> 
> Regards,
> Kamil
> 
>>
>> Cc: marcin.bernatowicz@intel.com
>> Cc: matthew.brost@intel.com
>> Cc: pravalika.gurram@intel.com
>> Cc: kamil.konieczny@linux.intel.com
>> Signed-off-by: Peter Senna Tschudin <peter.senna@linux.intel.com>
>> ---
>>   tests/intel/intel_sysfs_debugfs.c | 66 -------------------------------
>>   1 file changed, 66 deletions(-)
>>
>> diff --git a/tests/intel/intel_sysfs_debugfs.c b/tests/intel/intel_sysfs_debugfs.c
>> index 6beb94109..431934aee 100644
>> --- a/tests/intel/intel_sysfs_debugfs.c
>> +++ b/tests/intel/intel_sysfs_debugfs.c
>> @@ -322,63 +322,6 @@ xe_test_base(int fd, struct drm_xe_query_config *config)
>>   	xe_validate_entries(fd, "", expected_files, ARRAY_SIZE(expected_files));
>>   }
>>   
>> -/**
>> - * SUBTEST: xe-gt
>> - * Description: Check all gt debugfs devnodes
>> - * TODO: add support for ``force_reset`` entries
>> - */
>> -static void
>> -xe_test_gt(int fd, int gt_id)
>> -{
>> -	char name[256];
>> -	static const char * const expected_files[] = {
>> -		"uc",
>> -		"steering",
>> -		"topology",
>> -		"sa_info",
>> -		"hw_engines",
>> -		"pat",
>> -		"mocs",
>> -//		"force_reset"
>> -		"ggtt",
>> -		"register-save-restore",
>> -		"workarounds",
>> -		"default_lrc_rcs",
>> -		"default_lrc_ccs",
>> -		"default_lrc_bcs",
>> -		"default_lrc_vcs",
>> -		"default_lrc_vecs",
>> -		"hwconfig"
>> -
>> -	};
>> -	static const char * const expected_files_uc[] = {
>> -		"huc_info",
>> -		"guc_log",
>> -		"guc_info",
>> -//		"guc_ct_selftest"
>> -	};
>> -
>> -	for (int i = 0; i < ARRAY_SIZE(expected_files); i++) {
>> -		sprintf(name, "gt%d/%s", gt_id, expected_files[i]);
>> -		igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> -		if (igt_debugfs_is_dir(fd, expected_files[i], gt_id))
>> -			continue;
>> -		igt_debugfs_dump(fd, name);
>> -	}
>> -
>> -	for (int i = 0; i < ARRAY_SIZE(expected_files_uc); i++) {
>> -		sprintf(name, "gt%d/uc/%s", gt_id, expected_files_uc[i]);
>> -		igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> -		igt_debugfs_dump(fd, name);
>> -	}
>> -
>> -	sprintf(name, "/gt%d", gt_id);
>> -	xe_validate_entries(fd, name, expected_files, ARRAY_SIZE(expected_files));
>> -
>> -	sprintf(name, "/gt%d/uc", gt_id);
>> -	xe_validate_entries(fd, name, expected_files_uc, ARRAY_SIZE(expected_files_uc));
>> -}
>> -
>>   /**
>>    * SUBTEST: xe-forcewake
>>    * Description: Check forcewake debugfs devnode
>> @@ -475,15 +418,6 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
>>   			xe_test_base(fd, xe_config(fd));
>>   		}
>>   
>> -		igt_describe("Check all gt debugfs devnodes");
>> -		igt_subtest("xe-gt") {
>> -			xe_for_each_gt(fd, gt) {
>> -				snprintf(devnode, sizeof(devnode), "gt%d", gt);
>> -				igt_require(igt_debugfs_exists(fd, devnode, O_RDONLY));
>> -				xe_test_gt(fd, gt);
>> -			}
>> -		}
>> -
>>   		igt_describe("Check forcewake debugfs devnode");
>>   		igt_subtest("xe-forcewake") {
>>   			xe_test_forcewake(fd);
>> -- 
>> 2.43.0
>>