Re: [Mesa-dev] [PATCH 12/13] swr/rast: split gen_knobs template into .cpp and .h files

2017-08-01 Thread Rowley, Timothy O 

On Jul 31, 2017, at 3:18 PM, Emil Velikov 
> wrote:

Hi Tim,

What's the goal behind the split. Please add a couple of words in the
commit message.

Will do.


On 31 July 2017 at 20:40, Tim Rowley 
> wrote:
---
src/gallium/drivers/swr/Makefile.am|   3 +-
src/gallium/drivers/swr/SConscript |   4 +-
.../drivers/swr/rasterizer/codegen/gen_knobs.py|  14 +-
.../swr/rasterizer/codegen/templates/gen_knobs.cpp | 112 +---
.../swr/rasterizer/codegen/templates/gen_knobs.h   | 147 +
.../drivers/swr/rasterizer/core/knobs_init.h   |  12 +-
6 files changed, 166 insertions(+), 126 deletions(-)
create mode 100644 
src/gallium/drivers/swr/rasterizer/codegen/templates/gen_knobs.h

diff --git a/src/gallium/drivers/swr/Makefile.am 
b/src/gallium/drivers/swr/Makefile.am
index 73fe904..b20f128 100644
--- a/src/gallium/drivers/swr/Makefile.am
+++ b/src/gallium/drivers/swr/Makefile.am
@@ -115,7 +115,7 @@ rasterizer/codegen/gen_knobs.cpp: 
rasterizer/codegen/gen_knobs.py rasterizer/cod
   --output rasterizer/codegen/gen_knobs.cpp \
   --gen_cpp

-rasterizer/codegen/gen_knobs.h: rasterizer/codegen/gen_knobs.py 
rasterizer/codegen/knob_defs.py rasterizer/codegen/templates/gen_knobs.cpp 
rasterizer/codegen/gen_common.py
+rasterizer/codegen/gen_knobs.h: rasterizer/codegen/gen_knobs.py 
rasterizer/codegen/knob_defs.py rasterizer/codegen/templates/gen_knobs.h 
rasterizer/codegen/gen_common.py
   $(MKDIR_GEN)
   $(PYTHON_GEN) \
   $(srcdir)/rasterizer/codegen/gen_knobs.py \
@@ -347,5 +347,6 @@ EXTRA_DIST = \
   rasterizer/codegen/templates/gen_builder.hpp \
   rasterizer/codegen/templates/gen_header_init.hpp \
   rasterizer/codegen/templates/gen_knobs.cpp \
+   rasterizer/codegen/templates/gen_knobs.h \
   rasterizer/codegen/templates/gen_llvm.hpp \
   rasterizer/codegen/templates/gen_rasterizer.cpp
diff --git a/src/gallium/drivers/swr/SConscript 
b/src/gallium/drivers/swr/SConscript
index a32807d..b394cbc 100644
--- a/src/gallium/drivers/swr/SConscript
+++ b/src/gallium/drivers/swr/SConscript
@@ -53,8 +53,8 @@ env.CodeGenerate(
source = '',
command = python_cmd + ' $SCRIPT --output $TARGET --gen_h'
)
-Depends('rasterizer/codegen/gen_knobs.cpp',
Seems like this should have been gen_knobs.h in the first place - oops :-)

Yep, noticed that bug when updating the rule - I’ve pulled the fix into a 
separate commit.


-swrroot + 'rasterizer/codegen/templates/gen_knobs.cpp')
+Depends('rasterizer/codegen/gen_knobs.h',
+swrroot + 'rasterizer/codegen/templates/gen_knobs.h')


The build bits are
Reviewed-by: Emil Velikov 
>

