> -----Original Message----- > From: ibmir...@gmail.com [mailto:ibmir...@gmail.com] On Behalf Of Ilia > Mirkin > Sent: Thursday, January 21, 2016 4:33 PM > To: Marta Lofstedt > Cc: mesa-dev@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.lofst...@linux.intel.com> wrote: > > From: Marta Lofstedt <marta.lofst...@intel.com> > > > > This adds glsl support of GL_OES_geometry_shader for OpenGL ES 3.1. > > > > Signed-off-by: Marta Lofstedt <marta.lofst...@linux.intel.com> > > Reviewed-by: Ian Romanick <ian.d.roman...@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 <imir...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
