On 09/01/2015 04:40 PM, Brian Paul wrote:
On 09/01/2015 06:04 AM, Tapani Pälli wrote:


On 08/31/2015 05:56 PM, Brian Paul wrote:
Looks good.  Just some minor nitpicks below.

Reviewed-by: Brian Paul <bri...@vmware.com>

Thanks! I have one comment below ..


On 08/31/2015 01:23 AM, Tapani Pälli wrote:
Patch modifies existing shader source and replace functionality to work
with environment variables rather than enable dumping on compile time.
Also instead of _mesa_str_checksum, _mesa_sha1_compute is used to avoid
collisions.

Functionality is controlled via 2 environment variables:

s/2/two/



MESA_SHADER_DUMP_PATH - path where shader sources are dumped
MESA_SHADER_READ_PATH - path where replacement shaders are read

Signed-off-by: Tapani Pälli <tapani.pa...@intel.com>
Suggested-by: Eero Tamminen <eero.t.tammi...@intel.com>
---
  docs/shading.html         |   9 +++
  src/mesa/main/shaderapi.c | 136
++++++++++++++++++++++++++++++++--------------
  2 files changed, 105 insertions(+), 40 deletions(-)

diff --git a/docs/shading.html b/docs/shading.html
index 77a0ee4..784329e 100644
--- a/docs/shading.html
+++ b/docs/shading.html
@@ -63,6 +63,15 @@ execution.  These are generally used for debugging.
  Example:  export MESA_GLSL=dump,nopt
  </p>

+<p>
+Shaders can be dumped and replaced on runtime for debugging purposes.
+This is controlled via following environment variables:
+<ul>
+<li><b>MESA_SHADER_DUMP_PATH</b> - path where shader sources are dumped
+<li><b>MESA_SHADER_READ_PATH</b> - path where replacement shaders are
read

Do these directories need to be different to prevent collisions between
the dumped shaders and the replacement shaders?

Yes, I thought a bit of solutions to work only in one dir but I think
having 2 makes everything more clear and here user can be only one
overwriting edited contents. I could add some text here : "These paths
should be separate to avoid overwriting modified contents." (?)

How about "When both are set, these paths should be different so the dumped shaders do not clobber the replacement shaders."


OK, I'll go with that. I've also now noticed one big problem about the patch, if one does not have one of the sha-1 libraries installed this fails in awful way. I think I need to add some define like HAVE_SHA and surround the code with this.

-Brian





+</ul>
+Note, path set must exist before running for dumping or replacing to
work.
+</p>

  <h2 id="support">GLSL Version</h2>

diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 0e0e0d6..d3abfc9 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -53,15 +53,13 @@
  #include "program/prog_parameter.h"
  #include "util/ralloc.h"
  #include "util/hash_table.h"
+#include "util/mesa-sha1.h"
  #include <stdbool.h>
  #include "../glsl/glsl_parser_extras.h"
  #include "../glsl/ir.h"
  #include "../glsl/ir_uniform.h"
  #include "../glsl/program.h"

-/** Define this to enable shader substitution (see below) */
-#define SHADER_SUBST 0
-

  /**
   * Return mask of GLSL_x flags by examining the MESA_GLSL env var.
@@ -1512,24 +1510,101 @@ _mesa_LinkProgram(GLhandleARB programObj)
     link_program(ctx, programObj);
  }

+/**
+ * Generate a SHA-1 hash value string for given source string.
+ */
+static void
+generate_sha1(const char *source, char sha_str[64])
+{
+   unsigned char sha[20];
+   _mesa_sha1_compute(source, strlen(source), sha);
+   _mesa_sha1_format(sha_str, sha);
+}
+
+/**
+ * Construct a full path for shader replacement functionality using
+ * following format:
+ *
+ * <path>/<stage prefix>_<CHECKSUM>.glsl
+ */
+static void
+construct_name(const gl_shader_stage stage, const char *source,
+               const char *path, char *name, unsigned length)
+{
+   char sha[64];
+   static const char *types[] = {
+      "VS", "TC", "TE", "GS", "FS", "CS",
+   };

+   generate_sha1(source, sha);
+   _mesa_snprintf(name, length, "%s/%s_%s.glsl", path, types[stage],
+                  sha);
+}
+
+/**
+ * Write given shader source to a file in MESA_SHADER_DUMP_PATH.
+ */
+static void
+dump_shader(const gl_shader_stage stage, const char *source)
+{
+   char name[PATH_MAX];
+   static bool path_exists = true;
+   char *dump_path;
+   FILE *f;
+
+   if (!path_exists)
+      return NULL;
+
+   dump_path = getenv("MESA_SHADER_DUMP_PATH");
+

Could remove that empty line.

+   if (!dump_path) {
+      path_exists = false;
+      return NULL;
+   }
+
+   construct_name(stage, source, dump_path, name, PATH_MAX);
+
+   f = fopen(name, "w");
+   if (f) {
+      fputs(source, f);
+      fclose(f);
+   } else {
+      GET_CURRENT_CONTEXT(ctx);
+ _mesa_warning(ctx, "could not open %s for dumping shader", name);
+   }
+}

  /**
   * Read shader source code from a file.
   * Useful for debugging to override an app's shader.
   */
  static GLcharARB *
