On Tue, 2016-06-07 at 15:25 +1000, Dave Airlie wrote:
> From: Dave Airlie <airl...@redhat.com>
>
> One piece of ARB_shader_subroutine I ignored was the fact that it
> needs to store the subroutine index data per context and not per
> shader program.
>
> There is one CTS test that tests this:
> GL45-CTS.shader_subroutine.multiple_contexts
>
> However the test only does a write to context and readback,
> it never renders using the values, so this is enough to fix the
> test however not enough to do what the spec says.
>
> So with this patch the info is now stored per context, but
> it gets updated into the program at UseProgram and when the
> values are inserted into the context, which won't help if
> a multiple contexts are in use in multiple threads.
Stray 'a' in the beginning of the line above.
>
> Signed-off-by: Dave Airlie <airl...@redhat.com>
> ---
> src/mesa/main/mtypes.h | 10 ++++++
> src/mesa/main/pipelineobj.c | 2 +-
> src/mesa/main/shaderapi.c | 75
> +++++++++++++++++++++++++++------------------
> src/mesa/main/shaderapi.h | 3 +-
> 4 files changed, 58 insertions(+), 32 deletions(-)
>
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 471d41d..a7ffbf7 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -4307,6 +4307,15 @@ struct gl_atomic_buffer_binding
> };
>
> /**
> + * Shader subroutines storage
> + */
> +struct gl_subroutine_index_binding
> +{
> + int NumIndex;
> + int *IndexPtr;
> +};
> +
> +/**
> * Mesa rendering context.
> *
> * This is the central context data structure for Mesa. Almost all
> @@ -4544,6 +4553,7 @@ struct gl_context
> */
> struct gl_image_unit ImageUnits[MAX_IMAGE_UNITS];
>
> + struct gl_subroutine_index_binding SubroutineIndex[MESA_SHADER_STAGES];
> /*@}*/
>
> struct gl_meta_state *Meta; /**< for "meta" operations */
> diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
> index 5a46cfe..def2539 100644
> --- a/src/mesa/main/pipelineobj.c
> +++ b/src/mesa/main/pipelineobj.c
> @@ -469,7 +469,7 @@ _mesa_bind_pipeline(struct gl_context *ctx,
> FLUSH_VERTICES(ctx, _NEW_PROGRAM | _NEW_PROGRAM_CONSTANTS);
>
> for (i = 0; i < MESA_SHADER_STAGES; i++)
> -
> _mesa_shader_program_init_subroutine_defaults(ctx->_Shader->CurrentProgram[i]);
> + _mesa_shader_program_init_subroutine_defaults(ctx,
> ctx->_Shader->CurrentProgram[i]);
>
> if (ctx->Driver.UseProgram)
> ctx->Driver.UseProgram(ctx, NULL);
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 9d440a0..818a88d 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -65,6 +65,7 @@
> #define PATH_MAX _MAX_PATH
> #endif
>
> +static void _mesa_shader_write_subroutine_index(struct gl_context *ctx,
> struct gl_shader *sh);
> /**
> * Return mask of GLSL_x flags by examining the MESA_GLSL env var.
> */
> @@ -1189,7 +1190,7 @@ use_shader_program(struct gl_context *ctx,
> gl_shader_stage stage,
> shProg = NULL;
>
> if (shProg)
> - _mesa_shader_program_init_subroutine_defaults(shProg);
> + _mesa_shader_program_init_subroutine_defaults(ctx, shProg);
>
> if (*target != shProg) {
> /* Program is current, flush it */
> @@ -2628,27 +2629,15 @@ _mesa_UniformSubroutinesuiv(GLenum shadertype,
> GLsizei count,
> _mesa_error(ctx, GL_INVALID_OPERATION, "%s", api_name);
> return;
> }
> +
> + ctx->SubroutineIndex[sh->Stage].IndexPtr[j] = indices[j];
I think this isn't safe: isn't indices[] owned by the caller?
> }
> i += uni_count;
> } while(i < count);
>
> FLUSH_VERTICES(ctx, _NEW_PROGRAM_CONSTANTS);
> - i = 0;
> - do {
> - struct gl_uniform_storage *uni = sh->SubroutineUniformRemapTable[i];
> - if (uni == NULL) {
> - i++;
> - continue;
> - }
> -
> - int uni_count = uni->array_elements ? uni->array_elements : 1;
> -
> - memcpy(&uni->storage[0], &indices[i],
> - sizeof(GLuint) * uni_count);
>
> - _mesa_propagate_uniforms_to_driver_storage(uni, 0, uni_count);
> - i += uni_count;
> - } while(i < count);
> + _mesa_shader_write_subroutine_index(ctx, sh);
> }
>
>
> @@ -2690,12 +2679,7 @@ _mesa_GetUniformSubroutineuiv(GLenum shadertype, GLint
> location,
> return;
> }
>
> - {
> - struct gl_uniform_storage *uni =
> sh->SubroutineUniformRemapTable[location];
> - int offset = location - uni->opaque[stage].index;
> - memcpy(params, &uni->storage[offset],
> - sizeof(GLuint));
> - }
> + *params = ctx->SubroutineIndex[sh->Stage].IndexPtr[location];
so by the time users call here, they might have freed indices[] or
something...
> }
>
>
> @@ -2803,29 +2787,60 @@ find_compat_subroutine(struct gl_shader *sh, const
> struct glsl_type *type)
> }
>
> static void
> -_mesa_shader_init_subroutine_defaults(struct gl_shader *sh)
> +_mesa_shader_write_subroutine_index(struct gl_context *ctx,
> + struct gl_shader *sh)
> {
> int i, j;
>
> - for (i = 0; i < sh->NumSubroutineUniformRemapTable; i++) {
> + if (sh->NumSubroutineUniformRemapTable == 0)
> + return;
> +
> + i = 0;
> + do {
> struct gl_uniform_storage *uni = sh->SubroutineUniformRemapTable[i];
> int uni_count;
> int val;
>
> - if (!uni)
> + if (!uni) {
> + i++;
> continue;
If you are going to do this as a do..while then you need to check here
that i < sh->NumSubroutineUniformRemapTable before continuing. I think
I'd rather leave this as a for loop to avoid that, you can leave the
increment statement of the for empty and handle that yourself inside the
loop body as in the do..while case.
> + }
> uni_count = uni->array_elements ? uni->array_elements : 1;
> - val = find_compat_subroutine(sh, uni->type);
> -
> - for (j = 0; j < uni_count; j++)
> + for (j = 0; j < uni_count; j++) {
> + val = ctx->SubroutineIndex[sh->Stage].IndexPtr[i + j];
> memcpy(&uni->storage[j], &val, sizeof(int));
> + }
>
> _mesa_propagate_uniforms_to_driver_storage(uni, 0, uni_count);
> + i += uni_count;
> + } while(i < sh->NumSubroutineUniformRemapTable);
> +}
> +
> +static void
> +_mesa_shader_init_subroutine_defaults(struct gl_context *ctx,
> + struct gl_shader *sh)
> +{
> + int i;
> +
> + if (ctx->SubroutineIndex[sh->Stage].NumIndex !=
> sh->NumSubroutineUniformRemapTable) {
> + ctx->SubroutineIndex[sh->Stage].IndexPtr =
> realloc(ctx->SubroutineIndex[sh->Stage].IndexPtr,
> sh->NumSubroutineUniformRemapTable * (sizeof(int)));
> + ctx->SubroutineIndex[sh->Stage].NumIndex =
> sh->NumSubroutineUniformRemapTable;
> + }
> +
> + for (i = 0; i < sh->NumSubroutineUniformRemapTable; i++) {
> + struct gl_uniform_storage *uni = sh->SubroutineUniformRemapTable[i];
> +
> + if (!uni)
> + continue;
> + ctx->SubroutineIndex[sh->Stage].IndexPtr[i] =
> find_compat_subroutine(sh, uni->type);
> }
> +
> + _mesa_shader_write_subroutine_index(ctx, sh);
> }
>
> void
> -_mesa_shader_program_init_subroutine_defaults(struct gl_shader_program
> *shProg)
> +_mesa_shader_program_init_subroutine_defaults(struct gl_context *ctx,
> + struct gl_shader_program
> *shProg)
> {
> int i;
>
> @@ -2836,6 +2851,6 @@ _mesa_shader_program_init_subroutine_defaults(struct
> gl_shader_program *shProg)
> if (!shProg->_LinkedShaders[i])
> continue;
>
> - _mesa_shader_init_subroutine_defaults(shProg->_LinkedShaders[i]);
> + _mesa_shader_init_subroutine_defaults(ctx, shProg->_LinkedShaders[i]);
> }
> }
> diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
> index 09b9525..b3de5fa 100644
> --- a/src/mesa/main/shaderapi.h
> +++ b/src/mesa/main/shaderapi.h
> @@ -286,7 +286,8 @@ _mesa_PatchParameterfv(GLenum pname, const GLfloat
> *values);
>
> /* GL_ARB_shader_subroutine */
> void
> -_mesa_shader_program_init_subroutine_defaults(struct gl_shader_program
> *shProg);
> +_mesa_shader_program_init_subroutine_defaults(struct gl_context *ctx,
> + struct gl_shader_program
> *shProg);
>
> extern GLint GLAPIENTRY
> _mesa_GetSubroutineUniformLocation(GLuint program, GLenum shadertype,
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
