GNOME Bugzilla – Bug 779837
Make EDID reading less fragile
Last modified: 2017-03-11 01:19:15 UTC
These commits will make reading the EDID work a bit more reliably when libmutter is both working as the login manager (gdm) and session compositor (regular gnome-shell). Before the various gnome-shell's would all on each hot plug event query the kernel for an up to date drm state and update monitor layouts etc even when not being the DRM master (i.e. when not active). It got even worse when a third mutter (debug mutter running on a separate VT or multiple logged in users). After these, only the currently active libmutter instance will query the drm state on hot plug events, but also when being resumed. This is arguably a kernel bug, but there is no reason why mutter should read the drm state while paused anyway.
Created attachment 347602 [details] [review] backends/native: Move pause/resume handling to backend Move the handling of pause/resume events from the launcher to the backend.
Created attachment 347603 [details] [review] monitor-manager-kms: Put uevent signal management in helpers Bonus: also disconnect handler on cleanup.
Created attachment 347604 [details] [review] monitor-manager-kms: Improve EDID error reporting Include the connector name in the error message, and only include the reason in the GError message.
Created attachment 347605 [details] [review] monitor-manager-kms: Don't listen on hotplugs when paused When mutter is paused (i.e. not the DRM master), stop listening on hotplug events. Instead read the current state and set modes when resumed. This avoids a race condition in the drm API which currently only manages to properly deal with one application querying the EDID state at the same time when there are multiple mutter instances running at the same time (e.g. gnome-shell driving gdm at the same time as gnome-shell as the session instance).
Created attachment 347606 [details] [review] monitor-manager-kms: Don't try to wait for EDID on hot plug The mitigation to avoid missing EDID blob was incorrect; the reason it sometimes failed to read was a race between different applications all trying to read the EDID at the same time. E.g. gnome-shell as GDM would at the same time as the session gnome-shell try to read the EDID of the same connector at the same time, triggering a race in the kernel, making the blob reading ioctl occationally fail with ENOENT. Remove this mitigation, as it didn't really mitigate anything; the race could just as well happen when doing the actual read later.
Review of attachment 347604 [details] [review]: ++
Review of attachment 347603 [details] [review]: The bonus wasn't required before because the gobject instance was being finalized anyway
Review of attachment 347602 [details] [review]: sure
Review of attachment 347606 [details] [review]: yay less code
Review of attachment 347605 [details] [review]: Aha, so it was a kernel bug all along... it has always bugged me, thanks for getting to the bottom of it ::: src/backends/native/meta-monitor-manager-kms.c @@ +1797,3 @@ + meta_monitor_manager_kms_connect_uevent_handler (manager_kms); + meta_monitor_manager_read_current_state (manager); + meta_monitor_manager_on_hotplug (manager); we already have this little handle_hotplug_event() function that does this, would be nice to call it here instead
(In reply to Rui Matos from comment #7) > Review of attachment 347603 [details] [review] [review]: > > The bonus wasn't required before because the gobject instance was being > finalized anyway Good point. I'll remove the bogus bonus. (In reply to Rui Matos from comment #10) > Review of attachment 347605 [details] [review] [review]: > > Aha, so it was a kernel bug all along... it has always bugged me, thanks for > getting to the bottom of it Indeed. There are still more issues on my F25 kernel with missing EDID (and modes); but it seems to be a somewhat known issue upstream, and possibly to be fixed in the 4.12 kernel (I have yet to test the current rc). In the case of those bugs, there is nothing we can do from mutter. The only work around is to reconnect the monitor. > > ::: src/backends/native/meta-monitor-manager-kms.c > @@ +1797,3 @@ > + meta_monitor_manager_kms_connect_uevent_handler (manager_kms); > + meta_monitor_manager_read_current_state (manager); > + meta_monitor_manager_on_hotplug (manager); > > we already have this little handle_hotplug_event() function that does this, > would be nice to call it here instead Fixed.
Attachment 347602 [details] pushed as cf6b7bc - backends/native: Move pause/resume handling to backend Attachment 347603 [details] pushed as 73b2b30 - monitor-manager-kms: Put uevent signal management in helpers Attachment 347604 [details] pushed as 5aa02c0 - monitor-manager-kms: Improve EDID error reporting Attachment 347605 [details] pushed as db14e60 - monitor-manager-kms: Don't listen on hotplugs when paused Attachment 347606 [details] pushed as eb3ff3f - monitor-manager-kms: Don't try to wait for EDID on hot plug