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

[Ian Romanick]

On 12/21/2014 10:03 PM, Kenneth Graunke wrote:
> 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.

Dang it.  That was actually a typo.  Gl32Batch7 had no difference.
These were the timings for Gl21Batch7.  I'll fix that in the commit message.

>> ---
>>  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.

I thought we had it configured so that those functions weren't even in
the dispatch tables for core profile or ES.  I can whip up a piglit test...

> 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.

I wasn't too worried about it.  Pretty much all applications that use
the assembly shaders also use fixed-function state that doesn't exist in
core profile.  I think you'd really have to work to make an application
that was both core profile and assembly shader.

>> +   } 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.

Yes... that is weird.  I'll reword it.

>> +       * 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: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141222/09e1feab/attachment.sig>