[Mesa-dev] [PATCH 1/5] mesa/subroutines: start adding per-context subroutine index support

Iago Toral itoral at igalia.com
Tue Jun 7 14:23:16 UTC 2016


On Tue, 2016-06-07 at 15:25 +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at 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 <airlied at 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,




More information about the mesa-dev mailing list