-read_shader(const char *fname)
+read_shader(const gl_shader_stage stage, const char *source)
  {
-   int shader_size = 0;
-   FILE *f = fopen(fname, "r");
-   GLcharARB *buffer, *shader;
-   int len;
+   char name[PATH_MAX];
+   char *read_path;
+   static bool path_exists = true;
+   int len, shader_size = 0;
+   GLcharARB *buffer;
+   FILE *f;

-   if (!f) {
+   if (!path_exists)
+      return NULL;
+
+   read_path = getenv("MESA_SHADER_READ_PATH");
+
+   if (!read_path) {
+      path_exists = false;
        return NULL;
     }

+   construct_name(stage, source, read_path, name, PATH_MAX);
+
+   f = fopen(name, "r");
+

Could remove that empty line.

+   if (!f)
+      return NULL;
+
     /* allocate enough room for the entire shader */
     fseek(f, 0, SEEK_END);
     shader_size = ftell(f);
@@ -1547,13 +1622,9 @@ read_shader(const char *fname)

     fclose(f);

-   shader = strdup(buffer);
-   free(buffer);
-
-   return shader;
+   return buffer;
  }

-
  /**
* Called via glShaderSource() and glShaderSourceARB() API functions. * Basically, concatenate the source code strings into one long string
@@ -1566,14 +1637,15 @@ _mesa_ShaderSource(GLhandleARB shaderObj,
GLsizei count,
     GET_CURRENT_CONTEXT(ctx);
     GLint *offsets;
     GLsizei i, totalLength;
-   GLcharARB *source;
-   GLuint checksum;
+   GLcharARB *source, *replacement;

     if (!shaderObj || string == NULL) {
        _mesa_error(ctx, GL_INVALID_VALUE, "glShaderSourceARB");
        return;
     }

+   struct gl_shader *sh = _mesa_lookup_shader(ctx, shaderObj);

For C sources, I think we're still trying to declare all variables at
the top of scope.


+
     /*
      * This array holds offsets of where the appropriate string ends,
thus the
* last element will be set to the total length of the source code.
@@ -1620,35 +1692,19 @@ _mesa_ShaderSource(GLhandleARB shaderObj,
GLsizei count,
     source[totalLength - 1] = '\0';
     source[totalLength - 2] = '\0';

-   if (SHADER_SUBST) {
-      /* Compute the shader's source code checksum then try to open a
file
-       * named newshader_<CHECKSUM>.  If it exists, use it in place
of the
-       * original shader source code.  For debugging.
-       */
-      char filename[100];
-      GLcharARB *newSource;
-
-      checksum = _mesa_str_checksum(source);
-
-      _mesa_snprintf(filename, sizeof(filename), "newshader_%d",
checksum);
+ /* Dump original shader source to MESA_SHADER_DUMP_PATH and replace
+    * if corresponding entry found from MESA_SHADER_READ_PATH.
+    */
+   dump_shader(sh->Stage, source);

-      newSource = read_shader(filename);
-      if (newSource) {
-         fprintf(stderr, "Mesa: Replacing shader %u chksum=%d with
%s\n",
-                       shaderObj, checksum, filename);
-         free(source);
-         source = newSource;
-      }
+   replacement = read_shader(sh->Stage, source);
+   if (replacement) {
+      free(source);
+      source = replacement;
     }

     shader_source(ctx, shaderObj, source);

-   if (SHADER_SUBST) {
-      struct gl_shader *sh = _mesa_lookup_shader(ctx, shaderObj);
-      if (sh)
-         sh->SourceChecksum = checksum; /* save original checksum */
-   }
-
     free(offsets);
  }





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

Reply via email to