[Mesa-dev] [PATCH 4/7] glsl: add core plumbing for GL_ANDROID_extension_pack_es31a

[Matt Turner]

On Sun, Aug 28, 2016 at 7:10 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>  src/compiler/glsl/glsl_parser_extras.cpp | 57 +++++++++++++++++++++++---------
>  src/compiler/glsl/glsl_parser_extras.h   |  2 ++
>  src/mesa/main/extensions_table.h         |  2 ++
>  src/mesa/main/mtypes.h                   |  1 +
>  4 files changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp b/src/compiler/glsl/glsl_parser_extras.cpp
> index b33cd3a..a44f014 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -523,6 +523,11 @@ struct _mesa_glsl_extension {
>     const char *name;
>
>     /**
> +    * Whether this extension is a part of AEP
> +    */
> +   bool aep;

In reviewing this patch, I developed some doubts whether this is the
right approach. Looking at the list of extensions in the AEP spec,
it's difficult to compare with our code to confirm that they're all
included.

Take OES_sample_shading for instance. It's enabled if
OES_sample_variables is. OES_sample_variables is also in AEP, so using
EXT_AEP seems fine, but it takes a bit of analysis.

Or take EXT_copy_image. It's enabled based on OES_copy_image. Does
that mean that OES_copy_image should use EXT_AEP even though it's not
part of AEP?

I wonder if handling AEP in version.c like we do with the ver_3_0
variables and the list of extensions would be better.

> +
> +   /**
>      * Predicate that checks whether the relevant extension is available for
>      * this context.
>      */
> @@ -565,9 +570,14 @@ has_##name_str(const struct gl_context *ctx, gl_api api, uint8_t version) \
>  #undef EXT
>
>  #define EXT(NAME)                                           \
> -   { "GL_" #NAME, has_##NAME,                         \
> -         &_mesa_glsl_parse_state::NAME##_enable,            \
> -         &_mesa_glsl_parse_state::NAME##_warn }
> +   { "GL_" #NAME, false, has_##NAME,                        \
> +     &_mesa_glsl_parse_state::NAME##_enable,                \
> +     &_mesa_glsl_parse_state::NAME##_warn }
> +
> +#define EXT_AEP(NAME)                                       \
> +   { "GL_" #NAME, true, has_##NAME,                         \
> +     &_mesa_glsl_parse_state::NAME##_enable,                \
> +     &_mesa_glsl_parse_state::NAME##_warn }
>
>  /**
>   * Table of extensions that can be enabled/disabled within a shader,
> @@ -623,7 +633,7 @@ static const _mesa_glsl_extension _mesa_glsl_supported_extensions[] = {
>
>     /* KHR extensions go here, sorted alphabetically.
>      */
> -   EXT(KHR_blend_equation_advanced),
> +   EXT_AEP(KHR_blend_equation_advanced),
>
>     /* OES extensions go here, sorted alphabetically.
>      */
> @@ -632,17 +642,17 @@ static const _mesa_glsl_extension _mesa_glsl_supported_extensions[] = {
>     EXT(OES_geometry_shader),
>     EXT(OES_gpu_shader5),
>     EXT(OES_primitive_bounding_box),
> -   EXT(OES_sample_variables),
> -   EXT(OES_shader_image_atomic),
> +   EXT_AEP(OES_sample_variables),
> +   EXT_AEP(OES_shader_image_atomic),
>     EXT(OES_shader_io_blocks),
> -   EXT(OES_shader_multisample_interpolation),
> +   EXT_AEP(OES_shader_multisample_interpolation),
>     EXT(OES_standard_derivatives),
>     EXT(OES_tessellation_point_size),
>     EXT(OES_tessellation_shader),
>     EXT(OES_texture_3D),
>     EXT(OES_texture_buffer),
>     EXT(OES_texture_cube_map_array),
> -   EXT(OES_texture_storage_multisample_2d_array),
> +   EXT_AEP(OES_texture_storage_multisample_2d_array),
>
>     /* All other extensions go here, sorted alphabetically.
>      */
> @@ -651,23 +661,24 @@ static const _mesa_glsl_extension _mesa_glsl_supported_extensions[] = {
>     EXT(AMD_shader_trinary_minmax),
>     EXT(AMD_vertex_shader_layer),
>     EXT(AMD_vertex_shader_viewport_index),
> +   EXT(ANDROID_extension_pack_es31a),
>     EXT(EXT_blend_func_extended),
>     EXT(EXT_draw_buffers),
>     EXT(EXT_clip_cull_distance),
>     EXT(EXT_geometry_point_size),
> -   EXT(EXT_geometry_shader),
> -   EXT(EXT_gpu_shader5),
> -   EXT(EXT_primitive_bounding_box),
> +   EXT_AEP(EXT_geometry_shader),
> +   EXT_AEP(EXT_gpu_shader5),
> +   EXT_AEP(EXT_primitive_bounding_box),
>     EXT(EXT_separate_shader_objects),
>     EXT(EXT_shader_framebuffer_fetch),
>     EXT(EXT_shader_integer_mix),
> -   EXT(EXT_shader_io_blocks),
> +   EXT_AEP(EXT_shader_io_blocks),
>     EXT(EXT_shader_samples_identical),
>     EXT(EXT_tessellation_point_size),
> -   EXT(EXT_tessellation_shader),
> +   EXT_AEP(EXT_tessellation_shader),
>     EXT(EXT_texture_array),
> -   EXT(EXT_texture_buffer),
> -   EXT(EXT_texture_cube_map_array),
> +   EXT_AEP(EXT_texture_buffer),
> +   EXT_AEP(EXT_texture_cube_map_array),
>     EXT(MESA_shader_integer_functions),

I think we're missing:

KHR_texture_compression_astc_ldr
OES_texture_stencil8
EXT_copy_image
EXT_draw_buffers_indexed
EXT_texture_border_clamp
EXT_texture_sRGB_decode

>  };
>
> @@ -713,7 +724,6 @@ static const _mesa_glsl_extension *find_extension(const char *name)
>     return NULL;
>  }
>
> -
>  bool
>  _mesa_glsl_process_extension(const char *name, YYLTYPE *name_locp,
>                              const char *behavior_string, YYLTYPE *behavior_locp,
> @@ -768,6 +778,21 @@ _mesa_glsl_process_extension(const char *name, YYLTYPE *name_locp,
>        const _mesa_glsl_extension *extension = find_extension(name);
>        if (extension && extension->compatible_with_state(state, api, gl_version)) {
>           extension->set_flags(state, behavior);
> +         if (extension->available_pred == has_ANDROID_extension_pack_es31a) {
> +            for (unsigned i = 0;
> +                 i < ARRAY_SIZE(_mesa_glsl_supported_extensions); ++i) {
> +               const _mesa_glsl_extension *extension =
> +                  &_mesa_glsl_supported_extensions[i];
> +
> +               if (!extension->aep)
> +                  continue;
> +               // AEP should not be enabled if all of the sub-extensions can't
> +               // also be enabled. This is not the proper layer to do such
> +               // error-checking though.

Please use /* */ comments, and leave a newline between code and the
start of the multiline comment.