[PATCH v1] tests/xe_debugfs: Improve test_gt subtest

[Lucas De Marchi]

On Thu, Jan 09, 2025 at 05:42:37PM +0000, Gurram, Pravalika wrote:
>
>
>> -----Original Message-----
>> From: De Marchi, Lucas <lucas.demarchi at intel.com>
>> Sent: Thursday, January 9, 2025 10:33 PM
>> To: Gurram, Pravalika <pravalika.gurram at intel.com>
>> Cc: igt-dev at lists.freedesktop.org
>> Subject: Re: [PATCH v1] tests/xe_debugfs: Improve test_gt subtest
>>
>> On Thu, Jan 09, 2025 at 04:27:21PM +0530, Pravalika Gurram wrote:
>> >Read the debugfs entries in the loop to improve the readability.
>> >
>> >Signed-off-by: Pravalika Gurram <pravalika.gurram at intel.com>
>> >---
>> > tests/intel/xe_debugfs.c | 84 +++++++---------------------------------
>> > 1 file changed, 13 insertions(+), 71 deletions(-)
>> >
>> >diff --git a/tests/intel/xe_debugfs.c b/tests/intel/xe_debugfs.c index
>> >bcbb5036a..3d8c17921 100644
>> >--- a/tests/intel/xe_debugfs.c
>> >+++ b/tests/intel/xe_debugfs.c
>> >@@ -180,77 +180,19 @@ test_gt(int fd, int gt_id)
>> > //		"guc_ct_selftest"
>> > 	};
>> >
>> >-	sprintf(name, "gt%d/hw_engines", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/sa_info", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/steering", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/topology", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/pat", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/mocs", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/ggtt", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/register-save-restore", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/workarounds", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/default_lrc_rcs", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/default_lrc_ccs", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/default_lrc_bcs", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/default_lrc_vecs", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/default_lrc_vcs", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/hwconfig", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/uc/guc_info", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/uc/huc_info", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >-
>> >-	sprintf(name, "gt%d/uc/guc_log", gt_id);
>> >-	igt_assert(igt_debugfs_exists(fd, name, O_RDONLY));
>> >-	igt_debugfs_dump(fd, name);
>> >+	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 (!strstr(name, "uc")) {
>>
>> strstr will find any substring "uc". You could do a strcmp() instead, but I don't
>> understand why we are doing this here.
>>
>> I'd just remove 'uc' from the array and update validate_entries to ignore
>> directories.
>>
>> Lucas De Marchi
>>
>
>Strcmp will not work here because sprintf will save  name = "gt0/uc"

if (strcmp(expected_files[i], "uc"))

>If we do strcmp(name,"gt0/uc") gt cannot be fixed 0  always correct it can change
>that's why I have added strstr

suppose we have a new file called chuck or touch or truck or truce or
yucks or.... They would all fall into the else condition which is not what we
want.

>
>I did not understand which directories can be ignored you meant say "uc"
>we don't need to dump "uc/*" data?

any dir. In that loop, just skip any check for 
de->d_type == DT_DIR

Lucas De Marchi

>
>--Pravalika
>> >+			igt_debugfs_dump(fd, name);
>> >+		} else {
>> >+			for (int j = 0; j < ARRAY_SIZE(expected_files_uc); j++)
>> {
>> >+				sprintf(name, "gt%d/uc/%s", gt_id,
>> expected_files_uc[j]);
>> >+				igt_assert(igt_debugfs_exists(fd, name,
>> O_RDONLY));
>> >+				igt_debugfs_dump(fd, name);
>> >+			}
>> >+		}
>> >+	}
>> >
>> > 	sprintf(name, "/gt%d", gt_id);
>> > 	validate_entries(fd, name, expected_files,
>> >ARRAY_SIZE(expected_files));
>> >--
>> >2.34.1
>> >