[Mesa-dev] [PATCH 4/8] mesa: Only validate shaders that can exist in the context

[Kenneth Graunke]

On Friday, December 19, 2014 02:20:55 PM Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
> 
> On Bay Trail-D using Fedora 20 compile flags (-m64 -O2 -mtune=generic
> for 64-bit and -m32 -march=i686 -mtune=atom for 32-bit), affects
> Gl32Batch7:
> 
> 32-bit: Difference at 95.0% confidence 0.495267% +/- 0.202063% (n=40)
> 64-bit: Difference at 95.0% confidence 3.57576% +/- 0.288175% (n=40)
> 
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>

I'm guessing it would help legacy context programs even more.

> ---
>  src/mesa/main/context.c | 78 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 49 insertions(+), 29 deletions(-)
> 
> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> index 400c158..74f9004 100644
> --- a/src/mesa/main/context.c
> +++ b/src/mesa/main/context.c
> @@ -1903,49 +1903,69 @@ shader_linked_or_absent(struct gl_context *ctx,
>  GLboolean
>  _mesa_valid_to_render(struct gl_context *ctx, const char *where)
>  {
> -   bool from_glsl_shader[MESA_SHADER_COMPUTE] = { false };
>     unsigned i;
>  
>     /* This depends on having up to date derived state (shaders) */
>     if (ctx->NewState)
>        _mesa_update_state(ctx);
>  
> -   for (i = 0; i < MESA_SHADER_COMPUTE; i++) {
> -      if (!shader_linked_or_absent(ctx, ctx->_Shader->CurrentProgram[i],
> -                                   &from_glsl_shader[i], where))
> -         return GL_FALSE;
> -   }
> +   if (ctx->API == API_OPENGL_CORE || ctx->API == API_OPENGLES2) {
> +      bool from_glsl_shader[MESA_SHADER_COMPUTE] = { false };
>  
> -   /* Any shader stages that are not supplied by the GLSL shader and have
> -    * assembly shaders enabled must now be validated.
> -    */
> -   if (!from_glsl_shader[MESA_SHADER_VERTEX]
> -       && ctx->VertexProgram.Enabled && !ctx->VertexProgram._Enabled) {
> -      _mesa_error(ctx, GL_INVALID_OPERATION,
> -		  "%s(vertex program not valid)", where);
> -      return GL_FALSE;
> -   }
> +      for (i = 0; i < MESA_SHADER_COMPUTE; i++) {
> +         if (!shader_linked_or_absent(ctx, ctx->_Shader->CurrentProgram[i],
> +                                      &from_glsl_shader[i], where))
> +            return GL_FALSE;
> +      }
>  
> -   /* FINISHME: If GL_NV_geometry_program4 is ever supported, the current
> -    * FINISHME: geometry program should validated here.
> -    */
> -   (void) from_glsl_shader[MESA_SHADER_GEOMETRY];
> +      /* In OpenGL Core Profile and OpenGL ES 2.0 / 3.0, there are no assembly
> +       * shaders.  Don't check state related to those.
> +       */

FWIW, I don't think we actually enforce this restriction.  Although we don't
advertise the extension, the API functions don't appear to contain any
API_OPENGL_CORE -> raise GL error code.

We might want to do that.  In the unlikely event that there are applications
attempting to use ARB programs in core profile without checking for the
extension, landing those checks first would make debugging the problem easier:

1. broken app somehow works
2. broken app starts returning INVALID_OPERATION
3. code to support broken applications is removed.

*shrug*.  It probably doesn't actually matter.

> +   } else {
> +      bool has_vertex_shader = false;
> +      bool has_fragment_shader = false;
> +
> +      /* In OpenGL Compatibility Profile, there is only vertex shader and
> +       * fragment shader.  We take this path also for API_OPENGLES because

"there is only vertex shader" sounds a little odd.  Perhaps "there are only
vertex and fragment shaders" or "there are only vertex and fragment shader stages"?

Whatever you decide is fine.

> +       * optimizing that path would make the other (more common) paths
> +       * slightly slower.
> +       */
> +      if (!shader_linked_or_absent(ctx,
> +                                   ctx->_Shader->CurrentProgram[MESA_SHADER_VERTEX],
> +                                   &has_vertex_shader, where))
> +         return GL_FALSE;
>  
> -   if (!from_glsl_shader[MESA_SHADER_FRAGMENT]) {
> -      if (ctx->FragmentProgram.Enabled && !ctx->FragmentProgram._Enabled) {
> -	 _mesa_error(ctx, GL_INVALID_OPERATION,
> -		     "%s(fragment program not valid)", where);
> -	 return GL_FALSE;
> -      }
> +      if (!shader_linked_or_absent(ctx,
> +                                   ctx->_Shader->CurrentProgram[MESA_SHADER_FRAGMENT],
> +                                   &has_fragment_shader, where))
> +         return GL_FALSE;
>  
> -      /* If drawing to integer-valued color buffers, there must be an
> -       * active fragment shader (GL_EXT_texture_integer).
> +      /* Any shader stages that are not supplied by the GLSL shader and have
> +       * assembly shaders enabled must now be validated.
>         */
> -      if (ctx->DrawBuffer && ctx->DrawBuffer->_IntegerColor) {
> +      if (!has_vertex_shader
> +          && ctx->VertexProgram.Enabled && !ctx->VertexProgram._Enabled) {
>           _mesa_error(ctx, GL_INVALID_OPERATION,
> -                     "%s(integer format but no fragment shader)", where);
> +                     "%s(vertex program not valid)", where);
>           return GL_FALSE;
>        }
> +
> +      if (!has_fragment_shader) {
> +         if (ctx->FragmentProgram.Enabled && !ctx->FragmentProgram._Enabled) {
> +            _mesa_error(ctx, GL_INVALID_OPERATION,
> +                        "%s(fragment program not valid)", where);
> +            return GL_FALSE;
> +         }
> +
> +         /* If drawing to integer-valued color buffers, there must be an
> +          * active fragment shader (GL_EXT_texture_integer).
> +          */
> +         if (ctx->DrawBuffer && ctx->DrawBuffer->_IntegerColor) {
> +            _mesa_error(ctx, GL_INVALID_OPERATION,
> +                        "%s(integer format but no fragment shader)", where);
> +            return GL_FALSE;
> +         }
> +      }
>     }
>  
>     /* A pipeline object is bound */

I like how this handles compatibily and core separately.  It's much easier to
read that way.  We might also be able to merge in some of check_valid_to_render
someday...

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141221/b81e7b3c/attachment.sig>