[igt-dev] [PATCH i-g-t 0/1] Add a script to document IGT tests

[igt-dev] [PATCH i-g-t 0/1] Add a script to document IGT tests

[Mauro Carvalho Chehab]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

As the IGT and driver's code evolves, the meaning of the tests may
become unclear. The best way to avoid bitrot is to have in-code
documentation explaining what each test and subtest does.

This script adds support for in-code documentation placed on
a style similar to kernel-doc, e. g. it should parse test descriptions
like:

/**
 * TEST: Check if new IGT test documentation logic functionality is working
 * Category: Software build block
 * Sub-category: documentation
 * Coverered functionality: test documentation
 * Test type: ReST generation
 * Run type: IGT kunit test
 * Issue: none
 * Platforms: all
 * Platform requirements: none
 * Depends on: @igt@deadbeef@basic
 * Requirements: Need at least a script to test it
 * Description: Complete description of this test
 *
 * SUBTEST: foo
 * Description: do foo things
 * 	with description continuing on another line
 *
 * SUBTEST: bar
 * Description: do bar things
 * 	with description continuing on another line
 */

/**
 * SUBTEST: test-%s-binds-%s-with-%ld-size
 * Description: Test arg[2] arg[1] binds with arg[3] size
 *
 * arg[1]:
 *
 * @large:	large
 * 		something
 * @mixed:	mixed
 * 		something
 * @small:	small
 * 		something
 *
 * arg[2]:
 *
 * @binds:			foo
 * @misaligned-binds:		misaligned
 * @userptr-binds:		userptr
 * @userptr-misaligned-binds:	userptr misaligned
 *
 * arg[3]:	buffer size
 * 		in Kb
 */

Please notice that I'm sending this script in advance, in order to
have some upstream review. Such script currently is capable of
describing igt tests and IGT subtests, with wildcard support.

It is not currently prepared to handle dynamic subtests, nor to
describe Kernel selftest/KUnit. For those, it is probably worth
to have the documentation inside the Kernel, to keep them as close
as possible to the actual code.

The end goal is to use such script to describe the test bench for
the new Xe driver, auto-generating documentation. The Xe IGT upstream
work can be seen at https://patchwork.freedesktop.org/series/112191/.

Mauro Carvalho Chehab (1):
  scripts: add a parser to produce documentation from Kernel C file
    metatags

 scripts/igt-doc | 647 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 647 insertions(+)
 create mode 100755 scripts/igt-doc

-- 
2.39.0

[igt-dev] [PATCH i-g-t 1/1] scripts: add a parser to produce documentation from Kernel C file metatags

[Mauro Carvalho Chehab]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

On a similar approach to Kernel's kernel-doc script, add a parser
that will produce documentation from special-purpose documentation
tags inside IGT tests.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
---
 scripts/igt-doc | 647 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 647 insertions(+)
 create mode 100755 scripts/igt-doc

