[PATCH xserver] glamor: Use eglGetPlatformDisplayEXT not eglGetDisplay

[Emil Velikov]

On 4 October 2016 at 18:34, Adam Jackson <ajax at redhat.com> wrote:
> eglGetDisplay forces the implementation to guess which kind of display
> it's been handed. glvnd does something different from Mesa, and in
> general it's impossible for the library to get this right. Instead use
> the API where you specify what kind of display it is.
>
> The explicit call to eglGetProcAddress is to work around a bug in
> libepoxy 1.3, which does not understand EGL client extensions and so its
> resolver code will crash.
>
> Signed-off-by: Adam Jackson <ajax at redhat.com>
> ---
>  glamor/glamor_egl.c           | 9 ++++++++-
>  hw/xwayland/xwayland-glamor.c | 8 +++++++-
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/glamor/glamor_egl.c b/glamor/glamor_egl.c
> index 2b9e0e1..51d8147 100644
> --- a/glamor/glamor_egl.c
> +++ b/glamor/glamor_egl.c
> @@ -736,6 +736,7 @@ glamor_egl_init(ScrnInfoPtr scrn, int fd)
>  {
>      struct glamor_egl_screen_private *glamor_egl;
>      const char *version;
> +    PFNEGLGETPLATFORMDISPLAYEXTPROC getPlatformDisplay;
>
>      EGLint config_attribs[] = {
>  #ifdef GLAMOR_GLES2
> @@ -768,7 +769,13 @@ glamor_egl_init(ScrnInfoPtr scrn, int fd)
>          ErrorF("couldn't get display device\n");
>          goto error;
>      }
> -    glamor_egl->display = eglGetDisplay(glamor_egl->gbm);
> +
> +    getPlatformDisplay = (PFNEGLGETPLATFORMDISPLAYEXTPROC)
> +        eglGetProcAddress("eglGetPlatformDisplayEXT");
> +
> +    if (getPlatformDisplay)
> +        glamor_egl->display = getPlatformDisplay (EGL_PLATFORM_GBM_MESA,
> +                                                  glamor_egl->gbm, NULL);
An implementation can return non NULL value even if the extension is
not supported. So one needs to add something like:

ext = eglQueryString(NO_DISPLAY, EGL_EXTENSIONS);
if (ext && (has_extension(ext, "EGL_MESA_platform_gbm") ||
has_extension(ext, "EGL_KHR_platform_gbm")) {
  // your code
  ...
}

Fwiw one can even make this a static inline and use it in both places.
Adding a note about epoxy is a good idea imho.


>  #else
>      glamor_egl->display = eglGetDisplay((EGLNativeDisplayType) (intptr_t) fd);
>  #endif
We seem to be missing the error checking after we (attempt to) get the dpy.

> diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
> index 068c224..23402f9 100644
> --- a/hw/xwayland/xwayland-glamor.c
> +++ b/hw/xwayland/xwayland-glamor.c
> @@ -280,6 +280,7 @@ xwl_drm_init_egl(struct xwl_screen *xwl_screen)
>          GLAMOR_GL_CORE_VER_MINOR,
>          EGL_NONE
>      };
> +    PFNEGLGETPLATFORMDISPLAYEXTPROC getPlatformDisplay;
>
>      if (xwl_screen->egl_display)
>          return;
> @@ -292,7 +293,12 @@ xwl_drm_init_egl(struct xwl_screen *xwl_screen)
>          return;
>      }
>
> -    xwl_screen->egl_display = eglGetDisplay(xwl_screen->gbm);
> +    getPlatformDisplay = (PFNEGLGETPLATFORMDISPLAYEXTPROC)
> +        eglGetProcAddress("eglGetPlatformDisplayEXT");
> +
> +    if (getPlatformDisplay)
> +        xwl_screen->egl_display = getPlatformDisplay(EGL_PLATFORM_GBM_MESA,
> +                                                     xwl_screen->gbm, NULL);
>      if (xwl_screen->egl_display == EGL_NO_DISPLAY) {
>          ErrorF("eglGetDisplay() failed\n");
s/eglGetDisplay/eglGetPlatformDisplayEXT/

Regards,
Emil
P.S. Do you have some work on the final EGL_KHR_debug patches or we
can land them ?