From 17c217848d7b80d3b23924812666267705fb0253 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Fri, 2 Aug 2019 21:20:04 +0200 Subject: [PATCH 1/8] udev: Fix wrong closure function usage for the "device-added" signal The "device-added" signal should use g_cclosure_marshal_VOID__OBJECT not g_cclosure_marshal_VOID__VOID. Instead of fixing this manually, simply replace the closure function for both signals with NULL, glib will then automatically set the correct va_marshaller. https://gitlab.gnome.org/GNOME/mutter/merge_requests/713 --- src/backends/native/meta-udev.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/backends/native/meta-udev.c b/src/backends/native/meta-udev.c index ee7cc3ad482..71946613138 100644 --- a/src/backends/native/meta-udev.c +++ b/src/backends/native/meta-udev.c @@ -212,15 +212,13 @@ meta_udev_class_init (MetaUdevClass *klass) g_signal_new ("hotplug", G_TYPE_FROM_CLASS (object_class), G_SIGNAL_RUN_LAST, - 0, NULL, NULL, - g_cclosure_marshal_VOID__VOID, + 0, NULL, NULL, NULL, G_TYPE_NONE, 0); signals[DEVICE_ADDED] = g_signal_new ("device-added", G_TYPE_FROM_CLASS (object_class), G_SIGNAL_RUN_LAST, - 0, NULL, NULL, - g_cclosure_marshal_VOID__VOID, + 0, NULL, NULL, NULL, G_TYPE_NONE, 1, G_UDEV_TYPE_DEVICE); } -- GitLab From 6792903c4f492c31cfcd8b9f0dd6ca0d7c8bef1d Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Fri, 2 Aug 2019 22:41:00 +0200 Subject: [PATCH 2/8] udev: Add device-removed signal Add a device-removed signal which gets emitted when a GPU is removed. https://gitlab.gnome.org/GNOME/mutter/merge_requests/713 --- src/backends/native/meta-udev.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/backends/native/meta-udev.c b/src/backends/native/meta-udev.c index 71946613138..7c8080a56e4 100644 --- a/src/backends/native/meta-udev.c +++ b/src/backends/native/meta-udev.c @@ -31,6 +31,7 @@ enum { HOTPLUG, DEVICE_ADDED, + DEVICE_REMOVED, N_SIGNALS }; @@ -163,6 +164,8 @@ on_uevent (GUdevClient *client, if (g_str_equal (action, "add")) g_signal_emit (udev, signals[DEVICE_ADDED], 0, device); + else if (g_str_equal (action, "remove")) + g_signal_emit (udev, signals[DEVICE_REMOVED], 0, device); if (g_udev_device_get_property_as_boolean (device, "HOTPLUG")) g_signal_emit (udev, signals[HOTPLUG], 0); @@ -221,4 +224,11 @@ meta_udev_class_init (MetaUdevClass *klass) 0, NULL, NULL, NULL, G_TYPE_NONE, 1, G_UDEV_TYPE_DEVICE); + signals[DEVICE_REMOVED] = + g_signal_new ("device-removed", + G_TYPE_FROM_CLASS (object_class), + G_SIGNAL_RUN_LAST, + 0, NULL, NULL, NULL, + G_TYPE_NONE, 1, + G_UDEV_TYPE_DEVICE); } -- GitLab From 76445bcb979190beec9e43db0c4ba0a55b9e13bb Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Mon, 5 Aug 2019 11:21:28 +0200 Subject: [PATCH 3/8] kms: Remove unused fields from MetaKmsCrtcState struct https://gitlab.gnome.org/GNOME/mutter/merge_requests/713 --- src/backends/native/meta-kms-crtc.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/backends/native/meta-kms-crtc.h b/src/backends/native/meta-kms-crtc.h index 39a89a7518e..fa993819922 100644 --- a/src/backends/native/meta-kms-crtc.h +++ b/src/backends/native/meta-kms-crtc.h @@ -33,10 +33,6 @@ typedef struct _MetaKmsCrtcState gboolean is_drm_mode_valid; drmModeModeInfo drm_mode; - uint32_t common_possible_crtcs; - uint32_t common_possible_clones; - uint32_t encoder_device_idxs; - struct { uint16_t *red; uint16_t *green; -- GitLab From 73db35c53c71759b3ea4e864db64d1fb57094a1c Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Mon, 5 Aug 2019 12:09:29 +0200 Subject: [PATCH 4/8] kms: Fix meta_kms_crtc_read_state gamma table memory leak Before this commit meta_kms_crtc_read_state was overwriting the entire MetaKmsCrtcState struct stored in crtc->current_state including the gamma (sub)struct. This effectively zero-s the gamma struct each time before calling read_gamma_state, setting the pointers where the previous gamma values were stored to NULL without freeing the memory. Luckily this zero-ing also sets gamma.size to 0, causing read_gamma_state to re-alloc the arrays on each meta_kms_crtc_update_state call. But this does mean that were leaking the old gamma arrays on each meta_kms_crtc_update_state call. This commit fixes this by making meta_kms_crtc_read_state only overwrite the other values in the MetaKmsCrtcState struct and leaving the gamma sub-struct alone, this will make read_gamma_state correctly re-use the gamma tables if the gamma table size is unchanged; or re-alloc them (freeing the old ones) if the size has changed, fixing the memory leak. https://gitlab.gnome.org/GNOME/mutter/merge_requests/713 --- src/backends/native/meta-kms-crtc.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/backends/native/meta-kms-crtc.c b/src/backends/native/meta-kms-crtc.c index 2dd2b8da4c2..b07a6e7d5c2 100644 --- a/src/backends/native/meta-kms-crtc.c +++ b/src/backends/native/meta-kms-crtc.c @@ -110,17 +110,16 @@ meta_kms_crtc_read_state (MetaKmsCrtc *crtc, MetaKmsImplDevice *impl_device, drmModeCrtc *drm_crtc) { - crtc->current_state = (MetaKmsCrtcState) { - .rect = { - .x = drm_crtc->x, - .y = drm_crtc->y, - .width = drm_crtc->width, - .height = drm_crtc->height, - }, - .is_drm_mode_valid = drm_crtc->mode_valid, - .drm_mode = drm_crtc->mode, + crtc->current_state.rect = (MetaRectangle) { + .x = drm_crtc->x, + .y = drm_crtc->y, + .width = drm_crtc->width, + .height = drm_crtc->height, }; + crtc->current_state.is_drm_mode_valid = drm_crtc->mode_valid; + crtc->current_state.drm_mode = drm_crtc->mode; + read_gamma_state (crtc, impl_device, drm_crtc); } -- GitLab From 578ff2246411e9dbd4f14cbc34e03ebe173de8d0 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Fri, 2 Aug 2019 22:57:28 +0200 Subject: [PATCH 5/8] kms: drmModeGetCrtc may fail drmModeGetCrtc may fail and return NULL. This will trigger when meta_kms_crtc_update_state gets called from meta_kms_update_states_sync after a GPU has been unplugged leading to a NULL pointer deref causing a crash. This commit fixes this by checking for NULL and clearing the current_state when NULL is returned. https://gitlab.gnome.org/GNOME/mutter/merge_requests/713 --- src/backends/native/meta-kms-crtc.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/backends/native/meta-kms-crtc.c b/src/backends/native/meta-kms-crtc.c index b07a6e7d5c2..2b0f5385a7a 100644 --- a/src/backends/native/meta-kms-crtc.c +++ b/src/backends/native/meta-kms-crtc.c @@ -132,6 +132,13 @@ meta_kms_crtc_update_state (MetaKmsCrtc *crtc) impl_device = meta_kms_device_get_impl_device (crtc->device); drm_crtc = drmModeGetCrtc (meta_kms_impl_device_get_fd (impl_device), crtc->id); + if (!drm_crtc) + { + crtc->current_state.rect = (MetaRectangle) { }; + crtc->current_state.is_drm_mode_valid = FALSE; + return; + } + meta_kms_crtc_read_state (crtc, impl_device, drm_crtc); drmModeFreeCrtc (drm_crtc); } -- GitLab From 3ccb7cf4b23c9d25a9ced4b6981e9018e52f75a9 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Fri, 2 Aug 2019 22:48:41 +0200 Subject: [PATCH 6/8] kms: drmModeGetConnector may fail drmModeGetConnector may fail and return NULL, this may happen when a connector is removed underneath us (which can happen with e.g. DP MST or GPU hot unplug). Deal with this by skipping the connector when enumerating and by assuming it is disconnected when checking its connection state. https://gitlab.gnome.org/GNOME/mutter/merge_requests/713 --- src/backends/native/meta-kms-connector.c | 2 +- src/backends/native/meta-kms-impl-device.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/backends/native/meta-kms-connector.c b/src/backends/native/meta-kms-connector.c index 70d3336d3e2..226dd5e7168 100644 --- a/src/backends/native/meta-kms-connector.c +++ b/src/backends/native/meta-kms-connector.c @@ -456,7 +456,7 @@ meta_kms_connector_read_state (MetaKmsConnector *connector, g_clear_pointer (&connector->current_state, meta_kms_connector_state_free); - if (drm_connector->connection != DRM_MODE_CONNECTED) + if (!drm_connector || drm_connector->connection != DRM_MODE_CONNECTED) return; state = meta_kms_connector_state_new (); diff --git a/src/backends/native/meta-kms-impl-device.c b/src/backends/native/meta-kms-impl-device.c index 09114ea1782..c46d746462a 100644 --- a/src/backends/native/meta-kms-impl-device.c +++ b/src/backends/native/meta-kms-impl-device.c @@ -209,6 +209,9 @@ init_connectors (MetaKmsImplDevice *impl_device, drm_connector = drmModeGetConnector (impl_device->fd, drm_resources->connectors[i]); + if (!drm_connector) + continue; + connector = meta_kms_connector_new (impl_device, drm_connector, drm_resources); drmModeFreeConnector (drm_connector); -- GitLab From 0eb355e29d0a0644dcf30732f07c56b83ac9fd1c Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Fri, 2 Aug 2019 22:56:38 +0200 Subject: [PATCH 7/8] kms: Fix drm_connector mem-leak in meta_kms_connector_update_state https://gitlab.gnome.org/GNOME/mutter/merge_requests/713 --- src/backends/native/meta-kms-connector.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/backends/native/meta-kms-connector.c b/src/backends/native/meta-kms-connector.c index 226dd5e7168..8eed112295e 100644 --- a/src/backends/native/meta-kms-connector.c +++ b/src/backends/native/meta-kms-connector.c @@ -490,6 +490,8 @@ meta_kms_connector_update_state (MetaKmsConnector *connector, meta_kms_connector_read_state (connector, impl_device, drm_connector, drm_resources); + if (drm_connector) + drmModeFreeConnector (drm_connector); } static void -- GitLab From f3660dc60e76b13e35f361a1ea6816f8b0af66d8 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Mon, 5 Aug 2019 14:08:08 +0200 Subject: [PATCH 8/8] kms: Deal with GPUs being unplugged Add meta-kms and meta-monitor-manager-kms listener for the udev device-removed signal and on this signal update the device state / re-enumerate the monitors, so that the monitors properly get updated to disconnected state on GPU removal. We really should also have meta-backend-native remove the GPU itself from our list of GPU objects. But that is more involved, see: https://gitlab.gnome.org/GNOME/mutter/issues/710 This commit at least gets us to a point where we properly update the list of monitors when a GPU gets unplugged; and where we no longer crash the first time the user changes the monitor configuration after a GPU was unplugged. Specifically before this commit we would hit the first g_error () in meta_renderer_native_create_view () as soon as some monitor (re)configuration is done after a GPU was unplugged. https://gitlab.gnome.org/GNOME/mutter/merge_requests/713 --- src/backends/native/meta-kms.c | 25 +++++++++++++++++-- .../native/meta-monitor-manager-kms.c | 14 +++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/backends/native/meta-kms.c b/src/backends/native/meta-kms.c index 641aaa19826..13dc396d33a 100644 --- a/src/backends/native/meta-kms.c +++ b/src/backends/native/meta-kms.c @@ -149,6 +149,7 @@ struct _MetaKms MetaBackend *backend; guint hotplug_handler_id; + guint removed_handler_id; MetaKmsImpl *impl; gboolean in_impl_task; @@ -474,8 +475,7 @@ meta_kms_update_states_sync (MetaKms *kms, } static void -on_udev_hotplug (MetaUdev *udev, - MetaKms *kms) +handle_hotplug_event (MetaKms *kms) { g_autoptr (GError) error = NULL; @@ -483,6 +483,21 @@ on_udev_hotplug (MetaUdev *udev, g_warning ("Updating KMS state failed: %s", error->message); } +static void +on_udev_hotplug (MetaUdev *udev, + MetaKms *kms) +{ + handle_hotplug_event (kms); +} + +static void +on_udev_device_removed (MetaUdev *udev, + GUdevDevice *device, + MetaKms *kms) +{ + handle_hotplug_event (kms); +} + MetaBackend * meta_kms_get_backend (MetaKms *kms) { @@ -525,6 +540,9 @@ meta_kms_new (MetaBackend *backend, kms->hotplug_handler_id = g_signal_connect (udev, "hotplug", G_CALLBACK (on_udev_hotplug), kms); + kms->removed_handler_id = + g_signal_connect (udev, "device-removed", + G_CALLBACK (on_udev_device_removed), kms); return kms; } @@ -548,6 +566,9 @@ meta_kms_finalize (GObject *object) if (kms->hotplug_handler_id) g_signal_handler_disconnect (udev, kms->hotplug_handler_id); + if (kms->removed_handler_id) + g_signal_handler_disconnect (udev, kms->removed_handler_id); + G_OBJECT_CLASS (meta_kms_parent_class)->finalize (object); } diff --git a/src/backends/native/meta-monitor-manager-kms.c b/src/backends/native/meta-monitor-manager-kms.c index 9bac1357642..26c2ddb61e2 100644 --- a/src/backends/native/meta-monitor-manager-kms.c +++ b/src/backends/native/meta-monitor-manager-kms.c @@ -76,6 +76,7 @@ struct _MetaMonitorManagerKms MetaMonitorManager parent_instance; guint hotplug_handler_id; + guint removed_handler_id; }; struct _MetaMonitorManagerKmsClass @@ -484,6 +485,14 @@ on_udev_hotplug (MetaUdev *udev, handle_hotplug_event (manager); } +static void +on_udev_device_removed (MetaUdev *udev, + GUdevDevice *device, + MetaMonitorManager *manager) +{ + handle_hotplug_event (manager); +} + static void meta_monitor_manager_kms_connect_hotplug_handler (MetaMonitorManagerKms *manager_kms) { @@ -494,6 +503,9 @@ meta_monitor_manager_kms_connect_hotplug_handler (MetaMonitorManagerKms *manager manager_kms->hotplug_handler_id = g_signal_connect_after (udev, "hotplug", G_CALLBACK (on_udev_hotplug), manager); + manager_kms->removed_handler_id = + g_signal_connect_after (udev, "device-removed", + G_CALLBACK (on_udev_device_removed), manager); } static void @@ -505,6 +517,8 @@ meta_monitor_manager_kms_disconnect_hotplug_handler (MetaMonitorManagerKms *mana g_signal_handler_disconnect (udev, manager_kms->hotplug_handler_id); manager_kms->hotplug_handler_id = 0; + g_signal_handler_disconnect (udev, manager_kms->removed_handler_id); + manager_kms->removed_handler_id = 0; } void -- GitLab