diff --git a/scripts/igt-doc b/scripts/igt-doc
new file mode 100755
index 000000000000..3ed2be144e61
--- /dev/null
+++ b/scripts/igt-doc
@@ -0,0 +1,647 @@
+#!/usr/bin/env perl
+# SPDX-License-Identifier: GPL-2.0
+
+## Copyright (C) 2023    Intel Corporation                 ##
+## Author: Mauro Carvalho Chehab <mchehab@kernel.org>      ##
+## Inspired on Linux kernel-doc script                     ##
+
+use warnings;
+use strict;
+use Getopt::Long;
+use Pod::Usage qw/pod2usage/;
+
+my $igt_build_path = "build/";
+my $igt_runner = "/runner/igt_runner";
+
+# Fields that mat be inside TEST and SUBTEST macros
+my @fields = (
+	# Hardware building block / Software building block / ...
+	"Category",
+
+	# waitfence / dmabuf/ sysfs / debugfs / ...
+	"Sub-category",
+
+	# functionality test / pereformance / stress
+	"Test type",
+
+	# BAT / workarouds / developer-specific / ...
+	"Run type",
+
+	# Bug tracker issue(s)
+	"Issues?",
+
+	# all / DG1 / DG2 / TGL / MTL / PVC / ATS-M / ...
+	"Platforms?",
+
+	# Any other specific platform requirements
+	"Platform requirements?",
+
+	# some other IGT test, like igt@test@subtest
+	"Depends on",
+
+	# other non-platform requirements
+	"Requirements?",
+
+	# Description of the test
+	"Description",
+);
+
+my %doc;
+my $min_test_prefix;
+
+my $out = "";
+
+sub print_subtest($$$)
+{
+	my $fname = shift;
+	my $test = shift;
+	my $key = shift;
+
+	my $name = $fname;
+	$name =~ s,.*tests/,,;
+	$name =~ s/.[ch]$//;
+
+	my $test_name = "igt\@" . $name;
+
+	my $summary = $test_name . "@" . $doc{$test}{"subtest"}{$key}{"Summary"};
+
+	return if (!$summary);
+
+	my $num_vars = $summary =~ tr/%//;
+
+	# Trivial case: no variables at the subtest
+	if ($num_vars == 0) {
+		printf "\n\n$summary\n";
+		printf '=' x length($summary);
+		printf "\n\n";
+
+		foreach my $k (sort keys %{$doc{$test}{"subtest"}{$key}}) {
+			next if ($k eq "Summary");
+			next if ($k eq "arg");
+
+			next if ($doc{$test}{"subtest"}{$key}{$k} eq $doc{$test}{$k});
+
+			printf ":$k: %s\n", $doc{$test}{"subtest"}{$key}{$k};
+		}
+		print "\n";
+
+		return;
+	}
+
+	# Convert arguments into an array
+	my @arg_array;
+	my $arg_ref = $doc{$test}{"subtest"}{$key}{"arg"};
+
+	foreach my $n (keys %{$arg_ref}) {
+		next if ($n >= $num_vars);
+		foreach my $el (sort keys %{$arg_ref->{$n}}) {
+			push @{$arg_array[$n]}, $el;
+		}
+	}
+
+	my $size = scalar @arg_array;
+
+	if ($size < $num_vars) {
+		printf STDERR
+			"$fname: subtest '%s' subtest needs %d arguments but only %d are defined.\n",
+			$summary, $num_vars, $size;
+		exit 1;
+	}
+
+	for (my $j = 0; $j < $num_vars; $j++) {
+		if (!defined($arg_array[$j][0])) {
+			printf STDERR
+				"$fname: subtest '%s' needs arg[%d], but this is not defined.\n",
+				$summary, $j+1;
+			exit 1;
+		}
+	}
+
+	# convert numeric wildcards to string ones
+	$summary =~ s/%(d|ld|lld|i|u|lu|llu)/%s/;
+
+	my @pos = map { 0 } 1..$num_vars;
+	my @args;
+	my @arg_map;
+
+	while (1) {
+		for (my $j = 0; $j < $num_vars; $j++) {
+			my $a = $arg_array[$j][$pos[$j]];
+			$args[$j] = $a;
+
+			if (defined($arg_ref->{$j}{$a})) {
+				$arg_map[$j] = $arg_ref->{$j}{$a};
+			} else {
+				$arg_map[$j] = $a;
+			}
+			if ($arg_ref->{$j}{$a} =~ m/\<.*\>/) {
+				$args[$j] = "<$a>";
+			}
+		}
+
+		my $arg_summary = sprintf $summary, @args;
+
+		# Output the elements
+
+		printf "\n\n$arg_summary\n";
+		printf '=' x length($arg_summary);
+		printf "\n\n";
+
+		foreach my $k (sort keys %{$doc{$test}{"subtest"}{$key}}) {
+			next if ($k eq "Summary");
+			next if ($k eq "arg");
+
+
+			my $str = $doc{$test}{"subtest"}{$key}{$k};
+
+			$str =~ s/\%?\barg\[(\d+)\]/$arg_map[$1 - 1]/g;
+
+			next if ($str eq $doc{$test}{$k});
+
+			printf ":$k: %s\n", $str;
+		}
+		print "\n";
+
+		# Increment variable inside the array
+
+		my $p = 0;
+		while (!defined(@{$arg_array[$p]}[$pos[$p] + 1])) {
+			$pos[$p] = 0;
+			$p++;
+		    last if ($p >= $num_vars);
+		}
+		last if ($p >= $num_vars);
+		$pos[$p]++;
+	}
+}
+
+sub print_test()
+{
+	foreach my $test (sort {$a <=> $b} keys %doc) {
+		my $fname = $doc{$test}{"File"};
+		my $name = $fname;
+
+		$name =~ s,.*tests/,,;
+		$name =~ s/.[ch]$//;
+
+		my $test_name = "igt\@" . $name;
+
+
+		print '=' x length($test_name);
+		print "\n$test_name\n";
+		print '=' x length($test_name);
+		print "\n\n";
+
+		foreach my $k (sort keys %{$doc{$test}}) {
+			next if ($k eq "subtest");
+
+			printf ":$k: %s\n", $doc{$test}{$k};
+		}
+
+		foreach my $key (sort {$a <=> $b} keys %{$doc{$test}{"subtest"}}) {
+			print_subtest($fname, $test, $key);
+		}
+	}
+}
+
+sub get_subtests()
+{
+	my @subtests;
+
+	foreach my $test (keys %doc) {
+		my $fname = $doc{$test}{"File"};
+		my $name = $fname;
+
+		$name =~ s,.*tests/,,;
+		$name =~ s/.[ch]$//;
+
+		my $test_name = "igt\@" . $name;
+
+		foreach my $key (keys %{$doc{$test}{"subtest"}}) {
+			my $summary = $test_name . "@" . $doc{$test}{"subtest"}{$key}{"Summary"};
+
+			next if (!$summary);
+
+			my $num_vars = $summary =~ tr/%//;
+
+			# Trivial case: no variables at the subtest
+			if ($num_vars == 0) {
+				push @subtests, $summary;
+			} else {
+				# Convert arguments into an array
+				my @arg_array;
+				my $arg_ref = $doc{$test}{"subtest"}{$key}{"arg"};
+
+				foreach my $n (keys %{$arg_ref}) {
+					next if ($n >= $num_vars);
+					foreach my $el (sort keys %{$arg_ref->{$n}}) {
+						push @{$arg_array[$n]}, $el;
+					}
+				}
+
+				my $size = scalar @arg_array;
+
+				if ($size < $num_vars) {
+					printf STDERR
+					       "$fname: subtest '%s' subtest needs %d arguments but only %d are defined.\n",
+					       $summary, $num_vars, $size;
+					exit 1;
+				}
+
+				for (my $j = 0; $j < $num_vars; $j++) {
+					if (!defined($arg_array[$j][0])) {
+						printf STDERR
+						       "$fname: subtest '%s' needs arg[%d], but this is not defined.\n",
+						       $summary, $j+1;
+						exit 1;
+					}
+				}
+
+				# convert numeric wildcards to string ones
+				$summary =~ s/%(d|ld|lld|i|u|lu|llu)/%s/;
+				my @pos = map { 0 } 1..$num_vars;
+				my @args;
+
+				while (1) {
+					for (my $j = 0; $j < $num_vars; $j++) {
+						my $a = $arg_array[$j][$pos[$j]];
+						$args[$j] = $a;
+						if ($arg_ref->{$j}{$a} =~ m/\<.*\>/) {
+							$args[$j] = "<$a>";
+						}
+					}
+
+					my $arg_summary = sprintf $summary, @args;
+
+					push @subtests, $arg_summary;
+
+					# Increment variable inside the array
+
+					my $p = 0;
+					while (!defined(@{$arg_array[$p]}[$pos[$p] + 1])) {
+						$pos[$p] = 0;
+						$p++;
+					    last if ($p >= $num_vars);
+					}
+					last if ($p >= $num_vars);
+					$pos[$p]++;
+				}
+			}
+		}
+	}
+
+	return @subtests;
+}
+
+sub check_testlist()
+{
+	my @doc_subtests = get_subtests();
+
+	# Get a list of tests from
+	my @run_subtests;
+
+	open TESTLIST, "$igt_build_path/$igt_runner -L -t '$min_test_prefix' $igt_build_path/tests |" or die "Can't run igt_runner";
+	while (<TESTLIST>) {
+		s/\n//;
+		push @run_subtests, $_;
+	}
+	close TESTLIST;
+
+	# Compare arrays
+
+	my @run_missing;
+	my @doc_uneeded;
+
+	foreach my $t1 (@doc_subtests) {
+		my $re = $t1;
+		$re =~ s,\<[^\>]+\>,\\d+,g;
+
+		my $match = 0;
+		foreach my $t2 (sort @run_subtests) {
+			if ($t2 =~ m/^$re$/) {
+				$match = 1;
+			}
+		}
+		push @doc_uneeded, $t1 if (!$match);
+	}
+
+	foreach my $t2 (sort @run_subtests) {
+		my $found = 0;
+		foreach my $t1 (@doc_subtests) {
+			my $re = $t1;
+			$re =~ s,\<[^\>]+\>,\\d+,g;
+			if ($t2 =~ m/^$re$/) {
+				$found = 1;
+			}
+		}
+		push @run_missing, $t2 if (!$found);
+	}
+
+	if (@doc_uneeded) {
+		printf "Unused documentation:\n";
+		foreach my $t (@doc_uneeded) {
+			print "\t$t\n";
+		}
+	}
+
+	if (@run_missing) {
+		printf "Missing documentation:\n";
+		foreach my $t (@run_missing) {
+			print "\t$t\n";
+		}
+	}
+}
+
+
+
+#
+# Main: parse the files and fill %doc struct
+#
+
+my $rest;
+my $show_subtests;
+my $check_testlist;
+my $help;
+my $man;
+
+GetOptions(
+	"show-subtests" => \$show_subtests,
+	"igt-build-path" => \$igt_build_path,
+	"check-testlist" => \$check_testlist,
+	"rest|rst" => \$rest,
+	'help|?' => \$help,
+	man => \$man
+) or pod2usage(2);
+
+pod2usage(1) if $help;
+pod2usage(-exitstatus => 0, -verbose => 2) if $man;
+
+my $current_test;
+my $current_subtest;
+
+my $handle_section = "";
+my $current_field = "";
+my $cur_arg = -1;
+my $cur_arg_element = 0;
+my $test_number = 0;
+
+my  $field_re = join '|', @fields;
+
+while ($ARGV[0]) {
+	my $fname = $ARGV[0];
+	shift @ARGV;
+
+	# Dynamically build a test prefix from the file name
+
+	my $prefix = $fname;
+
+	$prefix =~ s,.*tests/,,;
+	$prefix =~ s,(.*/).*,$1,;
+	$prefix = "igt\@" . $prefix;
+
+	if (!$min_test_prefix) {
+		$min_test_prefix = $prefix;
+	} elsif (length($prefix) < length($min_test_prefix)) {
+		$min_test_prefix = $prefix;
+	}
+
+	open IN, $fname or die "Can't open $fname";
+
+	my $arg_ref;
+
+	$current_test = "";
+	my $subtest_number = 0;
+
+	while (<IN>) {
+		my $ln = $_;
+
+		next if ($ln =~ m/^\s*\*$/);
+
+		if ($ln =~ m,^\s*\*/$,) {
+			$handle_section = "";
+			undef($current_subtest);
+			undef($arg_ref);
+			$cur_arg = -1;
+
+			next;
+		}
+
+		if ($ln =~ m,^\s*/\*\*$,) {
+			$handle_section = "1";
+			next;
+		}
+
+		next if (!$handle_section);
+
+		$ln =~ s/^\s*\*\s*//;
+
+		# Handle only known sections
+		if ($handle_section eq "1") {
+			$current_field = "";
+			if ($ln =~ m/^TEST:\s*(.*)/) {
+				$current_test = $test_number;
+				$handle_section = "test";
+				$test_number++;
+
+				$doc{$current_test} = ();
+				$doc{$current_test}{"Summary"} = $1;
+				$doc{$current_test}{"File"} = $fname;
+				undef($current_subtest);
+
+				next;
+			}
+		}
+
+		if ($ln =~ m/^SUBTESTS?:\s*(.*)/) {
+			$current_field = "";
+			$handle_section = "subtest";
+			$current_subtest = $subtest_number++;
+
+			$doc{$current_test}{"subtest"}{$current_subtest} = ();
+
+			$doc{$current_test}{"subtest"}{$current_subtest}{"Summary"} = $1;
+			$doc{$current_test}{"subtest"}{$current_subtest}{"Description"} = "";
+
+			if (!defined($arg_ref)) {
+				my %new_arg = ();
+
+				$arg_ref = \%new_arg;
+			}
+			$doc{$current_test}{"subtest"}{$current_subtest}{"arg"} = $arg_ref;
+		}
+
+		# It is a known section. Parse its contents
+
+		if ($ln =~ m/($field_re):\s*(.*)/i) {
+			$current_field = lc $1;
+			my $v = $2;
+
+			$current_field =~ s/^([a-z])/\U$1/;
+			if ($handle_section eq "test") {
+				$doc{$current_test}{$current_field} = $v;
+			} else {
+				$doc{$current_test}{"subtest"}{$current_subtest}{$current_field} = $v;
+			}
+
+			$cur_arg = -1;
+			next;
+		}
+
+		# Use hashes for arguments to avoid duplication
+		if ($ln =~ m/arg\[(\d+)\]:\s*(.*)/) {
+			$current_field = "";
+			if (!defined($arg_ref)) {
+				die "$fname:$.: arguments should be defined after one or more subtests, at the same comment\n";
+			}
+
+			$cur_arg = $1 - 1;
+			$cur_arg_element = $2;
+
+			# Should be used only for numeric values
+			$arg_ref->{$cur_arg}{$cur_arg_element} = "<$2>" if ($2);
+			next;
+		}
+		if ($cur_arg >= 0 && $ln =~ m/\@(\S+):\s*(.*)/) {
+			$current_field = "";
+			if (!defined($arg_ref)) {
+				die "$fname:$.: arguments should be defined after one or more subtests, at the same comment\n";
+			}
+
+			$cur_arg_element = $1;
+			$arg_ref->{$cur_arg}{$cur_arg_element} = $2;
+			next;
+		}
+		if ($ln =~ m/\@(\S+):\s*(.*)/) {
+			$current_field = "";
+			if (!defined($arg_ref)) {
+				die "$fname:$.: arguments should be defined after one or more subtests, at the same comment\n";
+			}
+
+			printf "$fname:$.: Warning: invalid argument: \@$1: $2\n";
+		}
+
+		# Handle multi-line field contents
+		if ($current_field) {
+			if (m/\s*\*?\s*(.*)/) {
+				if ($handle_section eq "test") {
+					$doc{$current_test}{$current_field} .= " $1";
+				} else {
+					$doc{$current_test}{"subtest"}{$current_subtest}{$current_field} .= " $1";
+				}
+			}
+		}
+
+		# Handle multi-line argument contents
+		if ($cur_arg >= 0) {
+			if (m/\s*\*?\s*(.*)/) {
+				my $v = $1;
+				if ($arg_ref->{$cur_arg}{$cur_arg_element} =~ m/(.*)>$/) {
+					$arg_ref->{$cur_arg}{$cur_arg_element} = "$1 $v>";
+				} else {
+					$arg_ref->{$cur_arg}{$cur_arg_element} .= " $v";
+				}
+			}
+		}
+	}
+	close IN;
+}
+
+# Now all files were read, do some output
+my $run = 0;
+
+if ($show_subtests) {
+	$run = 1;
+	my @subtests = get_subtests();
+
+	for my $subtest (sort @subtests) {
+		print "$subtest\n";
+	}
+}
+
+if ($check_testlist) {
+	$run = 1;
+
+	check_testlist;
+}
+
+if (!$run || $rest) {
+	print_test;
+}
+
+# Script documentation
+
+__END__
+
+=head1 NAME
+
+igt-doc - Print formatted kernel documentation to stdout
+
+=head1 SYNOPSIS
+
+ igt-doc [options] FILES
+
+ Where [options] can be:
+
+	--rest			- Produces documentation in ReST format
+	--show-subtests		- Shows a list of documented subtests
+	--check-testlist	- Audit if the documentation is aligned with
+				  the sources
+	--man			- Shows a man-style page
+	--help			- Shows help
+
+=head1 DESCRIPTION
+
+This script looks for special documentation markups inside the IGT
+source code and produces documents in ReST format from them.
+
+=head1 OPTIONS
+
+=over 8
+
+=item B<--rest>
+
+Generate documentation from the source files, in ReST file format.
+
+Called by default if no other output mode parameter is used.
+
+=item B<--show-subtests>
+
+Shows the name of the documented subtests in alphabetical order.
+
+=item B<--igt-build-path>
+
+Path where the IGT runner is sitting. Used by B<--check-testlist>.
+
+Default: build/
+
+=item B<--check-testlist>
+
+Check the documentation against the testlist as identified by IGT runner,
+identifying documentation gaps.
+
+=item B<--help>
+
+Displays a help message.
+
+=item B<--man>
+
+Displays a man-style message.
+
+=back
+
+=head1 BUGS
+
+Report bugs to Mauro Carvalho Chehab <mchehab@kernel.org>
+
+=head1 COPYRIGHT
+
+Authored 2023 by Mauro Carvalho Chehab <mchehab@kernel.org>
+
+Copyright (c) 2023 Intel Corporation
+
+License GPLv2: GNU GPL version 2 <http://gnu.org/licenses/gpl.html>.
+
+This is free software: you are free to change and redistribute it.
+There is NO WARRANTY, to the extent permitted by law.
+
+=cut
-- 
2.39.0

