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

[Emil Velikov]

On 27 November 2015 at 18:36, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Fri, Nov 27, 2015 at 1:31 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 27 November 2015 at 18:02, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>> On Fri, Nov 27, 2015 at 9:31 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>
>>>> ---
>>>>  src/glsl/builtin_variables.cpp  | 17 +++++------------
>>>>  src/glsl/glsl_parser.yy         |  4 ++--
>>>>  src/glsl/glsl_parser_extras.cpp |  1 +
>>>>  src/glsl/glsl_parser_extras.h   |  2 ++
>>>>  4 files changed, 10 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/src/glsl/builtin_variables.cpp b/src/glsl/builtin_variables.cpp
>>>> index e8eab80..6a53789 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->is_version(150, 320) || state->OES_geometry_shader_enable) {
>>>>        add_const("gl_MaxVertexOutputComponents",
>>>>                  state->Const.MaxVertexOutputComponents);
>>>>        add_const("gl_MaxGeometryInputComponents",
>>>> @@ -729,11 +729,7 @@ builtin_variable_generator::generate_constants()
>>>>                  state->Const.MaxCombinedAtomicCounters);
>>>>        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->es_shader || state->OES_geometry_shader_enable) {
>>>>           add_const("gl_MaxGeometryAtomicCounters",
>>>>                     state->Const.MaxGeometryAtomicCounters);
>>>>           add_const("gl_MaxTessControlAtomicCounters",
>>>
>>> Do you really want to be adding this in for OES_geometry_shader?
>>>
>>>> @@ -753,10 +749,7 @@ 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->es_shader || state->OES_geometry_shader_enable) {
>>>>           add_const("gl_MaxGeometryAtomicCounterBuffers",
>>>>                     state->Const.MaxGeometryAtomicCounterBuffers);
>>>>           add_const("gl_MaxTessControlAtomicCounterBuffers",
>>>> @@ -814,7 +807,7 @@ builtin_variable_generator::generate_constants()
>>>>        add_const("gl_MaxCombinedImageUniforms",
>>>>                  state->Const.MaxCombinedImageUniforms);
>>>>
>>>> -      if (!state->es_shader) {
>>>> +      if (!state->es_shader || state->OES_geometry_shader_enable) {
>>>>           add_const("gl_MaxCombinedImageUnitsAndFragmentOutputs",
>>>>                     state->Const.MaxCombinedShaderOutputResources);
>>>>           add_const("gl_MaxImageSamples",
>>>> @@ -1057,7 +1050,7 @@ builtin_variable_generator::generate_fs_special_vars()
>>>>     if (state->is_version(120, 100))
>>>>        add_input(VARYING_SLOT_PNTC, vec2_t, "gl_PointCoord");
>>>>
>>>> -   if (state->is_version(150, 0)) {
>>>> +   if (state->is_version(150, 320) || state->OES_geometry_shader_enable) {
>>>>        var = add_input(VARYING_SLOT_PRIMITIVE_ID, int_t, "gl_PrimitiveID");
>>>>        var->data.interpolation = INTERP_QUALIFIER_FLAT;
>>>>     }
>>>> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
>>>> index 5a8f980..fae6d0b 100644
>>>> --- a/src/glsl/glsl_parser.yy
>>>> +++ b/src/glsl/glsl_parser.yy
>>>> @@ -1262,7 +1262,7 @@ layout_qualifier_id:
>>>>              }
>>>>           }
>>>>
>>>> -         if ($$.flags.i && !state->is_version(150, 0)) {
>>>> +         if ($$.flags.i && !state->is_version(150, 320) && !state->OES_geometry_shader_enable) {
>>>>              _mesa_glsl_error(& @1, state, "#version 150 layout "
>>>>                               "qualifier `%s' used", $1);
>>>>           }
>>>> @@ -1499,7 +1499,7 @@ layout_qualifier_id:
>>>>        if (match_layout_qualifier("max_vertices", $1, state) == 0) {
>>>>           $$.flags.q.max_vertices = 1;
>>>>           $$.max_vertices = new(ctx) ast_layout_expression(@1, $3);
>>>> -         if (!state->is_version(150, 0)) {
>>>> +         if (!state->is_version(150, 310)) {
>>>
>>> Why is this one different? Shouldn't this also be the same as the above check?
>>>
>> Depends on how much the OES_geometry_shader text differs from the one
>> in GLES 3.2. If they are the same - one can fix the
>> _mesa_glsl_supported_extensions[] handling to use the existing
>> _mesa_has_#extension_name() helpers from Nanley and drop the GLES
>> version checks above.
>
> Nope, those helpers would be of no help here. An entirely separate set
> of helpers could be made, of course, but that's wholly unrelated to
> this change. And it has absolutely nothing with the comment I made,
> which was why checking for 3.10 here but 3.20 above, and not checking
> for the OES_geometry_shader enable.
>
Actually things made perfect sense although I could have been too subtle.

>From a very quick look the functionality provided by the extension is
identical to the one in the core GLES 3.2 spec. Thus one could just
check for the extension boolean, omitting the ES version check. On the
other hand the extension enable is quite hacky (look at
compatible_with_state()), so instead of adding more error prone code
one can just reuse (ok build on top) of the existing helpers.

As usual I'm looking for a proper long term solution, which sadly
diverges from the core goal here :-\

Cheers,
Emil