<div dir="ltr"><div><div><div>Agreed ! </div>I am going send the patch again, same name ("gl-renderer: fix EGL initialization steps") but modified in this sense and with another commit message, </div>Regards, </div>Manuel </div><div class="gmail_extra"> <div class="gmail_quote">2015-04-02 11:51 GMT+02:00 Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>>: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, 30 Mar 2015 21:48:32 +0200 Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org">manuel.bachmann@open.eurogiciel.org</a>> 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 : > <a href="http://cgit.freedesktop.org/mesa/mesa/commit/src/egl/main/eglglobals.c?id=468cc866b4b308cee40470f06b31002c6c56da96" target="_blank">http://cgit.freedesktop.org/mesa/mesa/commit/src/egl/main/eglglobals.c?id=468cc866b4b308cee40470f06b31002c6c56da96</a> > > 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 <div class="HOEnZb"><div class="h5"> > 2015-03-30 21:38 GMT+02:00 Manuel Bachmann < > <a href="mailto:manuel.bachmann@open.eurogiciel.org">manuel.bachmann@open.eurogiciel.org</a>>: > > > 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 <<a href="mailto:daniel@fooishbar.org">daniel@fooishbar.org</a>>: > > > >> Hi, > >> > >> On 30 March 2015 at 17:38, Manuel Bachmann > >> <<a href="mailto:manuel.bachmann@open.eurogiciel.org">manuel.bachmann@open.eurogiciel.org</a>> 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]: > >> <a href="http://cgit.freedesktop.org/mesa/mesa/commit/src/egl/main/eglglobals.c?id=468cc866b4b308cee40470f06b31002c6c56da96" target="_blank">http://cgit.freedesktop.org/mesa/mesa/commit/src/egl/main/eglglobals.c?id=468cc866b4b308cee40470f06b31002c6c56da96</a> > >> > >> > 2015-03-30 14:40 GMT+02:00 Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>>: > >> >> > >> >> On Sun, 29 Mar 2015 05:03:09 +0200 > >> >> Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org">manuel.bachmann@open.eurogiciel.org</a>> wrote: > >> >> > >> >> > From: Manuel Bachmann <<a href="mailto:manuel.bachmann@open.eurogiciel.org">manuel.bachmann@open.eurogiciel.org</a>> > >> >> > > >> >> > 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) > >> >> > >> >> </div></div></blockquote></div> -- <div class="gmail_signature"><div dir="ltr">Regards, Manuel BACHMANN Tizen Project VANNES-FR </div></div> </div>