Re: [igt-dev] [PATCH i-g-t 1/1] scripts: add a parser to produce documentation from Kernel C file metatags

[Mauro Carvalho Chehab]

On Fri, 3 Feb 2023 12:16:27 +0200
Petri Latvala <adrinael@adrinael.net> wrote:

> On Fri, Feb 03, 2023 at 09:26:50AM +0100, Mauro Carvalho Chehab wrote:
> > From: Mauro Carvalho Chehab <mchehab@kernel.org>
> > 
> > On a similar approach to Kernel's kernel-doc script, add a parser
> > that will produce documentation from special-purpose documentation
> > tags inside IGT tests.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> > ---
> >  scripts/igt-doc | 647 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 647 insertions(+)
> >  create mode 100755 scripts/igt-doc
> > 
> > diff --git a/scripts/igt-doc b/scripts/igt-doc
> > new file mode 100755
> > index 000000000000..3ed2be144e61
> > --- /dev/null
> > +++ b/scripts/igt-doc
> > @@ -0,0 +1,647 @@
> > +#!/usr/bin/env perl  
> 
> 
> When you talked about this beforehand, you talked about using
> Python. Does this need to be a perl script? I mean, it's from kernel,
> I understand that, but I'll be completely unable to review perl code
> myself =(

No, I didn't talk about writing it in Python. There are a couple of
reasons why I opted to use Perl:

1. kernel-doc script is in perl too. It probably makes sense to also
   add support the Kernel itself for test-related tags, in order to be
   able to document KUnit tests. By using the same language, it is 
   easier to place automation there too;

2. We have already perl scripts inside IGT (see code_cov_parse_info);

3. Perl APIs are *a lot* more stable than Python; scripts written
   a long time ago still runs OK without changes. Python, on the other
   hand, keeps deprecating their APIs or changing them on non-compatible
   ways from time to time, requiring more maintainance efforts just due
   to its API changes.

4. I believe that each programmer should pick its own poison, writing
   stuff using the environments they're more comfortable with. 
   I'm not a Python programmer. I can probably review and write basic
   changes on it, but writing something new there, using dictionaries
   on an optimized code [1] is something that it would require a lot
   of time from my side.

[1] this script takes 100ms to parse and convert a set of tags for all
    Xe IGT tags into ReST. On the (few) attempts I did to write similar
    scripts, Python performed a lot worse.

Regards,
Mauro

Re: [igt-dev] [PATCH i-g-t 1/1] scripts: add a parser to produce documentation from Kernel C file metatags

[Mauro Carvalho Chehab]

On Fri, 03 Feb 2023 12:23:52 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Fri, 03 Feb 2023, Mauro Carvalho Chehab <mauro.chehab@linux.intel.com> wrote:
> > From: Mauro Carvalho Chehab <mchehab@kernel.org>
> >
> > On a similar approach to Kernel's kernel-doc script, add a parser
> > that will produce documentation from special-purpose documentation
> > tags inside IGT tests.  
> 
> kernel-doc is really not a great model to duplicate elsewhere.

Why not? Kernel has been succeed keeping its interfaces documented. Ok,
there would be other alternatives to be used, but the main point is really
to keep the documentation as close as possible to the source code, in a
parseable way. 

> Why hand craft another C parser? There's a cost to maintaining this, and
> it's not helped by the choice of implementation language.

There is always a cost of maintaining documentation, and a cost of not
having them.

One of the biggest costs we have is related to the time for CI to run
IGT tests. The more tests we add, the higher the costs increase. That's
by far the most relevant cost here.

Right now, there are tests written several years ago, that people don't
have a clear idea why they're doing, and if they should belong to a
basic acceptance criteria or if some other tests are already covering
the functionality of them. So, multiple tests for the same feature could
end being running as part of the test plans, increasing the costs of
running such tests (energy costs, machine costs, etc).

Having test plans documenting what's inside each test file allows to
decide what of those tests make sense to run for each new patch added
to the tree, and what of those are there just to cover some borderline
cases and won't need to be run every time.

> The script alone isn't enough; how do you bolt this into other
> documentation? More ad hoc scripting like
> docs/reference/igt-gpu-tools/{generate_description_xml.py,generate_programs_xml.sh}?

No need. This script could be called to produce, let's say, Xe documentation by adding
a target at tests/xe/meson.build that would do something similar to:

	${SRCDIR}/scripts/igt-doc ${SRCDIR}/tests/xe/*.c > ${DESTDIR}/docs/xe_tests.rst

And CI would be running:

	${SRCDIR}/scripts/igt-doc ${SRCDIR}/tests/xe/*.c --check

In order to verify if all the tests inside a given directory (in this 
example, tests/xe) are documented.

With such automation in place, if someone writes a new test or subtest
and forgets to update the test description, CI could return warning
messages, asking the developer to fill in the gaps.

Regards,
Mauro

Re: [igt-dev] [PATCH i-g-t 1/1] scripts: add a parser to produce documentation from Kernel C file metatags

[Mauro Carvalho Chehab]

On Fri, 3 Feb 2023 11:36:39 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> On Fri, 3 Feb 2023 at 11:24, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >
> > On Fri, 03 Feb 2023, Mauro Carvalho Chehab <mauro.chehab@linux.intel.com> wrote:  
> > > From: Mauro Carvalho Chehab <mchehab@kernel.org>
> > >
> > > On a similar approach to Kernel's kernel-doc script, add a parser
> > > that will produce documentation from special-purpose documentation
> > > tags inside IGT tests.  
> >
> > kernel-doc is really not a great model to duplicate elsewhere.  
> 
> Yes.
> 
> Like just "yes".
> 
> Also quite frankly I think a large chunk of the problem many igts have
> (especially in the i915-gem area) is not the lacking documentation.

The goal is not to to change anything at the i915 documentation (or the
lack of), but, instead, to provide an infrastructure to have clear
test documentation and test plans for newer driver. We're basically
aiming on having test documentation for Xe, keeping it updated as the
tests and the driver changes.

In the specific case of the curent tests, there is a lack of proper
documentation in iGT. Right now we have around 62K tests on iGT upstream:

$ ./build/runner/igt_runner -L build/tests/|wc -l
62773

So far I was unable to identify any document describing what each test does,
on what platforms the tests run, etc. I bet not many people know what those
~62K tests on IGT actually does, and how many of them are ok to be removed,
covers some border cases on very platforms or had their scope replaced by
some other test.

The goal here is to have a tool that will help keeping the tests and their
respective documentation in a sane state.

> -Daniel
> 
> > Why hand craft another C parser? There's a cost to maintaining this, and
> > it's not helped by the choice of implementation language.
> >
> > The script alone isn't enough; how do you bolt this into other
> > documentation? More ad hoc scripting like
> > docs/reference/igt-gpu-tools/{generate_description_xml.py,generate_programs_xml.sh}?
> >  
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > ---
> > >  scripts/igt-doc | 647 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 647 insertions(+)
> > >  create mode 100755 scripts/igt-doc
> > >
> > > diff --git a/scripts/igt-doc b/scripts/igt-doc
> > > new file mode 100755
> > > index 000000000000..3ed2be144e61
> > > --- /dev/null
> > > +++ b/scripts/igt-doc
> > > @@ -0,0 +1,647 @@
> > > +#!/usr/bin/env perl
> > > +# SPDX-License-Identifier: GPL-2.0  
> >
> > All of IGT is MIT, except the headers copied from kernel. Is this
> > GPL-2.0 because there's so much kernel-doc in it...
> >  
> > > +## Copyright (C) 2023    Intel Corporation                 ##
> > > +## Author: Mauro Carvalho Chehab <mchehab@kernel.org>      ##
> > > +## Inspired on Linux kernel-doc script                     ##  
> >
> > ...
> >  
> > > +igt-doc - Print formatted kernel documentation to stdout  
> >                              ^^^^^^
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center  
> 
> 
> 

Re: [igt-dev] [PATCH i-g-t 1/1] scripts: add a parser to produce documentation from Kernel C file metatags

[Mauro Carvalho Chehab]

On Fri, 03 Feb 2023 15:24:41 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Fri, 03 Feb 2023, Mauro Carvalho Chehab <mauro.chehab@linux.intel.com> wrote:
> > On Fri, 3 Feb 2023 12:16:27 +0200
> > Petri Latvala <adrinael@adrinael.net> wrote:
> >  
> >> On Fri, Feb 03, 2023 at 09:26:50AM +0100, Mauro Carvalho Chehab wrote:  
> >> > From: Mauro Carvalho Chehab <mchehab@kernel.org>
> >> > 
> >> > On a similar approach to Kernel's kernel-doc script, add a parser
> >> > that will produce documentation from special-purpose documentation
> >> > tags inside IGT tests.
> >> > 
> >> > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> >> > ---
> >> >  scripts/igt-doc | 647 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 647 insertions(+)
> >> >  create mode 100755 scripts/igt-doc
> >> > 
> >> > diff --git a/scripts/igt-doc b/scripts/igt-doc
> >> > new file mode 100755
> >> > index 000000000000..3ed2be144e61
> >> > --- /dev/null
> >> > +++ b/scripts/igt-doc
> >> > @@ -0,0 +1,647 @@
> >> > +#!/usr/bin/env perl    
> >> 
> >> 
> >> When you talked about this beforehand, you talked about using
> >> Python. Does this need to be a perl script? I mean, it's from kernel,
> >> I understand that, but I'll be completely unable to review perl code
> >> myself =(  
> >
> > No, I didn't talk about writing it in Python. There are a couple of
> > reasons why I opted to use Perl:
> >
> > 1. kernel-doc script is in perl too. It probably makes sense to also
> >    add support the Kernel itself for test-related tags, in order to be
> >    able to document KUnit tests. By using the same language, it is 
> >    easier to place automation there too;  
> 
> I don't think kernel-doc the script is a good argument here. On the
> contrary, it should be an example of what not to do. It only exists as a
> perl script because of legacy, and nothing else.
> 
> > 2. We have already perl scripts inside IGT (see code_cov_parse_info);  
> 
> Which you added, and nobody else has touched since. Just saying.

So what? The same applies to almost all Python scripts there as well:

	$ for i in $(find . -name *.py); do echo -n "$i author(s):"; echo $(git log --pretty="%an" $i|sort|uniq); done
	./.gitlab-ci/list_undocumented_tests.py author(s):Arkadiusz Hiler
	./lib/i915/shaders/converter.py author(s):Katarzyna Dec
	./lib/i915/perf-configs/perf-equations-codegen.py author(s):Lionel Landwerlin
	./lib/i915/perf-configs/oa_guid_registry.py author(s):Lionel Landwerlin
	./lib/i915/perf-configs/codegen.py author(s):Arkadiusz Hiler Lionel Landwerlin
	./lib/i915/perf-configs/update-guids.py author(s):Lionel Landwerlin
	./lib/i915/perf-configs/mdapi-xml-convert.py author(s):Lionel Landwerlin
	./lib/i915/perf-configs/perf-registers-codegen.py author(s):Lionel Landwerlin
	./lib/i915/perf-configs/perf-metricset-codegen.py author(s):Lionel Landwerlin
	./docs/reference/igt-gpu-tools/generate_description_xml.py author(s):Arkadiusz Hiler
	./scripts/quick-testlist.py author(s):Thomas Wood
	./scripts/convert_itp.py author(s):Ben Widawsky
	./scripts/throttle.py author(s):Chris Wilson

The script had reviewers, so I'm not the only one working with perl.

> > 3. Perl APIs are *a lot* more stable than Python; scripts written
> >    a long time ago still runs OK without changes. Python, on the other
> >    hand, keeps deprecating their APIs or changing them on non-compatible
> >    ways from time to time, requiring more maintainance efforts just due
> >    to its API changes.
> >
> > 4. I believe that each programmer should pick its own poison, writing
> >    stuff using the environments they're more comfortable with. 
> >    I'm not a Python programmer. I can probably review and write basic
> >    changes on it, but writing something new there, using dictionaries
> >    on an optimized code [1] is something that it would require a lot
> >    of time from my side.  
> 
> The above holds if you're working alone in a silo. IGT is above all a
> team effort, and you need to consider the overall productivity of the
> team, and the team's ability to maintain the code. Who's going to step
> up if you're not there to do it? Are they going to have to rewrite the
> whole thing in some other language?

Frankly, I don't mind if anyone wants to write it on any other language,
or use some already-existing tool.

We're diverting from the issue: we need to have proper documentation
and a way to validate if the documentation is outdated.

That is the scope of this patch: to add a way to do that. If you have
a better solution, feel free to send the patches.

Regards,
Mauro

Re: [igt-dev] [PATCH i-g-t 1/1] scripts: add a parser to produce documentation from Kernel C file metatags

[Mauro Carvalho Chehab]

On Fri, 03 Feb 2023 15:29:49 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> >> The script alone isn't enough; how do you bolt this into other
> >> documentation? More ad hoc scripting like
> >> docs/reference/igt-gpu-tools/{generate_description_xml.py,generate_programs_xml.sh}?  
> >
> > No need. This script could be called to produce, let's say, Xe documentation by adding
> > a target at tests/xe/meson.build that would do something similar to:
> >
> > 	${SRCDIR}/scripts/igt-doc ${SRCDIR}/tests/xe/*.c > ${DESTDIR}/docs/xe_tests.rst
> >
> > And CI would be running:
> >
> > 	${SRCDIR}/scripts/igt-doc ${SRCDIR}/tests/xe/*.c --check  
> 
> I think we have different ideas of what it means to *nicely* integrate

No matter what solution used, some rules are needed at meson.build to
produce documentation. One of the requirements is to have a way to validate
if the documents are updated.

So, it needs to be something that:

1. it is easy to be parsed;
2. it is easy for developers to write;
3. it is embedded at the C file that contains the tests.

Anything different than that will lead into the documentation gaps
that we currently have.

See, if you pick a couple of random IGT test files:

	$ for i in $(seq 1 10); do find tests/ -name "*.c" | shuf -n 1; done

I doubt you'll be able to quickly discover details on what each
test does (what the test does, what platforms are supported, what it is
required to run them, etc), even if you write a script to parse it.
You'll probably spend some time to find the documentation, and that's if 
they're available and updated.

By having comments inside the code like what it is proposed on this
patch, you can get them with a simple command:

	$ grep -A10 -E "\* TEST\b" $(find tests/ -name "*.c")


Assuming that we place the documentation embedded in a C file, it
means that meson.build will require some addition any way by running
some executable. As meson was built exactly with the purpose of
executing external programs (mesa, gcc, etc), calling an external
program seems *nicely integrated* to me.

Regards,
Mauro