[Mesa-dev] [PATCH 2/2] egl: Check if API is supported when using eglBindAPI.

[Ian Romanick]

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 <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> 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?
> 
> 
> https://www.khronos.org/registry/egl/sdk/docs/man/html/eglBindAPI.xhtml
> 
> "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."

>     > Signed-off-by: Plamena Manolova <plamena.manolova at intel.com
>     <mailto:plamena.manolova at intel.com>>
>     > ---
>     >  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.
>  
> 
>     >  {
>     > -   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;
>     >  }
>     >
>     >
>