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

[Eric Anholt]

Ian Romanick <idr at freedesktop.org> writes:

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

Yeah, I understood what was happening here.  I was assuming, though,
that if you have a successful lookup of a shProg, then you don't need to
throw an error if there happens to be a shader with the same name (if
that's even possible).  So the code doesn't need to do the extra lookup
in the success case or look confusing at all.

>>> +
>>> +      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."
>     */

That seems fine.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140313/91c2fef4/attachment-0001.pgp>