[Piglit] [PATCH] built-in-constants: check required GLSL versions for #extensions

[Ilia Mirkin]

On Tue, Nov 14, 2017 at 2:43 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Tue, Nov 14, 2017 at 2:26 PM, Juan A. Suarez Romero
> <jasuarez at igalia.com> wrote:
>> On Mon, 2017-11-13 at 13:24 -0500, Ilia Mirkin wrote:
>>> On Mon, Nov 13, 2017 at 12:48 PM, Juan A. Suarez Romero
>>> <jasuarez at igalia.com> wrote:
>>> > GL_OES_geometry_shader and GL_OES_tessellation_shader specifications
>>> > require OpenGL ES Shading Language 3.10.
>>> >
>>> > GL_ARB_tessellation_shader specification requires GLSL 1.50.
>>> > ---
>>> >  tests/shaders/built-in-constants.c | 8 +++++---
>>> >  1 file changed, 5 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/tests/shaders/built-in-constants.c b/tests/shaders/built-in-constants.c
>>> > index d470fe1bf..6ad26dccf 100644
>>> > --- a/tests/shaders/built-in-constants.c
>>> > +++ b/tests/shaders/built-in-constants.c
>>> > @@ -386,7 +386,7 @@ create_shader(GLenum type)
>>> >                         return 0;
>>> >         } else {
>>> >                 if (is_tessellation_type(type) &&
>>> > -                   (required_glsl_version < 400 &&
>>> > +                   (required_glsl_version < 150 ||
>>> >                      !piglit_is_extension_supported("GL_ARB_tessellation_shader")))
>>>
>>> I believe this code was correct as-is. With your version, you'll
>>> disallow a shader with #version 400 when the impl doesn't advertise
>>> the separate GL_ARB_tessellation_shader (which there's no requirement
>>> it advertise, AFAIK).
>>>
>>
>> Yes, you're right. I think this hunk was added in the patchset by
>> mistake.
>>
>>
>>> The one thing is that it'll allow shaders through with
>>> required_glsl_version < 150 if the impl supports tess, but ... hard to
>>> care about that.
>>>
>>> >                         return 0;
>>> >
>>> > @@ -461,7 +461,7 @@ piglit_init(int argc, char **argv)
>>> >         /* Geometry shaders must use the #extension directive in GLSL ES
>>> >          * before version 3.20.
>>> >          */
>>> > -       if (es_shader && required_glsl_version < 320 &&
>>> > +       if (es_shader && required_glsl_version == 310 &&
>>> >             piglit_is_extension_supported("GL_OES_geometry_shader")) {
>>> >                 assert(num_required_extensions < ARRAY_SIZE(required_extensions));
>>> >                 required_extensions[num_required_extensions] =
>>>
>>> What situation are you trying to fix here?
>>>
>>> > @@ -473,7 +473,9 @@ piglit_init(int argc, char **argv)
>>> >         const char *const tess_ext_name = es_shader
>>> >                 ? "GL_OES_tessellation_shader"
>>> >                 : "GL_ARB_tessellation_shader";
>>> > -       if (piglit_is_extension_supported(tess_ext_name)) {
>>> > +       if (((es_shader && required_glsl_version == 310) ||
>>> > +           (!es_shader && required_glsl_version >= 150)) &&
>>> > +           piglit_is_extension_supported(tess_ext_name)) {
>>> >                 assert(num_required_extensions < ARRAY_SIZE(required_extensions));
>>> >                 required_extensions[num_required_extensions] =
>>> >                         strdup(tess_ext_name);
>>>
>>> And here?
>>>
>>
>>
>> First all, I'll push a new version of the patch, as I did some changes
>> there.
>>
>>
>> The fix I am trying to do is that in order to declare
>> OES_tessellation_shader or OES_geometry_shader extensions in the
>> shader, OpenGL ES Shading Language must be 3.10 or newer.
>>
>> Thus we need to avoid adding those extensions in the shader code on
>> older versions.
>
> Is this for a hypothetical implementation that advertises
> GL_OES_geometry_shader but not ES 3.1?

Oh, I see. You could have a file that specifies ES 3.0, and in that
case you don't want to start sending it geometry shaders.