[PATCH weston] gl-renderer: fix EGL initialization steps

Thu Apr 2 02:51:56 PDT 2015

On Mon, 30 Mar 2015 21:48:32 +0200
Manuel Bachmann <manuel.bachmann at open.eurogiciel.org> wrote:

> OK. Found it, thanks to your hint, Daniel.
> 
> This version of Mesa (10.1.3) does not feature these extensions.
> 
> They were added here :
> http://cgit.freedesktop.org/mesa/mesa/commit/src/egl/main/eglglobals.c?id=468cc866b4b308cee40470f06b31002c6c56da96
> 
> Considering that recent distributions, such as Ubuntu 14.04 LTS (Debian,
> RHEL.. are even older) may not package a sufficiently recent version of
> Mesa, maybe it is wise to ignore the test if "extensions" is only
> "EGL_EXT_client_extensions" (which should not happen with recent Mesa,
> because at least one platform should be supported there) ?

Hi,

I think the correct test is to check if the string contains
"EGL_EXT_platform_base". If the client extensions string cannot be
queried or does not contain "EGL_EXT_platform_base", then supports()
should return 0 to indicate "maybe", which means we fall back to the old
way.

Thanks,
pq

> 2015-03-30 21:38 GMT+02:00 Manuel Bachmann <
> manuel.bachmann at open.eurogiciel.org>:
> 
> > Hi Daniel, and thanks,
> >
> > "What is the complete client extension string on that platform?"
> >
> > It is simply "EGL_EXT_client_extensions".
> >
> > Here are the details : kernel 3.13.0 - Mesa 10.1.3 (built with
> > "with-egl-platforms=wayland,x11,drm") - kernel module is "i915" - EGL is :
> > 1.4 (DRI2) - GL vendor is : Intel Open Source Technology Center - GL
> > renderer is : Mesa DRI Intel(R) Ivybridge Model.
> > GPU seems to be : Intel HP Graphics 4000 (core i7 CPU)
> >
> > 2015-03-30 20:52 GMT+02:00 Daniel Stone <daniel at fooishbar.org>:
> >
> >> Hi,
> >>
> >> On 30 March 2015 at 17:38, Manuel Bachmann
> >> <manuel.bachmann at open.eurogiciel.org> wrote:
> >> > "You removed the early return here...
> >> > ...which means this check may be accessing NULL extensions, no?"
> >> >
> >> > You are right ! Sorry for that, did not even look at the #ifdef, will
> >> rework
> >> > this part.
> >>
> >> Apart from the configless_context damage, the rest (don't make lack of
> >> client extensions query fatal) is very much correct. I think that got
> >> lost in review. :\
> >>
> >> > "You do want to keep the -1 return as fatal here. gl_renderer_supports()
> >> > should already return 0 meaning "maybe" if client extensions cannot be
> >> > queried. The supports function is very nicely documented."
> >> >
> >> > Well, I have here a pratical case of a Ubuntu 14.04 laptop (Intel GPU,
> >> i915
> >> > driver, will let you know the hardware, Mesa and libs versions soon
> >> when I
> >> > get my hands on it), where the supports() function is able to find an
> >> > "*extensions" strings, but then fails to locate these precise
> >> extensions,
> >> > and thus returns -1. It prevents the X11 and DRM compositors to run,
> >> but it
> >> > works nicely when removing this check. Does it make sense to you ?
> >> > (regarding the purpose of the function, is that when building EGL with
> >> X11
> >> > support, it will always report X11 as a supported platform ? Maybe
> >> there are
> >> > versions of Mesa which do not honor this. Will check.)
> >>
> >> Weird - the only way I can see that happening is if Mesa advertises
> >> support for platform_base, but not for platform_{x11,gbm}, which seems
> >> very odd indeed. Ever since the addition of platform_base support[0],
> >> Mesa has supported EXT variants of x11 and wayland, and a MESA variant
> >> of gbm. So given the (fairly exhaustive) checks in
> >> gl_renderer_supports, I can't see how we'd end up in this situation.
> >> What is the complete client extension string on that platform?
> >>
> >> Cheers,
> >> Daniel
> >>
> >> [0]:
> >> http://cgit.freedesktop.org/mesa/mesa/commit/src/egl/main/eglglobals.c?id=468cc866b4b308cee40470f06b31002c6c56da96
> >>
> >> > 2015-03-30 14:40 GMT+02:00 Pekka Paalanen <ppaalanen at gmail.com>:
> >> >>
> >> >> On Sun, 29 Mar 2015 05:03:09 +0200
> >> >> Manuel Bachmann <manuel.bachmann at open.eurogiciel.org> wrote:
> >> >>
> >> >> > From: Manuel Bachmann <manuel.bachmann at open.eurogiciel.org>
> >> >> >
> >> >> > We should not prevent gl-renderer to initalize if client
> >> >> > extensions were not found. Practically, this prevented
> >> >> > Weston from running with GL on i915 DRI platforms.
> >> >> >
> >> >> > Some DRI drivers, including VMware vmwgfx, do not support
> >> >> > calling eglQueryString() with a EGL_NO_DISPLAY parameter.
> >> >> > Just as we do in gl_renderer_supports(), which returns 0
> >> >> > but does not fail in this case, do not fail in
> >> >> > gl_renderer_setup_egl_extensions().
> >> >> > ---
> >> >> >  src/gl-renderer.c | 18 +++++++-----------
> >> >> >  1 file changed, 7 insertions(+), 11 deletions(-)
> >> >> >
> >> >> > diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> >> >> > index b3b2364..28ecc63 100644
> >> >> > --- a/src/gl-renderer.c
> >> >> > +++ b/src/gl-renderer.c
> >> >> > @@ -2136,15 +2136,14 @@ gl_renderer_setup_egl_extensions(struct
> >> >> > weston_compositor *ec)
> >> >> >               (const char *) eglQueryString(EGL_NO_DISPLAY,
> >> >> > EGL_EXTENSIONS);
> >> >> >       if (!extensions) {
> >> >> >               weston_log("Retrieving EGL client extension string
> >> >> > failed.\n");
> >> >> > -             return -1;
> >> >> > +     } else {
> >> >> > +             if (strstr(extensions, "EGL_EXT_platform_base"))
> >> >> > +                     gr->create_platform_window =
> >> >> > +                             (void *)
> >> >> > eglGetProcAddress("eglCreatePlatformWindowSurfaceEXT");
> >> >> > +             else
> >> >> > +                     weston_log("warning: EGL_EXT_platform_base not
> >> >> > supported.\n");
> >> >> >       }
> >> >>
> >> >> You removed the early return here...
> >> >>
> >> >> >
> >> >> > -     if (strstr(extensions, "EGL_EXT_platform_base"))
> >> >> > -             gr->create_platform_window =
> >> >> > -                     (void *)
> >> >> > eglGetProcAddress("eglCreatePlatformWindowSurfaceEXT");
> >> >> > -     else
> >> >> > -             weston_log("warning: EGL_EXT_platform_base not
> >> >> > supported.\n");
> >> >> > -
> >> >> >  #ifdef EGL_MESA_configless_context
> >> >> >       if (strstr(extensions, "EGL_MESA_configless_context"))
> >> >> >               gr->has_configless_context = 1;
> >> >>
> >> >> ...which means this check may be accessing NULL extensions, no?
> >> >>
> >> >> Hmm, I wonder if the configless context check is looking at the right
> >> >> extensions string here.
> >> >>
> >> >> > @@ -2256,12 +2255,9 @@ gl_renderer_create(struct weston_compositor
> >> *ec,
> >> >> > EGLenum platform,
> >> >> >       EGLint major, minor;
> >> >> >       int supports = 0;
> >> >> >
> >> >> > -     if (platform) {
> >> >> > +     if (platform)
> >> >> >               supports = gl_renderer_supports(
> >> >> >                       ec, platform_to_extension(platform));
> >> >> > -             if (supports < 0)
> >> >> > -                     return -1;
> >> >> > -     }
> >> >>
> >> >> Just make sure that if EXT_platform_base exists, but the specific
> >> >> platform we are looking for does not, we fail this function.
> >> >>
> >> >> You do want to keep the -1 return as fatal here. gl_renderer_supports()
> >> >> should already return 0 meaning "maybe" if client extensions cannot be
> >> >> queried. The supports function is very nicely documented.
> >> >>
> >> >> >
> >> >> >       gr = zalloc(sizeof *gr);
> >> >> >       if (gr == NULL)
> >> >>
> >> >>