GNOME Bugzilla – Bug 742678
gl: Prefer gl3 over the gl driver
Last modified: 2015-12-11 16:04:48 UTC
See patch.
Created attachment 294189 [details] [review] gl: Prefer gl3 over the gl driver Instead of using the deprecated gl features try using a core profile (3.1+) first. This is in line with what gtk's gl support does and mesa's gl 3.1 supportis in a stable and working now.
Review of attachment 294189 [details] [review]: sounds fair to me. I guess you'll need an ACK from Robert or Neil on the cogl mailing list.
OK talked with Neil about this on IRC ... we can't really do that basically because of bug 728640 But if we want to change it in clutter we could ask cogl to load the gl3 driver and if it fails try again with ANY (which will load the gl driver). We might also want to check performance but perf.gnome.org should take care of that i.e if the change causes a noticeable slowdown we could simply revert it.
Created attachment 294368 [details] [review] backend: Check for a known set of drivers We want to use the Cogl GL3 driver, if possible, and then go through a known list of Cogl drivers, before giving up and using COGL_DRIVER_ANY.
it seems that the extension check is not done during cogl_renderer_connect(), but at a later point. this means that cogl_renderer_connect(COGL_DRIVER_GL3) will happily oblige, but then cogl_context_init() will die horribly. I don't think this is going to be as pretty as hoped. we'll have to not only try renderer_connect() in a loop, but also to build up a CoglContext and check if it works.
Created attachment 308164 [details] [review] winsys-glx: Add error traps around create_gl3_context This might fail with a GLXBadFBConfig when the GL3 driver is loaded on a non GL3 supporting driver like llvmpipe.
Created attachment 308165 [details] [review] backend: Check for a known set of drivers We want to use the Cogl GL3 driver, if possible, and then go through a known list of Cogl drivers, before giving up and using COGL_DRIVER_ANY. Based on original patch from Emmanuele Bassi. We have to create and tear down the whole context when trying out the drivers though because the extension checks do not happen until cogl_context_init. ---- OK with this it works i.e it tryes to load the drivers and sets up the context in a loop. Seems to work fine when switching between llvmpipe and real hw drivers, the former properly falls back to the legacy context.
Review of attachment 308164 [details] [review]: Looks generally good, with one minor issue. ::: cogl/winsys/cogl-winsys-glx.c @@ +1079,3 @@ + { + CoglXlibTrapState old_state; + _cogl_xlib_renderer_trap_errors (display->renderer, &old_state); I would put the error trape around the whole if…else block; glXCreateNewContext() can also fail with an X11 error, and it's a sheer accident that nothing triggered it until now.
Review of attachment 308165 [details] [review]: ::: clutter/clutter-backend.c @@ +366,3 @@ + GError **error) +{ + static const struct { Coding style: the whole indentation of the patch is off. @@ +377,3 @@ + + GError *internal_error = NULL; + gboolean found = FALSE; Instead of an additional boolean variable, you can check if `backend->cogl_context` is set. @@ +386,3 @@ + if (clutter_backend_do_real_create_context (backend, known_drivers[i].driver_id, &internal_error)) + { + found = TRUE; Coding style: too much indentation. @@ +391,3 @@ + if (internal_error) + { + CLUTTER_NOTE (BACKEND, "Unable to use the %s driver: %s", Coding style: please, line up the arguments after the opening parenthesis. @@ +398,3 @@ + } + + if (!found) Use backend->cogl_context != NULL instead. @@ +400,3 @@ + if (!found) + { + if (internal_error != NULL) Coding style: too much indentation. @@ +405,3 @@ + g_set_error_literal (error, CLUTTER_INIT_ERROR, + CLUTTER_INIT_ERROR_BACKEND, + _("Unable to initialize the Clutter backend")); You need to return FALSE here, otherwise you're going to create a CoglSource for a missing CoglContext. @@ +408,3 @@ + } + + backend->cogl_source = cogl_glib_source_new (backend->cogl_context, Coding style: too much indentation. @@ +412,3 @@ + g_source_attach (backend->cogl_source, NULL); + + return found; If we got here, then we only should return TRUE.
Created attachment 308169 [details] [review] winsys-glx: Add error traps in create_context Both create_gl3_context and glXCreateNewContext can fail with an X error.
Created attachment 308170 [details] [review] backend: Check for a known set of drivers We want to use the Cogl GL3 driver, if possible, and then go through a known list of Cogl drivers, before giving up and using COGL_DRIVER_ANY. Based on original patch from Emmanuele Bassi. We have to create and tear down the whole context when trying out the drivers though because the extension checks do not happen until cogl_context_init. --- * Fixed broken indentation * Removed the found boolean * Don't create a CoglSource for a missing context
Review of attachment 308169 [details] [review]: Looks good.
Review of attachment 308170 [details] [review]: Looks good, please push to master.
Comment on attachment 308169 [details] [review] winsys-glx: Add error traps in create_context Attachment 308169 [details] pushed as 4f8254f - winsys-glx: Add error traps in create_context
Attachment 308170 [details] pushed as 9a510c0 - backend: Check for a known set of drivers
This causes a regression on my setup (EGL/GLES2, ARM Mali). Before this was in place, mutter was nice and fast, using the GPU. Now mutter runs at ~0.2fps and git bisect lead me to this commit. I guess it is now preferring GL (software rendering) over GLES2.
The list of backend tried are, in order: GL3 core profile GL legacy GLES 2.0 ANY So the EGL/GLES2, ARM Mali platform seems to still provide GL3 core profile support. I wonder if you should build Cogl without GL support, but with GLES2 support only for that platform. Otherwise, it should be possible to add an environment variable to override the order of the drivers on a platform-specific basis.
Created attachment 317019 [details] [review] backend: Allow overriding the Cogl drivers chain We have an hardcoded list of drivers we have to go through when creating a Cogl context. Some platforms may expose those drivers, but not be the preferred ones. In order to allow users and system integrators to override the list of drivers, we should crib the same approach used by GDK, and have an environment variable with a list of drivers to try. The new environment variable is called `CLUTTER_DRIVER` and accepts a comma-separated list of driver names, which will be tested in sequence until one succeeds. There's also an additional '*' token which is used to ask Clutter to fall back to the internally defined preferred list of drivers.
Daniel: could you try applying the patch in attachment 317019 [details] [review] and using the CLUTTER_DRIVER environment variable? Would it be a satisfactory solution for you?
It doesn't seem ideal. Somehow, previous to this patch, clutter/cogl supported both GL & GLES2 but knew to prefer GLES2. If there was a good reason why that was happening (perhaps the --with-default-driver=gles2 that we pass to cogl?), it would be nice if that could remain.
(In reply to Daniel Drake from comment #20) > It doesn't seem ideal. Somehow, previous to this patch, clutter/cogl > supported both GL & GLES2 but knew to prefer GLES2. Prior to this patch, Clutter defaulted to the 'ANY' driver for Cogl — which would be resolved to the GLES 2.0 driver using --with-default-driver. As this bug describes, we cannot ask Cogl to check which driver to use — it has to be the calling library that decides, and eventually falls back to the default driver inside Cogl. We also want the GL 3.2 core profile driver to be the default one because it's the one that gives us better performance and memory usage going forward on desktop GL. > If there was a good > reason why that was happening (perhaps the --with-default-driver=gles2 that > we pass to cogl?), it would be nice if that could remain. Since Cogl on the Mali 400-based platform will always use GLES 2.0, why not disable GL support when building Cogl there, instead of building with both desktop GL and GLES 2.0 enabled? Without GL support, Cogl would automatically skip the first two drivers and fall back to GLES 2.0 automatically. I can also add a default driver to Clutter as well, so you could compiler Clutter using --with-default-driver=gles2.
Created attachment 317040 [details] [review] backend: Select the 'any' Cogl driver after the GL3 one Fall back to the default Cogl driver if loading the GL3 driver fails.
Created attachment 317041 [details] [review] Add a configuration option for deciding the Cogl drivers to use Using environment variables only is not convenient for all platforms, and in some cases it's beneficial to decide the default driver when building Clutter. Cogl already has a similar configuration switch, and since Clutter is overriding the default Cogl behaviour, it should offer the same mechanism.
With attachment 317041 [details] [review] you can use `--with-default-drivers='gles2,*'` when building Clutter, and the gles2 driver will be used first, with a fallback to the existing chain.
I disabled the GL driver in cogl as this made some sense regardless, and at that point, clutter starts using gles2 without any changes. I don't grasp enough of the situation to know which of the above clutter changes makes the most sense. Perhaps out of the scope of this bug report, but it would be worth giving this situation a bit more thought. Currently, with cogl/clutter, there is no way for a distro to ship a "one size fits all" setup. Right now the GLES vs GL choice is made at compile time, and even if it could be changed with an environment variable, that's also not ideal, as it still wouldn't be "works out of the box" on both setups.
Review of attachment 317040 [details] [review]: That looks like it would fix Daniel's issue ... but if we do that is there any reason to keep gl and gles in the list?
(In reply to drago01 from comment #26) > Review of attachment 317040 [details] [review] [review]: > > That looks like it would fix Daniel's issue ... It depends on whether at some point llvmpipe/softrast are going to support GL3, in which case we're going back to square one, and Clutter will favour GL to GLES. Not that I'm holding my breath on that one, given the IP issues that prevent Mesa software rasterisers to support GL 3.2+ core profile. > but if we do that is there any reason to keep gl and gles in the list? It'd reasonable to drop them if the 'any' driver is moved up, yes.
(In reply to Daniel Drake from comment #25) > I disabled the GL driver in cogl as this made some sense regardless, and at > that point, clutter starts using gles2 without any changes. Yes, that's expected, since both GL and GL3 will be disabled inside Cogl, and either Clutter will use the GLES2 driver or fall back to the ANY driver. > I don't grasp > enough of the situation to know which of the above clutter changes makes the > most sense. I'll land them all anyway, because it does make sense to allow overriding the chain; we do that in other places. > Perhaps out of the scope of this bug report, but it would be worth giving > this situation a bit more thought. Currently, with cogl/clutter, there is no > way for a distro to ship a "one size fits all" setup. Because there is no such thing of a "one size fits all" set up, especially when it comes down to GL; as long as there are implementations that provide both GL and GLES on the same platform (which are basically all of them, if we count software rasterisers), you need a way to determine the driver to use. That's the whole point of the driver API inside Cogl. We cannot divine which driver to use from the outside of a GL implementation because GL is layered in such a way where you don't really know if a capability is actually implemented in hardware or software. We could look at the context created, check via the vendor string if it's a software renderer, and skip the driver when walking down the list, but it's perfectly legitimate to use a software renderer in various cases — like virtual machines. Alternatively, we could have API inside Clutter that lets the caller specify the driver, but we'd just move the problem up in the stack to every user of the Clutter API; now each application, like Mutter, would have to decide whether to use the GL driver or the GLES driver — depending on environment variables, or configuration files, or whatever else.
Pushed all the patches, except the one that moves the 'any' driver before gl legacy and gles2; I want Clutter to retain control of the list of available drivers, so that users can override it wherever needed. For integrators building a system with a hardware accelerated GLES 2.0 implementation and a software GL implementation, the suggested behaviour is to pick any one of these: - build Cogl without GL support - configure Clutter with --with-default-drivers='gles2' - ship a 'settings.ini' configuration file in SYSCONFDIR/clutter-1.0 that specifies the 'gles2' Cogl driver as the default one - ship an environment file that sets the CLUTTER_DRIVER environment variable to 'gles2' As far as I'm concerned, between OpenGL and OpenGL ES, Clutter will always select the former, in absence of per-system overrides.