Merge lp:~albertomilone/gdm/lp1697882 into lp:~ubuntu-desktop/gdm/ubuntu

[Alberto Milone]

This change, which is (Ubuntu specific), will hide away the X11 sessions when KMS is enabled in the nvidia driver, leaving the Wayland session as the only option.

Disabling KMS will make the sessions available again after a reboot.

Fixes LP: #1697882

[Iain Lane (laney)]

Thanks. Please could you file upstream and then add that URL into the patch as "Forwarded: <url>"?

Is there a better way than looking at a configuration file such as looking at the loaded modules somehow? Isn't that distro specific or at least fragile?

review: Needs Information

[Alberto Milone (albertomilone)]

> Thanks. Please could you file upstream and then add that URL into the patch as
> "Forwarded: <url>"?
>

This is something that will have to be fixed in the kernel, when the Unix Device Memory Allocator API lands in the kernel. It will take a while. I don't think it's a GNOME specific problem, but we might want to work around it for now.

> Is there a better way than looking at a configuration file such as looking at
> the loaded modules somehow? Isn't that distro specific or at least fragile?

Sure, /sys/module/nvidia_drm/parameters/modeset would be the best place to check the availability of KMS. Unfortunately, that would require root privileges, which (correct me if I'm wrong) we don't have.

[Iain Lane (laney)]

On Wed, Jun 28, 2017 at 04:35:35PM -0000, Alberto Milone wrote:
> > Thanks. Please could you file upstream and then add that URL into the patch as
> > "Forwarded: <url>"?
> >
>
> This is something that will have to be fixed in the kernel, when the Unix Device Memory Allocator API lands in the kernel. It will take a while. I don't think it's a GNOME specific problem, but we might want to work around it for now.

Right, but it is correct that this workaround (disabling X on nvidia +
modesetting) is not Ubuntu specific, isn't it?

> > Is there a better way than looking at a configuration file such as looking at
> > the loaded modules somehow? Isn't that distro specific or at least fragile?
>
> Sure, /sys/module/nvidia_drm/parameters/modeset would be the best place to check the availability of KMS. Unfortunately, that would require root privileges, which (correct me if I'm wrong) we don't have.

gdm3 runs as root. I'm not sure if it drops permissions before getting
to this point - I think probably not, and this is worth looking into.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

[Alberto Milone (albertomilone)]

> On Wed, Jun 28, 2017 at 04:35:35PM -0000, Alberto Milone wrote:
> > > Thanks. Please could you file upstream and then add that URL into the
> patch as
> > > "Forwarded: <url>"?
> > >
> >
> > This is something that will have to be fixed in the kernel, when the Unix
> Device Memory Allocator API lands in the kernel. It will take a while. I don't
> think it's a GNOME specific problem, but we might want to work around it for
> now.
>
> Right, but it is correct that this workaround (disabling X on nvidia +
> modesetting) is not Ubuntu specific, isn't it?
>

Correct. It is how we check KMS that, with the current approach, is Ubuntu specific (because of the root privileges thing).

> gdm3 runs as root. I'm not sure if it drops permissions before getting
> to this point - I think probably not, and this is worth looking into.
>

I'm pretty sure I ran into this problem when I started working on this. I can double check, as the effort should be minimal.

[Alberto Milone (albertomilone)]

Unfortunately, I have just checked, and we no longer have root privileges by the time can_create_environment() is called.

[Iain Lane (laney)]

On Wed, Jun 28, 2017 at 06:41:32PM -0000, Alberto Milone wrote:
> Unfortunately, I have just checked, and we no longer have root privileges by the time can_create_environment() is called.

Ok, that's annoying - I don't suppose it's possible to grab this value
before privileges are dropped and pass it around?

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

[Alberto Milone (albertomilone)]

> On Wed, Jun 28, 2017 at 06:41:32PM -0000, Alberto Milone wrote:
> > Unfortunately, I have just checked, and we no longer have root privileges by
> the time can_create_environment() is called.
>
> Ok, that's annoying - I don't suppose it's possible to grab this value
> before privileges are dropped and pass it around?
>

I'm not sure about that. It's probably easier to use the current udev rule, that the nvidia packages ship, to check the value when the module is loaded, and to write it somewhere in /run, so that GDM can read it later.

What do you think?

[Iain Lane (laney)]

On Thu, Jun 29, 2017 at 10:29:58AM -0000, Alberto Milone wrote:
> > On Wed, Jun 28, 2017 at 06:41:32PM -0000, Alberto Milone wrote:
> > > Unfortunately, I have just checked, and we no longer have root privileges by
> > the time can_create_environment() is called.
> >
> > Ok, that's annoying - I don't suppose it's possible to grab this value
> > before privileges are dropped and pass it around?
> >
>
> I'm not sure about that. It's probably easier to use the current udev rule, that the nvidia packages ship, to check the value when the module is loaded, and to write it somewhere in /run, so that GDM can read it later.
>
> What do you think?

I like that more than reading the configuration file - but it doesn't
sounds like something that could go in gdm upstream? Unless maybe you
can get some buy in for standardising on this. Maybe file a bug on
bugzilla and ask for opinions.

If it's too hard to grab the value from /sys, I guess we can do it this
way.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

[Alberto Milone (albertomilone)]

> I like that more than reading the configuration file - but it doesn't
> sounds like something that could go in gdm upstream? Unless maybe you
> can get some buy in for standardising on this. Maybe file a bug on
> bugzilla and ask for opinions.
>

I don't think it should be upstreamed, and I hope we can drop this in the near future. I think we can live with a small patch for now (which I am going to maintain).

> If it's too hard to grab the value from /sys, I guess we can do it this
> way.

Ok, I'll work on it.

[Iain Lane (laney)]

On Thu, Jun 29, 2017 at 11:59:23AM -0000, Alberto Milone wrote:
> > I like that more than reading the configuration file - but it doesn't
> > sounds like something that could go in gdm upstream? Unless maybe you
> > can get some buy in for standardising on this. Maybe file a bug on
> > bugzilla and ask for opinions.
> >
>
> I don't think it should be upstreamed, and I hope we can drop this in the near future. I think we can live with a small patch for now (which I am going to maintain).

Why not? Does this somehow work on other distributions? If it doesn't,
why would it be desirable to keep showing broken sessions there?

> > If it's too hard to grab the value from /sys, I guess we can do it this
> > way.
>
> Ok, I'll work on it.

k.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

[Alberto Milone (albertomilone)]

I have just filed an upstream bug report about this:
https://bugzilla.gnome.org/show_bug.cgi?id=784470

[lp:~albertomilone/gdm/lp1697882]

411. By Ken VanDine on 2017-07-03

Only prompt the user to choose display managers when we haven't already determined a migration to gdm3 is recommended.

412. By Ken VanDine on 2017-07-03

Merged

413. By Ken VanDine on 2017-07-03

* debian/gdm3.config
  - Only prompt the user to choose display managers when we haven't already
    determined a migration to gdm3 is recommended.
* debian/control.in
  - Explicitly depend on ubuntu-session so we can ensure gdm3 configure gets
    run after ubuntu-session's configure to handle the transition from
    lightdm to gdm3

414. By Ken VanDine on 2017-07-10

released

415. By Ken VanDine on 2017-07-11

* debian/gdm3.config
  - Handle the transition from lightdm in gdm3 rather than ubuntu-session
* debian/control.in
  - Dropped depends from ubuntu-session

416. By Ken VanDine on 2017-07-11

Fixed ubuntu-session version check

417. By Ken VanDine on 2017-07-11

Dropped depends for ubuntu-session

418. By Ken VanDine on 2017-07-11

releasing package gdm3 version 3.24.2-1ubuntu5

419. By Didier Roche on 2017-07-12

debian/patches/ubuntu_prefer_x11_session.patch:
Prefer X11 session over wayland. As we are not ready yet for
switching to wayland yet, let's prefer X11 session when no specific
choice was made. (LP: #1703601)
This will be reverted once we switch to wayland by default.

420. By Didier Roche on 2017-07-12

releasing package gdm3 version 3.24.2-1ubuntu6

421. By Iain Lane on 2017-07-12

debian/gdm3.config: Check the version of gdm3 instead of ubuntu-session.

And some other minor improvements.

422. By Iain Lane on 2017-07-12

Compare with a ~ version, so that any backports of this exact version aren't re-migrated

423. By Iain Lane on 2017-07-12

releasing package gdm3 version 3.24.2-1ubuntu7

424. By Daniel van Vugt on 2017-07-13

First attempt

425. By Daniel van Vugt on 2017-07-13

Remember the changelog

[Daniel van Vugt (vanvugt)]

Pretty sure this will have conflicts now, sorry.

review: Needs Fixing

[lp:~albertomilone/gdm/lp1697882]

426. By Will Cooke on 2017-07-13

* debian/default.pa
  - Added new file to disable Bluetooth audio devices in PulseAudio from
    gdm3. (LP: #1703415) (LP: #1489651)
* debian/gdm3.install
  - Added details of the default.pa file
* debian/gdm3.postinst
  - Added installation of default.pa and creation of dir if it doesn't
    exist.

427. By Jeremy Bicha on 2017-07-13

releasing package gdm3 version 3.24.2-1ubuntu9

428. By Jeremy Bicha on 2017-08-14

New upstream release 3.25.90.1

* debian/control.in:
  - Bump minimum gnome-settings-daemon to 3.24
* Drop patches applied in new release:
  - Avoid-double-NULL-terminated-array-LP-1704050.patch
  - revert_drop_gdm_shell_session.patch

429. By Jeremy Bicha on 2017-08-14

releasing package gdm3 version 3.25.90.1-0ubuntu1

430. By Didier Roche on 2017-08-21

* debian/patches/ubuntu_prefer_x11_session.patch:
  - remove x11 session preference now that both Ubuntu and GNOME
    switched to wayland

431. By Didier Roche on 2017-08-21

releasing package gdm3 version 3.25.90.1-0ubuntu2

[Alberto Milone (albertomilone)]

I have come up with a better solution, which I attached to the upstream bug report:
https://bugzilla.gnome.org/show_bug.cgi?id=784470

[lp:~albertomilone/gdm/lp1697882]

432. By Alberto Milone on 2017-08-24

Hide X11 sessions when KMS is enabled in the nvidia driver

NOTE: this doesn't affect systems with hybrid graphics

Fixes LP: #1697882

[Alberto Milone (albertomilone)]

The latest patch is based on what upstream recommended. I have already pushed a change to the udev rule included in the NVIDIA driver in Artful, so that the Gnome shell (which doesn't run with root privileges) can check KMS when getting the sessions using libgdm.

My patch works well here.

[Iain Lane (laney)]

I left some comments upstream - please consider those for Ubuntu before the 17.10 release - but I think we can probably upload this now.

Thanks!

review: Approve

=== added file 'debian/patches/95_hide_x11_sessions_with_nvidia_kms.patch'
--- debian/patches/95_hide_x11_sessions_with_nvidia_kms.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/95_hide_x11_sessions_with_nvidia_kms.patch	2017-08-24 09:08:19 +0000
@@ -0,0 +1,255 @@
+From 06a4117ae5761c184c2c9fe54e4f297becf1d2a9 Mon Sep 17 00:00:00 2001
+From: Alberto Milone <[e-mail]>
+Date: Tue, 20 Jun 2017 16:49:10 +0200
+Subject: [PATCH 1/1] Hide X11 sessions when KMS is enabled in the nvidia
+ driver
+
+Fixes LP: #1697882
+---
+ common/gdm-common.c                | 78 ++++++++++++++++++++++++++++++++++++++
+ common/gdm-common.h                |  2 +
+ daemon/gdm-display.c               | 10 +++++
+ daemon/gdm-local-display-factory.c |  5 +++
+ daemon/gdm-session.c               |  6 ++-
+ libgdm/Makefile.am                 |  3 ++
+ libgdm/gdm-sessions.c              |  8 +++-
+ 7 files changed, 109 insertions(+), 3 deletions(-)
+
+diff --git a/common/gdm-common.c b/common/gdm-common.c
+index 31fc810..0dcbbe2 100644
+--- a/common/gdm-common.c
++++ b/common/gdm-common.c
+@@ -47,6 +47,8 @@
+ 
+ G_DEFINE_QUARK (gdm-common-error, gdm_common_error);
+ 
++int nvidia_has_kms = -1;
++
+ const char *
+ gdm_make_temp_dir (char *template)
+ {
+@@ -797,3 +799,79 @@ gdm_shell_expand (const char *str,
+         }
+         return g_string_free (s, FALSE);
+ }
++
++static gboolean
++is_module_loaded (const char *module) {
++        char              line[4096];
++        gboolean          status = FALSE;
++        FILE              *file = NULL;
++        const char        *modules_file = "/proc/modules";
++
++        file = fopen (modules_file, "r");
++        if (!file) {
++                g_debug ("Failed to parse %s\n", modules_file);
++                return status;
++        }
++
++        while (fgets (line, sizeof (line), file)) {
++                char *tok;
++                tok = strtok (line, " \t");
++                if (strstr (tok, module) != NULL) {
++                        status = TRUE;
++                        break;
++                }
++        }
++        fclose(file);
++
++        return status;
++}
++
++gboolean
++is_nvidia_kms_available (void)
++{
++        gsize             length;
++        GError            *error = NULL;
++        gboolean          status = FALSE;
++        gchar             *contents = NULL;
++        const char        *kms_file = "/sys/module/nvidia_drm/parameters/modeset";
++
++        g_debug ("Checking if the NVIDIA driver is loaded, and has KMS enabled\n");
++
++        /* Use the cached value */
++        if (nvidia_has_kms > -1) {
++                g_debug ("is_nvidia_kms_available: using cached value %d\n", nvidia_has_kms);
++                return (nvidia_has_kms == 1);
++        }
++
++        /* Let's leave hybrid graphics alone */
++        if (is_module_loaded ("i915")) {
++                g_debug ("is_nvidia_kms_available: Intel detected, doing nothing.\n");
++                goto end;
++        }
++
++        if (!g_file_test (kms_file, G_FILE_TEST_EXISTS)) {
++                g_debug ("is_nvidia_kms_available: Cannot access '%s'", kms_file);
++                goto end;
++        }
++
++        if (!g_file_get_contents (kms_file, &contents, &length, &error)) {
++                g_debug ("is_nvidia_kms_available: Failed to parse '%s': %s",
++                         kms_file,
++                         (error && error->message) ? error->message : "(null)");
++                g_error_free (error);
++                goto end;
++        }
++
++        g_debug ("is_nvidia_kms_available: contents, \n%s\n", contents);
++
++        status = (strstr (contents, "Y\n") != NULL);
++        g_free (contents);
++
++end:
++        /* Cache the value */
++        nvidia_has_kms = status ? 1 : 0;
++
++        g_debug ("is_nvidia_kms_available: setting status to %d\n", status);
++
++        return status;
++}
+diff --git a/common/gdm-common.h b/common/gdm-common.h
+index 8d83a12..6f88f2f 100644
+--- a/common/gdm-common.h
++++ b/common/gdm-common.h
+@@ -71,6 +71,8 @@ char *        gdm_shell_expand            (const char *str,
+                                            GdmExpandVarFunc expand_func,
+                                            gpointer user_data);
+ 
++gboolean      is_nvidia_kms_available     (void);
++
+ G_END_DECLS
+ 
+ #endif /* _GDM_COMMON_H */
+diff --git a/daemon/gdm-display.c b/daemon/gdm-display.c
+index 0057e2c..42648f9 100644
+--- a/daemon/gdm-display.c
++++ b/daemon/gdm-display.c
+@@ -54,6 +54,8 @@
+ 
+ #define GDM_DISPLAY_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), GDM_TYPE_DISPLAY, GdmDisplayPrivate))
+ 
++extern int nvidia_has_kms;
++
+ struct GdmDisplayPrivate
+ {
+         char                 *id;
+@@ -118,6 +120,7 @@ enum {
+         PROP_DOING_INITIAL_SETUP,
+ };
+ 
++
+ static void     gdm_display_class_init  (GdmDisplayClass *klass);
+ static void     gdm_display_init        (GdmDisplay      *self);
+ static void     gdm_display_finalize    (GObject         *object);
+@@ -1447,6 +1450,13 @@ can_create_environment (const char *session_id)
+         char *path;
+         gboolean session_exists;
+ 
++#ifdef ENABLE_WAYLAND_SUPPORT
++        /* Disable X11 sessions if KMS is enabled in the nvidia driver
++         * See LP: #1697882.
++         */
++        if (is_nvidia_kms_available ())
++                return FALSE;
++#endif
+         path = g_strdup_printf (GNOME_SESSION_SESSIONS_PATH "/%s.session", session_id);
+         session_exists = g_file_test (path, G_FILE_TEST_EXISTS);
+ 
+diff --git a/daemon/gdm-local-display-factory.c b/daemon/gdm-local-display-factory.c
+index 7a4643d..50ddaea 100644
+--- a/daemon/gdm-local-display-factory.c
++++ b/daemon/gdm-local-display-factory.c
+@@ -48,6 +48,8 @@
+ 
+ #define MAX_DISPLAY_FAILURES 5
+ 
++extern int nvidia_has_kms;
++
+ struct GdmLocalDisplayFactoryPrivate
+ {
+         GdmDBusLocalDisplayFactory *skeleton;
+@@ -459,6 +461,9 @@ gdm_local_display_factory_sync_seats (GdmLocalDisplayFactory *factory)
+                         is_initial = FALSE;
+                 }
+ 
++                if (is_nvidia_kms_available ())
++                        session_type = "wayland";
++
+                 create_display (factory, seat, session_type, is_initial);
+         }
+ 
+diff --git a/daemon/gdm-session.c b/daemon/gdm-session.c
+index 0b83223..5e597c4 100644
+--- a/daemon/gdm-session.c
++++ b/daemon/gdm-session.c
+@@ -62,6 +62,8 @@
+ 
+ #define GDM_WORKER_DBUS_PATH "/org/gnome/DisplayManager/Worker"
+ 
++extern int nvidia_has_kms;
++
+ typedef struct
+ {
+         GdmSession            *session;
+@@ -354,7 +356,9 @@ get_system_session_dirs (GdmSession *self)
+ 
+         search_array = g_array_new (TRUE, TRUE, sizeof (char *));
+ 
+-        g_array_append_vals (search_array, x_search_dirs, G_N_ELEMENTS (x_search_dirs));
++        if (!is_nvidia_kms_available ()) {
++                g_array_append_vals (search_array, x_search_dirs, G_N_ELEMENTS (x_search_dirs));
++        }
+ 
+ #ifdef ENABLE_WAYLAND_SUPPORT
+         if (!self->priv->ignore_wayland) {
+diff --git a/libgdm/Makefile.am b/libgdm/Makefile.am
+index 99ada9a..66af6b5 100644
+--- a/libgdm/Makefile.am
++++ b/libgdm/Makefile.am
+@@ -36,6 +36,8 @@ AM_CPPFLAGS = \
+         -I$(builddir)                                                   \
+         -I$(top_srcdir)                                                 \
+         -I$(top_builddir)                                               \
++        -I$(top_srcdir)/common                                          \
++        -I$(top_builddir)/common                                        \
+         -DG_LOG_DOMAIN=\"Gdm\"                                          \
+         -DDMCONFDIR=\""$(dmconfdir)"\"                                        \
+         -DDATADIR=\""$(datadir)"\"                                            \
+@@ -64,6 +66,7 @@ libgdm_la_LDFLAGS =                                                    \
+         $(END_OF_LIST)
+ 
+ libgdm_la_LIBADD =                                                     \
++        $(top_builddir)/common/libgdmcommon.la                         \
+         $(LIBGDM_LIBS)                                                 \
+         $(SYSTEMD_LIBS)                                                \
+         $(END_OF_LIST)
+diff --git a/libgdm/gdm-sessions.c b/libgdm/gdm-sessions.c
+index a645224..419f055 100644
+--- a/libgdm/gdm-sessions.c
++++ b/libgdm/gdm-sessions.c
+@@ -37,6 +37,8 @@
+ 
+ #include "gdm-sessions.h"
+ 
++extern int nvidia_has_kms;
++
+ typedef struct _GdmSessionFile {
+         char    *id;
+         char    *path;
+@@ -208,8 +210,10 @@ collect_sessions (void)
+                                                                     g_free, g_free);
+         }
+ 
+-        for (i = 0; xorg_search_dirs [i] != NULL; i++) {
+-                collect_sessions_from_directory (xorg_search_dirs [i]);
++        if (!is_nvidia_kms_available ()) {
++                for (i = 0; xorg_search_dirs [i] != NULL; i++) {
++                        collect_sessions_from_directory (xorg_search_dirs [i]);
++                }
+         }
+ 
+ #ifdef ENABLE_WAYLAND_SUPPORT
+-- 
+2.7.4
+

=== modified file 'debian/patches/series'
--- debian/patches/series	2017-08-21 08:48:14 +0000
+++ debian/patches/series	2017-08-24 09:08:19 +0000
@@ -5,6 +5,7 @@
 92_systemd_unit.patch
 93_translate-default-desktop.patch
 94_retain_xorg_log.patch
+95_hide_x11_sessions_with_nvidia_kms.patch
 ubuntu_run_xsession.d.patch
 ubuntu_xresources_is_a_dir.patch
 ubuntu_nvidia_prime.patch