GNOME Bugzilla – Bug 771442
gdm should fallback to Xorg on laptops with 2 gpu-s
Last modified: 2016-10-29 02:30:36 UTC
Hi, Since wayland does not yet support prime and esp. not slave outputs, gdm really should fallback to Xorg if there are 2 /dev/dri/card# devices. Otherwise external connectors on laptops which are only connected to the discrete gpu will not be usable (without manually switching to a gnome on xorg session). I guess this has been a problem wrt the login screen not showing on such external displays for a while now, but although for the login screen this typically is not a big problem, this is a problem with the actual gnome-session, think e.g. not being able to use a projector on conferences because of this. Regards, Hans
So I think this workaround should be done in mutter, since mutter is the place that will ultimately need to be fixed.
Created attachment 337980 [details] [review] native: only match drm subsystem devices Despite g_udev_client_new taking a list of subsystems, it doesn't implicitly filter results to those subsystems. This commit explicitly adds a subsystem match to make sure sound cards don't end up in the resulting list of video cards.
Created attachment 337981 [details] [review] native: fail on systems with connectors spread across multiple gpus We don't support using GPUs as slave devices yet, so we should fail if we encounter that situation, so GDM will fall back to X.
Review of attachment 337981 [details] [review]: ::: src/backends/native/meta-launcher.c @@ +299,3 @@ + + /* filter out the real card devices, we only care about the connectors */ + if (g_udev_device_get_device_type (dev) != G_UDEV_DEVICE_TYPE_NONE) Are we sure only connectors are TYPE_NONE ? @@ +310,3 @@ + continue; + + parent = g_udev_device_get_parent (dev); Should we check that parent is a drm char device? @@ +313,3 @@ + g_hash_table_insert (cards, (gpointer) g_udev_device_get_name (parent), parent); + } + num_devices = g_hash_table_size (cards); Doesn't matter much here but this returns an unsigned int. Maybe this whole function should as well. @@ +348,3 @@ + num_devices = count_devices_with_connectors (seat_name, devices); + if (num_devices != 1) + goto out; This leaks the devices list.
Review of attachment 337980 [details] [review]: ::: src/backends/native/meta-launcher.c @@ +295,2 @@ g_udev_enumerator_add_match_name (enumerator, "card*"); + g_udev_enumerator_add_match_subsystem (enumerator, "drm"); GUdevEnumerator should do this internally IMO, it's what I'd expect from the API at least. Anyway, it never hurts to be explicit either.
FWIW, it seems to support Prime on my machine (T430s with Intel and NVidia cards). We need to make sure that Wayland is still usable on those machines, when the discrete GPU is only used for render offloading.
> GUdevEnumerator should do this internally IMO, it's what I'd expect from the > API at least. Yea, I should probably file a bug about that, and link to the bug in a comment above the add_match_subsystem call.
(In reply to Ray Strode [halfline] from comment #7) > > GUdevEnumerator should do this internally IMO, it's what I'd expect from the > > API at least. > Yea, I should probably file a bug about that, and link to the bug in a > comment above the add_match_subsystem call. Patches welcome, says the defacto maintainer (ie. me).
(In reply to Bastien Nocera from comment #6) > FWIW, it seems to support Prime on my machine (T430s with Intel and NVidia > cards). We need to make sure that Wayland is still usable on those machines, > when the discrete GPU is only used for render offloading. Well, "support" on some level. We currently completely ignore the card that's not used at boot up, which is sufficient for now, as long as there aren't connectors hardwired to it. Keeping that case from regressing is the main reason the patch isn't just seeing if there is more than one card (the other complication is multiseat)
(In reply to Ray Strode [halfline] from comment #9) > (In reply to Bastien Nocera from comment #6) > > FWIW, it seems to support Prime on my machine (T430s with Intel and NVidia > > cards). We need to make sure that Wayland is still usable on those machines, > > when the discrete GPU is only used for render offloading. > > Well, "support" on some level. We currently completely ignore the card > that's not used at boot up, which is sufficient for now, as long as there > aren't connectors hardwired to it. Keeping that case from regressing is the > main reason the patch isn't just seeing if there is more than one card (the > other complication is multiseat) I can make apps render on the 2nd GPU, displayed on the first. That's how I implemented (and tested) https://bugzilla.gnome.org/show_bug.cgi?id=773213 which worked on both X.org and Wayland.
(In reply to Bastien Nocera from comment #8) > Patches welcome, says the defacto maintainer (ie. me). Okay I therw a quick patch in bug 773224. Don't push it until I try it and report that it works !
(In reply to Bastien Nocera from comment #10) > I can make apps render on the 2nd GPU, displayed on the first. That's how I > implemented (and tested) https://bugzilla.gnome.org/show_bug.cgi?id=773213 > which worked on both X.org and Wayland. Oh yea, that's true. That's all handled magically under the hood by mesa.
(In reply to Rui Matos from comment #4) > Review of attachment 337981 [details] [review] [review]: > > ::: src/backends/native/meta-launcher.c > @@ +299,3 @@ > + > + /* filter out the real card devices, we only care about the > connectors */ > + if (g_udev_device_get_device_type (dev) != G_UDEV_DEVICE_TYPE_NONE) > > Are we sure only connectors are TYPE_NONE ? Well, the device list is only devices that start with card* glob. At the moment, that's only connectors and the char devices. I don't think that's likely to change, but if you like I can explicitly add a g_udev_device_has_sysfs_attr (dev, "modes"); call, too. > > @@ +310,3 @@ > + continue; > + > + parent = g_udev_device_get_parent (dev); > > Should we check that parent is a drm char device? Wouldn't hurt. i'll do it.
Created attachment 338035 [details] [review] native: only match drm subsystem devices Despite g_udev_client_new taking a list of subsystems, it doesn't implicitly filter results to those subsystems. This commit explicitly adds a subsystem match to make sure sound cards don't end up in the resulting list of video cards.
Created attachment 338036 [details] [review] native: shore up matching of card device Right now we accept any character device that matches the glob card*. That's fine, but we can be a little more specific by checking that the devtype is what we expect. This commit does that.
Created attachment 338037 [details] [review] native: fail on systems with connectors spread across multiple gpus We don't support using GPUs as slave devices yet, so we should fail if we encounter that situation, so GDM will fall back to X.
Review of attachment 338035 [details] [review]: ok
Review of attachment 338036 [details] [review]: looks good
Review of attachment 338037 [details] [review]: lgtm
Attachment 338035 [details] pushed as ef20000 - native: only match drm subsystem devices Attachment 338036 [details] pushed as f1e1a5f - native: shore up matching of card device Attachment 338037 [details] pushed as e2bfaf0 - native: fail on systems with connectors spread across multiple gpus
Created attachment 338045 [details] [review] native: don't call steal_pointer prematurely commit e2bfaf07514ed633f8721b5f521577685b6cccc0 does this: g_hash_table_insert (cards, g_udev_device_get_name (parent_device), g_steal_pointer (&parent_device)); The problem is the g_steal_pointer call may happen before the g_udev_device_get_name call leading to a crash. This commit does the get_name call on an earlier line
Comment on attachment 338045 [details] [review] native: don't call steal_pointer prematurely Attachment 338045 [details] pushed as d491063 - native: don't call steal_pointer prematurely
FWIW, this now makes me regress highly (on my machine) since on Fedora 25 that has these patches, I can't disable this as I get hit by this bug https://bugzilla.redhat.com/show_bug.cgi?id=1356115 Sadness of side effects. (this is just a FYI, nothing else, I'd love to have the whole discrete GPU switcheroo works)
Theres a variable you can put in etc environment to get old behavior. Check patch. (i would but im one handed on a phone right now)
I found MUTTER_ALLOW_HYBRID_GPUS. I'll try that. Thanks for the pointer!
Workaround works ! Thanks ! (blacklisting nouveau didn't, but that's another story)