Re: [Mesa-dev] [PATCH 1/2] util: move brw_env_var_as_boolean() to util

2015-11-19 Thread Nicolai Hähnle 

Hi Rob,

On 18.11.2015 23:20, Rob Clark wrote:

From: Rob Clark 

Kind of a handy function.  And I'll what it available outside of i965
for common nir-pass helpers.

Signed-off-by: Rob Clark 
---
  src/mesa/drivers/dri/i965/brw_context.c  |  5 +++--
  src/mesa/drivers/dri/i965/brw_nir.c  |  4 +++-
  src/mesa/drivers/dri/i965/brw_shader.cpp |  3 ++-
  src/mesa/drivers/dri/i965/intel_debug.c  | 25 -
  src/mesa/drivers/dri/i965/intel_debug.h  |  2 --
  src/util/debug.c | 25 +
  src/util/debug.h |  2 ++
  7 files changed, 35 insertions(+), 31 deletions(-)

[.. snip ...]

diff --git a/src/util/debug.c b/src/util/debug.c
index 3729ce8..98b1853 100644
--- a/src/util/debug.c
+++ b/src/util/debug.c
@@ -51,3 +51,28 @@ parse_debug_string(const char *debug,

 return flag;
  }
+
+/**
+ * Reads an environment variable and interprets its value as a boolean.
+ *
+ * Recognizes 0/false/no and 1/true/yes.  Other values result in the default 
value.
+ */
+bool
+env_var_as_boolean(const char *var_name, bool default_value)
+{
+   const char *str = getenv(var_name);
+   if (str == NULL)
+  return default_value;
+
+   if (strcmp(str, "1") == 0 ||
+   strcasecmp(str, "true") == 0 ||
+   strcasecmp(str, "yes") == 0) {
+  return true;
+   } else if (strcmp(str, "0") == 0 ||
+  strcasecmp(str, "false") == 0 ||
+  strcasecmp(str, "no") == 0) {
+  return false;
+   } else {
+  return default_value;
+   }
+}


This all looks good to me. I do have two suggestions to slightly improve 
usability:


1) Add "on" and "off" as recognized values.

2) Add something to the effect of `fprintf(stderr, "%s: value not 
recognized, using default.\n", var_name);` to the default value branch.


Either way, feel free to add my R-b.

Cheers,
Nicolai


diff --git a/src/util/debug.h b/src/util/debug.h
index 801736a..3555417 100644
--- a/src/util/debug.h
+++ b/src/util/debug.h
@@ -38,6 +38,8 @@ struct debug_control {
  uint64_t
  parse_debug_string(const char *debug,
 const struct debug_control *control);
+bool
+env_var_as_boolean(const char *var_name, bool default_value);

  #ifdef __cplusplus
  } /* extern C */



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

Re: [Mesa-dev] [PATCH 1/2] util: move brw_env_var_as_boolean() to util

