[Mesa-dev] [PATCH 4/7] glsl: add core plumbing for GL_ANDROID_extension_pack_es31a
Ilia Mirkin
imirkin at alum.mit.edu
Mon Aug 29 18:58:31 UTC 2016
On Mon, Aug 29, 2016 at 2:45 PM, Matt Turner <mattst88 at gmail.com> wrote:
> 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.
>From the spec:
Including the following line in a shader:
#extension GL_ANDROID_extension_pack_es31a : <behavior>
has the same effect as including the following lines:
#extension GL_KHR_blend_equation_advanced : <behavior>
#extension GL_OES_sample_variables : <behavior>
#extension GL_OES_shader_image_atomic : <behavior>
#extension GL_OES_shader_multisample_interpolation : <behavior>
#extension GL_OES_texture_storage_multisample_2d_array : <behavior>
#extension GL_EXT_geometry_shader : <behavior>
#extension GL_EXT_gpu_shader5 : <behavior>
#extension GL_EXT_primitive_bounding_box : <behavior>
#extension GL_EXT_shader_io_blocks : <behavior>
#extension GL_EXT_tessellation_shader : <behavior>
#extension GL_EXT_texture_buffer : <behavior>
#extension GL_EXT_texture_cube_map_array : <behavior>
What I do here is directly equivalent. Note that this EXT_AEP thing is
solely for the GLSL end of things, not for the extension string.
I think you may be confusing the two concepts... the ANDROID_foo
extension enable in ctx->Extensions should only be flipped on when
those various exts (and the ones you listed below) are enabled by the
driver, which will make it available to be enabled in GLSL. This isn't
the only extension with funny dependencies. The approach thus far has
been to just add a new bit and letting the driver deal with it rather
than trying to build logic into version.c or other places. (See, for
example, the recent OES_texture_cube_map_array enable, which is
basically ARB_texture_cube_map_array && OES_geometry_shader.)
>
>> +
>> + /**
>> * 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
Nope. Those have no GLSL behaviors defined, and as you can see, they
are not in the above list at all.
>
>> };
>>
>> @@ -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.
Hm, I thought we used // comments in C++ files, but that doesn't
appear to be the case, at least in this file. Will change.
-ilia
More information about the mesa-dev
mailing list