[Mesa-stable] [PATCH 14.1/15] glapi: Store exec table version info outside the XML

Ian Romanick idr at freedesktop.org
Wed May 20 20:22:22 PDT 2015


On 05/19/2015 07:29 PM, Ilia Mirkin wrote:
> On Tue, May 19, 2015 at 3:54 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> Currently on the functions that are exclusive to core-profile are
>> implemented.  The remainder continue to live in the XML.  Additional
>> functions can be moved later.
>>
>> The functions for GL_ARB_draw_indirect and GL_ARB_multi_draw_indirect
>> are put in the dispatch table inside the VBO module, so they do not need
>> to be moved over.
>>
>> The diff of src/mesa/main/api_exec.c before and after this patch is as
>> expected.  All of the functions listed in apiexec.py moved out of a 'if
>> (_mesa_is_desktop(ctx))' block into a new 'if (ctx->API ==
>> API_OPENGL_CORE)' block.
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> Cc: Dave Airlie <airlied at redhat.com>
>> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
>> Cc: Dylan Baker <baker.dylan.c at gmail.com>
>> Cc: "10.6" <mesa-stable at lists.freedesktop.org>
>> ---
>>  src/mapi/glapi/gen/Makefile.am   |   3 +-
>>  src/mapi/glapi/gen/apiexec.py    | 142 +++++++++++++++++++++++++++++++++++++++
>>  src/mapi/glapi/gen/gl_genexec.py |  54 ++++++++++++---
>>  3 files changed, 187 insertions(+), 12 deletions(-)
>>  create mode 100644 src/mapi/glapi/gen/apiexec.py
>>
>> diff --git a/src/mapi/glapi/gen/Makefile.am b/src/mapi/glapi/gen/Makefile.am
>> index adebd5c..64bc2ff 100644
>> --- a/src/mapi/glapi/gen/Makefile.am
>> +++ b/src/mapi/glapi/gen/Makefile.am
>> @@ -61,6 +61,7 @@ EXTRA_DIST= \
>>         $(MESA_GLAPI_DIR)/glapi_x86-64.S \
>>         $(MESA_GLAPI_DIR)/glapi_sparc.S \
>>         $(COMMON_GLX) \
>> +       apiexec.py \
>>         gl_apitemp.py \
>>         gl_enums.py \
>>         gl_genexec.py \
>> @@ -267,7 +268,7 @@ $(MESA_GLAPI_DIR)/glapi_sparc.S: gl_SPARC_asm.py $(COMMON)
>>  $(MESA_DIR)/main/enums.c: gl_enums.py $(COMMON)
>>         $(PYTHON_GEN) $< -f $(srcdir)/gl_and_es_API.xml > $@
>>
>> -$(MESA_DIR)/main/api_exec.c: gl_genexec.py $(COMMON)
>> +$(MESA_DIR)/main/api_exec.c: gl_genexec.py apiexec.py $(COMMON)
>>         $(PYTHON_GEN) $< -f $(srcdir)/gl_and_es_API.xml > $@
>>
>>  $(MESA_DIR)/main/dispatch.h: gl_table.py $(COMMON)
>> diff --git a/src/mapi/glapi/gen/apiexec.py b/src/mapi/glapi/gen/apiexec.py
>> new file mode 100644
>> index 0000000..1a9f042
>> --- /dev/null
>> +++ b/src/mapi/glapi/gen/apiexec.py
>> @@ -0,0 +1,142 @@
>> +#!/usr/bin/env python2
> 
> I wouldn't put that line there (or make the file +x, which you didn't
> as well). It's not an executable script.
> 
>> +
>> +# Copyright (C) 2015 Intel Corporation
>> +#
>> +# Permission is hereby granted, free of charge, to any person obtaining a
>> +# copy of this software and associated documentation files (the "Software"),
>> +# to deal in the Software without restriction, including without limitation
>> +# the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> +# and/or sell copies of the Software, and to permit persons to whom the
>> +# Software is furnished to do so, subject to the following conditions:
>> +#
>> +# The above copyright notice and this permission notice (including the next
>> +# paragraph) shall be included in all copies or substantial portions of the
>> +# Software.
>> +#
>> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> +# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> +# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> +# IN THE SOFTWARE.
>> +
>> +class exec_info():
>> +    """Information relating GL APIs to a function.
>> +
>> +    Each of the four attributes of this class, compatibility, core, es1, and
>> +    es2, specify the minimum API version where a function can possibly exist
>> +    in Mesa.  The version is specified as an integer of (real GL version *
>> +    10).  For example, glCreateProgram was added in OpenGL 2.0, so
>> +    compatibility=20 and core=31.
>> +
>> +    If the attribute is None, then it cannot be supported by that
>> +    API.  For example, glNewList was removed from core profiles, so
>> +    compatibility=10 and core=None.
>> +
>> +    Each of the attributes that is not None must have a valid value.  The
>> +    valid ranges are:
>> +
>> +        compatiblity: [10, 30]
>> +        core: [31, )
>> +        es1: [10, 11]
>> +        es2: [20, )
>> +
>> +    These ranges are enforced by the constructor.
> 
> No harm in doing this, but in practice, only the es2 version is used
> for anything, right?

For now.  There are quite a few functions that are only added by a GL
version that currently either incorrectly don't check the version (e.g.,
everything in src/mesa/main/shaderapi.c) or have spurious checks that
could be removed.

>> +    """
>> +    def __init__(self, compatibility=None, core=None, es1=None, es2=None):
>> +        if compatibility is not None:
>> +            assert isinstance(compatibility, int)
>> +            assert compatibility >= 10
>> +            assert compatibility <= 30
>> +
>> +        if core is not None:
>> +            assert isinstance(core, int)
>> +            assert core >= 31
>> +
>> +        if es1 is not None:
>> +            assert isinstance(es1, int)
>> +            assert es1 == 10 or es1 == 11
>> +
>> +        if es2 is not None:
>> +            assert isinstance(es2, int)
>> +            assert es2 >= 20
>> +
>> +        self.compatibility = compatibility
>> +        self.core = core
>> +        self.es1 = es1
>> +        self.es2 = es2
>> +
>> +functions = {
>> +    # OpenGL 3.1 / GL_ARB_texture_buffer_object.  Mesa only exposes this
>> +    # extension with core profile.
>> +    "TexBuffer": exec_info(core=31),
>> +
>> +    # OpenGL 3.2 / GL_ARB_geometry_shader4.  Mesa does not support
>> +    # GL_ARB_geometry_shader4, so OpenGL 3.2 is required.
>> +    "FramebufferTexture": exec_info(core=32),
>> +
>> +    # OpenGL 4.0 / GL_ARB_gpu_shader_fp64.  The extension spec says:
>> +    #
>> +    #     "OpenGL 3.2 and GLSL 1.50 are required."
>> +    "Uniform1d": exec_info(core=32),
>> +    "Uniform2d": exec_info(core=32),
>> +    "Uniform3d": exec_info(core=32),
>> +    "Uniform4d": exec_info(core=32),
>> +    "Uniform1dv": exec_info(core=32),
>> +    "Uniform2dv": exec_info(core=32),
>> +    "Uniform3dv": exec_info(core=32),
>> +    "Uniform4dv": exec_info(core=32),
>> +    "UniformMatrix2dv": exec_info(core=32),
>> +    "UniformMatrix3dv": exec_info(core=32),
>> +    "UniformMatrix4dv": exec_info(core=32),
>> +    "UniformMatrix2x3dv": exec_info(core=32),
>> +    "UniformMatrix2x4dv": exec_info(core=32),
>> +    "UniformMatrix3x2dv": exec_info(core=32),
>> +    "UniformMatrix3x4dv": exec_info(core=32),
>> +    "UniformMatrix4x2dv": exec_info(core=32),
>> +    "UniformMatrix4x3dv": exec_info(core=32),
>> +    "GetUniformdv": exec_info(core=32),
>> +
>> +    # OpenGL 4.1 / GL_ARB_vertex_attrib_64bit.  The extension spec says:
>> +    #
>> +    #     "OpenGL 3.0 and GLSL 1.30 are required.
>> +    #
>> +    #     ARB_gpu_shader_fp64 (or equivalent functionality) is required."
>> +    #
>> +    # For Mesa this effectively means OpenGL 3.2 is required.  It seems
>> +    # unlikely that Mesa will ever get support for any of the NV extensions
>> +    # that add "equivalent functionality."
>> +    "VertexAttribL1d": exec_info(core=32),
>> +    "VertexAttribL2d": exec_info(core=32),
>> +    "VertexAttribL3d": exec_info(core=32),
>> +    "VertexAttribL4d": exec_info(core=32),
>> +    "VertexAttribL1dv": exec_info(core=32),
>> +    "VertexAttribL2dv": exec_info(core=32),
>> +    "VertexAttribL3dv": exec_info(core=32),
>> +    "VertexAttribL4dv": exec_info(core=32),
>> +    "VertexAttribLPointer": exec_info(core=32),
>> +    "GetVertexAttribLdv": exec_info(core=32),
>> +
>> +    # OpenGL 4.1 / GL_ARB_viewport_array.  The extension spec says:
>> +    #
>> +    #     "OpenGL 3.2 or the EXT_geometry_shader4 or ARB_geometry_shader4
>> +    #     extensions are required."
>> +    #
>> +    # Mesa does not support either of the geometry shader extensions, so
>> +    # OpenGL 3.2 is required.
>> +    "ViewportArrayv": exec_info(core=32),
>> +    "ViewportIndexedf": exec_info(core=32),
>> +    "ViewportIndexedfv": exec_info(core=32),
>> +    "ScissorArrayv": exec_info(core=32),
>> +    "ScissorIndexed": exec_info(core=32),
>> +    "ScissorIndexedv": exec_info(core=32),
>> +    "DepthRangeArrayv": exec_info(core=32),
>> +    "DepthRangeIndexed": exec_info(core=32),
>> +    # GetFloati_v also GL_ARB_shader_atomic_counters
>> +    # GetDoublei_v also GL_ARB_shader_atomic_counters
>> +
>> +    # OpenGL 4.3 / GL_ARB_texture_buffer_range.  Mesa can expose the extension
>> +    # with OpenGL 3.1.
>> +    "TexBufferRange": exec_info(core=31),
>> +}
>> diff --git a/src/mapi/glapi/gen/gl_genexec.py b/src/mapi/glapi/gen/gl_genexec.py
>> index 4e76fe3..ab00e32 100644
>> --- a/src/mapi/glapi/gen/gl_genexec.py
>> +++ b/src/mapi/glapi/gen/gl_genexec.py
>> @@ -29,6 +29,7 @@ import collections
>>  import license
>>  import gl_XML
>>  import sys, getopt
>> +import apiexec
>>
>>
>>  exec_flavor_map = {
>> @@ -175,18 +176,49 @@ class PrintCode(gl_XML.gl_print_base):
>>                  raise Exception(
>>                      'Unrecognized exec flavor {0!r}'.format(f.exec_flavor))
>>              condition_parts = []
>> -            if f.desktop:
>> -                if f.deprecated:
>> +            if f.name in apiexec.functions:
>> +                ex = apiexec.functions[f.name]
>> +                unconditional_count = 0
>> +
>> +                if ex.compatibility is not None:
>>                      condition_parts.append('ctx->API == API_OPENGL_COMPAT')
>> -                else:
>> -                    condition_parts.append('_mesa_is_desktop_gl(ctx)')
>> -            if 'es1' in f.api_map:
>> -                condition_parts.append('ctx->API == API_OPENGLES')
>> -            if 'es2' in f.api_map:
>> -                if f.api_map['es2'] > 2.0:
>> -                    condition_parts.append('(ctx->API == API_OPENGLES2 && ctx->Version >= {0})'.format(int(f.api_map['es2'] * 10)))
>> -                else:
>> -                    condition_parts.append('ctx->API == API_OPENGLES2')
>> +                    unconditional_count += 1
>> +
>> +                if ex.core is not None:
>> +                    condition_parts.append('ctx->API == API_OPENGL_CORE')
>> +                    unconditional_count += 1
>> +
>> +                if ex.es1 is not None:
>> +                    condition_parts.append('ctx->API == API_OPENGLES')
>> +                    unconditional_count += 1
>> +
>> +                if ex.es2 is not None:
>> +                    if ex.es2 > 20:
>> +                        condition_parts.append('(ctx->API == API_OPENGLES2 && ctx->Version >= {0})'.format(ex.es2))
>> +                    else:
>> +                        condition_parts.append('ctx->API == API_OPENGLES2')
>> +                        unconditional_count += 1
>> +
>> +                # If the function is unconditionally available in all four
>> +                # APIs, then it is always available.  Replace the complex
>> +                # tautology condition with "true" and let GCC do the right
>> +                # thing.
>> +                if unconditional_count == 4:
>> +                    condition_parts = ['true']
>> +            else:
>> +                if f.desktop:
>> +                    if f.deprecated:
>> +                        condition_parts.append('ctx->API == API_OPENGL_COMPAT')
>> +                    else:
>> +                        condition_parts.append('_mesa_is_desktop_gl(ctx)')
>> +                if 'es1' in f.api_map:
>> +                    condition_parts.append('ctx->API == API_OPENGLES')
>> +                if 'es2' in f.api_map:
>> +                    if f.api_map['es2'] > 2.0:
>> +                        condition_parts.append('(ctx->API == API_OPENGLES2 && ctx->Version >= {0})'.format(int(f.api_map['es2'] * 10)))
>> +                    else:
>> +                        condition_parts.append('ctx->API == API_OPENGLES2')
>> +
>>              if not condition_parts:
>>                  # This function does not exist in any API.
>>                  continue
>> --
>> 2.1.0
>>
> 
> This all seems fine. I trust you on the function selection. However as
> this is all subtle and easily breakable, could I convince you to write
> a piglit test that verifies some of this behaviour? i.e. that
> GetProcAddress fails for a few of those functions in a compat context
> [and you can skip if the extensions are advertised, so that we don't
> fail on blobs that provide a high-version GL]

Unfortunately, that would be a large amount of work, but I'd love to see
this kind of testing happen.

glXGetProcAddress will *never* return NULL.  If you call
glXGetProcAddress on a function libGL doesn't know, it will generate a
new dispatch function.  Since you can call glXGetProcAddress before you
create a context, libGL has no way to know what the driver will support.
 Even if you have a context, you could create another context on a
different GPU with different capabilities.  If the driver never puts
anything in that new location in the dispatch table, calling the
function returned by glXGetProcAddress may crash.

A hypothetical piglit test would need to:

1. Call glXGetProcAddress directly without Waffle or some other layer
"helping."  If NULL is returned (as on all the closed-source drivers),
the test passes.

2.  Call the function.  If GL_INVALID_OPERATION is generated, the test
passes.  If the test crashes, the test passes.  That last bit is the
hard part. :)

I would really like to have tests like this for a lot of different
functions.  I first started thinking about it when we removed the
ARB_vertex_program / ARB_fragment_program functions from core profile.

That said, dispatch_sanity can cover most of the useful things.  I have
some patches on the way out...

> With that, and the above nits,
> 
> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-stable
> 



More information about the mesa-stable mailing list