On Mon, 2018-11-05 at 10:04 +0530, Veluri Mithun wrote: > Signed-off-by: Veluri Mithun <velurimithu...@gmail.com>
Thanks for looking into this! Many of these comments apply equally to the GLX extension I think; if you wanted to write a patch for the equivalent text for that extension too, that'd be awesome. > + EGL 1.4 is required. I'm not sure this is true, from the EGL types you're (currently) using it looks like even an EGL 1.0 implementation could support this. Probably better to say "EGL 1.0 is required; this extension is written against the text of the EGL 1.4 specification." > + EGL_ARB_create_context and EGL_ARB_create_context_profile are required. > + > + This extension interacts with EGL_EXT_create_context_es2_profile and > + EGL_EXT_create_context_es_profile. There are GLX extensions with almost these names, but not EGL: https://www.khronos.org/registry/EGL/ These references should all be deleted. > +New Procedures and Functions EGL functions start with 'egl', lowercase. > + Bool EGLQueryRendererIntegerMESA(EGLDisplay *dpy, int screen, > + int renderer, int attribute, > + unsigned int *value); EGL displays don't have screens, that's an X11-ism. The screen parameter should be deleted. > + const char *EGLQueryRendererStringMESA(EGLDisplay *dpy, int screen, > + int renderer, int attribute); Same comment about 'screen' here. > +New Tokens > + > + Accepted as an <attribute> in EGLQueryRendererIntegerMESA and > + EGLQueryCurrentRendererIntegerMESA: > + > + EGL_RENDERER_VENDOR_ID_MESA 0xXXXX > + EGL_RENDERER_DEVICE_ID_MESA 0xXXXX > + EGL_RENDERER_VERSION_MESA 0xXXXX > + EGL_RENDERER_ACCELERATED_MESA 0xXXXX > + EGL_RENDERER_VIDEO_MEMORY_MESA 0xXXXX > + EGL_RENDERER_UNIFIED_MEMORY_ARCHITECTURE_MESA 0xXXXX > + EGL_RENDERER_PREFERRED_PROFILE_MESA 0xXXXX > + EGL_RENDERER_OPENGL_CORE_PROFILE_VERSION_MESA 0xXXXX > + EGL_RENDERER_OPENGL_COMPATIBILITY_PROFILE_VERSION_MESA 0xXXXX > + EGL_RENDERER_OPENGL_ES_PROFILE_VERSION_MESA 0xXXXX > + EGL_RENDERER_OPENGL_ES2_PROFILE_VERSION_MESA 0xXXXX > + > + Accepted as an <attribute> in EGLQueryRendererStringMESA and > + EGLQueryCurrentRendererStringMESA: > + > + EGL_RENDERER_VENDOR_ID_MESA > + EGL_RENDERER_DEVICE_ID_MESA > + > + Accepted as an attribute name in <*attrib_list> in > + EGLCreateContextAttribsARB: > + > + EGL_RENDERER_ID_MESA 0xXXXX Normally these are reserved by creating a request against the EGL registry on github. However we happen to have a block of 16 enum values already reserved: > <enums namespace="EGL" start="0x3290" end="0x329F" vendor="MESA" > comment="Reserved for John Kåre Alsaker (Public bug 757)"> > <unused start="0x3290" end="0x329F"/> > </enums> That "public bug 757" is for the abandoned EGL_MESA_image_sRGB extension: https://www.khronos.org/bugzilla/show_bug.cgi?id=757 So I think it's safe to use enums from that range, and we'll still have two free. > +Additions to the EGL 1.4 Specification > + > + [Add the following to Section X.Y.Z of the EGL Specification] Should be Section 3.3 "EGL Versioning". If this were against the EGL 1.5 text it'd be the more-obviously-correct Section 3.3 "EGL Queries". > + To obtain information about the available renderers for a particular > + display and screen, > + > + Bool EGLQueryRendererIntegerMESA(EGLDisplay *dpy, int screen, int > renderer, > + int attribute, unsigned int *value); The corresponding eglQueryCurrentRendererIntegerMESA is not documented. I'm not entirely sure it should even exist, to be honest; I'd prefer if these attributes were instead newly legal values to pass to eglQueryContext. Too late to make that change for GLX I suppose. > + String versions of some attributes may also be queried using > + > + const char *EGLQueryRendererStringMESA(EGLDisplay *dpy, int screen, > + int renderer, int attribute); Likewise eglQueryCurrentRendererStringMESA is not documented. This could not be queried with eglQueryContext since that can only return integers not pointers. > + [Add to section section 3.3.7 "Rendering Contexts"] > + > + The attribute name EGL_RENDERER_ID_MESA specified the index of the render > + against which the context should be created. The default value of > + EGL_RENDERER_ID_MESA is 0. This startled me to read, I didn't think GLX_MESA_query_renderer had this. Turns out it does have this text in the extension spec, but the functionality is not actually implemented. Worse, there's no way to enumerate how many renderers a display (or display/screen in GLX) has. To address this, I would: 1) Remove this text from both GLX and EGL extension specs (Both here and above where the enums are listed) 2) Update both specs with an explicit way to enumerate renderers (probably something like "if renderer is -1 and the attribute is XXX_RENDERER_COUNT_MESA, return one integer") 3) Add a layered extension to add this functionality to CreateContext #2 and #3 are optional, I suppose. Probably it's better to have only one way to do that, and in EGL that way is probably using the EGL device extensions instead. But GLX doesn't have anything like that, so this functionality might make sense there. > + [Add to list of errors for EGLCreateContextAttribsARB in section section > + 3.3.7 "Rendering Contexts"] > + > + * If the value of EGL_RENDERER_ID_MESA specifies a non-existent > + renderer, BadMatch is generated. This should be deleted too. > +Dependencies on EGL_EXT_create_context_es_profile and > +EGL_EXT_create_context_es2_profile > + > + If neither extension is supported, remove all mention of > + EGL_RENDERER_OPENGL_ES2_PROFILE_VERSION_MESA from the spec. > + > + If EGL_EXT_create_context_es_profile is not supported, remove all > mention of > + EGL_RENDERER_OPENGL_ES_PROFILE_VERSION_MESA from the spec. Neither of these extensions exist, and the text already says what happens if those APIs aren't supported, so this can be deleted as well. - ajax _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
