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

Rowley, Timothy O timothy.o.rowley at intel.com
Tue Aug 1 16:59:15 UTC 2017 

On Jul 31, 2017, at 3:18 PM, Emil Velikov <emil.l.velikov at gmail.com<mailto:emil.l.velikov at gmail.com>> 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 <timothy.o.rowley at intel.com<mailto: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 :-)

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 <emil.velikov at collabora.com<mailto: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<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 <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.

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

commit 566ea1983277bf62f07ea02571854009b667081f
Author: Tim Rowley <timothy.o.rowley at intel.com<mailto:timothy.o.rowley at intel.com>>
Date:   Mon Jul 31 17:22:54 2017 -0500

    swr/rast: simplify knob default value setup

commit f83d47cd01d987600b59106828bb75c672ea610c
Author: Tim Rowley <timothy.o.rowley at intel.com<mailto:timothy.o.rowley at intel.com>>
Date:   Mon Jul 31 17:22:12 2017 -0500

    swr/rast: split gen_knobs templates into .h/.cpp

    Switch to a 1:1 mapping template:generated for future maintenance.

commit 51af078588be9e5f25d8f7d31135cbdf0e526c37
Author: Tim Rowley <timothy.o.rowley at intel.com<mailto:timothy.o.rowley at intel.com>>
Date:   Mon Jul 31 17:01:54 2017 -0500

    swr/rast: gen_knobs template code style

commit fdb5c7018c159d31233e452250938afca7add189
Author: Tim Rowley <timothy.o.rowley at intel.com<mailto:timothy.o.rowley at intel.com>>
Date:   Mon Jul 31 17:48:12 2017 -0500

    swr/rast: switch gen_knobs.cpp license

    Unintentionally added with an apache2 license; relicense to match
    the rest of the tree.

commit 5191465819bb9c860a805b7163fe2f2ca2b49620
Author: Tim Rowley <timothy.o.rowley at intel.com<mailto:timothy.o.rowley at intel.com>>
Date:   Mon Jul 31 16:59:06 2017 -0500

    swr/rast: fix scons gen_knobs.h dependency

    Copy/paste error was duplicating a gen_knobs.cpp rule.



-Emil

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170801/ae2e1b3f/attachment-0001.html>

More information about the mesa-dev mailing list