--- /dev/null
+++ b/src/gallium/drivers/swr/rasterizer/codegen/templates/gen_knobs.h
@@ -0,0 +1,147 @@
+/**
+*
+* Copyright 2015-2017
+* Intel Corporation
+*
+* Licensed under the Apache License, Version 2.0 (the "License");
+* you may not use this file except in compliance with the License.
+* You may obtain a copy of the License at
+*
+* http 
://www.apache.org/licenses/LICENSE-2.0
+*
I'm not a lawyer so I'm not sure if having Apache licensed code is
fine with rest of Mesa.

Considering that rest of SWR (barring the original gen_knobs.cpp where
this is comes from) uses MIT X11/Expat I'd stay consistent and
re-license this/these files.
If possible, of course.

Adding files with another license was unintentional - I’ve added a relicense 
commit prior to the split.



--- a/src/gallium/drivers/swr/rasterizer/core/knobs_init.h
+++ b/src/gallium/drivers/swr/rasterizer/core/knobs_init.h
@@ -91,16 +91,18 @@ static inline void ConvertEnvToKnob(const char* pOverride, 
std::string& knobValu
template 
static inline void InitKnob(T& knob)
{
-
-// TODO, read registry first
-
-// Second, read environment variables
+// Read environment variables
const char* pOverride = getenv(knob.Name());

if (pOverride)
{
-auto knobValue = knob.Value();
+auto knobValue = knob.DefaultValue();
ConvertEnvToKnob(pOverride, knobValue);
knob.Value(knobValue);
}
+else
+{
+// Set default value
+knob.Value(knob.DefaultValue());
This and the underlying code seems to have changed a bit.

Would be nice to keep "dummy split" and functionality changes as
separate patches.
Then again: it's not my code, so please don't read too much into my suggestion.

I’ve unwoven this commit into five commits for the upcoming v2 of the patchset:

commit 566ea1983277bf62f07ea02571854009b667081f
Author: Tim Rowley 
>
Date:   Mon Jul 31 17:22:54 2017 -0500

swr/rast: simplify knob

Re: [Mesa-dev] [PATCH 12/13] swr/rast: split gen_knobs template into .cpp and .h files

2017-07-31 Thread Emil Velikov 
Hi Tim,

What's the goal behind the split. Please add a couple of words in the
commit message.

On 31 July 2017 at 20:40, Tim Rowley  wrote:
> ---
>  src/gallium/drivers/swr/Makefile.am|   3 +-
>  src/gallium/drivers/swr/SConscript |   4 +-
>  .../drivers/swr/rasterizer/codegen/gen_knobs.py|  14 +-
>  .../swr/rasterizer/codegen/templates/gen_knobs.cpp | 112 +---
>  .../swr/rasterizer/codegen/templates/gen_knobs.h   | 147 
> +
>  .../drivers/swr/rasterizer/core/knobs_init.h   |  12 +-
>  6 files changed, 166 insertions(+), 126 deletions(-)
>  create mode 100644 
> src/gallium/drivers/swr/rasterizer/codegen/templates/gen_knobs.h
>
> diff --git a/src/gallium/drivers/swr/Makefile.am 
> b/src/gallium/drivers/swr/Makefile.am
> index 73fe904..b20f128 100644
> --- a/src/gallium/drivers/swr/Makefile.am
> +++ b/src/gallium/drivers/swr/Makefile.am
> @@ -115,7 +115,7 @@ rasterizer/codegen/gen_knobs.cpp: 
> rasterizer/codegen/gen_knobs.py rasterizer/cod
> --output rasterizer/codegen/gen_knobs.cpp \
> --gen_cpp
>
> -rasterizer/codegen/gen_knobs.h: rasterizer/codegen/gen_knobs.py 
> rasterizer/codegen/knob_defs.py rasterizer/codegen/templates/gen_knobs.cpp 
> rasterizer/codegen/gen_common.py
> +rasterizer/codegen/gen_knobs.h: rasterizer/codegen/gen_knobs.py 
> rasterizer/codegen/knob_defs.py rasterizer/codegen/templates/gen_knobs.h 
> rasterizer/codegen/gen_common.py
> $(MKDIR_GEN)
> $(PYTHON_GEN) \
> $(srcdir)/rasterizer/codegen/gen_knobs.py \
> @@ -347,5 +347,6 @@ EXTRA_DIST = \
> rasterizer/codegen/templates/gen_builder.hpp \
> rasterizer/codegen/templates/gen_header_init.hpp \
> rasterizer/codegen/templates/gen_knobs.cpp \
> +   rasterizer/codegen/templates/gen_knobs.h \
> rasterizer/codegen/templates/gen_llvm.hpp \
> rasterizer/codegen/templates/gen_rasterizer.cpp
> diff --git a/src/gallium/drivers/swr/SConscript 
> b/src/gallium/drivers/swr/SConscript
> index a32807d..b394cbc 100644
> --- a/src/gallium/drivers/swr/SConscript
> +++ b/src/gallium/drivers/swr/SConscript
> @@ -53,8 +53,8 @@ env.CodeGenerate(
>  source = '',
>  command = python_cmd + ' $SCRIPT --output $TARGET --gen_h'
>  )
> -Depends('rasterizer/codegen/gen_knobs.cpp',
Seems like this should have been gen_knobs.h in the first place - oops :-)

> -swrroot + 'rasterizer/codegen/templates/gen_knobs.cpp')
> +Depends('rasterizer/codegen/gen_knobs.h',
> +swrroot + 'rasterizer/codegen/templates/gen_knobs.h')
>

The build bits are
Reviewed-by: Emil Velikov 

> --- /dev/null
> +++ b/src/gallium/drivers/swr/rasterizer/codegen/templates/gen_knobs.h
> @@ -0,0 +1,147 @@
> +/**
> +*
> +* Copyright 2015-2017
> +* Intel Corporation
> +*
> +* Licensed under the Apache License, Version 2.0 (the "License");
> +* you may not use this file except in compliance with the License.
> +* You may obtain a copy of the License at
> +*
> +* http ://www.apache.org/licenses/LICENSE-2.0
> +*
I'm not a lawyer so I'm not sure if having Apache licensed code is
fine with rest of Mesa.

Considering that rest of SWR (barring the original gen_knobs.cpp where
this is comes from) uses MIT X11/Expat I'd stay consistent and
re-license this/these files.
If possible, of course.


> --- a/src/gallium/drivers/swr/rasterizer/core/knobs_init.h
> +++ b/src/gallium/drivers/swr/rasterizer/core/knobs_init.h
> @@ -91,16 +91,18 @@ static inline void ConvertEnvToKnob(const char* 
> pOverride, std::string& knobValu
>  template 
>  static inline void InitKnob(T& knob)
>  {
> -
> -// TODO, read registry first
> -
> -// Second, read environment variables
> +// Read environment variables
>  const char* pOverride = getenv(knob.Name());
>
>  if (pOverride)
>  {
> -auto knobValue = knob.Value();
> +auto knobValue = knob.DefaultValue();
>  ConvertEnvToKnob(pOverride, knobValue);
>  knob.Value(knobValue);
>  }
> +else
> +{
> +// Set default value
> +knob.Value(knob.DefaultValue());
This and the underlying code seems to have changed a bit.

Would be nice to keep "dummy split" and functionality changes as
separate patches.
Then again: it's not my code, so please don't read too much into my suggestion.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 12/13] swr/rast: split gen_knobs template into .cpp and .h files

2017-07-31 Thread Tim Rowley 
---
 src/gallium/drivers/swr/Makefile.am|   3 +-
 src/gallium/drivers/swr/SConscript |   4 +-
 .../drivers/swr/rasterizer/codegen/gen_knobs.py|  14 +-
 .../swr/rasterizer/codegen/templates/gen_knobs.cpp | 112 +---
 .../swr/rasterizer/codegen/templates/gen_knobs.h   | 147 +
 .../drivers/swr/rasterizer/core/knobs_init.h   |  12 +-
 6 files changed, 166 insertions(+), 126 deletions(-)
 create mode 100644 
src/gallium/drivers/swr/rasterizer/codegen/templates/gen_knobs.h

diff --git a/src/gallium/drivers/swr/Makefile.am 
b/src/gallium/drivers/swr/Makefile.am
index 73fe904..b20f128 100644
--- a/src/gallium/drivers/swr/Makefile.am
+++ b/src/gallium/drivers/swr/Makefile.am
@@ -115,7 +115,7 @@ rasterizer/codegen/gen_knobs.cpp: 
rasterizer/codegen/gen_knobs.py rasterizer/cod
--output rasterizer/codegen/gen_knobs.cpp \
--gen_cpp
 
-rasterizer/codegen/gen_knobs.h: rasterizer/codegen/gen_knobs.py 
rasterizer/codegen/knob_defs.py rasterizer/codegen/templates/gen_knobs.cpp 
rasterizer/codegen/gen_common.py
+rasterizer/codegen/gen_knobs.h: rasterizer/codegen/gen_knobs.py 
rasterizer/codegen/knob_defs.py rasterizer/codegen/templates/gen_knobs.h 
rasterizer/codegen/gen_common.py
$(MKDIR_GEN)
$(PYTHON_GEN) \
$(srcdir)/rasterizer/codegen/gen_knobs.py \
@@ -347,5 +347,6 @@ EXTRA_DIST = \
rasterizer/codegen/templates/gen_builder.hpp \
rasterizer/codegen/templates/gen_header_init.hpp \
rasterizer/codegen/templates/gen_knobs.cpp \
+   rasterizer/codegen/templates/gen_knobs.h \
rasterizer/codegen/templates/gen_llvm.hpp \
rasterizer/codegen/templates/gen_rasterizer.cpp
diff --git a/src/gallium/drivers/swr/SConscript 
b/src/gallium/drivers/swr/SConscript
index a32807d..b394cbc 100644
--- a/src/gallium/drivers/swr/SConscript
+++ b/src/gallium/drivers/swr/SConscript
@@ -53,8 +53,8 @@ env.CodeGenerate(
 source = '',
 command = python_cmd + ' $SCRIPT --output $TARGET --gen_h'
 )
-Depends('rasterizer/codegen/gen_knobs.cpp',
-swrroot + 'rasterizer/codegen/templates/gen_knobs.cpp')
+Depends('rasterizer/codegen/gen_knobs.h',
+swrroot + 'rasterizer/codegen/templates/gen_knobs.h')
 
 env.CodeGenerate(
 target = 'rasterizer/jitter/gen_state_llvm.h',
diff --git a/src/gallium/drivers/swr/rasterizer/codegen/gen_knobs.py 
b/src/gallium/drivers/swr/rasterizer/codegen/gen_knobs.py
index 2c271c7..33f62a2 100644
--- a/src/gallium/drivers/swr/rasterizer/codegen/gen_knobs.py
+++ b/src/gallium/drivers/swr/rasterizer/codegen/gen_knobs.py
@@ -37,27 +37,25 @@ def main(args=sys.argv[1:]):
 args = parser.parse_args()
 
 cur_dir = os.path.dirname(os.path.abspath(__file__))
-template_file = os.path.join(cur_dir, 'templates', 'gen_knobs.cpp')
+template_cpp = os.path.join(cur_dir, 'templates', 'gen_knobs.cpp')
+template_h = os.path.join(cur_dir, 'templates', 'gen_knobs.h')
 
 if args.gen_h:
 MakoTemplateWriter.to_file(
-template_file,
+template_h,
 args.output,
 cmdline=sys.argv,
 filename='gen_knobs',
-knobs=knob_defs.KNOBS,
-includes=['core/knobs_init.h', 'common/os.h', 'sstream', 
'iomanip'],
-gen_header=True)
+knobs=knob_defs.KNOBS)
 
 if args.gen_cpp:
 MakoTemplateWriter.to_file(
-template_file,
+template_cpp,
 args.output,
 cmdline=sys.argv,
 filename='gen_knobs',
 knobs=knob_defs.KNOBS,
-includes=['core/knobs_init.h', 'common/os.h', 'sstream', 
'iomanip'],
-gen_header=False)
+includes=['core/knobs_init.h', 'common/os.h', 'sstream', 
'iomanip'])
 
 return 0
 
diff --git a/src/gallium/drivers/swr/rasterizer/codegen/templates/gen_knobs.cpp 
b/src/gallium/drivers/swr/rasterizer/codegen/templates/gen_knobs.cpp
index 06b93bd..01cb801 100644
--- a/src/gallium/drivers/swr/rasterizer/codegen/templates/gen_knobs.cpp
+++ b/src/gallium/drivers/swr/rasterizer/codegen/templates/gen_knobs.cpp
@@ -15,11 +15,7 @@
 * See the License for the specific language governing permissions and
 * limitations under the License.
 *
-% if gen_header:
-* @file ${filename}.h
-% else:
 * @file ${filename}.cpp
-% endif 
 *
 * @brief Dynamic Knobs for Core.
 *
@@ -30,105 +26,6 @@
 *
 **/
 <% calc_max_knob_len(knobs) %>
-%if gen_header:
-#pragma once
-#include 
-
-struct KnobBase
-{
-private:
-// Update the input string.
-static void autoExpandEnvironmentVariables(std::string );
-
-protected:
-// Leave input alone and return new string.
-static std::string expandEnvironmentVariables(std::string const )
-{
-std::string text = input;
-autoExpandEnvironmentVariables(text);
-return text;
-}
-
-template 
-