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

[Manuel Bachmann]

"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.

"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.)

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)
>
>
> Thanks,
> pq
>



-- 
Regards,



*Manuel BACHMANN Tizen Project VANNES-FR*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150330/bbbfaa6f/attachment.html>