GNOME Bugzilla – Bug 686806
./gnome-session-check-accelerated-helper doesn't work with EGL
Last modified: 2016-04-09 04:09:29 UTC
On Ubuntu ARM, libcogl is built with the EGL backend and gnome-shell can run on top of that. ./gnome-session-check-accelerated-helper uses only libGL and reports gnome-session-is-accelerated: No hardware 3D support. despite the fact that Clutter is happily running the shell. Probably we should have the tool based on libcogl since that is what we really want to know: will Clutter apps run?
sounds reasonable.
In the meanwhile, is it perhaps possible to force loading shell without the acceleration check?
you can remove the IsRunnableHelper=/usr/libexec/gnome-session-check-accelerated line from /usr/share/gnome-session/sessions/gnome.session
*** Bug 728757 has been marked as a duplicate of this bug. ***
*** Bug 707222 has been marked as a duplicate of this bug. ***
Created attachment 309982 [details] [review] Make OpenGL dependency optinal Temporal solution until we don't port to GLES
Review of attachment 309982 [details] [review]: i don't think that's sufficient. we use the helper, so if you don't build it you're going to get runtime problems right?
See also bug 754256.
gnome-session has '--disable-acceleration-check', most likely it is easiest/fastest way to temporarily disable this check. Exec=gnome-session --disable-acceleration-check and/or Exec=gnome-session --session=gnome-wayland --disable-acceleration-check
Created attachment 325294 [details] [review] tools: rename GL check helper Since we're going to add another one based on gles.
Created attachment 325295 [details] [review] Add a helper to check if session can run on GLES This is needed on platforms where GLX is not typically available, such as when running on an ARM Mali chipset. The helper is currently not wired, but it will be in a later commit.
Created attachment 325296 [details] [review] gl-helper: add a --print-renderer option This can be useful for other programs (e.g. gnome-control-center) to display the renderer string in an UI. The GLES version of this already has the same option.
Created attachment 325297 [details] [review] Check for GLES support when determining if session is accelerated Connect the new GLES helper to the code that determines if the session is accelerated.
Created attachment 325298 [details] [review] tools: remove unused code
These patches add another helper to use for GLES. A possible further improvement would be to add Wayland support to the GLES helper, but that can be done in a follow up after this gets in.
Review of attachment 325294 [details] [review]: Looks good.
Review of attachment 325295 [details] [review]: Can't really comment on the GLES code, but looks fine overall. Could you try to find somebody to review the GLES code itself?
Review of attachment 325296 [details] [review]: Looks good. Although, if we were to actually use this from gnome-control-center, and as gnome-session already runs this tool, maybe we could have gnome-session export this information, instead of running the tools *again* when the settings panel is opened. A bit more complicated for marginal gains, but cleaner IMO.
Review of attachment 325297 [details] [review]: Looks good otherwise. ::: tools/gnome-session-check-accelerated.c @@ +186,3 @@ + + if (error != NULL) { + g_printerr ("gnome-session-check-accelerated: Failed to run GL helper: %s\n", error->message); Will this end up always printing an error for GLES platforms? If so, we should probably delay the printing of an error until both helpers have been run.
Review of attachment 325298 [details] [review]: Sure.
Created attachment 325599 [details] [review] Check for GLES support when determining if session is accelerated Connect the new GLES helper to the code that determines if the session is accelerated.
Thanks for the review - I updated the patch for your comments, Bastien. I considered storing the renderer string, but I would rather keep this patch simpler for now - and storing it on e.g. a X property would not work anyway on Wayland. So I would rather merge this now and perhaps keep that in mind as a future improvement.
Review of attachment 325599 [details] [review]: Looks good. ::: tools/gnome-session-check-accelerated.c @@ +175,3 @@ + /* First, try the GL helper */ + if (g_spawn_sync (NULL, (char**)gl_helper_argv, NULL, 0, "char **" @@ +186,3 @@ + + /* Then, try the GLES helper */ + if (g_spawn_sync (NULL, (char**)gles_helper_argv, NULL, 0, Ditto.
(In reply to Cosimo Cecchi from comment #22) > Thanks for the review - I updated the patch for your comments, Bastien. > > I considered storing the renderer string, but I would rather keep this patch > simpler for now - and storing it on e.g. a X property X property? This isn't the 90's, I was talking about a D-Bus property ;) The stdout of the helper can give it back, and we'd store it in a D-Bus property. > would not work anyway > on Wayland. So I would rather merge this now and perhaps keep that in mind > as a future improvement. Like for when you'll want to duplicate most of this code in gnome-control-center's Details panel? :)
Fair enough :-) I was mentioning an X property because that's where other similar things are stored by the helper. I will try to export this over DBus as a start.
Created attachment 325616 [details] [review] Add a helper to check if session can run on GLES This is needed on platforms where GLX is not typically available, such as when running on an ARM Mali chipset. The helper is currently not wired, but it will be in a later commit.
Created attachment 325617 [details] [review] gl-helper: add a --print-renderer option This can be useful for other programs (e.g. gnome-control-center) to display the renderer string in an UI. The GLES version of this already has the same option.
Created attachment 325618 [details] [review] Check for GLES support when determining if session is accelerated Connect the new GLES helper to the code that determines if the session is accelerated.
Created attachment 325619 [details] [review] Export GL/GLES renderer string over DBus This is useful to avoid repeating the same code e.g. in the Details settings panel. Note that it's possible that both the display manager and the user session run gnome-session, and that they both share the same X server. In that case, we need to store the renderer string as an X property on the root window (like the helper already does for other properties).
Comment on attachment 325616 [details] [review] Add a helper to check if session can run on GLES Resetting patch status
Comment on attachment 325617 [details] [review] gl-helper: add a --print-renderer option Resetting patch status
Comment on attachment 325618 [details] [review] Check for GLES support when determining if session is accelerated Resetting patch status
Added a patch to export the renderer over DBus. See the patches in bug 754256 for the gnome-control-center side that makes use of that information.
Review of attachment 325619 [details] [review]: Looks good to me apart from the doc in the XML. ::: gnome-session/org.gnome.SessionManager.xml @@ +428,3 @@ + <doc:doc> + <doc:description> + <doc:para>The renderer for the session that has been loaded.</doc:para> Can you be more explicit here? For example, mention that it's not used by Wayland, that it's there for GL and GLES, etc.
Attachment 325294 [details] pushed as cac547e - tools: rename GL check helper Attachment 325298 [details] pushed as 00b382c - tools: remove unused code Attachment 325616 [details] pushed as 143dfdc - Add a helper to check if session can run on GLES Attachment 325617 [details] pushed as 101aa21 - gl-helper: add a --print-renderer option Attachment 325618 [details] pushed as f144cd2 - Check for GLES support when determining if session is accelerated Attachment 325619 [details] pushed as 122fa57 - Export GL/GLES renderer string over DBus Pushed all to master. Thanks for the reviews!