[Mesa-dev] [PATCH] mesa: debug facility to replace shaders when running apps
Tapani Pälli
tapani.palli at intel.com
Sun Aug 23 22:39:13 PDT 2015
On 08/21/2015 04:30 PM, Brian Paul wrote:
> On Fri, Aug 21, 2015 at 2:17 AM, Tapani Pälli <tapani.palli at intel.com
> <mailto:tapani.palli at intel.com>> wrote:
>
> Patch adds shader source and replace functionality in to the compiler.
>
>
> I had some very primitive support for this sort of thing already. See
> SHADER_SUBST in shaderapi.c However, feel free to replace it with
> something better/nicer.
>
> Also, please document this feature in docs/shading.html
>
Wow, I've managed to miss this :) I could modify your SHADER_SUBST code
by adding environment variable support and use SHA1 to avoid collisions.
> This can be used to debug individual (failing) shaders and measure
> performance impact of each shader.
>
> Functionality is controlled via 2 environment variables:
>
> MESA_SHADER_DUMP - path where shader sources are dumped
> MESA_SHADER_READ - path where replacement shaders are read
>
>
> Let's put _PATH on the end of both of those to better describe the vars.
ok
>
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com
> <mailto:tapani.palli at intel.com>>
> Suggested-by: Eero Tamminen <eero.t.tamminen at intel.com
> <mailto:eero.t.tamminen at intel.com>>
> ---
> src/glsl/glsl_parser_extras.cpp | 76
> +++++++++++++++++++++++++++++++++++++++++
> src/mesa/main/mtypes.h | 2 +-
> 2 files changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/src/glsl/glsl_parser_extras.cpp
> b/src/glsl/glsl_parser_extras.cpp
> index 46896d7..17b79b0 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
>
>
> Are you sure this is the right file for this new code? Maybe it should
> go into a new source file. glsl_parser_extras.cpp already seems to have
> become a dumping ground for a lot of unrelated things.
agreed, I think I'll just modify your existing code
>
> @@ -1570,10 +1570,86 @@ set_shader_inout_layout(struct gl_shader
> *shader,
>
> extern "C" {
>
> +#include <sys/stat.h>
> +#include "util/mesa-sha1.h"
> +
> +static void
> +generate_sha1(struct gl_shader *shader, char sha_str[64])
>
>
> const-qualify shader?
ok
> +{
> + unsigned char sha[20];
> + _mesa_sha1_compute(shader->Source, strlen(shader->Source), sha);
> + _mesa_sha1_format(sha_str, sha);
> +}
> +
> +static void
> +construct_name(struct gl_shader *shader, const char *path,
>
>
> const qualify shader?
>
> Please add a comment to the function explaining what it does.
ok
> + char *name, unsigned length)
> +{
> + char sha[64];
> + static const char *types[] = {
> + "VS", "TC", "TE", "GS", "FS", "CS",
> + };
> +
> + generate_sha1(shader, sha);
> + _mesa_snprintf(name, length, "%s/%s_%s.glsl", path,
> types[shader->Stage],
> + sha);
> +}
> +
> +static void
> +read_shader(struct gl_shader *shader)
>
>
> Please put a comment on the function explaining what it does.
>
>
> +{
> + char name[PATH_MAX];
> + static char *read_path = getenv("MESA_SHADER_READ");
> +
> + if (!read_path)
> + return;
> +
> + construct_name(shader, read_path, name, PATH_MAX);
> +
> + FILE *in = fopen(name, "r");
> + if (in) {
> + fseek(in, 0, SEEK_END);
> + long size = ftell(in);
> + rewind(in);
> + char *source = (char *) malloc(size + 1);
>
>
> Should probably check for source==NULL here. Otherwise, tools like
> coverity will probably complain.
will use your code with the checks!
>
> + fread(source, size, 1, in);
> + source[size] = '\0';
> + free(shader->Source);
> + shader->Source = source;
> + fclose(in);
> + }
> +}
> +
> +static void
> +dump_shader(struct gl_shader *shader)
>
>
> const-qualify shader?
>
>
> +{
> + char name[PATH_MAX];
> + static char *dump_path = getenv("MESA_SHADER_DUMP");
> +
> + if (!dump_path)
> + return;
> +
> + construct_name(shader, dump_path, name, PATH_MAX);
> +
> + FILE *out = fopen(name, "w");
> + if (out) {
> + fprintf(out, "%s", shader->Source);
>
>
> fputs() instead?
>
> + fclose(out);
> + } else {
> + fprintf(stderr, "could not open %s for dumping shader\n", name);
>
>
> Or, _mesa_warning()?
Yep, I was lazy with this because _mesa_warning would need additional
changes to Makefiles and linking.
>
> + }
> +}
> +
> void
> _mesa_glsl_compile_shader(struct gl_context *ctx, struct gl_shader
> *shader,
> bool dump_ast, bool dump_hir)
> {
> + /* Dump original shader source to MESA_SHADER_DUMP and replace
> + * if corresponding entry found from MESA_SHADER_READ path.
> + */
> + dump_shader(shader);
> + read_shader(shader);
> +
> struct _mesa_glsl_parse_state *state =
> new(shader) _mesa_glsl_parse_state(ctx, shader->Stage, shader);
> const char *source = shader->Source;
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index f61a245..fb47a22 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -2346,7 +2346,7 @@ struct gl_shader
> bool IsES; /**< True if this shader uses GLSL ES */
>
> GLuint SourceChecksum; /**< for debug/logging purposes */
> - const GLchar *Source; /**< Source code string */
> + GLchar *Source; /**< Source code string */
>
>
> Not sure why that change is needed.
Sure, not really needed.
> -Brian
>
>
> struct gl_program *Program; /**< Post-compile assembly code */
> GLchar *InfoLog;
> --
> 2.4.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
More information about the mesa-dev
mailing list