mutter: reduce server grabs for window creation

[Daniel Drake <drake endlessm com>]

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

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

This is undoubtedly a Mali bug, but I'd like to use the opportunity to
explore doing less server grabbing. 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)
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?

As a point of discussion, here's a patch to work around the issue.

Further input much appreciated!

---

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);