Merge branch 'benzea/wait-seat-graphical' into 'master'

local-display-factory: Wait for seats to become graphical

Closes #662

See merge request GNOME/gdm!128

3 files changed, 187 insertions, 81 deletions

diff --git a/common/gdm-common.c b/common/gdm-common.c
index 2e9114f2..bf8364a8 100644
--- a/common/gdm-common.c
+++ b/common/gdm-common.c
@@ -512,24 +512,6 @@ goto_login_session (GDBusConnection  *connection,
                 return FALSE;
         }
 
-        res = sd_seat_can_multi_session (seat_id);
-        if (res < 0) {
-                free (seat_id);
-
-                g_debug ("failed to determine whether seat can do multi session: %s", strerror (-res));
-                g_set_error (error, GDM_COMMON_ERROR, 0, _("The system is unable to determine whether to switch to an existing login screen or start up a new login screen."));
-
-                return FALSE;
-        }
-
-        if (res == 0) {
-                free (seat_id);
-
-                g_set_error (error, GDM_COMMON_ERROR, 0, _("The system is unable to start up a new login screen."));
-
-                return FALSE;
-        }
-
         res = gdm_get_login_window_session_id (seat_id, &session_id);
         if (res && session_id != NULL) {
                 res = gdm_activate_session_by_id (connection, seat_id, session_id);
diff --git a/daemon/gdm-local-display-factory.c b/daemon/gdm-local-display-factory.c
index e7cafeb1..bc55f0c4 100644
--- a/daemon/gdm-local-display-factory.c
+++ b/daemon/gdm-local-display-factory.c
@@ -48,6 +48,7 @@
 
 #define MAX_DISPLAY_FAILURES 5
 #define WAIT_TO_FINISH_TIMEOUT 10 /* seconds */
+#define SEAT0_GRAPHICS_CHECK_TIMEOUT 10 /* seconds */
 
 struct _GdmLocalDisplayFactory
 {
@@ -62,6 +63,10 @@ struct _GdmLocalDisplayFactory
 
         guint            seat_new_id;
         guint            seat_removed_id;
+        guint            seat_properties_changed_id;
+
+        gboolean         seat0_graphics_check_timed_out;
+        guint            seat0_graphics_check_timeout_id;
 
 #if defined(ENABLE_USER_DISPLAY_SERVER)
         unsigned int     active_vt;
@@ -78,10 +83,8 @@ static void     gdm_local_display_factory_class_init    (GdmLocalDisplayFactoryC
 static void     gdm_local_display_factory_init          (GdmLocalDisplayFactory      *factory);
 static void     gdm_local_display_factory_finalize      (GObject                     *object);
 
-static GdmDisplay *create_display                       (GdmLocalDisplayFactory      *factory,
-                                                         const char                  *seat_id,
-                                                         const char                  *session_type,
-                                                         gboolean                    initial_display);
+static void     ensure_display_for_seat                 (GdmLocalDisplayFactory      *factory,
+                                                         const char                  *seat_id);
 
 static void     on_display_status_changed               (GdmDisplay                  *display,
                                                          GParamSpec                  *arg1,
@@ -386,19 +389,11 @@ on_display_status_changed (GdmDisplay             *display,
 
                         factory->num_failures++;
 
-                        if (factory->num_failures > MAX_DISPLAY_FAILURES) {
-                                /* oh shit */
+                        /* oh shit */
+                        if (factory->num_failures > MAX_DISPLAY_FAILURES)
                                 g_warning ("GdmLocalDisplayFactory: maximum number of X display failures reached: check X server log for errors");
-                        } else {
-#ifdef ENABLE_WAYLAND_SUPPORT
-                                if (g_strcmp0 (session_type, "wayland") == 0) {
-                                        g_free (session_type);
-                                        session_type = NULL;
-                                }
-
-#endif
-                                create_display (factory, seat_id, session_type, is_initial);
-                        }
+                        else
+                                ensure_display_for_seat (factory, seat_id);
                 }
                 break;
         case GDM_DISPLAY_UNMANAGED:
@@ -459,21 +454,117 @@ lookup_prepared_display_by_seat_id (const char *id,
         return lookup_by_seat_id (id, display, user_data);
 }
 
-static GdmDisplay *
-create_display (GdmLocalDisplayFactory *factory,
-                const char             *seat_id,
-                const char             *session_type,
-                gboolean                initial)
+static int
+on_seat0_graphics_check_timeout (gpointer user_data)
 {
+        GdmLocalDisplayFactory *factory = user_data;
+
+        factory->seat0_graphics_check_timeout_id = 0;
+
+        /* Simply try to re-add seat0. If it is there already (i.e. CanGraphical
+         * turned TRUE, then we'll find it and it will not be created again).
+         */
+        factory->seat0_graphics_check_timed_out = TRUE;
+        ensure_display_for_seat (factory, "seat0");
+
+        return G_SOURCE_REMOVE;
+}
+
+static void
+ensure_display_for_seat (GdmLocalDisplayFactory *factory,
+                         const char             *seat_id)
+{
+        int ret;
+        gboolean seat_supports_graphics;
+        gboolean is_seat0;
+        const char *session_type = "wayland";
         GdmDisplayStore *store;
         GdmDisplay      *display = NULL;
         g_autofree char *login_session_id = NULL;
 
+        ret = sd_seat_can_graphical (seat_id);
+
+        if (ret < 0) {
+                g_critical ("Failed to query CanGraphical information for seat %s", seat_id);
+                return;
+        }
+
+        if (ret == 0) {
+                g_debug ("GdmLocalDisplayFactory: System doesn't currently support graphics");
+                seat_supports_graphics = FALSE;
+        } else {
+                g_debug ("GdmLocalDisplayFactory: System supports graphics");
+                seat_supports_graphics = TRUE;
+        }
+
+        if (g_strcmp0 (seat_id, "seat0") == 0) {
+                is_seat0 = TRUE;
+
+                /* If we've failed, or are explicitly told to, fall back to legacy X11 support
+                 */
+                if (factory->num_failures > 0 || !gdm_local_display_factory_use_wayland ()) {
+                        session_type = NULL;
+                        g_debug ("GdmLocalDisplayFactory: New displays on seat0 will use X11 fallback");
+                } else {
+                        g_debug ("GdmLocalDisplayFactory: New displays on seat0 will use wayland");
+                }
+        } else {
+                is_seat0 = FALSE;
+
+                g_debug ("GdmLocalDisplayFactory: New displays on seat %s will use X11 fallback", seat_id);
+                /* Force legacy X11 for all auxiliary seats */
+                seat_supports_graphics = TRUE;
+                session_type = NULL;
+        }
+
+        /* For seat0, we have a fallback logic to still try starting it after
+         * SEAT0_GRAPHICS_CHECK_TIMEOUT seconds. i.e. we simply continue even if
+         * CanGraphical is unset.
+         * This is ugly, but it means we'll come up eventually in some
+         * scenarios where no master device is present.
+         * Note that we'll force an X11 fallback even though there might be
+         * cases where an wayland capable device is present and simply not marked as
+         * master-of-seat. In these cases, this should likely be fixed in the
+         * udev rules.
+         *
+         * At the moment, systemd always sets CanGraphical for non-seat0 seats.
+         * This is because non-seat0 seats are defined by having master-of-seat
+         * set. This means we can avoid the fallback check for non-seat0 seats,
+         * which simplifies the code.
+         */
+        if (is_seat0) {
+                if (!seat_supports_graphics) {
+                        if (!factory->seat0_graphics_check_timed_out) {
+                                if (factory->seat0_graphics_check_timeout_id == 0) {
+                                        g_debug ("GdmLocalDisplayFactory: seat0 doesn't yet support graphics.  Waiting %d seconds to try again.", SEAT0_GRAPHICS_CHECK_TIMEOUT);
+                                        factory->seat0_graphics_check_timeout_id = g_timeout_add_seconds (SEAT0_GRAPHICS_CHECK_TIMEOUT,
+                                                                                                          on_seat0_graphics_check_timeout,
+                                                                                                          factory);
+
+                                } else {
+                                        /* It is not yet time to force X11 fallback. */
+                                        g_debug ("GdmLocalDisplayFactory: seat0 display requested when there is no graphics support before graphics check timeout.");
+                                        return;
+                                }
+                        }
+
+                        g_debug ("GdmLocalDisplayFactory: Assuming we can use seat0 for X11 even though system says it doesn't support graphics!");
+                        g_debug ("GdmLocalDisplayFactory: This might indicate an issue where the framebuffer device is not tagged as master-of-seat in udev.");
+                        seat_supports_graphics = TRUE;
+                        session_type = NULL;
+                } else {
+                        g_clear_handle_id (&factory->seat0_graphics_check_timeout_id, g_source_remove);
+                }
+        }
+
+        if (!seat_supports_graphics)
+                return;
+
         g_debug ("GdmLocalDisplayFactory: %s login display for seat %s requested",
                  session_type? : "X11", seat_id);
         store = gdm_display_factory_get_display_store (GDM_DISPLAY_FACTORY (factory));
 
-        if (sd_seat_can_multi_session (seat_id))
+        if (is_seat0)
                 display = gdm_display_store_find (store, lookup_prepared_display_by_seat_id, (gpointer) seat_id);
         else
                 display = gdm_display_store_find (store, lookup_by_seat_id, (gpointer) seat_id);
@@ -481,7 +572,7 @@ create_display (GdmLocalDisplayFactory *factory,
         /* Ensure we don't create the same display more than once */
         if (display != NULL) {
                 g_debug ("GdmLocalDisplayFactory: display already created");
-                return NULL;
+                return;
         }
 
         /* If we already have a login window, switch to it */
@@ -498,14 +589,14 @@ create_display (GdmLocalDisplayFactory *factory,
                         g_debug ("GdmLocalDisplayFactory: session %s found, activating.",
                                  login_session_id);
                         gdm_activate_session_by_id (factory->connection, seat_id, login_session_id);
-                        return NULL;
+                        return;
                 }
         }
 
         g_debug ("GdmLocalDisplayFactory: Adding display on seat %s", seat_id);
 
 #ifdef ENABLE_USER_DISPLAY_SERVER
-        if (g_strcmp0 (seat_id, "seat0") == 0) {
+        if (is_seat0) {
                 display = gdm_local_display_new ();
                 if (session_type != NULL) {
                         g_object_set (G_OBJECT (display), "session-type", session_type, NULL);
@@ -522,7 +613,7 @@ create_display (GdmLocalDisplayFactory *factory,
         }
 
         g_object_set (display, "seat-id", seat_id, NULL);
-        g_object_set (display, "is-initial", initial, NULL);
+        g_object_set (display, "is-initial", is_seat0, NULL);
 
         store_display (factory, display);
 
@@ -533,7 +624,7 @@ create_display (GdmLocalDisplayFactory *factory,
                 gdm_display_unmanage (display);
         }
 
-        return display;
+        return;
 }
 
 static void
@@ -579,18 +670,7 @@ gdm_local_display_factory_sync_seats (GdmLocalDisplayFactory *factory)
         g_variant_iter_init (&iter, array);
 
         while (g_variant_iter_loop (&iter, "(&so)", &seat, NULL)) {
-                gboolean is_initial;
-                const char *session_type = NULL;
-
-                if (g_strcmp0 (seat, "seat0") == 0) {
-                        is_initial = TRUE;
-                        if (gdm_local_display_factory_use_wayland ())
-                                session_type = "wayland";
-                } else {
-                        is_initial = FALSE;
-                }
-
-                create_display (factory, seat, session_type, is_initial);
+                ensure_display_for_seat (factory, seat);
         }
 
         g_variant_unref (result);
@@ -610,7 +690,7 @@ on_seat_new (GDBusConnection *connection,
         const char *seat;
 
         g_variant_get (parameters, "(&s&o)", &seat, NULL);
-        create_display (GDM_LOCAL_DISPLAY_FACTORY (user_data), seat, NULL, FALSE);
+        ensure_display_for_seat (GDM_LOCAL_DISPLAY_FACTORY (user_data), seat);
 }
 
 static void
@@ -628,6 +708,53 @@ on_seat_removed (GDBusConnection *connection,
         delete_display (GDM_LOCAL_DISPLAY_FACTORY (user_data), seat);
 }
 
+static void
+on_seat_properties_changed (GDBusConnection *connection,
+                            const gchar     *sender_name,
+                            const gchar     *object_path,
+                            const gchar     *interface_name,
+                            const gchar     *signal_name,
+                            GVariant        *parameters,
+                            gpointer         user_data)
+{
+        const gchar *seat = NULL;
+        g_autoptr(GVariant) changed_props = NULL;
+        g_autoptr(GVariant) changed_prop = NULL;
+        g_autofree const gchar **invalidated_props = NULL;
+        gboolean changed = FALSE;
+        int ret;
+
+        /* Extract seat id, i.e. the last element of the object path. */
+        seat = strrchr (object_path, '/');
+        if (seat == NULL)
+                return;
+        seat += 1;
+
+        /* Valid seat IDs must start with seat, i.e. ignore "auto" */
+        if (!g_str_has_prefix (seat, "seat"))
+                return;
+
+        g_variant_get (parameters, "(s@a{sv}^a&s)", NULL, &changed_props, &invalidated_props);
+
+        changed_prop = g_variant_lookup_value (changed_props, "CanGraphical", NULL);
+        if (changed_prop)
+                changed = TRUE;
+        if (!changed && g_strv_contains (invalidated_props, "CanGraphical"))
+                changed = TRUE;
+
+        if (!changed)
+                return;
+
+        ret = sd_seat_can_graphical (seat);
+        if (ret < 0)
+                return;
+
+        if (ret != 0)
+                ensure_display_for_seat (GDM_LOCAL_DISPLAY_FACTORY (user_data), seat);
+        else
+                delete_display (GDM_LOCAL_DISPLAY_FACTORY (user_data), seat);
+}
+
 static gboolean
 lookup_by_session_id (const char *id,
                       GdmDisplay *display,
@@ -701,7 +828,6 @@ on_vt_changed (GIOChannel    *source,
         g_autofree char *login_session_id = NULL;
         g_autofree char *active_session_id = NULL;
         unsigned int previous_vt, new_vt, login_window_vt = 0;
-        const char *session_type = NULL;
         int ret, n_returned;
 
         g_debug ("GdmLocalDisplayFactory: received VT change event");
@@ -796,6 +922,8 @@ on_vt_changed (GIOChannel    *source,
         if (factory->active_vt != login_window_vt) {
                 GdmDisplay *display;
 
+                g_clear_handle_id (&factory->seat0_graphics_check_timeout_id, g_source_remove);
+
                 display = gdm_display_store_find (store,
                                                   lookup_by_tty,
                                                   (gpointer) tty_of_active_vt);
@@ -830,12 +958,9 @@ on_vt_changed (GIOChannel    *source,
                 return G_SOURCE_CONTINUE;
         }
 
-        if (gdm_local_display_factory_use_wayland ())
-                session_type = "wayland";
-
         g_debug ("GdmLocalDisplayFactory: creating new display on seat0 because of VT change");
 
-        create_display (factory, "seat0", session_type, TRUE);
+        ensure_display_for_seat (factory, "seat0");
 
         return G_SOURCE_CONTINUE;
 }
@@ -866,6 +991,16 @@ gdm_local_display_factory_start_monitor (GdmLocalDisplayFactory *factory)
                                                                              on_seat_removed,
                                                                              g_object_ref (factory),
                                                                              g_object_unref);
+        factory->seat_properties_changed_id = g_dbus_connection_signal_subscribe (factory->connection,
+                                                                                  "org.freedesktop.login1",
+                                                                                  "org.freedesktop.DBus.Properties",
+                                                                                  "PropertiesChanged",
+                                                                                  NULL,
+                                                                                  "org.freedesktop.login1.Seat",
+                                                                                  G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE,
+                                                                                  on_seat_properties_changed,
+                                                                                  g_object_ref (factory),
+                                                                                  g_object_unref);
 
 #if defined(ENABLE_USER_DISPLAY_SERVER)
         io_channel = g_io_channel_new_file ("/sys/class/tty/tty0/active", "r", NULL);
@@ -894,6 +1029,11 @@ gdm_local_display_factory_stop_monitor (GdmLocalDisplayFactory *factory)
                                                       factory->seat_removed_id);
                 factory->seat_removed_id = 0;
         }
+        if (factory->seat_properties_changed_id) {
+                g_dbus_connection_signal_unsubscribe (factory->connection,
+                                                      factory->seat_properties_changed_id);
+                factory->seat_properties_changed_id = 0;
+        }
 #if defined(ENABLE_USER_DISPLAY_SERVER)
         if (factory->active_vt_watch_id) {
                 g_source_remove (factory->active_vt_watch_id);
@@ -979,6 +1119,8 @@ gdm_local_display_factory_stop (GdmDisplayFactory *base_factory)
                                               G_CALLBACK (on_display_removed),
                                               factory);
 
+        g_clear_handle_id (&factory->seat0_graphics_check_timeout_id, g_source_remove);
+
         return TRUE;
 }
 
diff --git a/libgdm/gdm-user-switching.c b/libgdm/gdm-user-switching.c
index 20235fd8..b39d21db 100644
--- a/libgdm/gdm-user-switching.c
+++ b/libgdm/gdm-user-switching.c
@@ -218,24 +218,6 @@ goto_login_session (GDBusConnection  *connection,
                 return FALSE;
         }
 
-        res = sd_seat_can_multi_session (seat_id);
-        if (res < 0) {
-                free (seat_id);
-
-                g_debug ("failed to determine whether seat can do multi session: %s", strerror (-res));
-                g_set_error (error, GDM_CLIENT_ERROR, 0, _("The system is unable to determine whether to switch to an existing login screen or start up a new login screen."));
-
-                return FALSE;
-        }
-
-        if (res == 0) {
-                free (seat_id);
-
-                g_set_error (error, GDM_CLIENT_ERROR, 0, _("The system is unable to start up a new login screen."));
-
-                return FALSE;
-        }
-
         res = get_login_window_session_id (seat_id, &session_id);
         if (res && session_id != NULL) {
                 res = activate_session_id (connection, cancellable, seat_id, session_id, error);