<div dir="ltr"><div class="gmail_extra"> <div class="gmail_quote">On Wed, May 18, 2016 at 8:44 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 05/18/2016 11:45 AM, Manolova, Plamena wrote: > Hi Ian, > Thanks for reviewing! > > On Wed, May 18, 2016 at 4:33 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a> > <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>>> wrote: > > On 05/17/2016 09:35 AM, Plamena Manolova wrote: > > According to the EGL specifications before binding an API > > we must check whether it's supported first. If not eglBindAPI > > should return EGL_FALSE and generate a EGL_BAD_PARAMETER error. > > Can you provide a spec quotation? > > > <a href="https://www.khronos.org/registry/egl/sdk/docs/man/html/eglBindAPI.xhtml" rel="noreferrer" target="_blank">https://www.khronos.org/registry/egl/sdk/docs/man/html/eglBindAPI.xhtml</a> > > "EGL_BAD_PARAMETER is generated if api is not one of the accepted > tokens, or if the > specified client API is not supported by the EGL implementation." That's the man page, not the spec. We have found a few problems over the years in using man page quotations, so we generally prefer to use the spec. The biggest issue is that it's harder to track changes in the man pages, but we can pretty easily tell when something changed between, say, EGL 1.2 and EGL 1.5. If nothing else, it makes the quotation practice more consistent to always use the same kind of source. In Mesa, changes like this should be accompanied by a quotation, in a canonical format, from the specification. Ideally, the quotation should go in the code being changed. It may also be acceptable to include the quotation in the commit message. The next person to look at this code is either going to just look at the code or look at the commit that added it (after using git-blame). For this case, the proper spec quotation would be: Section 3.7 (Rendering Contexts) of the EGL 1.5 spec says: "api must specify one of the supported client APIs, either EGL_OPENGL_API, EGL_OPENGL_ES_API, or EGL_OPENVG_API... If api is not one of the values specified above, or if the client API specified by api is not supported by the implementation, an EGL_BAD_PARAMETER error is generated." </blockquote><div> </div><div>Thanks for clearing this up for me Ian, I'm pretty new to Mesa so pointers like this</div><div>are really appreciated. I'll keep it mind for next time. I'll also include the quotation in</div><div>the code for my follow up patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > Signed-off-by: Plamena Manolova <<a href="mailto:plamena.manolova@intel.com">plamena.manolova@intel.com</a> > <mailto:<a href="mailto:plamena.manolova@intel.com">plamena.manolova@intel.com</a>>> <div class="HOEnZb"><div class="h5">> > --- > > src/egl/main/eglcurrent.h | 33 ++++++++++++++++++++++++++++++--- > > 1 file changed, 30 insertions(+), 3 deletions(-) > > > > diff --git a/src/egl/main/eglcurrent.h b/src/egl/main/eglcurrent.h > > index 1e386ac..f2e19cc 100644 > > --- a/src/egl/main/eglcurrent.h > > +++ b/src/egl/main/eglcurrent.h > > @@ -32,7 +32,8 @@ > > #include "c99_compat.h" > > > > #include "egltypedefs.h" > > - > > +#include "eglglobals.h" > > +#include "egldisplay.h" > > > > #ifdef __cplusplus > > extern "C" { > > @@ -62,14 +63,40 @@ struct _egl_thread_info > > EGLint CurrentAPIIndex; > > }; > > > > - > > +static inline EGLBoolean > > +_eglDisplaySupportsApi(_EGLDisplay *dpy, EGLenum api) > > Since this is only used internally, please use bool/true/false. Based > on my comments at the bottom, I think this function should go directly > in eglapi.c. > > > +{ > > This is a really complex way of doing something quite simple. How > about. > > unsigned api_bit; > > if (!dpy->Initialized) > return false: > > switch (api) { > case EGL_OPENGL_API: > api_bit = EGL_OPENGL_BIT; > break; > case EGL_OPENGL_ES_API: > api_bit = EGL_OPENGL_ES_BIT | > EGL_OPENGL_ES2_BIT | > EGL_OPENGL_ES3_BIT_KHR; > break; > case EGL_OPENVG_API: > api_bit = EGL_OPENVG_BIT; > break; > default: > api_bit = 0; > break; > } > > return (dpy->ClientAPIs & api_bit) != 0; > > > I'll make those changes. > > > > > + if (!dpy->Initialized) { > > + return EGL_FALSE; > > + } else if (api == EGL_OPENGL_API && dpy->ClientAPIs & EGL_OPENGL_BIT) { > > + return EGL_TRUE; > > + } else if (api == EGL_OPENGL_ES_API && > > + (dpy->ClientAPIs & EGL_OPENGL_ES_BIT || > > + dpy->ClientAPIs & EGL_OPENGL_ES2_BIT || > > + dpy->ClientAPIs & EGL_OPENGL_ES3_BIT_KHR)) { > > + return EGL_TRUE; > > + } else if (api == EGL_OPENVG_API && dpy->ClientAPIs & EGL_OPENVG_BIT) { > > + return EGL_TRUE; > > + } else { > > + return EGL_FALSE; > > + } > > +} > > /** > > * Return true if a client API enum is recognized. > > */ > > static inline EGLBoolean > > _eglIsApiValid(EGLenum api) > > There's only one user of this function, and I suspect there will only > every be that one user. I would do a first patch that moves this > function into eglapi.c... maybe even just move this function into the > only caller. Then this patch would change the way it works. > > > Makes sense I'll go ahead and move it to the actual caller. > > </div></div><div class="HOEnZb"><div class="h5">> > { > > - return (api >= _EGL_API_FIRST_API && api <= _EGL_API_LAST_API); > > + _EGLDisplay *dpy = _eglGlobal.DisplayList; > > + > > + while (dpy) { > > + if (_eglDisplaySupportsApi(dpy, api) == EGL_TRUE) > > Even without changing _eglDisplaySupportsApi to return bool, you can > drop the '== EGL_TRUE'. > > > + return EGL_TRUE; > > + > > + dpy = dpy->Next; > > + } > > + > > + return EGL_FALSE; > > } > > > > > </div></div></blockquote></div> </div></div>