From a5f1bdff1a909358350157a016d416fd1075c123 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michel=20D=C3=A4nzer?= Date: Tue, 7 Jan 2025 18:31:20 +0100 Subject: [PATCH 1/8] kms: Delete unused members of MetaKms Unused since f27ca241f9a2 ("renderer/native: Move per frame KMS update to MetaFrameNative") / a6baa77eab89 ("kms: Split out impl/non-impl separation into MetaThread(Impl)"). Part-of: --- src/backends/native/meta-kms.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/backends/native/meta-kms.c b/src/backends/native/meta-kms.c index e42e05963e6..c72d97f9e26 100644 --- a/src/backends/native/meta-kms.c +++ b/src/backends/native/meta-kms.c @@ -52,17 +52,8 @@ struct _MetaKms gulong lease_handler_id; gulong removed_handler_id; - MetaKmsImpl *impl; - gboolean in_impl_task; - gboolean waiting_for_impl_task; - GList *devices; - GList *pending_updates; - - GList *pending_callbacks; - guint callback_source_id; - int kernel_thread_inhibit_count; MetaKmsCursorManager *cursor_manager; -- GitLab From ec73076e07640fc0752d9ecc66c5934726db2d7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michel=20D=C3=A4nzer?= Date: Fri, 3 Jan 2025 18:46:43 +0100 Subject: [PATCH 2/8] monitor-manager/native: Remove _read_current_state specialization It forced the power save mode to META_POWER_SAVE_ON when reading the current KMS state. This was problematic when a hotplug event is emitted while the session is locked, which can be triggered by monitors polling all their inputs for a signal: There's no mechanism to restore the previous power save mode in this case, so the monitors would fail to actually enter power saving mode but stayed on with a blank screen. This was at least one cause of the symptoms described in https://gitlab.freedesktop.org/drm/amd/-/issues/662 . Moreover, I suspect it hasn't had any effect for the actual reading of KMS state since 5f6aee341959 ("kms/update: Make power saving an update wide change"), as changing the power save mode to META_POWER_SAVE_ON no longer results in any immediate KMS state change, it's now only taken into account for the next mode set. It's not clear what the intended effect was in the first place, it was originally added as part of 65db8efbe8b1 ("MonitorManager: add a KMS backend") without rationale. It might have been cargo-culted from somewhere else. It shouldn't be necessary from a KMS API PoV though. Also adjust the KMS hotplug test to assert that a hotplug event doesn't implicitly change the power save mode. v2: (Sebastian Wick) * Fix shortlog of commit which added meta_monitor_manager_native_read_current_state. v3: * Adjust KMS hotplug test for the change. Part-of: --- .../native/meta-monitor-manager-native.c | 23 ------------------- src/tests/native-kms-hotplug.c | 10 ++++---- 2 files changed, 6 insertions(+), 27 deletions(-) diff --git a/src/backends/native/meta-monitor-manager-native.c b/src/backends/native/meta-monitor-manager-native.c index 0b1d2833584..b6c6ce42003 100644 --- a/src/backends/native/meta-monitor-manager-native.c +++ b/src/backends/native/meta-monitor-manager-native.c @@ -99,27 +99,6 @@ meta_monitor_manager_native_read_edid (MetaMonitorManager *manager, return meta_output_native_read_edid (META_OUTPUT_NATIVE (output)); } -static void -meta_monitor_manager_native_read_current_state (MetaMonitorManager *manager) -{ - MetaMonitorManagerClass *parent_class = - META_MONITOR_MANAGER_CLASS (meta_monitor_manager_native_parent_class); - MetaPowerSave power_save_mode; - - power_save_mode = meta_monitor_manager_get_power_save_mode (manager); - if (power_save_mode != META_POWER_SAVE_ON) - { - MetaPowerSaveChangeReason reason; - - reason = META_POWER_SAVE_CHANGE_REASON_HOTPLUG; - meta_monitor_manager_power_save_mode_changed (manager, - META_POWER_SAVE_ON, - reason); - } - - parent_class->read_current_state (manager); -} - static void meta_monitor_manager_native_set_power_save_mode (MetaMonitorManager *manager, MetaPowerSave mode) @@ -712,8 +691,6 @@ meta_monitor_manager_native_class_init (MetaMonitorManagerNativeClass *klass) manager_class->read_edid = meta_monitor_manager_native_read_edid; - manager_class->read_current_state = - meta_monitor_manager_native_read_current_state; manager_class->ensure_initial_config = meta_monitor_manager_native_ensure_initial_config; manager_class->apply_monitors_config = diff --git a/src/tests/native-kms-hotplug.c b/src/tests/native-kms-hotplug.c index db97a6f72f1..193166e26ef 100644 --- a/src/tests/native-kms-hotplug.c +++ b/src/tests/native-kms-hotplug.c @@ -385,7 +385,7 @@ emulate_hotplug (void) } static void -meta_test_power_save_implicit_on (void) +meta_test_power_save_no_implicit_on (void) { MetaBackend *backend = meta_context_get_backend (test_context); MetaMonitorManager *monitor_manager = @@ -423,9 +423,11 @@ meta_test_power_save_implicit_on (void) disconnect_connector_filter, NULL); emulate_hotplug (); - while (!power_save_mode_changed || !monitors_changed) + while (!monitors_changed) g_main_context_iteration (NULL, TRUE); + g_assert_false (power_save_mode_changed); + g_signal_handler_disconnect (monitor_manager, power_save_handler_id); g_signal_handler_disconnect (monitor_manager, monitors_changed_handler_id); @@ -445,8 +447,8 @@ init_tests (void) meta_test_disconnect_connect); g_test_add_func ("/hotplug/switch-config", meta_test_switch_config); - g_test_add_func ("/hotplug/power-save-implicit-off", - meta_test_power_save_implicit_on); + g_test_add_func ("/hotplug/power-save-no-implicit-on", + meta_test_power_save_no_implicit_on); } int -- GitLab From 7f78b6a9e527ff2e0cc475e3013fe9d781cdb85d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michel=20D=C3=A4nzer?= Date: Mon, 13 Jan 2025 17:06:35 +0100 Subject: [PATCH 3/8] kms: Use encoded strings instead of UpdateStatesData Handle NULL string pointer in meta_kms_update_states_in_impl. Drop second parameter of meta_kms_update_states_sync, which wasn't used by the external test caller anyway. Split out static update_states_sync function which takes a hotplug event string pointer instead. Preparation for next commit. v2: * Drop UpdteStatesData for encoded hotplug event strings. v3: * Put device path at the end of encoded string, allows simplifying meta_kms_update_states_in_impl slightly further. v4: (Sebastian Wick) * Use g_autofree for hotplug_event string in on_udev_hotplug, fixes leak. * Store pointer to hotplug event device path string in local variable in meta_kms_update_states_in_impl. v5: * Initialize `path` local variable to `NULL` and test it instead of the `hotplug_event` parameter. Avoids (false-positive) compiler warning about `path` possibly being used uninitialized. Part-of: --- src/backends/native/meta-kms-private.h | 3 +- src/backends/native/meta-kms.c | 94 +++++++++++++++----------- src/tests/native-kms-device.c | 2 +- 3 files changed, 56 insertions(+), 43 deletions(-) diff --git a/src/backends/native/meta-kms-private.h b/src/backends/native/meta-kms-private.h index cc81c748f6b..f92fa89ce31 100644 --- a/src/backends/native/meta-kms-private.h +++ b/src/backends/native/meta-kms-private.h @@ -39,8 +39,7 @@ gpointer meta_kms_run_impl_task_sync (MetaKms *kms, GError **error); META_EXPORT_TEST -MetaKmsResourceChanges meta_kms_update_states_sync (MetaKms *kms, - GUdevDevice *udev_device); +MetaKmsResourceChanges meta_kms_update_states_sync (MetaKms *kms); gboolean meta_kms_in_impl_task (MetaKms *kms); diff --git a/src/backends/native/meta-kms.c b/src/backends/native/meta-kms.c index c72d97f9e26..042bf47d1d7 100644 --- a/src/backends/native/meta-kms.c +++ b/src/backends/native/meta-kms.c @@ -173,18 +173,13 @@ meta_kms_is_waiting_for_impl_task (MetaKms *kms) return meta_thread_is_waiting_for_impl_task (thread); } -typedef struct _UpdateStatesData -{ - const char *device_path; - uint32_t crtc_id; - uint32_t connector_id; -} UpdateStatesData; - static MetaKmsResourceChanges -meta_kms_update_states_in_impl (MetaKms *kms, - UpdateStatesData *update_data) +meta_kms_update_states_in_impl (MetaKms *kms, + char *hotplug_event) { MetaKmsResourceChanges changes = META_KMS_RESOURCE_CHANGE_NONE; + uint32_t crtc_id = 0, connector_id = 0; + char *path = NULL; GList *l; COGL_TRACE_BEGIN_SCOPED (MetaKmsUpdateStates, @@ -195,28 +190,30 @@ meta_kms_update_states_in_impl (MetaKms *kms, if (!kms->devices) return META_KMS_RESOURCE_CHANGE_NO_DEVICES; + if (hotplug_event) + { + sscanf (hotplug_event, "%08x:%08x:%*s", &crtc_id, &connector_id); + path = hotplug_event + 2 * strlen ("12345678:"); + } + for (l = kms->devices; l; l = l->next) { MetaKmsDevice *kms_device = META_KMS_DEVICE (l->data); const char *kms_device_path = meta_kms_device_get_path (kms_device); - if (update_data->device_path && - g_strcmp0 (kms_device_path, update_data->device_path) != 0) + if (path && strcmp (path, kms_device_path) != 0) continue; - if (update_data->crtc_id > 0 && - !meta_kms_device_find_crtc_in_impl (kms_device, update_data->crtc_id)) + if (crtc_id > 0 && + !meta_kms_device_find_crtc_in_impl (kms_device, crtc_id)) continue; - if (update_data->connector_id > 0 && - !meta_kms_device_find_connector_in_impl (kms_device, - update_data->connector_id)) + if (connector_id > 0 && + !meta_kms_device_find_connector_in_impl (kms_device, connector_id)) continue; changes |= - meta_kms_device_update_states_in_impl (kms_device, - update_data->crtc_id, - update_data->connector_id); + meta_kms_device_update_states_in_impl (kms_device, crtc_id, connector_id); } return changes; @@ -227,42 +224,37 @@ update_states_in_impl (MetaThreadImpl *thread_impl, gpointer user_data, GError **error) { - UpdateStatesData *data = user_data; + char *hotplug_event = user_data; MetaKmsImpl *impl = META_KMS_IMPL (thread_impl); MetaKms *kms = meta_kms_impl_get_kms (impl); - return GUINT_TO_POINTER (meta_kms_update_states_in_impl (kms, data)); + return GUINT_TO_POINTER (meta_kms_update_states_in_impl (kms, hotplug_event)); } -MetaKmsResourceChanges -meta_kms_update_states_sync (MetaKms *kms, - GUdevDevice *udev_device) +static MetaKmsResourceChanges +update_states_sync (MetaKms *kms, + char *hotplug_event) { - UpdateStatesData data = {}; gpointer ret; - if (udev_device) - { - data.device_path = g_udev_device_get_device_file (udev_device); - data.crtc_id = - CLAMP (g_udev_device_get_property_as_int (udev_device, "CRTC"), - 0, UINT32_MAX); - data.connector_id = - CLAMP (g_udev_device_get_property_as_int (udev_device, "CONNECTOR"), - 0, UINT32_MAX); - } - - ret = meta_kms_run_impl_task_sync (kms, update_states_in_impl, &data, NULL); + ret = meta_kms_run_impl_task_sync (kms, update_states_in_impl, + hotplug_event, NULL); return GPOINTER_TO_UINT (ret); } +MetaKmsResourceChanges +meta_kms_update_states_sync (MetaKms *kms) +{ + return update_states_sync (kms, NULL); +} + static void handle_hotplug_event (MetaKms *kms, - GUdevDevice *udev_device, + char *hotplug_event, MetaKmsResourceChanges changes) { - changes |= meta_kms_update_states_sync (kms, udev_device); + changes |= update_states_sync (kms, hotplug_event); if (changes != META_KMS_RESOURCE_CHANGE_NONE) meta_kms_emit_resources_changed (kms, changes); @@ -287,12 +279,34 @@ meta_kms_resume (MetaKms *kms) meta_kms_run_impl_task_sync (kms, resume_in_impl, NULL, NULL); } +static char * +hotplug_event_from_udev_device (GUdevDevice *udev_device) +{ + const gchar *device_path; + uint32_t crtc_id, connector_id; + + if (!udev_device) + return NULL; + + device_path = g_udev_device_get_device_file (udev_device); + crtc_id = + CLAMP (g_udev_device_get_property_as_int (udev_device, "CRTC"), + 0, UINT32_MAX); + connector_id = + CLAMP (g_udev_device_get_property_as_int (udev_device, "CONNECTOR"), + 0, UINT32_MAX); + return g_strdup_printf ("%08x:%08x:%s", crtc_id, connector_id, device_path); +} + static void on_udev_hotplug (MetaUdev *udev, GUdevDevice *udev_device, MetaKms *kms) { - handle_hotplug_event (kms, udev_device, META_KMS_RESOURCE_CHANGE_NONE); + g_autofree char *hotplug_event = NULL; + + hotplug_event = hotplug_event_from_udev_device (udev_device); + handle_hotplug_event (kms, hotplug_event, META_KMS_RESOURCE_CHANGE_NONE); } static void diff --git a/src/tests/native-kms-device.c b/src/tests/native-kms-device.c index dfbf85514b4..f9cadf3f5b0 100644 --- a/src/tests/native-kms-device.c +++ b/src/tests/native-kms-device.c @@ -311,7 +311,7 @@ meta_test_kms_device_mode_set (void) ==, meta_kms_crtc_get_id (crtc)); - meta_kms_update_states_sync (meta_kms_device_get_kms (device), NULL); + meta_kms_update_states_sync (meta_kms_device_get_kms (device)); assert_crtc_state_equals (&crtc_state, meta_kms_crtc_get_current_state (crtc)); assert_connector_state_equals (&connector_state, -- GitLab From 313860e2fab15249337f70d0c298dd02b903c7b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michel=20D=C3=A4nzer?= Date: Thu, 9 Jan 2025 17:20:11 +0100 Subject: [PATCH 4/8] kms: Wait for 2 seconds before handling udev hotplug events More precisely, wait until no further udev hotplug events have arrived in 2 seconds. Keep a list of unique UpdateStatesData structs which have received hotplug events, and call handle_hotplug_event for all of them once the timeout expires. This avoids problems due to some monitors generating hotplug events when power saving is enabled on locking the session, and then temporarily appearing disconnected. v2: * Make meta_test_disconnect_connect wait and run main context dispatch before checking the number of logical monitors. v3: * Call g_source_unref immediately after g_source_attach, simplifies source cleanup. (Sebastian Wick) * Free hotplug devices list in meta_kms_finalize. (Sebastian Wick) * Add KMS debug logging in on_udev_hotplug & hotplug_timeout. * Bump timeout to 2 seconds. With my affected monitor, the second udev hotplug event normally arrives almost a second after the first one, occasionally more than 1.5 seconds though. * Use UpdateStatesData with custom compare function for hotplug_devices list data, since the GUdevDevice / device path string pointer values are always different. * Don't wait for timeout if meta_is_udev_test_device returns TRUE, to hopefully fix VKMS CI tests. Drop meta_test_disconnect_connect changes again. v4: * Use hash table instead of list for hotplug_events set. (Sebastian Wick) * Add comment describing the keys & values in the hash table. (Sebastian Wick) * Add KMS debug logging in handle_hotplug_event as well. * Dropped Closes:, this might not suffice to fully address https://gitlab.gnome.org/GNOME/mutter/-/issues/3831 after all. v5: * Simplify hash table annotation comment. (Sebastian Wick) v6: * Rename hotplug_timeout local string pointer to hotplug_event. * Drop g_clear_pointer in favour of g_autofree in on_udev_hotplug. (Sebastian Wick) Part-of: --- src/backends/native/meta-kms.c | 86 ++++++++++++++++++++++++++++++++-- 1 file changed, 81 insertions(+), 5 deletions(-) diff --git a/src/backends/native/meta-kms.c b/src/backends/native/meta-kms.c index 042bf47d1d7..58c129cc2c7 100644 --- a/src/backends/native/meta-kms.c +++ b/src/backends/native/meta-kms.c @@ -52,6 +52,10 @@ struct _MetaKms gulong lease_handler_id; gulong removed_handler_id; + GSource *hotplug_timeout; + /* Set: Pointer to "::" string */ + GHashTable *hotplug_events; + GList *devices; int kernel_thread_inhibit_count; @@ -252,10 +256,14 @@ meta_kms_update_states_sync (MetaKms *kms) static void handle_hotplug_event (MetaKms *kms, char *hotplug_event, - MetaKmsResourceChanges changes) + MetaKmsResourceChanges changes, + const char *caller) { changes |= update_states_sync (kms, hotplug_event); + meta_topic (META_DEBUG_KMS, "%s -> %s for '%s', changes=0x%x", + caller, __func__, hotplug_event, changes); + if (changes != META_KMS_RESOURCE_CHANGE_NONE) meta_kms_emit_resources_changed (kms, changes); } @@ -274,11 +282,56 @@ resume_in_impl (MetaThreadImpl *thread_impl, void meta_kms_resume (MetaKms *kms) { - handle_hotplug_event (kms, NULL, META_KMS_RESOURCE_CHANGE_FULL); + handle_hotplug_event (kms, NULL, META_KMS_RESOURCE_CHANGE_FULL, __func__); meta_kms_run_impl_task_sync (kms, resume_in_impl, NULL, NULL); } +static gboolean +hotplug_timeout (gpointer user_data) +{ + MetaKms *kms = user_data; + GHashTableIter iter; + char *hotplug_event; + + if (meta_is_topic_enabled (META_DEBUG_KMS)) + { + int64_t dispatch_time = g_source_get_time (kms->hotplug_timeout); + int64_t ready_time = g_source_get_ready_time (kms->hotplug_timeout); + + meta_topic (META_DEBUG_KMS, + "%s: %" G_GINT64_FORMAT " (dispatch time)" + " - %" G_GINT64_FORMAT " (ready time)" + " = %" G_GINT64_FORMAT "µs", + __func__, dispatch_time, ready_time, + dispatch_time - ready_time); + } + + g_hash_table_iter_init (&iter, kms->hotplug_events); + while (g_hash_table_iter_next (&iter, (gpointer *) &hotplug_event, NULL)) + { + handle_hotplug_event (kms, hotplug_event, META_KMS_RESOURCE_CHANGE_NONE, + __func__); + g_hash_table_iter_remove (&iter); + } + + kms->hotplug_timeout = NULL; + return G_SOURCE_REMOVE; +} + +static void +ensure_hotplug_timeout_source (MetaKms *kms) +{ + if (kms->hotplug_timeout) + return; + + kms->hotplug_timeout = g_timeout_source_new_seconds (2); + g_source_set_callback (kms->hotplug_timeout, hotplug_timeout, kms, NULL); + g_source_set_name (kms->hotplug_timeout, "[mutter] MetaKms hotplug timeout"); + g_source_attach (kms->hotplug_timeout, NULL); + g_source_unref (kms->hotplug_timeout); +} + static char * hotplug_event_from_udev_device (GUdevDevice *udev_device) { @@ -286,7 +339,7 @@ hotplug_event_from_udev_device (GUdevDevice *udev_device) uint32_t crtc_id, connector_id; if (!udev_device) - return NULL; + return g_strdup (""); device_path = g_udev_device_get_device_file (udev_device); crtc_id = @@ -303,10 +356,27 @@ on_udev_hotplug (MetaUdev *udev, GUdevDevice *udev_device, MetaKms *kms) { + int64_t now = g_get_monotonic_time (); g_autofree char *hotplug_event = NULL; + meta_topic (META_DEBUG_KMS, + "%s called at %" G_GINT64_FORMAT, + __func__, now); + hotplug_event = hotplug_event_from_udev_device (udev_device); - handle_hotplug_event (kms, hotplug_event, META_KMS_RESOURCE_CHANGE_NONE); + + if (meta_is_udev_test_device (udev_device)) + { + handle_hotplug_event (kms, hotplug_event, + META_KMS_RESOURCE_CHANGE_NONE, __func__); + return; + } + + ensure_hotplug_timeout_source (kms); + g_source_set_ready_time (kms->hotplug_timeout, now + 2 * G_USEC_PER_SEC); + + g_hash_table_insert (kms->hotplug_events, g_steal_pointer (&hotplug_event), + NULL); } static void @@ -314,7 +384,7 @@ on_udev_device_removed (MetaUdev *udev, GUdevDevice *device, MetaKms *kms) { - handle_hotplug_event (kms, NULL, META_KMS_RESOURCE_CHANGE_NONE); + handle_hotplug_event (kms, NULL, META_KMS_RESOURCE_CHANGE_NONE, __func__); } static void @@ -490,6 +560,9 @@ meta_kms_finalize (GObject *object) g_list_free_full (kms->devices, g_object_unref); + g_clear_pointer (&kms->hotplug_timeout, g_source_destroy); + g_hash_table_destroy (kms->hotplug_events); + g_clear_signal_handler (&kms->hotplug_handler_id, udev); g_clear_signal_handler (&kms->lease_handler_id, udev); g_clear_signal_handler (&kms->removed_handler_id, udev); @@ -501,6 +574,9 @@ static void meta_kms_init (MetaKms *kms) { kms->cursor_manager = meta_kms_cursor_manager_new (kms); + + kms->hotplug_events = + g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL); } static void -- GitLab From 71603c22fbc1f13f4dbc9b3bd1e94e289fbe989f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michel=20D=C3=A4nzer?= Date: Mon, 27 Jan 2025 11:52:04 +0100 Subject: [PATCH 5/8] kms/connector: Refactor edid_equal helper out of _state_changes No functional change intended. Part-of: --- src/backends/native/meta-kms-connector.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/backends/native/meta-kms-connector.c b/src/backends/native/meta-kms-connector.c index afd3d5e0f50..338b037a491 100644 --- a/src/backends/native/meta-kms-connector.c +++ b/src/backends/native/meta-kms-connector.c @@ -895,6 +895,17 @@ meta_kms_connector_state_free (MetaKmsConnectorState *state) G_DEFINE_AUTOPTR_CLEANUP_FUNC (MetaKmsConnectorState, meta_kms_connector_state_free); +static gboolean +edid_equal (GBytes *old_edid, + GBytes *new_edid) +{ + if ((old_edid && !new_edid) || !new_edid || + !g_bytes_equal (old_edid, new_edid)) + return FALSE; + + return TRUE; +} + static gboolean kms_modes_equal (GList *modes, GList *other_modes) @@ -968,8 +979,7 @@ meta_kms_connector_state_changes (MetaKmsConnectorState *state, if (!meta_tile_info_equal (&state->tile_info, &new_state->tile_info)) return META_KMS_RESOURCE_CHANGE_FULL; - if ((state->edid_data && !new_state->edid_data) || !state->edid_data || - !g_bytes_equal (state->edid_data, new_state->edid_data)) + if (!edid_equal (state->edid_data, new_state->edid_data)) return META_KMS_RESOURCE_CHANGE_FULL; if (!kms_modes_equal (state->modes, new_state->modes)) -- GitLab From 6fe8993e35e9d31bebcb466a932990c8d0df3678 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michel=20D=C3=A4nzer?= Date: Mon, 27 Jan 2025 12:01:15 +0100 Subject: [PATCH 6/8] kms/connector: Return TRUE from edid_equal if both EDID pointers are NULL Fixes: a8d11161b6b4 ("kms: Only emit resources-changed signal if we recorded a change") Part-of: --- src/backends/native/meta-kms-connector.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/backends/native/meta-kms-connector.c b/src/backends/native/meta-kms-connector.c index 338b037a491..c368d1865f8 100644 --- a/src/backends/native/meta-kms-connector.c +++ b/src/backends/native/meta-kms-connector.c @@ -899,11 +899,10 @@ static gboolean edid_equal (GBytes *old_edid, GBytes *new_edid) { - if ((old_edid && !new_edid) || !new_edid || - !g_bytes_equal (old_edid, new_edid)) - return FALSE; + if (old_edid && new_edid) + return g_bytes_equal (old_edid, new_edid); - return TRUE; + return !old_edid && !new_edid; } static gboolean -- GitLab From 3fde2f710095eedf97299a87523f37f4f2eb5749 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michel=20D=C3=A4nzer?= Date: Sat, 8 Feb 2025 15:19:39 +0100 Subject: [PATCH 7/8] kms/connector: Fix kms_modes_equal It would return FALSE if any pair of one mode from each list didn't match, which is always the case if either list has at least two modes. Closes: https://gitlab.gnome.org/GNOME/mutter/-/issues/3831 Fixes: a8d11161b6b4 ("kms: Only emit resources-changed signal if we recorded a change") Part-of: --- src/backends/native/meta-kms-connector.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/backends/native/meta-kms-connector.c b/src/backends/native/meta-kms-connector.c index c368d1865f8..054ead2f404 100644 --- a/src/backends/native/meta-kms-connector.c +++ b/src/backends/native/meta-kms-connector.c @@ -918,14 +918,21 @@ kms_modes_equal (GList *modes, { GList *k; MetaKmsMode *mode = l->data; + gboolean found_match = FALSE; for (k = other_modes; k; k = k->next) { MetaKmsMode *other_mode = k->data; if (!meta_kms_mode_equal (mode, other_mode)) - return FALSE; + continue; + + found_match = TRUE; + break; } + + if (!found_match) + return FALSE; } return TRUE; -- GitLab From ba4cb3e021621e1fe89117c202ffb694d532715c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michel=20D=C3=A4nzer?= Date: Tue, 21 Jan 2025 18:51:55 +0100 Subject: [PATCH 8/8] kms: Add debug logging about full resource change triggers Part-of: --- src/backends/native/meta-kms-connector.c | 135 +++++++++++++++++---- src/backends/native/meta-kms-crtc.c | 18 ++- src/backends/native/meta-kms-impl-device.c | 12 ++ 3 files changed, 140 insertions(+), 25 deletions(-) diff --git a/src/backends/native/meta-kms-connector.c b/src/backends/native/meta-kms-connector.c index 054ead2f404..24d3eed46c6 100644 --- a/src/backends/native/meta-kms-connector.c +++ b/src/backends/native/meta-kms-connector.c @@ -943,78 +943,144 @@ meta_kms_connector_state_changes (MetaKmsConnectorState *state, MetaKmsConnectorState *new_state) { if (state->current_crtc_id != new_state->current_crtc_id) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "current CRTC ID changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->common_possible_crtcs != new_state->common_possible_crtcs) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "common possible CRTCs changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->common_possible_clones != new_state->common_possible_clones) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "common possible clones changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->encoder_device_idxs != new_state->encoder_device_idxs) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "encoder device idxs changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->width_mm != new_state->width_mm) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "width_mm changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->height_mm != new_state->height_mm) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "height_mm changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->has_scaling != new_state->has_scaling) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "has_scaling changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->non_desktop != new_state->non_desktop) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "non_desktop changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->subpixel_order != new_state->subpixel_order) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "subpixel order changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->suggested_x != new_state->suggested_x) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "suggested_x changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->suggested_y != new_state->suggested_y) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "suggested_y changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->hotplug_mode_update != new_state->hotplug_mode_update) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "hotplug_mode_update changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->panel_orientation_transform != new_state->panel_orientation_transform) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "panel orientation transform changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (!meta_tile_info_equal (&state->tile_info, &new_state->tile_info)) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "tile info changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (!edid_equal (state->edid_data, new_state->edid_data)) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "EDID changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (!kms_modes_equal (state->modes, new_state->modes)) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "modes changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->max_bpc.supported != new_state->max_bpc.supported || state->max_bpc.value != new_state->max_bpc.value || state->max_bpc.min_value != new_state->max_bpc.min_value || state->max_bpc.max_value != new_state->max_bpc.max_value) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "max_bpc changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->colorspace.value != new_state->colorspace.value || state->colorspace.supported != new_state->colorspace.supported) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "colorspace changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->hdr.supported != new_state->hdr.supported || state->hdr.unknown != new_state->hdr.unknown || !meta_output_hdr_metadata_equal (&state->hdr.value, &new_state->hdr.value)) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "HDR changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->broadcast_rgb.value != new_state->broadcast_rgb.value || state->broadcast_rgb.supported != new_state->broadcast_rgb.supported) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "broadcast_rgb changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->vrr_capable != new_state->vrr_capable) - return META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, "vrr_capable changed"); + return META_KMS_RESOURCE_CHANGE_FULL; + } if (state->privacy_screen_state != new_state->privacy_screen_state) - return META_KMS_RESOURCE_CHANGE_PRIVACY_SCREEN; + { + meta_topic (META_DEBUG_KMS, "privacy screen state changed"); + return META_KMS_RESOURCE_CHANGE_PRIVACY_SCREEN; + } return META_KMS_RESOURCE_CHANGE_NONE; } @@ -1065,6 +1131,9 @@ meta_kms_connector_read_state (MetaKmsConnector *connector, if (drm_connector->connection != connector->connection) { connector->connection = drm_connector->connection; + meta_topic (META_DEBUG_KMS, + "%s: connector status changed", + __func__); changes |= META_KMS_RESOURCE_CHANGE_FULL; } @@ -1089,13 +1158,31 @@ meta_kms_connector_read_state (MetaKmsConnector *connector, if (drm_connector->connection != connector->connection) { connector->connection = drm_connector->connection; + meta_topic (META_DEBUG_KMS, + "%s: connector status changed", + __func__); changes |= META_KMS_RESOURCE_CHANGE_FULL; } if (!current_state) - connector_changes = META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, + "%s: no current connector state for reference", + __func__); + connector_changes = META_KMS_RESOURCE_CHANGE_FULL; + } else - connector_changes = meta_kms_connector_state_changes (current_state, state); + { + connector_changes = meta_kms_connector_state_changes (current_state, state); + + if (connector_changes & META_KMS_RESOURCE_CHANGE_FULL) + { + meta_topic (META_DEBUG_KMS, + "%s: meta_kms_connector_state_changes triggered " + "returned META_KMS_RESOURCE_CHANGE_FULL", + __func__); + } + } changes |= connector_changes; diff --git a/src/backends/native/meta-kms-crtc.c b/src/backends/native/meta-kms-crtc.c index ecab769de4c..048b36ea68e 100644 --- a/src/backends/native/meta-kms-crtc.c +++ b/src/backends/native/meta-kms-crtc.c @@ -302,11 +302,24 @@ meta_kms_crtc_read_state (MetaKmsCrtc *crtc, if (!crtc_state.is_active) { if (crtc->current_state.is_active) - changes |= META_KMS_RESOURCE_CHANGE_FULL; + { + meta_topic (META_DEBUG_KMS, + "%s: CRTC is_active disabled", + __func__); + changes |= META_KMS_RESOURCE_CHANGE_FULL; + } } else { changes = meta_kms_crtc_state_changes (&crtc->current_state, &crtc_state); + + if (changes & META_KMS_RESOURCE_CHANGE_FULL) + { + meta_topic (META_DEBUG_KMS, + "%s: meta_kms_crtc_state_changes returned " + "META_KMS_RESOURCE_CHANGE_FULL", + __func__); + } } g_clear_pointer (&crtc->current_state.gamma.value, @@ -346,6 +359,9 @@ meta_kms_crtc_update_state_in_impl (MetaKmsCrtc *crtc) crtc->current_state.is_active = FALSE; crtc->current_state.rect = (MtkRectangle) { }; crtc->current_state.is_drm_mode_valid = FALSE; + meta_topic (META_DEBUG_KMS, + "%s: drm_crtc=%p drm_props=%p", + __func__, drm_crtc, drm_props); changes = META_KMS_RESOURCE_CHANGE_FULL; goto out; } diff --git a/src/backends/native/meta-kms-impl-device.c b/src/backends/native/meta-kms-impl-device.c index e956b5a78ea..230adb37305 100644 --- a/src/backends/native/meta-kms-impl-device.c +++ b/src/backends/native/meta-kms-impl-device.c @@ -701,6 +701,18 @@ update_connectors (MetaKmsImplDevice *impl_device, g_list_length (connectors) == g_list_length (priv->connectors)) return changes; + if (added_connector) + { + meta_topic (META_DEBUG_KMS, "%s: New connector(s) added", __func__); + } + else + { + meta_topic (META_DEBUG_KMS, + "%s: Connectors list length changed from %d to %d", + __func__, g_list_length (priv->connectors), + g_list_length (connectors)); + } + g_list_free_full (priv->connectors, g_object_unref); priv->connectors = g_list_reverse (g_steal_pointer (&connectors)); -- GitLab