[Mesa-dev] [PATCH 07/12] mesa/sso: Implement _mesa_UseProgramStages

[Ian Romanick]

On 03/05/2014 12:35 PM, Eric Anholt wrote:
> Ian Romanick <idr at freedesktop.org> writes:
> 
>> From: Gregory Hainaut <gregory.hainaut at gmail.com>
>>
>> Now arb_separate_shader_object-GetProgramPipelineiv should pass.
>>
>> V3 (idr):
>> * Change spec references to core OpenGL versions instead of issues in
>>   the extension spec.
>> * Split out from previous uber patch.
>>
>> v4 (idr): Use _mesa_has_geometry_shaders in _mesa_UseProgramStages to
>> detect availability of geometry shaders.
>>
>> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>  src/mesa/main/pipelineobj.c | 115 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 115 insertions(+)
>>
>> diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c
>> index 849c781..7149578 100644
>> --- a/src/mesa/main/pipelineobj.c
>> +++ b/src/mesa/main/pipelineobj.c
> 
>> +   GLbitfield any_valid_stages = GL_VERTEX_SHADER_BIT | GL_FRAGMENT_SHADER_BIT;
>> +   if (_mesa_has_geometry_shaders(ctx))
>> +      any_valid_stages |= GL_GEOMETRY_SHADER_BIT;
>> +
>> +   if (stages != GL_ALL_SHADER_BITS && (stages  & ~any_valid_stages) != 0) {
> 
> Weird double space before &.
> 
>> +      _mesa_error(ctx, GL_INVALID_VALUE, "glUseProgramStages(Stages)");
>> +      return;
>> +   }
> 
>> +   if (program) {
>> +      /* Section 2.11.1 (Shader Objects) of the OpenGL 3.1 spec (and probably
>> +       * earlier) says:
>> +       *
>> +       *     "Commands that accept shader or program object names will
>> +       *     generate the error INVALID_VALUE if the provided name is not the
>> +       *     name of either a shader or program object and INVALID_OPERATION
>> +       *     if the provided name identifies an object that is not the
>> +       *     expected type."
>> +       */
>> +      struct gl_shader *sh = _mesa_lookup_shader(ctx, program);
>> +      if (sh != NULL) {
>> +         _mesa_error(ctx, GL_INVALID_OPERATION,
>> +               "glUseProgramStages(progam is a shader object)");
>> +         return;
>> +      }
> 
> This block could get dropped into the shProg == NULL case below, right?
> The code confused me as is.

The first block is checking whether the name is a shader, and the other
is checking that the name is a program.  The neat thing is the spec says
using a shader where a program is expected gives one error, but using a
name that is neither give a different error.

You've never seen code like this elsewhere in Mesa because it's usually
handled by _mesa_lookup_shader_program_err.  I'll change this code to
use that function instead.

>> +
>> +      shProg = _mesa_lookup_shader_program(ctx, program);
>> +      if (shProg == NULL) {
>> +         _mesa_error(ctx, GL_INVALID_VALUE,
>> +               "glUseProgramStages(progam is not a program object)");
>> +         return;
>> +      }
> 
> 
>> +   /* Section 2.11.4 (Program Pipeline Objects) of the OpenGL 4.1 spec
>> +    * says:
>> +    *
>> +    *     "If UseProgramStages is called with program set to zero or with a
>> +    *     program object that contains no executable code for the given
>> +    *     stages, it is as if the pipeline object has no programmable stage
>> +    *     configured for the indicated shader stages."
>> +    */
>> +   if ((stages & GL_VERTEX_SHADER_BIT) != 0)
>> +      _mesa_use_shader_program(ctx, GL_VERTEX_SHADER, shProg, pipe);
>> +
>> +   if ((stages & GL_FRAGMENT_SHADER_BIT) != 0)
>> +      _mesa_use_shader_program(ctx, GL_FRAGMENT_SHADER, shProg, pipe);
>> +
>> +   if ((stages & GL_GEOMETRY_SHADER_BIT) != 0)
>> +      _mesa_use_shader_program(ctx, GL_GEOMETRY_SHADER, shProg, pipe);
>>  }
> 
> The spec cite here doesn't seem to be explaining the code it's next to,
> to me.

That is fair... and at least partially my fault.  Greg's original
comment above this code was:

   /*
    *  7.  What happens if you have a program object current for a shader stage,
    *    but the program object doesn't contain an executable for that stage?

    *    RESOLVED:  This is not an error; instead it is as though there were no
    *    program bound to that stage.  We have two different notions for
    *    programs bound to shader stages.  A program is "current" for a stage
    *    if it bound to that stage in the active program pipeline object.  A
    *    program is "active" for a stage if it is current and it has an
    *    executable for this stage.  In this case, the program would be current
    *    but not active.

    *    When no program is active for a stage, the stage will be replaced with
    *    fixed functionality logic (compatibility profile vertex and fragment),
    *    disabled (tessellation control and evaluation, geometry), or have
    *    undefined results (core profile vertex and fragment).
    */

Since the issues aren't captured in the core spec, I changed it to the
nearest thing I could find in the core spec.  Comparing the code with
the old comment and the new comment, I'm not very happy with it either.

How about:

   /* Enable individual stages from the program as requested by the application.
    * If there is no shader for a requested stage in the program,
    * _mesa_use_shader_program will enable fixed-function processing as dictated
    * by the spec.
    *
    * Section 2.11.4 (Program Pipeline Objects) of the OpenGL 4.1 spec
    * says:
    *
    *     "If UseProgramStages is called with program set to zero or with a
    *     program object that contains no executable code for the given
    *     stages, it is as if the pipeline object has no programmable stage
    *     configured for the indicated shader stages."
    */

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140312/74f806bb/attachment.pgp>