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

[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 <timothy.o.rowley at intel.com> 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 <emil.velikov at collabora.com>

> --- /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 <typename T>
>  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