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 <[email protected]> > Follows a list where gl_MaxViewports is. > > 2014-12-12 22:12 GMT+01:00 Maxence Le Doré <[email protected] > <mailto:[email protected]>>: > > 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 <[email protected] > <mailto:[email protected]>>: > > On Tue, Dec 9, 2014 at 11:09 PM, Maxence Le Doré > <[email protected] <mailto:[email protected]>> 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é <[email protected] > <mailto:[email protected]>>: > > 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 <[email protected] > <mailto:[email protected]>>: > > On Tue, Dec 9, 2014 at 11:09 PM, Maxence Le Doré > <[email protected] <mailto:[email protected]>> 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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