2015-11-19 Thread Rob Clark 
On Thu, Nov 19, 2015 at 5:31 AM, Nicolai Hähnle  wrote:
> Hi Rob,
>
> On 18.11.2015 23:20, Rob Clark wrote:
>>
>> From: Rob Clark 
>>
>> Kind of a handy function.  And I'll what it available outside of i965
>> for common nir-pass helpers.
>>
>> Signed-off-by: Rob Clark 
>> ---
>>   src/mesa/drivers/dri/i965/brw_context.c  |  5 +++--
>>   src/mesa/drivers/dri/i965/brw_nir.c  |  4 +++-
>>   src/mesa/drivers/dri/i965/brw_shader.cpp |  3 ++-
>>   src/mesa/drivers/dri/i965/intel_debug.c  | 25 -
>>   src/mesa/drivers/dri/i965/intel_debug.h  |  2 --
>>   src/util/debug.c | 25 +
>>   src/util/debug.h |  2 ++
>>   7 files changed, 35 insertions(+), 31 deletions(-)
>
> [.. snip ...]
>
>> diff --git a/src/util/debug.c b/src/util/debug.c
>> index 3729ce8..98b1853 100644
>> --- a/src/util/debug.c
>> +++ b/src/util/debug.c
>> @@ -51,3 +51,28 @@ parse_debug_string(const char *debug,
>>
>>  return flag;
>>   }
>> +
>> +/**
>> + * Reads an environment variable and interprets its value as a boolean.
>> + *
>> + * Recognizes 0/false/no and 1/true/yes.  Other values result in the
>> default value.
>> + */
>> +bool
>> +env_var_as_boolean(const char *var_name, bool default_value)
>> +{
>> +   const char *str = getenv(var_name);
>> +   if (str == NULL)
>> +  return default_value;
>> +
>> +   if (strcmp(str, "1") == 0 ||
>> +   strcasecmp(str, "true") == 0 ||
>> +   strcasecmp(str, "yes") == 0) {
>> +  return true;
>> +   } else if (strcmp(str, "0") == 0 ||
>> +  strcasecmp(str, "false") == 0 ||
>> +  strcasecmp(str, "no") == 0) {
>> +  return false;
>> +   } else {
>> +  return default_value;
>> +   }
>> +}
>
>
> This all looks good to me. I do have two suggestions to slightly improve
> usability:
>
> 1) Add "on" and "off" as recognized values.
>
> 2) Add something to the effect of `fprintf(stderr, "%s: value not
> recognized, using default.\n", var_name);` to the default value branch.

Those would be reasonable additions, although should be split out into
a different patch..  I prefer to avoid making unrelated changes at
same time I move code around

> Either way, feel free to add my R-b.

Thanks

BR,
-R

> Cheers,
> Nicolai
>
>
>> diff --git a/src/util/debug.h b/src/util/debug.h
>> index 801736a..3555417 100644
>> --- a/src/util/debug.h
>> +++ b/src/util/debug.h
>> @@ -38,6 +38,8 @@ struct debug_control {
>>   uint64_t
>>   parse_debug_string(const char *debug,
>>  const struct debug_control *control);
>> +bool
>> +env_var_as_boolean(const char *var_name, bool default_value);
>>
>>   #ifdef __cplusplus
>>   } /* extern C */
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/2] util: move brw_env_var_as_boolean() to util

2015-11-18 Thread Rob Clark 
From: Rob Clark 

Kind of a handy function.  And I'll what it available outside of i965
for common nir-pass helpers.

Signed-off-by: Rob Clark 
---
 src/mesa/drivers/dri/i965/brw_context.c  |  5 +++--
 src/mesa/drivers/dri/i965/brw_nir.c  |  4 +++-
 src/mesa/drivers/dri/i965/brw_shader.cpp |  3 ++-
 src/mesa/drivers/dri/i965/intel_debug.c  | 25 -
 src/mesa/drivers/dri/i965/intel_debug.h  |  2 --
 src/util/debug.c | 25 +
 src/util/debug.h |  2 ++
 7 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index 2ea0a9e..7e2fdcb 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -69,6 +69,7 @@
 #include "tnl/tnl.h"
 #include "tnl/t_pipeline.h"
 #include "util/ralloc.h"
