[Mesa-dev] [PATCH] list-workarounds: Extend the script to Mesa

[Damien Lespiau]

On Thu, Feb 04, 2016 at 06:14:02PM +0000, Kibey, Sameer wrote:
> Updated the list-workarounds script so that it
> can parse Mesa directory if provided. Moved the
> common code to a separate function to allow
> reuse for both kernel and mesa.
> 
> The new command line is:
> Usage: list-workarounds [options] path-to-kernel
>        -k path-to-kernel -m path-to-mesa
> 
> The legacy usage is retained to avoid breaking
> backwards compatibility. New parameters -k and
> -m are added for the new behavior.
> 
> Either kernel or mesa or both paths can be specified.
> If path-to-mesa is invalid, error is reported.
> 
> Signed-off-by: Sameer Kibey <sameer.kibey at intel.com>

Out of curiosity, how did you send the email? It doesn't seem to have
been sent with git send-email and so the patch isn't picked up by our
patchwork instance.

Out of the comments below, I guess the only serious one is allowing both
byt/vlv, but maybe mesa only uses one of the two? I wouldn't mind
landing the patch with that answered.

> ---
>  scripts/list-workarounds | 75 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 54 insertions(+), 21 deletions(-)
> 
> diff --git a/scripts/list-workarounds b/scripts/list-workarounds
> index d11b6a9..0b63541 100755
> --- a/scripts/list-workarounds
> +++ b/scripts/list-workarounds
> @@ -18,7 +18,7 @@ def find_nth(haystack, needle, n):
>  	return start
>  
>  valid_platforms = ('ctg', 'elk', 'ilk', 'snb', 'ivb', 'vlv', 'hsw', 'bdw',
> -		   'chv', 'skl', 'bxt')
> +		   'chv', 'skl', 'bxt', 'kbl', 'byt')

Do we really need both byt and vlv? that creates two different names for
the same platform, which sounds like a recipe to have the actual set of
W/As for this platform be the union of vlv and byt ones.

>  def parse_platforms(line, p):
>  	l =  p.split(',')
>  	for p in l:
> @@ -65,9 +65,15 @@ def execute(cmd):
>  	return out, err
>  
>  def parse_options(args):
> -	usage = "Usage: list-workarounds [options] path-to-kernel"
> +	usage = "Usage: list-workarounds [options] path-to-kernel -k path-to-kernel -m path-to-mesa"
>  	parser = optparse.OptionParser(usage, version=1.0)

Quite frankly, I'd just remove the old behaviour.

> +	parser.add_option("-k", "--kernel-path", dest="kernel_path", default=None,
> +			  help="path to kernel")
> +
> +	parser.add_option("-m", "--mesa-path", dest="mesa_path", default=None,
> +			  help="path to mesa")
> +
>  	parser.add_option("-v", "--verbose", action="store_true",
>  			  dest="verbose", default=False,
>  			  help="be more verbose")
> @@ -76,30 +82,14 @@ def parse_options(args):
>  			  help="List workarounds for the specified platform")
>  
>  	(options, args) = parser.parse_args()
> -
>  	return (options, args)
>  
> -if __name__ == '__main__':
> -	(options, args) = parse_options(sys.argv[1:])
> -	verbose = options.verbose
> -
> -	if not len(args):
> -		sys.stderr.write("error: A path to a kernel tree is required\n")
> -		sys.exit(1)
> -
> -	kernel_path = args[0]
> -	kconfig = os.path.join(kernel_path, 'Kconfig')
> -	if not os.path.isfile(kconfig):
> -		sys.stderr.write("error: %s does not point to a kernel tree \n"
> -				 % kernel_path)
> -		sys.exit(1)
> -
> -	i915_dir = os.path.join('drivers', 'gpu', 'drm', 'i915')
> +def print_workarounds(code_path, driver_dir):
>  	olddir = os.getcwd()
> -	os.chdir(kernel_path)
> +	os.chdir(code_path)

project_root?

>  	work_arounds, err = execute(['git', 'grep', '-n',
>  				     '-e', 'W[aA][A-Z0-9][a-zA-Z0-9_]\+',
> -				     i915_dir])
> +				     driver_dir])
>  	os.chdir(olddir)
>  	if err:
>  		print(err)
> @@ -111,3 +101,46 @@ if __name__ == '__main__':
>  			print("%s: %s" % (wa, ', '.join(workarounds[wa])))
>  		elif options.platform in workarounds[wa]:
>  			print(wa)
> +
> +
> +if __name__ == '__main__':
> +	(options, args) = parse_options(sys.argv)
> +	verbose = options.verbose
> +	kernel_path = None
> +
> +	if not len(args) and options.kernel_path == None and options.mesa_path == None:
> +		sys.stderr.write("error: A path to either a kernel tree or Mesa is required\n")
> +		sys.exit(1)
> +
> +	if len(args):
> +		kernel_path = args[0]
> +	elif options.kernel_path != None:
> +		kernel_path = options.kernel_path
> +
> +	if kernel_path != None:
> +	# --- list Kernel workarounds if path is provided ---
> +		kconfig = os.path.join(kernel_path, 'Kconfig')
> +		if not os.path.isfile(kconfig):
> +			sys.stderr.write("error: %s does not point to a kernel tree \n"
> +							% kernel_path)
> +			sys.exit(1)
> +
> +		i915_dir = os.path.join('drivers', 'gpu', 'drm', 'i915')
> +		print ("List of workarounds found in kernel:")
> +		print_workarounds(kernel_path, i915_dir)
> +
> +	# --- list mesa workarounds if path is provided ---
> +	if options.mesa_path != None:
> +		# reset workarounds array
> +		workarounds = {}
> +
> +		mesa_path = options.mesa_path
> +		i965_dir = os.path.join('src', 'mesa', 'drivers', 'dri', 'i965')
> +		mesa_dir = os.path.join(mesa_path, i965_dir)
> +		if not os.path.exists(mesa_dir):
> +			sys.stderr.write("error: %s does not point to a valid mesa path \n"
> +							% mesa_path)
> +			sys.exit(1)
> +
> +		print ("\nList of workarounds found in Mesa:")
> +		print_workarounds(mesa_path, i965_dir)

I believe the level where you start branching out between kernel and
mesa is too early, or could be factored out a bit more. Notice how
you're doing the same work in the two branches with some little changes
(eg. the \n at the start of print()).

> -- 
> 1.9.1
>