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

[Kibey, Sameer]

> -----Original Message-----
> From: Lespiau, Damien
> Sent: Friday, February 05, 2016 4:16 AM
> To: Kibey, Sameer
> Cc: intel-gfx at lists.freedesktop.org; mesa-dev at lists.freedesktop.org; Sharp,
> Sarah A; Widawsky, Benjamin
> Subject: Re: [PATCH] list-workarounds: Extend the script to Mesa
> 
> 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.

I sent the email manually, but will use git send-email next time. 

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

I will replace byt with vlv to keep it consistent. 

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

Agree, will remove byt. 

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

Originally I had removed the old behavior. Ben suggested keeping it in case some people have it in other scripts. 

> > +	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?

Will change to 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()).

I'll refactor this some more. 
 
> > --
> > 1.9.1
> >