[Mesa-dev] [PATCH v2] mesa: validate sampler uniforms during gluniform calls

[Francisco Jerez]

Tapani Pälli <tapani.palli at intel.com> writes:

> Patch fixes 'glsl-2types-of-textures-on-same-unit' in WebGL conformance
> test suite. No Piglit regressions, fixes gl-2.0-active-sampler-conflict.
>
> To avoid adding potentially heavy check during draw (valid_to_render),
> check is done during uniform updates by inspecting TexturesUsed mask.
>
> A new boolean variable is introduced to cache validation state.
>
> v2: take into account case where 2 uniforms use same unit (curro)
>     also do the check only when SSO is not in use, SSO has own
>     path for sampler validation.
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>

Thanks, looks good to me,
Reviewed-by: Francisco Jerez <currojerez at riseup.net>

> ---
>  src/mesa/main/context.c         | 12 +++++++++++
>  src/mesa/main/mtypes.h          |  1 +
>  src/mesa/main/uniform_query.cpp | 44 +++++++++--------------------------------
>  src/mesa/main/uniforms.c        | 14 +++++++++++++
>  4 files changed, 36 insertions(+), 35 deletions(-)
>
> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> index 5a8f718..25b9bfc 100644
> --- a/src/mesa/main/context.c
> +++ b/src/mesa/main/context.c
> @@ -133,6 +133,7 @@
>  #include "program/prog_print.h"
>  #include "math/m_matrix.h"
>  #include "main/dispatch.h" /* for _gloffset_COUNT */
> +#include "uniforms.h"
>  
>  #ifdef USE_SPARC_ASM
>  #include "sparc/sparc.h"
> @@ -1949,6 +1950,17 @@ _mesa_valid_to_render(struct gl_context *ctx, const char *where)
>        }
>     }
>  
> +   /* If a program is active and SSO not in use, check if validation of
> +    * samplers succeeded for the active program. */
> +   if (ctx->_Shader->ActiveProgram && ctx->_Shader != ctx->Pipeline.Current) {
> +      char errMsg[100];
> +      if (!_mesa_sampler_uniforms_are_valid(ctx->_Shader->ActiveProgram,
> +                                            errMsg, 100)) {
> +         _mesa_error(ctx, GL_INVALID_OPERATION, "%s", errMsg);
> +         return GL_FALSE;
> +      }
> +   }
> +
>     if (ctx->DrawBuffer->_Status != GL_FRAMEBUFFER_COMPLETE_EXT) {
>        _mesa_error(ctx, GL_INVALID_FRAMEBUFFER_OPERATION_EXT,
>                    "%s(incomplete framebuffer)", where);
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 5e9453b..27670bd 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -2873,6 +2873,7 @@ struct gl_shader_program
>     GLboolean LinkStatus;   /**< GL_LINK_STATUS */
>     GLboolean Validated;
>     GLboolean _Used;        /**< Ever used for drawing? */
> +   GLboolean SamplersValidated; /**< Samplers validated against texture units? */
>     GLchar *InfoLog;
>  
>     unsigned Version;       /**< GLSL version used for linking */
> diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
> index 1592c9b..c2776c0 100644
> --- a/src/mesa/main/uniform_query.cpp
> +++ b/src/mesa/main/uniform_query.cpp
> @@ -1064,42 +1064,16 @@ extern "C" bool
>  _mesa_sampler_uniforms_are_valid(const struct gl_shader_program *shProg,
>  				 char *errMsg, size_t errMsgLength)
>  {
> -   const glsl_type *unit_types[MAX_COMBINED_TEXTURE_IMAGE_UNITS];
> -
> -   memset(unit_types, 0, sizeof(unit_types));
> -
> -   for (unsigned i = 0; i < shProg->NumUserUniformStorage; i++) {
> -      const struct gl_uniform_storage *const storage =
> -	 &shProg->UniformStorage[i];
> -      const glsl_type *const t = (storage->type->is_array())
> -	 ? storage->type->fields.array : storage->type;
> -
> -      if (!t->is_sampler())
> -	 continue;
> -
> -      const unsigned count = MAX2(1, storage->type->array_size());
> -      for (unsigned j = 0; j < count; j++) {
> -	 const unsigned unit = storage->storage[j].i;
> -
> -	 /* The types of the samplers associated with a particular texture
> -	  * unit must be an exact match.  Page 74 (page 89 of the PDF) of the
> -	  * OpenGL 3.3 core spec says:
> -	  *
> -	  *     "It is not allowed to have variables of different sampler
> -	  *     types pointing to the same texture image unit within a program
> -	  *     object."
> -	  */
> -	 if (unit_types[unit] == NULL) {
> -	    unit_types[unit] = t;
> -	 } else if (unit_types[unit] != t) {
> -	    _mesa_snprintf(errMsg, errMsgLength,
> -			   "Texture unit %d is accessed both as %s and %s",
> -			   unit, unit_types[unit]->name, t->name);
> -	    return false;
> -	 }
> -      }
> +   /* Shader does not have samplers. */
> +   if (shProg->NumUserUniformStorage == 0)
> +      return true;
> +
> +   if (!shProg->SamplersValidated) {
> +      _mesa_snprintf(errMsg, errMsgLength,
> +                     "active samplers with a different type "
> +                     "refer to the same texture image unit");
> +      return false;
>     }
> -
>     return true;
>  }
>  
> diff --git a/src/mesa/main/uniforms.c b/src/mesa/main/uniforms.c
> index 0d0cbf5..598b4d4 100644
> --- a/src/mesa/main/uniforms.c
> +++ b/src/mesa/main/uniforms.c
> @@ -75,12 +75,26 @@ _mesa_update_shader_textures_used(struct gl_shader_program *shProg,
>     memcpy(prog->SamplerUnits, shader->SamplerUnits, sizeof(prog->SamplerUnits));
>     memset(prog->TexturesUsed, 0, sizeof(prog->TexturesUsed));
>  
> +   shProg->SamplersValidated = GL_TRUE;
> +
>     for (s = 0; s < MAX_SAMPLERS; s++) {
>        if (prog->SamplersUsed & (1 << s)) {
>           GLuint unit = shader->SamplerUnits[s];
>           GLuint tgt = shader->SamplerTargets[s];
>           assert(unit < Elements(prog->TexturesUsed));
>           assert(tgt < NUM_TEXTURE_TARGETS);
> +
> +         /* The types of the samplers associated with a particular texture
> +          * unit must be an exact match.  Page 74 (page 89 of the PDF) of the
> +          * OpenGL 3.3 core spec says:
> +          *
> +          *     "It is not allowed to have variables of different sampler
> +          *     types pointing to the same texture image unit within a program
> +          *     object."
> +          */
> +         if (prog->TexturesUsed[unit] & ~(1 << tgt))
> +            shProg->SamplersValidated = GL_FALSE;
> +
>           prog->TexturesUsed[unit] |= (1 << tgt);
>        }
>     }
> -- 
> 1.9.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141020/3332ce6e/attachment.sig>