On Tue, 2013-12-17 at 14:40 -0600, Daniel Drake wrote:
> Hi,
>
> Running mutter on ARM SoC with Mali GPU, we are facing frequent hangs while
> opening new windows.
>
> I tracked this down to a Mali driver design flaw. If a process performs an
> OpenGL operation while it has an X server grab, Mali is likely to deadlock.
> Full details at http://community.arm.com/thread/4759
I simply don't understand the point of doing your SwapBuffers in a thead
if you are using DRI2 and the X server is already doing the SwapBuffers
async in the background without blocking. Must have seemed like a good
idea to someone...
> In mutter context, this happens when meta_window_new() calls
> meta_display_grab() to take a server grab, then calls
> meta_window_new_with_attrs(). This then calls meta_workspace_add_window()
> which emits the window-added signal. This signal reaches the shell, which
> tries to draw something via cogl/clutter, the server is still grabbed here,
> --> mali deadlock
>From the partial backtrace posted on the bug, it looks like the code
called in response to ::add-window is not drawing (which would be fairly
surprising) but doing other GL operations - in the backtrace deleting a
texture.
> This is undoubtedly a Mali bug, but I'd like to use the opportunity to
> explore doing less server grabbing.
Abstractly, if we can reduce or eliminate the cases where we have the
server grabbed and emit signals and call out to arbitrary application
code, that's a good thing.
People tend to exaggerate the danger of server grabs - the main problem
with them in an application is that if you are debugging with gdb in a X
terminal, a grab can make your session lock up - but that's always the
case for the compositor. Still, certainly there are opportunities for
deadlock with them.
> For the window creation codepath, my
> colleague Sam suggests the following should suffice:
>
> 1. Grab server
> 2. Check if the window is valid (e.g., XGetWindowAttributes)
> 3. Do anything which would cause X Errors and leave us in an inconsistent
> state if the window was not valid (XReparentWindow into frame,
> XAddToSaveSet, XConfigureWindow so that it is in the right place in the
> stack)
I don't think we need to be quite that broad. If the XReparentWindow()
failed, for example, we would be fine - it's trapped in frame.c. The
only things we have to worry about are a) assumptions that the code is
making that round-trip calls to the X server are succeeding b) one-way
and round-trip calls that don't have an error trap because they were
known to be always called during the window-creation phase c) window
going away before we select for DestroyNotify.
With enough care, it might be possible to XUngrabServer() right after
selecting for DestroyNotify.
> 4. Ungrab server
> 5. Do anything which we don't depend on always succeeding because the window
> is going to receive a DestroyNotify later, this includes setting window
> properties and creating the window texture.
>
> Does this sound like a sensible strategy or are we missing anything?
One thing that definitely should be noted is that we still have a server
grab over everything when this code is called from
meta_screen_manage_all_windows(). From a brief look, it appears to me
that this happens *after* we initialize Javascript, set up signal
connections, etc - so probably the only reason that your patch works for
you is that the driver's internal swap-buffers thread isn't doing
anything during initialization.
This makes me less enthusiastic about the approach - we aren't avoiding
calling application code with a server grab, we're just not doing it in
the case that was causing a problem.
Other than that, I don't have large-scale objections to the patch other
than the inherent bug-introducing potential of moving things around in
meta_window_new_with_attrs(). I would certainly wonder if we could
simply move the ungrab up earlier in the function and fix up anything
in class a) and b) described above.
Have you created a torture test for your app - something that repeatedly
creates a new window and after a short random delay destroys it?
- Owen
> As a point of discussion, here's a patch to work around the issue.
>
> Make sure we call meta_workspace_add_window() with no X server grab held.
> (This function emits a window-added signal which does OpenGL operations)
>
> This is done by changing the semantics of meta_window_new_with_attrs()
> so that it is never called with a grab held. It takes its own grab
> for some operations, but the calls to
> meta_workspace_add_window() were then moved til after the point when the
> grab is released.
>
>
> Index: mutter-3.10.1/src/core/screen.c
> ===================================================================
> --- mutter-3.10.1.orig/src/core/screen.c 1999-12-31 20:15:39.695000032
> -0600
> +++ mutter-3.10.1/src/core/screen.c 1999-12-31 20:15:39.675000032 -0600
> @@ -931,8 +931,6 @@ meta_screen_manage_all_windows (MetaScre
> GList *windows;
> GList *list;
>
> - meta_display_grab (screen->display);
> -
> if (screen->guard_window == None)
> screen->guard_window = create_guard_window (screen->display->xdisplay,
> screen);
> @@ -952,8 +950,6 @@ meta_screen_manage_all_windows (MetaScre
>
> g_list_foreach (windows, (GFunc)g_free, NULL);
> g_list_free (windows);
> -
> - meta_display_ungrab (screen->display);
> }
>
> /**
> Index: mutter-3.10.1/src/core/window.c
> ===================================================================
> --- mutter-3.10.1.orig/src/core/window.c 1999-12-31 20:15:39.695000032
> -0600
> +++ mutter-3.10.1/src/core/window.c 2013-12-17 14:08:38.700000160 -0600
> @@ -664,46 +664,10 @@ meta_window_new (MetaDisplay *display,
> Window xwindow,
> gboolean must_be_viewable)
> {
> - XWindowAttributes attrs;
> - MetaWindow *window;
> -
> - meta_display_grab (display);
> - meta_error_trap_push (display); /* Push a trap over all of window
> - * creation, to reduce XSync() calls
> - */
> -
> - meta_error_trap_push_with_return (display);
> -
> - if (XGetWindowAttributes (display->xdisplay,xwindow, &attrs))
> - {
> - if(meta_error_trap_pop_with_return (display) != Success)
> - {
> - meta_verbose ("Failed to get attributes for window 0x%lx\n",
> - xwindow);
> - meta_error_trap_pop (display);
> - meta_display_ungrab (display);
> - return NULL;
> - }
> - window = meta_window_new_with_attrs (display, xwindow,
> - must_be_viewable,
> - META_COMP_EFFECT_CREATE,
> - &attrs);
> - }
> - else
> - {
> - meta_error_trap_pop_with_return (display);
> - meta_verbose ("Failed to get attributes for window 0x%lx\n",
> - xwindow);
> - meta_error_trap_pop (display);
> - meta_display_ungrab (display);
> - return NULL;
> - }
> -
> -
> - meta_error_trap_pop (display);
> - meta_display_ungrab (display);
> -
> - return window;
> + return meta_window_new_with_attrs (display, xwindow,
> + must_be_viewable,
> + META_COMP_EFFECT_CREATE,
> + NULL);
> }
>
> /* The MUTTER_WM_CLASS_FILTER environment variable is designed for
> @@ -822,6 +786,7 @@ meta_window_new_with_attrs (MetaDisplay
> MetaCompEffect effect,
> XWindowAttributes *attrs)
> {
> + XWindowAttributes _attrs;
> MetaWindow *window;
> GSList *tmp;
> MetaWorkspace *space;
> @@ -830,8 +795,6 @@ meta_window_new_with_attrs (MetaDisplay
> MetaMoveResizeFlags flags;
> MetaScreen *screen;
>
> - g_assert (attrs != NULL);
> -
> meta_verbose ("Attempting to manage 0x%lx\n", xwindow);
>
> if (meta_display_xwindow_is_a_no_focus_window (display, xwindow))
> @@ -841,6 +804,39 @@ meta_window_new_with_attrs (MetaDisplay
> return NULL;
> }
>
> + /* Grab server */
> + meta_display_grab (display);
> + meta_error_trap_push (display); /* Push a trap over all of window
> + * creation, to reduce XSync() calls
> + */
> +
> + if (!attrs)
> + {
> + meta_error_trap_push_with_return (display);
> +
> + if (XGetWindowAttributes (display->xdisplay, xwindow, &_attrs))
> + {
> + if(meta_error_trap_pop_with_return (display) != Success)
> + {
> + meta_verbose ("Failed to get attributes for window 0x%lx\n",
> + xwindow);
> + meta_error_trap_pop (display);
> + meta_display_ungrab (display);
> + return NULL;
> + }
> + }
> + else
> + {
> + meta_error_trap_pop_with_return (display);
> + meta_verbose ("Failed to get attributes for window 0x%lx\n",
> + xwindow);
> + meta_error_trap_pop (display);
> + meta_display_ungrab (display);
> + return NULL;
> + }
> + attrs = &_attrs;
> + }
> +
> screen = NULL;
> for (tmp = display->screens; tmp != NULL; tmp = tmp->next)
> {
> @@ -873,21 +869,19 @@ meta_window_new_with_attrs (MetaDisplay
> )
> ) {
> meta_verbose ("Not managing our own windows\n");
> + meta_error_trap_pop (display);
> + meta_display_ungrab (display);
> return NULL;
> }
>
> if (maybe_filter_window (display, xwindow, must_be_viewable, attrs))
> {
> meta_verbose ("Not managing filtered window\n");
> + meta_error_trap_pop (display);
> + meta_display_ungrab (display);
> return NULL;
> }
>
> - /* Grab server */
> - meta_display_grab (display);
> - meta_error_trap_push (display); /* Push a trap over all of window
> - * creation, to reduce XSync() calls
> - */
> -
> meta_verbose ("must_be_viewable = %d attrs->map_state = %d (%s)\n",
> must_be_viewable,
> attrs->map_state,
> @@ -1302,93 +1296,6 @@ meta_window_new_with_attrs (MetaDisplay
>
> window->on_all_workspaces = should_be_on_all_workspaces (window);
>
> - /* For the workspace, first honor hints,
> - * if that fails put transients with parents,
> - * otherwise put window on active space
> - */
> -
> - if (window->initial_workspace_set)
> - {
> - if (window->initial_workspace == (int) 0xFFFFFFFF)
> - {
> - meta_topic (META_DEBUG_PLACEMENT,
> - "Window %s is initially on all spaces\n",
> - window->desc);
> -
> - /* need to set on_all_workspaces first so that it will be
> - * added to all the MRU lists
> - */
> - window->on_all_workspaces_requested = TRUE;
> - window->on_all_workspaces = TRUE;
> - meta_workspace_add_window (window->screen->active_workspace,
> window);
> - }
> - else
> - {
> - meta_topic (META_DEBUG_PLACEMENT,
> - "Window %s is initially on space %d\n",
> - window->desc, window->initial_workspace);
> -
> - space =
> - meta_screen_get_workspace_by_index (window->screen,
> - window->initial_workspace);
> -
> - if (space)
> - meta_workspace_add_window (space, window);
> - }
> - }
> -
> - /* override-redirect windows are subtly different from other windows
> - * with window->on_all_workspaces == TRUE. Other windows are part of
> - * some workspace (so they can return to that if the flag is turned off),
> - * but appear on other workspaces. override-redirect windows are part
> - * of no workspace.
> - */
> - if (!window->override_redirect)
> - {
> - if (window->workspace == NULL &&
> - window->xtransient_for != None)
> - {
> - /* Try putting dialog on parent's workspace */
> - MetaWindow *parent;
> -
> - parent = meta_display_lookup_x_window (window->display,
> - window->xtransient_for);
> -
> - if (parent && parent->workspace)
> - {
> - meta_topic (META_DEBUG_PLACEMENT,
> - "Putting window %s on same workspace as parent
> %s\n",
> - window->desc, parent->desc);
> -
> - if (parent->on_all_workspaces_requested)
> - {
> - window->on_all_workspaces_requested = TRUE;
> - window->on_all_workspaces = TRUE;
> - }
> -
> - /* this will implicitly add to the appropriate MRU lists
> - */
> - meta_workspace_add_window (parent->workspace, window);
> - }
> - }
> -
> - if (window->workspace == NULL)
> - {
> - meta_topic (META_DEBUG_PLACEMENT,
> - "Putting window %s on active workspace\n",
> - window->desc);
> -
> - space = window->screen->active_workspace;
> -
> - meta_workspace_add_window (space, window);
> - }
> -
> - /* for the various on_all_workspaces = TRUE possible above */
> - meta_window_set_current_workspace_hint (window);
> -
> - meta_window_update_struts (window);
> - }
> -
> g_signal_emit_by_name (window->screen, "window-entered-monitor",
> window->monitor->number, window);
>
> /* Must add window to stack before doing move/resize, since the
> @@ -1485,6 +1392,93 @@ meta_window_new_with_attrs (MetaDisplay
> meta_error_trap_pop (display); /* pop the XSync()-reducing trap */
> meta_display_ungrab (display);
>
> + /* For the workspace, first honor hints,
> + * if that fails put transients with parents,
> + * otherwise put window on active space
> + */
> +
> + if (window->initial_workspace_set)
> + {
> + if (window->initial_workspace == (int) 0xFFFFFFFF)
> + {
> + meta_topic (META_DEBUG_PLACEMENT,
> + "Window %s is initially on all spaces\n",
> + window->desc);
> +
> + /* need to set on_all_workspaces first so that it will be
> + * added to all the MRU lists
> + */
> + window->on_all_workspaces_requested = TRUE;
> + window->on_all_workspaces = TRUE;
> + meta_workspace_add_window (window->screen->active_workspace,
> window);
> + }
> + else
> + {
> + meta_topic (META_DEBUG_PLACEMENT,
> + "Window %s is initially on space %d\n",
> + window->desc, window->initial_workspace);
> +
> + space =
> + meta_screen_get_workspace_by_index (window->screen,
> + window->initial_workspace);
> +
> + if (space)
> + meta_workspace_add_window (space, window);
> + }
> + }
> +
> + /* override-redirect windows are subtly different from other windows
> + * with window->on_all_workspaces == TRUE. Other windows are part of
> + * some workspace (so they can return to that if the flag is turned off),
> + * but appear on other workspaces. override-redirect windows are part
> + * of no workspace.
> + */
> + if (!window->override_redirect)
> + {
> + if (window->workspace == NULL &&
> + window->xtransient_for != None)
> + {
> + /* Try putting dialog on parent's workspace */
> + MetaWindow *parent;
> +
> + parent = meta_display_lookup_x_window (window->display,
> + window->xtransient_for);
> +
> + if (parent && parent->workspace)
> + {
> + meta_topic (META_DEBUG_PLACEMENT,
> + "Putting window %s on same workspace as parent
> %s\n",
> + window->desc, parent->desc);
> +
> + if (parent->on_all_workspaces_requested)
> + {
> + window->on_all_workspaces_requested = TRUE;
> + window->on_all_workspaces = TRUE;
> + }
> +
> + /* this will implicitly add to the appropriate MRU lists
> + */
> + meta_workspace_add_window (parent->workspace, window);
> + }
> + }
> +
> + if (window->workspace == NULL)
> + {
> + meta_topic (META_DEBUG_PLACEMENT,
> + "Putting window %s on active workspace\n",
> + window->desc);
> +
> + space = window->screen->active_workspace;
> +
> + meta_workspace_add_window (space, window);
> + }
> +
> + /* for the various on_all_workspaces = TRUE possible above */
> + meta_window_set_current_workspace_hint (window);
> +
> + meta_window_update_struts (window);
> + }
> +
> window->constructing = FALSE;
>
> meta_display_notify_window_created (display, window);
> _______________________________________________
> gnome-shell-list mailing list
> gnome-shell-list@gnome.org
> https://mail.gnome.org/mailman/listinfo/gnome-shell-list
_______________________________________________
gnome-shell-list mailing list
gnome-shell-list@gnome.org
https://mail.gnome.org/mailman/listinfo/gnome-shell-list
