On Tue, 2016-06-07 at 16:23 +0200, Iago Toral wrote:
> 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?
Since this is only assigning values, not memory addresses, I think it
is safe.
> > }
> > 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...
Doesn't apply.
> > }
> >
> >
> > @@ -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.
The continue in the do..while will cause a re-evaluation of the
condition, so the loop is correct, AFAIU.
Since this is also a re-factoring from the code that was doing this
before, I think it makes sense to keep the do..while as it is. It is
more coherent with the patch and with similar code in this file.
> > + }
> > 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
--
Br,
Andres
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