+#include "util/debug.h"
 
 /***
  * Mesa's Driver Functions
@@ -899,8 +900,8 @@ brwCreateContext(gl_api api,
brw->predicate.state = BRW_PREDICATE_STATE_RENDER;
 
brw->use_resource_streamer = screen->has_resource_streamer &&
-  (brw_env_var_as_boolean("INTEL_USE_HW_BT", false) ||
-   brw_env_var_as_boolean("INTEL_USE_GATHER", false));
+  (env_var_as_boolean("INTEL_USE_HW_BT", false) ||
+   env_var_as_boolean("INTEL_USE_GATHER", false));
 
ctx->VertexProgram._MaintainTnlProgram = true;
ctx->FragmentProgram._MaintainTexEnvProgram = true;
diff --git a/src/mesa/drivers/dri/i965/brw_nir.c 
b/src/mesa/drivers/dri/i965/brw_nir.c
index 79e70b9..4c5e036 100644
--- a/src/mesa/drivers/dri/i965/brw_nir.c
+++ b/src/mesa/drivers/dri/i965/brw_nir.c
@@ -171,12 +171,14 @@ brw_nir_lower_outputs(nir_shader *nir, bool is_scalar)
}
 }
 
+#include "util/debug.h"
+
 static bool
 should_clone_nir()
 {
static int should_clone = -1;
if (should_clone < 1)
-  should_clone = brw_env_var_as_boolean("NIR_TEST_CLONE", false);
+  should_clone = env_var_as_boolean("NIR_TEST_CLONE", false);
 
return should_clone;
 }
diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp 
b/src/mesa/drivers/dri/i965/brw_shader.cpp
index c4a567f..cd1a4c5 100644
--- a/src/mesa/drivers/dri/i965/brw_shader.cpp
+++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
@@ -31,6 +31,7 @@
 #include "glsl/ir_optimization.h"
 #include "glsl/glsl_parser_extras.h"
 #include "main/shaderapi.h"
+#include "util/debug.h"
 
 static void
 shader_debug_log_mesa(void *data, const char *fmt, ...)
@@ -87,7 +88,7 @@ brw_compiler_create(void *mem_ctx, const struct 
brw_device_info *devinfo)
compiler->scalar_stage[MESA_SHADER_VERTEX] =
   devinfo->gen >= 8 && !(INTEL_DEBUG & DEBUG_VEC4VS);
compiler->scalar_stage[MESA_SHADER_GEOMETRY] =
-  devinfo->gen >= 8 && brw_env_var_as_boolean("INTEL_SCALAR_GS", false);
+  devinfo->gen >= 8 && env_var_as_boolean("INTEL_SCALAR_GS", false);
compiler->scalar_stage[MESA_SHADER_FRAGMENT] = true;
compiler->scalar_stage[MESA_SHADER_COMPUTE] = true;
 
diff --git a/src/mesa/drivers/dri/i965/intel_debug.c 
b/src/mesa/drivers/dri/i965/intel_debug.c
index f53c4ab..7d8b966 100644
--- a/src/mesa/drivers/dri/i965/intel_debug.c
+++ b/src/mesa/drivers/dri/i965/intel_debug.c
@@ -103,28 +103,3 @@ brw_process_intel_debug_variable(void)
uint64_t intel_debug = parse_debug_string(getenv("INTEL_DEBUG"), 
debug_control);
(void) p_atomic_cmpxchg(_DEBUG, 0, intel_debug);
 }
-
-/**
- * Reads an environment variable and interprets its value as a boolean.
- *
- * Recognizes 0/false/no and 1/true/yes.  Other values result in the default 
value.
- */
-bool
-brw_env_var_as_boolean(const char *var_name, bool default_value)
-{
-   const char *str = getenv(var_name);
-   if (str == NULL)
-  return default_value;
-
-   if (strcmp(str, "1") == 0 ||
-   strcasecmp(str, "true") == 0 ||
-   strcasecmp(str, "yes") == 0) {
-  return true;
-   } else if (strcmp(str, "0") == 0 ||
-  strcasecmp(str, "false") == 0 ||
-  strcasecmp(str, "no") == 0) {
-  return false;
-   } else {
-  return default_value;
-   }
-}
diff --git a/src/mesa/drivers/dri/i965/intel_debug.h 
b/src/mesa/drivers/dri/i965/intel_debug.h
index 9c6030a..175ac68 100644
--- a/src/mesa/drivers/dri/i965/intel_debug.h
+++ b/src/mesa/drivers/dri/i965/intel_debug.h
@@ -120,5 +120,3 @@ extern uint64_t INTEL_DEBUG;
 extern uint64_t intel_debug_flag_for_shader_stage(gl_shader_stage stage);
 
 extern void brw_process_intel_debug_variable(void);
-
-extern bool brw_env_var_as_boolean(const char *var_name, bool default_value);
diff --git a/src/util/debug.c b/src/util/debug.c
index 3729ce8..98b1853 100644
--- a/src/util/debug.c
+++ b/src/util/debug.c
@@ -51,3 +51,28 @@ parse_debug_string(const char *debug,
 
return flag;
 }
+
+/**
+ * Reads an environment variable and interprets its value as a boolean.
+ *
+ * Recognizes