[Mesa-dev] [PATCH] mesa: change 'SHADER_SUBST' facility to work with env variables
Tapani Pälli
tapani.palli at intel.com
Tue Sep 1 05:04:51 PDT 2015
On 08/31/2015 05:56 PM, Brian Paul wrote:
> Looks good. Just some minor nitpicks below.
>
> Reviewed-by: Brian Paul <brianp at 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.palli at intel.com>
>> Suggested-by: Eero Tamminen <eero.t.tamminen at 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." (?)
>
>> +</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);
>> }
>>
>>
>
More information about the mesa-dev
mailing list