[Mesa-dev] [PATCH] glsl: Add gl_MaxViewports to available builtin constants

[Ian Romanick]

On 12/14/2014 11:47 AM, Maxence Le Doré wrote:
> Hi Matt,
> 
> To be sure to not missing something, I used a simple vertex shader I
> have there and had
> the following line in :
> 
> int a = gl_MaxDrawBuffers;
> 
> At runtime, everything was ok, and GLSL compiler didn't complain about
> anything. So there is nothing in mesa to prevent the use of some GLSL
> constants having some specifity to one or more shading stage.

There used to be some values that were only per-stage, but it looks like
we don't do that any more.  Weird.  It seems like there used to be a bug
and / or a piglit test related to this.

> But a more interesting stuff is when I took a look at GLSL 4.1 spec there :
> https://www.opengl.org/registry/doc/GLSLangSpec.4.10.6.clean.pdf
> 
> See section 7.3
> 
> "Built-In Constants
> The following built-in constants are provided to all shaders. The actual
> values used are implementation dependent, but must be at least the value
> shown. Some are deprecated, as indicated in comments."

Yeah, I think your patch is correct as-is.

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> Follows a list where gl_MaxViewports is.
> 
> 2014-12-12 22:12 GMT+01:00 Maxence Le Doré <maxence.ledore at gmail.com
> <mailto:maxence.ledore at gmail.com>>:
> 
>     I had exactly the same feeling while adding it. How to make it GS
>     specific (and eventually allow access to VS,
>     where AMD_vertex_shader_viewport_index is supported, even if the
>     spec don't say a word about gl_MaxViewports). In that kind of
>     situation, I always take a look at a similar case nearby :
>     gl_MaxDrawBuffers definition specificity to fragment shader stage.
>     And I haven't seen how glsl compiler in its current state prevent it
>     from being enabled in other stage too.
> 
>     2014-12-12 18:19 GMT+01:00 Matt Turner <mattst88 at gmail.com
>     <mailto:mattst88 at gmail.com>>:
> 
>         On Tue, Dec 9, 2014 at 11:09 PM, Maxence Le Doré
>         <maxence.ledore at gmail.com <mailto:maxence.ledore at gmail.com>> wrote:
>         > It seems to have been forgotten during viewports array
>         implementation time.
>         > ---
>         >  src/glsl/builtin_variables.cpp  | 4 ++++
>         >  src/glsl/glsl_parser_extras.cpp | 3 +++
>         >  src/glsl/glsl_parser_extras.h   | 3 +++
>         >  3 files changed, 10 insertions(+)
>         >
>         > diff --git a/src/glsl/builtin_variables.cpp
>         b/src/glsl/builtin_variables.cpp
>         > index c36d198..65e32ad 100644
>         > --- a/src/glsl/builtin_variables.cpp
>         > +++ b/src/glsl/builtin_variables.cpp
>         > @@ -724,6 +724,10 @@
>         builtin_variable_generator::generate_constants()
>         >        add_const("gl_MaxCombinedImageUniforms",
>         >                  state->Const.MaxCombinedImageUniforms);
>         >     }
>         > +
>         > +   if (state->is_version(410, 0) ||
>         > +       state->ARB_viewport_array_enable)
>         > +      add_const("gl_MaxViewports", state->Const.MaxViewports);
>         >  }
> 
>         Nice find. The only thing that's holding back my review is that the
>         specification says
> 
>             Add to the list of built in constants available to geometry
>         shaders in
>             Section 7.4:
> 
>                 const int gl_MaxViewports = 16;
> 
>         and I don't see how we're preventing gl_MaxViewports from being
>         enabled in other shader stages.
> 
>         Maybe idr can confirm.
> 
> 
> 2014-12-12 22:12 GMT+01:00 Maxence Le Doré <maxence.ledore at gmail.com
> <mailto:maxence.ledore at gmail.com>>:
> 
>     I had exactly the same feeling while adding it. How to make it GS
>     specific (and eventually allow access to VS,
>     where AMD_vertex_shader_viewport_index is supported, even if the
>     spec don't say a word about gl_MaxViewports). In that kind of
>     situation, I always take a look at a similar case nearby :
>     gl_MaxDrawBuffers definition specificity to fragment shader stage.
>     And I haven't seen how glsl compiler in its current state prevent it
>     from being enabled in other stage too.
> 
>     2014-12-12 18:19 GMT+01:00 Matt Turner <mattst88 at gmail.com
>     <mailto:mattst88 at gmail.com>>:
> 
>         On Tue, Dec 9, 2014 at 11:09 PM, Maxence Le Doré
>         <maxence.ledore at gmail.com <mailto:maxence.ledore at gmail.com>> wrote:
>         > It seems to have been forgotten during viewports array
>         implementation time.
>         > ---
>         >  src/glsl/builtin_variables.cpp  | 4 ++++
>         >  src/glsl/glsl_parser_extras.cpp | 3 +++
>         >  src/glsl/glsl_parser_extras.h   | 3 +++
>         >  3 files changed, 10 insertions(+)
>         >
>         > diff --git a/src/glsl/builtin_variables.cpp
>         b/src/glsl/builtin_variables.cpp
>         > index c36d198..65e32ad 100644
>         > --- a/src/glsl/builtin_variables.cpp
>         > +++ b/src/glsl/builtin_variables.cpp
>         > @@ -724,6 +724,10 @@
>         builtin_variable_generator::generate_constants()
>         >        add_const("gl_MaxCombinedImageUniforms",
>         >                  state->Const.MaxCombinedImageUniforms);
>         >     }
>         > +
>         > +   if (state->is_version(410, 0) ||
>         > +       state->ARB_viewport_array_enable)
>         > +      add_const("gl_MaxViewports", state->Const.MaxViewports);
>         >  }
> 
>         Nice find. The only thing that's holding back my review is that the
>         specification says
> 
>             Add to the list of built in constants available to geometry
>         shaders in
>             Section 7.4:
> 
>                 const int gl_MaxViewports = 16;
> 
>         and I don't see how we're preventing gl_MaxViewports from being
>         enabled in other shader stages.
> 
>         Maybe idr can confirm.
> 
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>