[Mesa-dev] [PATCH 3/4] glsl: add support for GL_OES_geometry_shader

[Lofstedt, Marta]

> -----Original Message-----
> From: ibmirkin at gmail.com [mailto:ibmirkin at gmail.com] On Behalf Of Ilia
> Mirkin
> Sent: Thursday, January 21, 2016 4:33 PM
> To: Marta Lofstedt
> Cc: mesa-dev at lists.freedesktop.org; Lofstedt, Marta
> Subject: Re: [PATCH 3/4] glsl: add support for GL_OES_geometry_shader
> 
> On Thu, Jan 21, 2016 at 10:17 AM, Marta Lofstedt
> <marta.lofstedt at linux.intel.com> wrote:
> > From: Marta Lofstedt <marta.lofstedt at intel.com>
> >
> > This adds glsl support of GL_OES_geometry_shader for OpenGL ES 3.1.
> >
> > Signed-off-by: Marta Lofstedt <marta.lofstedt at linux.intel.com>
> > Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> > ---
> >  src/glsl/builtin_variables.cpp  | 25 +++++++++++++------------
> >  src/glsl/glsl_parser.yy         |  4 ++--
> >  src/glsl/glsl_parser_extras.cpp |  1 +
> >  src/glsl/glsl_parser_extras.h   |  7 +++++++
> >  4 files changed, 23 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/glsl/builtin_variables.cpp
> > b/src/glsl/builtin_variables.cpp index 221aab0..ccc04c0 100644
> > --- a/src/glsl/builtin_variables.cpp
> > +++ b/src/glsl/builtin_variables.cpp
> > @@ -667,7 +667,7 @@ builtin_variable_generator::generate_constants()
> >        add_const("gl_MaxVaryingComponents", state->ctx-
> >Const.MaxVarying * 4);
> >     }
> >
> > -   if (state->is_version(150, 0)) {
> > +   if (state->has_geometry_shader()) {
> >        add_const("gl_MaxVertexOutputComponents",
> >                  state->Const.MaxVertexOutputComponents);
> >        add_const("gl_MaxGeometryInputComponents",
> > @@ -730,12 +730,11 @@ builtin_variable_generator::generate_constants()
> >        add_const("gl_MaxAtomicCounterBindings",
> >                  state->Const.MaxAtomicBufferBindings);
> >
> > -      /* When Mesa adds support for GL_OES_geometry_shader and
> > -       * GL_OES_tessellation_shader, this will need to change.
> > -       */
> > -      if (!state->es_shader) {
> > +      if (state->has_geometry_shader()) {
> >           add_const("gl_MaxGeometryAtomicCounters",
> >                     state->Const.MaxGeometryAtomicCounters);
> > +      }
> > +      if (!state->es_shader) {
> >           add_const("gl_MaxTessControlAtomicCounters",
> >                     state->Const.MaxTessControlAtomicCounters);
> >           add_const("gl_MaxTessEvaluationAtomicCounters",
> > @@ -753,12 +752,11 @@ builtin_variable_generator::generate_constants()
> >        add_const("gl_MaxAtomicCounterBufferSize",
> >                  state->Const.MaxAtomicCounterBufferSize);
> >
> > -      /* When Mesa adds support for GL_OES_geometry_shader and
> > -       * GL_OES_tessellation_shader, this will need to change.
> > -       */
> > -      if (!state->es_shader) {
> > +      if (state->has_geometry_shader()) {
> >           add_const("gl_MaxGeometryAtomicCounterBuffers",
> >                     state->Const.MaxGeometryAtomicCounterBuffers);
> > +      }
> > +      if (!state->es_shader) {
> >           add_const("gl_MaxTessControlAtomicCounterBuffers",
> >                     state->Const.MaxTessControlAtomicCounterBuffers);
> >           add_const("gl_MaxTessEvaluationAtomicCounterBuffers",
> > @@ -814,13 +812,16 @@ builtin_variable_generator::generate_constants()
> >        add_const("gl_MaxCombinedImageUniforms",
> >                  state->Const.MaxCombinedImageUniforms);
> >
> > +      if (state->has_geometry_shader()) {
> > +         add_const("gl_MaxGeometryImageUniforms",
> > +                   state->Const.MaxGeometryImageUniforms);
> > +      }
> 
> So.... this is *really* pedantic, and I think you can disregard my comment.
> However, I'd like to say it for posterity anyways:
> 
> ARB_shader_image_load_store defines a bunch of interactions, and none of
> them talk about removing gl_MaxGeometryImageUniforms when you don't
> have GL 3.2. I suspect similar is true of atomic counters but I didn't check
> explicitly.
> 
> If you want to be super-accurate, you'd make the formerly "if (!state-
> >es_shader)" blocks read something like
> 
> if (!state->es_shader || state->has_geometry_shader())
> 
> I also vaguely recall Ian talking about this. I forget what he said.
> If he said anything that conflicts with what I said, trust him, not me.
> 
> Either way, this is
> 
> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>

Thanks for the review and the comments. Since Ian already reviewed this patch and didn't comment on your comments. I will go for what I already had.

/Marta