From 8bc375005cd942e9b502976a32badcbd77466cd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20=C3=85dahl?= Date: Wed, 23 Nov 2022 23:08:33 +0100 Subject: [PATCH] kms/connector: Don't query the kernel twice when updating On hotplug, the events we receive from the kernel are async, and connectors in the kernel come and go as they please. In practice, this means that calling drmModeGetConnector() twice more or less directly after each other, there is no guarantee that the latter call will return anything if the former did. When updating the connector in response to hotplugs, we'd first update the list of existing connectors, and following that, query each and every one again for their current state, to update our internal representation; only the former handled drmModeGetConnector() returning NULL, meaning if unlucky, we'd end up doing a null pointer dereference when trying to update the state. Handle this by querying the kernel for the current connector state only once per connector, updating the list of connectors and their corresponding state at the same time. Fixes the following crash: #0 meta_kms_connector_read_state at ../src/backends/native/meta-kms-connector.c:684 #1 meta_kms_connector_update_state at ../src/backends/native/meta-kms-connector.c:767 #2 meta_kms_impl_device_update_states at ../src/backends/native/meta-kms-impl-device.c:916 #3 meta_kms_device_update_states_in_impl at ../src/backends/native/meta-kms-device.c:267 #4 meta_kms_update_states_in_impl at ../src/backends/native/meta-kms.c:604 #5 update_states_in_impl at ../src/backends/native/meta-kms.c:620 #6 meta_kms_run_impl_task_sync at ../src/backends/native/meta-kms.c:435 #7 meta_kms_update_states_sync at ../src/backends/native/meta-kms.c:641 #8 handle_hotplug_event at ../src/backends/native/meta-kms.c:651 #9 on_udev_hotplug at ../src/backends/native/meta-kms.c:668 Related: https://bugzilla.redhat.com/show_bug.cgi?id=2131269 Part-of: --- .../native/meta-kms-connector-private.h | 3 +- src/backends/native/meta-kms-connector.c | 8 ++---- src/backends/native/meta-kms-impl-device.c | 28 +++++++++---------- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/backends/native/meta-kms-connector-private.h b/src/backends/native/meta-kms-connector-private.h index 73757a8fc..66b2fa6fb 100644 --- a/src/backends/native/meta-kms-connector-private.h +++ b/src/backends/native/meta-kms-connector-private.h @@ -103,7 +103,8 @@ uint64_t meta_kms_connector_get_prop_drm_value (MetaKmsConnector *connector, uint64_t value); MetaKmsResourceChanges meta_kms_connector_update_state (MetaKmsConnector *connector, - drmModeRes *drm_resources); + drmModeRes *drm_resources, + drmModeConnector *drm_connector); void meta_kms_connector_disable (MetaKmsConnector *connector); diff --git a/src/backends/native/meta-kms-connector.c b/src/backends/native/meta-kms-connector.c index 2bc337291..0cf775bbb 100644 --- a/src/backends/native/meta-kms-connector.c +++ b/src/backends/native/meta-kms-connector.c @@ -754,20 +754,16 @@ out: MetaKmsResourceChanges meta_kms_connector_update_state (MetaKmsConnector *connector, - drmModeRes *drm_resources) + drmModeRes *drm_resources, + drmModeConnector *drm_connector) { MetaKmsImplDevice *impl_device; - drmModeConnector *drm_connector; MetaKmsResourceChanges changes; impl_device = meta_kms_device_get_impl_device (connector->device); - drm_connector = drmModeGetConnector (meta_kms_impl_device_get_fd (impl_device), - connector->id); - changes = meta_kms_connector_read_state (connector, impl_device, drm_connector, drm_resources); - g_clear_pointer (&drm_connector, drmModeFreeConnector); return changes; } diff --git a/src/backends/native/meta-kms-impl-device.c b/src/backends/native/meta-kms-impl-device.c index b1dd321a7..d9ea27637 100644 --- a/src/backends/native/meta-kms-impl-device.c +++ b/src/backends/native/meta-kms-impl-device.c @@ -397,12 +397,14 @@ find_existing_connector (MetaKmsImplDevice *impl_device, static MetaKmsResourceChanges update_connectors (MetaKmsImplDevice *impl_device, - drmModeRes *drm_resources) + drmModeRes *drm_resources, + uint32_t updated_connector_id) { MetaKmsImplDevicePrivate *priv = meta_kms_impl_device_get_instance_private (impl_device); g_autolist (MetaKmsConnector) connectors = NULL; gboolean added_connector = FALSE; + MetaKmsResourceChanges changes = META_KMS_RESOURCE_CHANGE_NONE; unsigned int i; int fd; @@ -421,6 +423,13 @@ update_connectors (MetaKmsImplDevice *impl_device, if (connector) { connector = g_object_ref (connector); + if (updated_connector_id == 0 || + meta_kms_connector_get_id (connector) == updated_connector_id) + { + changes |= meta_kms_connector_update_state (connector, + drm_resources, + drm_connector); + } } else { @@ -436,7 +445,7 @@ update_connectors (MetaKmsImplDevice *impl_device, if (!added_connector && g_list_length (connectors) == g_list_length (priv->connectors)) - return META_KMS_RESOURCE_CHANGE_NONE; + return changes; g_list_free_full (priv->connectors, g_object_unref); priv->connectors = g_list_reverse (g_steal_pointer (&connectors)); @@ -892,7 +901,7 @@ meta_kms_impl_device_update_states (MetaKmsImplDevice *impl_device, goto err; } - changes = update_connectors (impl_device, drm_resources); + changes = update_connectors (impl_device, drm_resources, connector_id); for (l = priv->crtcs; l; l = l->next) { @@ -905,17 +914,6 @@ meta_kms_impl_device_update_states (MetaKmsImplDevice *impl_device, changes |= meta_kms_crtc_update_state (crtc); } - for (l = priv->connectors; l; l = l->next) - { - MetaKmsConnector *connector = META_KMS_CONNECTOR (l->data); - - if (connector_id > 0 && - meta_kms_connector_get_id (connector) != connector_id) - continue; - - changes |= meta_kms_connector_update_state (connector, drm_resources); - } - drmModeFreeResources (drm_resources); return changes; @@ -1196,7 +1194,7 @@ meta_kms_impl_device_init_mode_setting (MetaKmsImplDevice *impl_device, init_fallback_modes (impl_device); - update_connectors (impl_device, drm_resources); + update_connectors (impl_device, drm_resources, 0); drmModeFreeResources (drm_resources);