On 27 November 2015 at 19:00, Ilia Mirkin <imir...@alum.mit.edu> wrote:
> On Fri, Nov 27, 2015 at 1:55 PM, Emil Velikov <emil.l.veli...@gmail.com>
> wrote:
>> On 27 November 2015 at 18:36, Ilia Mirkin <imir...@alum.mit.edu> wrote:
>>> On Fri, Nov 27, 2015 at 1:31 PM, Emil Velikov <emil.l.veli...@gmail.com>
>>> wrote:
>>>> On 27 November 2015 at 18:02, Ilia Mirkin <imir...@alum.mit.edu> wrote:
>>>>> On Fri, Nov 27, 2015 at 9:31 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>
>>>>>> ---
>>>>>> 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.
> And *still* unrelated to my comment, which is that the changed check
> is checking for the wrong thing.
Yet with the above [fixes/patches] in place, this type of mistake
would be close to impossible to make.
I guess I missed out the obvious - "yes there is something wrong here.
Here is how we can avoid these (make things robust) in the future..."
Hope this time things are clearer now. If not... oh well.
-Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
