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

[Ilia Mirkin]

On Fri, Nov 27, 2015 at 2:12 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 27 November 2015 at 19:00, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> On Fri, Nov 27, 2015 at 1:55 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> 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
>>
>> Let's assume that it is.
>>
>>> 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.
>>
>> On the other other hand, those helpers are for gl_context, not for the
>> glsl compiler state (I forget the struct name). They're just for a
>> totally different thing.
>>
> Hmm isn't the compiler state dependent on the API used ? I.e. if the
> API does not support foo, the glsl compiler state should reflect that.

Getting off-topic here. You might have GL 4.5 support, but if I hand you a

#version 130

shader, then you have to pretend it's GL 3.0-land. Similarly with
various extension enables. The mesa parser state keeps track of all
that.

  -ilia