<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> I've got an updated patch out. Note that the Python scripts were largely unmodified from the ones in the libglvnd tree. I don't know if divergence between the two is something anyone is likely to care about, though, and I'd expect the EGL-specific changes to libglvnd's version to be pretty minimal. Other responses inline. <div class="moz-cite-prefix">On 09/12/2016 03:32 PM, Dylan Baker wrote: </div> <blockquote cite="mid:147371595762.8745.6331670684366302239@alivia.ak.intel.com" type="cite"> <pre wrap="">Quoting Kyle Brenneman (2016-09-12 11:59:32) </pre> <blockquote type="cite"> <pre wrap="">Added separate --enable-libglvnd-glx and --enable-libglvnd-egl configure options to enable libglvnd for GLX and EGL. The existing --enable-libglvnd option will now enable both EGL and GLX. The new interface mostly just sits on top of the existing library. The only change to the existing EGL code is to split the client extension string into platform extensions and everything else. On non-glvnd builds, eglQueryString will just concatenate the two strings. The EGL dispatch stubs are all generated. The script is based on the one used to generate entrypoints in libglvnd itself. --- configure.ac | 161 ++- src/egl/Makefile.am | 65 +- src/egl/generate/egl.xml | 2412 ++++++++++++++++++++++++++++++++++ src/egl/generate/eglFunctionList.py | 191 +++ src/egl/generate/egl_other.xml | 47 + src/egl/generate/genCommon.py | 223 ++++ src/egl/generate/gen_egl_dispatch.py | 223 ++++ src/egl/main/50_mesa.json.in | 6 + src/egl/main/eglapi.c | 6 +- src/egl/main/egldispatchstubs.c | 121 ++ src/egl/main/egldispatchstubs.h | 26 + src/egl/main/eglglobals.c | 44 +- src/egl/main/eglglobals.h | 13 +- src/egl/main/eglglvnd.c | 75 ++ 14 files changed, 3525 insertions(+), 88 deletions(-) create mode 100755 src/egl/generate/egl.xml create mode 100644 src/egl/generate/eglFunctionList.py create mode 100644 src/egl/generate/egl_other.xml create mode 100644 src/egl/generate/genCommon.py create mode 100755 src/egl/generate/gen_egl_dispatch.py create mode 100644 src/egl/main/50_mesa.json.in create mode 100644 src/egl/main/egldispatchstubs.c create mode 100644 src/egl/main/egldispatchstubs.h create mode 100644 src/egl/main/eglglvnd.c diff --git a/src/egl/generate/eglFunctionList.py b/src/egl/generate/eglFunctionList.py new file mode 100644 index 0000000..39d3dd5 --- /dev/null +++ b/src/egl/generate/eglFunctionList.py @@ -0,0 +1,191 @@ +#!/usr/bin/python </pre> </blockquote> <pre wrap=""> Is this a script or a module? It looks more like a module. If it's a module please drop the chbang, other wise please change it to '/usr/bin/env python2'. Archlinux has made and stuck to a very bad decisions of having 'python' point to 'python3' instead of 'python2' like upstream suggests. Archlinux is a big enough consumer we need to cater to this, and we have't seen any breakages as a result. </pre> </blockquote> eglFunctionList.py and genCommon.py are both modules, so I'll take the chbangs out. As for the Python version, the script in the updated patch will work correctly with Python 2 or 3. The command line in the Makefile includes the Python executable anyway, so the chbang is more documentation than anything else. <blockquote cite="mid:147371595762.8745.6331670684366302239@alivia.ak.intel.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ +""" +Contains a list of EGL functions to generate dispatch functions for. + +This is used from gen_egl_dispatch.py. + +EGL_FUNCTIONS is a sequence of (name, eglData) pairs, where name is the name +of the function, and eglData is a dictionary containing data about that +function. + +The values in the eglData dictionary are: +- method (string): + How to select a vendor library. See "Method values" below. + +- prefix (string): + This string is prepended to the name of the dispatch function. If + unspecified, the default is "" (an empty string). + +- static (boolean) + If True, this function should be declared static. + +- "public" (boolean) + If True, the function should be exported from the library. Vendor libraries + generally should not use this. + +- extension (string): + If specified, this is the name of a macro to check for before defining a + function. Used for checking for extension macros and such. + +- retval (string): + If specified, this is a C expression with the default value to return if we + can't find a function to call. By default, it will try to guess from the + return type: EGL_NO_whatever for the various handle types, NULL for + pointers, and zero for everything else. + +method values: +- "custom" + The dispatch stub will be hand-written instead of generated. + +- "none" + No dispatch function exists at all, but the function should still have an + entry in the index array. This is for other functions that a stub may need + to call that are implemented in libEGL itself. + +- "display" + Select a vendor from an EGLDisplay argument. + +- "device" + Select a vendor from an EGLDeviceEXT argument. + +- "current" + Select the vendor that owns the current context. +""" + +def _eglFunc(name, method, static=False, public=False, inheader=None, prefix="", extension=None, retval=None): + """ + A convenience function to define an entry in the EGL function list. + """ + if (inheader == None): + inheader = (not public) </pre> </blockquote> <pre wrap=""> Simplify this to: 'inheader = inheader or not public'</pre> </blockquote> That changes the semantics if (inheader == False) instead of (inheader == None). The point of using None is that lets the caller explicitly specify True or False, but gives a convenient default if the caller doesn't specify either. <blockquote cite="mid:147371595762.8745.6331670684366302239@alivia.ak.intel.com" type="cite"> <pre wrap="">+) + diff --git a/src/egl/generate/genCommon.py b/src/egl/generate/genCommon.py new file mode 100644 index 0000000..5781275 --- /dev/null +++ b/src/egl/generate/genCommon.py @@ -0,0 +1,223 @@ +#!/usr/bin/env python </pre> <pre wrap=""> The same as the last chbang </pre> <blockquote type="cite"> <pre wrap="">+ +# (C) Copyright 2015, NVIDIA CORPORATION. +# All Rights Reserved. +# +# 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 +# on the rights to use, copy, modify, merge, publish, distribute, sub +# license, 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 NON-INFRINGEMENT. IN NO EVENT SHALL +# IBM AND/OR ITS SUPPLIERS 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. +# +# Authors: +# Kyle Brenneman <a class="moz-txt-link-rfc2396E" href="mailto:kbrenneman@nvidia.com"><kbrenneman@nvidia.com></a> + +import sys +import collections +import re +import xml.etree.cElementTree as etree </pre> </blockquote> <pre wrap=""> Please sort the above imports </pre> <blockquote type="cite"> <pre wrap=""> +def getFunctionsFromRoots(roots): + functions = {} + for root in roots: + for func in _getFunctionList(root): + functions[func.name] = func + functions = functions.values() + + # Sort the function list by name. + functions = sorted(functions, key=lambda f: f.name) </pre> </blockquote> <pre wrap=""> This avoids creating a dictionary, just to turn it into a list, and avoids building any intermediate data structures. functions = sorted((f for f in (_getFunctionList(r) for r in roots))), key=lambda f: f.name)</pre> </blockquote> That's not quite equivalent. Using the dictionary step ensures that the resulting list doesn't have any duplicate entries in it. <blockquote cite="mid:147371595762.8745.6331670684366302239@alivia.ak.intel.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ + # Assign a slot number to each function. This isn't strictly necessary, + # since you can just look at the index in the list, but it makes it easier + # to include the slot when formatting output. + for i in xrange(len(functions)): + functions[i] = functions[i]._replace(slot=i) </pre> </blockquote> <pre wrap=""> I really don't like relying on protected methods like this. It's asking for trouble</pre> </blockquote> The _replace method isn't protected -- it's part of the spec for collections.namedtuple. The leading underscore is just to avoid name conflicts with the actual member names: <a class="moz-txt-link-freetext" href="https://docs.python.org/2.7/library/collections.html#collections.somenamedtuple._replace">https://docs.python.org/2.7/library/collections.html#collections.somenamedtuple._replace</a> <blockquote cite="mid:147371595762.8745.6331670684366302239@alivia.ak.intel.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ + return functions + +def getExportNamesFromRoots(target, roots): + """ + Goes through the <feature> tags from gl.xml and returns a set of OpenGL + functions that a library should export. + + target should be one of "gl", "gldispatch", "opengl", "glesv1", or + "glesv2". + """ + featureNames = _LIBRARY_FEATURE_NAMES[target] + if (featureNames == None): </pre> </blockquote> <pre wrap=""> 'if featureNames is None'. Use is with singletons like 'None, False, True, etc' </pre> <blockquote type="cite"> <pre wrap="">+ return set(func.name for func in getFunctionsFromRoots(roots)) + + names = set() + for root in roots: + features = [] + for featElem in root.findall("feature"): + if (featElem.get("name") in featureNames): </pre> </blockquote> <pre wrap=""> It is really not idomatic to use parans in if statments in python, and it doesn't seem like we do that much in mesa, so could we please drop them? </pre> <blockquote type="cite"> <pre wrap="">+ features.append(featElem) + for featElem in root.findall("extensions/extension"): + if (featElem.get("name") in featureNames): + features.append(featElem) + for featElem in features: + for commandElem in featElem.findall("require/command"): + names.add(commandElem.get("name")) + return names + +class FunctionArg(collections.namedtuple("FunctionArg", "type name")): + @property + def dec(self): + """ + Returns a "TYPE NAME" string, suitable for a function prototype. + """ + rv = str(self.type) + if(not rv.endswith("*")): </pre> </blockquote> <pre wrap=""> 'if not rv.endswith... </pre> <blockquote type="cite"> <pre wrap="">+ rv += " " + rv += self.name + return rv + +class FunctionDesc(collections.namedtuple("FunctionDesc", "name rt args slot")): + def hasReturn(self): + """ + Returns true if the function returns a value. + """ + return (self.rt != "void") </pre> </blockquote> <pre wrap=""> No need for parens here either </pre> <blockquote type="cite"> <pre wrap="">+ + @property + def decArgs(self): + """ + Returns a string with the types and names of the arguments, as you + would use in a function declaration. + """ + if(len(self.args) == 0): </pre> </blockquote> <pre wrap=""> 'if not self.args' </pre> <blockquote type="cite"> <pre wrap="">+ return "void" + else: + return ", ".join(arg.dec for arg in self.args) + + @property + def callArgs(self): + """ + Returns a string with the names of the arguments, as you would use in a + function call. + """ + return ", ".join(arg.name for arg in self.args) + + @property + def basename(self): + assert(self.name.startswith("gl")) </pre> </blockquote> <pre wrap=""> Don't use parens with assert. Assert is a keyword in python, so using parens doesn't do what you expect. If you care to know why: Since assert's signature is 'assert <test>, <message>' someone will come along and extend this with a message 'assert(<test>, <message>), and then things fail spectacularly because you've passed assert a tuple with a string in it, so it now *always* evaluates True. </pre> <blockquote type="cite"> <pre wrap="">diff --git a/src/egl/generate/gen_egl_dispatch.py b/src/egl/generate/gen_egl_dispatch.py new file mode 100755 index 0000000..1e145aa --- /dev/null +++ b/src/egl/generate/gen_egl_dispatch.py @@ -0,0 +1,223 @@ +#!/usr/bin/python </pre> </blockquote> <pre wrap=""> Please modify this chbange as well Presumably this should have a license header as well? </pre> <blockquote type="cite"> <pre wrap="">+ +""" +Generates dispatch functions for EGL. + +The list of functions and arguments is read from the Khronos's XML files, with +additional information defined in the module eglFunctionList. +""" + +import sys +import collections +import imp </pre> </blockquote> <pre wrap=""> please sort these as well </pre> <blockquote type="cite"> <pre wrap="">+ +import genCommon + +def main(): + if (len(sys.argv) < 4): + print("Usage: %r source|header <function_list> <xml_file> [xml_file...]" % (sys.argv[0],)) + sys.exit(2) </pre> </blockquote> <pre wrap=""> Most of our scripts use argparse (which is built-in to python 2.7+, which is conincidentlyw hat we support), wcould w use that to simplify this as well? something like: parser = argparse.ArgumentParser() parser.add_argument('target', choices=['header', 'source'], help="...") parser.add_argument('func_list_file', help="...") parser.add_argument('xml_files', nargs='*', help="...") args = parser.parse_args() </pre> <blockquote type="cite"> <pre wrap="">+ + target = sys.argv[1] + funcListFile = sys.argv[2] + xmlFiles = sys.argv[3:] + + # The function list is a Python module, but it's specified on the command + # line. + eglFunctionList = imp.load_source("eglFunctionList", funcListFile) + + xmlFunctions = genCommon.getFunctions(xmlFiles) + xmlByName = dict((f.name, f) for f in xmlFunctions) + functions = [] + for (name, eglFunc) in eglFunctionList.EGL_FUNCTIONS: + func = xmlByName[name] + eglFunc = fixupEglFunc(func, eglFunc) + functions.append((func, eglFunc)) + + # Sort the function list by name. + functions = sorted(functions, key=lambda f: f[0].name) + + if (target == "header"): + text = generateHeader(functions) + elif (target == "source"): + text = generateSource(functions) + else: + raise ValueError("Invalid target: %r" % (target,)) </pre> </blockquote> <pre wrap=""> Using argparse would remove the need for tihs else too. </pre> <blockquote type="cite"> <pre wrap="">+ sys.stdout.write(text) + +def fixupEglFunc(func, eglFunc): + result = dict(eglFunc) + if (result.get("prefix") == None): </pre> </blockquote> <pre wrap=""> if result.get("prefix") is None: </pre> <blockquote type="cite"> <pre wrap="">+ result["prefix"] = "" + + if (result.get("extension") != None): </pre> </blockquote> <pre wrap=""> if result.get("extension") is not None: </pre> <blockquote type="cite"> <pre wrap="">+ text = "defined(" + result["extension"] + ")" + result["extension"] = text + + if (result["method"] in ("none", "custom")): + return result + + if (result["method"] not in ("display", "device", "current")): + raise ValueError("Invalid dispatch method %r for function %r" % (result["method"], func.name)) + + if (func.hasReturn()): + if (result.get("retval") == None): + result["retval"] = getDefaultReturnValue(func.rt) + + return result + +def generateHeader(functions): </pre> </blockquote> <pre wrap=""> textwrap.dedent is your friend here. </pre> <blockquote type="cite"> <pre wrap="">+ text = r""" +#ifndef G_EGLDISPATCH_STUBS_H +#define G_EGLDISPATCH_STUBS_H + +#ifdef __cplusplus +extern "C" { +#endif + +#include <EGL/egl.h> +#include <EGL/eglext.h> +#include "glvnd/libeglabi.h" + +""".lstrip("\n") + + text += "enum {\n" + for (func, eglFunc) in functions: + text += generateGuardBegin(func, eglFunc) + text += " __EGL_DISPATCH_" + func.name + ",\n" + text += generateGuardEnd(func, eglFunc) + text += " __EGL_DISPATCH_COUNT\n" + text += "};\n" + + for (func, eglFunc) in functions: + if (eglFunc["inheader"]): + text += generateGuardBegin(func, eglFunc) + text += "{f.rt} EGLAPIENTRY {ex[prefix]}{f.name}({f.decArgs});\n".format(f=func, ex=eglFunc) + text += generateGuardEnd(func, eglFunc) + + text += r""" +#ifdef __cplusplus +} +#endif +#endif // G_EGLDISPATCH_STUBS_H +""" + return text + +def generateSource(functions): </pre> </blockquote> <pre wrap=""> I would strongly look into use mako for this funciton, we already use it in mesa.</pre> </blockquote> Is there a strong preference for using Mako versus regular Python templates in a case like this? The original script from libglvnd uses normal templates since I didn't want to add the extra dependency, but if the rest of Mesa already requires Mako, then there's no reason this one couldn't either. <blockquote cite="mid:147371595762.8745.6331670684366302239@alivia.ak.intel.com" type="cite"> <blockquote type="cite"> <pre wrap="">-- 2.7.4 _______________________________________________ mesa-dev mailing list <a class="moz-txt-link-abbreviated" href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </pre> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ mesa-dev mailing list <a class="moz-txt-link-abbreviated" href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </pre> </blockquote> </blockquote> </body> </html>