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

[Daniel Stone]

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)
>>
>>
>> Thanks,
>> pq
>
>
>
>
> --
> Regards,
>
> Manuel BACHMANN
> Tizen Project
> VANNES-FR
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>