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

[Kamil Konieczny]

Hi,
On 2025-04-11 at 10:01:12 +0200, Bernatowicz, Marcin wrote:
> 
> 
> 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.

Good point.

> 
> 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?).
> 

I see that debugfs has two subtests for display-off and display-on
so maybe introduce something like this?
Also imho existance checks needs to be done separately for sysfs
as in sysfs they are user api. Debugfs should be considered unstable
so I am ok with removing this now but please work on improving
coverage.

Acked-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>

Regards,
Kamil

> --
> 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 at intel.com
> > > Cc: matthew.brost at intel.com
> > > Cc: pravalika.gurram at intel.com
> > > Cc: kamil.konieczny at linux.intel.com
> > > Signed-off-by: Peter Senna Tschudin <peter.senna at 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
> > > 
>