From b1273639823f4010d88557ab716c41aa200c5d76 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Mon, 23 Mar 2020 17:38:18 +0100 Subject: [PATCH 1/2] ply-device-manager: Only consume one udev event at a time Commit f9e376797a91 ("ply-device-manager: Consume all events in one go") changed ply-device-manager to consume all pending udev events in one go instead of consuming only 1 and then returning back to the mainloop. The idea here was to avoid the overhead of returning back to the mainloop, doing the poll again, seeing more events were pending and then re-enter ply-device-manager. In retrospect this is not a good idea. Systemd waits for oneshot units like plymouth-switch-root.service to finish and this can block the boot. Specifically plymouth-switch-root.service must complete before systemd in the initrd will exec the systemd from the real rootfs. This means that systemd inside the initrd waits for the: ExecStart=-/usr/bin/plymouth update-root-fs --new-root-dir=/sysroot Command to complete, if this command runs while we are consuming udev events from the graphics card (which sends a change event per probed connector during the initial probe), then plymouth will not send the ack to the plymouth boot-client (completing the ExecStart) until all udev events are consumed. On my main workstation with i915 graphics and 2 HDMI connected FHD monitors, this delays the actual switching of the root by 1.9 - 2.1 seconds, because the re-enumaration of the connectors in the drm plugin takes about 0.4 seconds per run. Other upcoming changes will greatly reduce that 0.4 seconds, but still returning to the main-loop after a single udev event so that we can answer any waiting boot-clients ASAP is a good idea. This reverts commit f9e376797a91ad5fbc1f8e8e4aea778f4f22397c. Signed-off-by: Hans de Goede --- src/libply-splash-core/ply-device-manager.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/libply-splash-core/ply-device-manager.c b/src/libply-splash-core/ply-device-manager.c index 160b8cb4..f65d7317 100644 --- a/src/libply-splash-core/ply-device-manager.c +++ b/src/libply-splash-core/ply-device-manager.c @@ -415,7 +415,7 @@ on_drm_udev_add_or_change (ply_device_manager_t *manager, } } -static bool +static void on_udev_event (ply_device_manager_t *manager) { struct udev_device *device; @@ -423,14 +423,14 @@ on_udev_event (ply_device_manager_t *manager) device = udev_monitor_receive_device (manager->udev_monitor); if (device == NULL) - return false; + return; action = udev_device_get_action (device); ply_trace ("got %s event for device %s", action, udev_device_get_sysname (device)); if (action == NULL) - return false; + return; if (strcmp (action, "add") == 0 || strcmp (action, "change") == 0) { const char *subsystem; @@ -450,14 +450,6 @@ on_udev_event (ply_device_manager_t *manager) } udev_device_unref (device); - return true; -} - -static void -on_udev_event_loop (ply_device_manager_t *manager) -{ - /* Call on_udev_event until all events are consumed */ - while (on_udev_event (manager)) {} } static void @@ -487,7 +479,7 @@ watch_for_udev_events (ply_device_manager_t *manager) fd, PLY_EVENT_LOOP_FD_STATUS_HAS_DATA, (ply_event_handler_t) - on_udev_event_loop, + on_udev_event, NULL, manager); } From 3aa76fcd872b5a528bf99a740cbdb2b443434ac0 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 24 Mar 2020 23:17:42 +0100 Subject: [PATCH 2/2] drm: Do not unnecessarily get output info twice When a kernel-mode-setting driver loads it will trigger an add udev event for /dev/dri/card0, followed by one udev change event per connector on the card. This means that after our initial probe of the card, create_heads_for_active_connectors is called a number of times for all the udev change events. After the initial enum our outputs array will contain active entries for all connected displays. Meaning that the first loop in create_heads_for_active_connectors would call get_output_info for these outputs. Under the hood this does a number of ioctls and especially the drmModeGetConnector call can be quite expensive. Then in the second loop create_heads_for_active_connectors would call get_output_info for all connectors, including for the once which were checked in the first loop. There is no reason why we cannot check if active connectors in the old outputs array have changed when we are calling get_output_info for all connectors to build the new array. This avoids unnecessarily making the expensive get_output_info call twice for active connectors in the old outputs array. Signed-off-by: Hans de Goede --- src/plugins/renderers/drm/plugin.c | 72 +++++++++++++++++++----------- 1 file changed, 46 insertions(+), 26 deletions(-) diff --git a/src/plugins/renderers/drm/plugin.c b/src/plugins/renderers/drm/plugin.c index 1ef2802a..4dbf8da8 100644 --- a/src/plugins/renderers/drm/plugin.c +++ b/src/plugins/renderers/drm/plugin.c @@ -1333,6 +1333,34 @@ remove_output (ply_renderer_backend_t *backend, ply_output_t *output) ply_renderer_head_remove_connector (backend, head, output->connector_id); } +/* Check if an output has changed since we last enumerated it; and if + * it has changed remove it from the head it is part of. + */ +static bool +check_if_output_has_changed (ply_renderer_backend_t *backend, + ply_output_t *new_output) +{ + ply_output_t *old_output = NULL; + int i; + + for (i = 0; i < backend->outputs_len; i++) { + if (backend->outputs[i].connector_id == new_output->connector_id) { + old_output = &backend->outputs[i]; + break; + } + } + + if (!old_output || !old_output->controller_id) + return false; + + if (memcmp(old_output, new_output, sizeof(ply_output_t)) == 0) + return false; + + ply_trace ("Output for connector %u changed, removing", old_output->connector_id); + remove_output (backend, old_output); + return true; +} + /* Update our outputs array to match the hardware state and * create and/or remove heads as necessary. * Returns true if any heads were modified. @@ -1341,37 +1369,18 @@ static bool create_heads_for_active_connectors (ply_renderer_backend_t *backend, bool change) { int i, j, number_of_setup_outputs, outputs_len; - ply_output_t output, *outputs; + ply_output_t *outputs; bool changed = false; /* Step 1: - * Remove existing outputs from heads if they have changed. - */ - ply_trace ("Checking currently connected outputs for changes"); - for (i = 0; i < backend->outputs_len; i++) { - if (!backend->outputs[i].controller_id) - continue; - - get_output_info (backend, backend->outputs[i].connector_id, &output); - - if (memcmp(&backend->outputs[i], &output, sizeof(ply_output_t))) { - ply_trace ("Output for connector %u changed, removing", - backend->outputs[i].connector_id); - remove_output (backend, &backend->outputs[i]); - changed = true; - } - } - - /* Step 2: - * Now that we've removed changed connectors from the heads, we can - * simply rebuild the outputs array from scratch. For any unchanged - * outputs for which we already have a head, we will end up in - * ply_renderer_head_add_connector which will ignore the already - * added connector. + * Query all outputs and: + * 1.1 Remove currently connected outputs from their heads if changed. + * 1.2 Build a new outputs array from scratch. For any unchanged + * outputs for which we already have a head, we will end up in + * ply_renderer_head_add_connector which will ignore the already + * added connector. */ ply_trace ("(Re)enumerating all outputs"); - free (backend->outputs); - backend->outputs = NULL; outputs = calloc (backend->resources->count_connectors, sizeof(*outputs)); outputs_len = backend->resources->count_connectors; @@ -1379,10 +1388,21 @@ create_heads_for_active_connectors (ply_renderer_backend_t *backend, bool change backend->connected_count = 0; for (i = 0; i < outputs_len; i++) { get_output_info (backend, backend->resources->connectors[i], &outputs[i]); + + if (check_if_output_has_changed (backend, &outputs[i])) + changed = true; + if (outputs[i].connected) backend->connected_count++; } + /* Step 2: + * Free the old outputs array, now that we have checked for changes + * we no longer need it. + */ + free (backend->outputs); + backend->outputs = NULL; + /* Step 3: * Drop controllers for clones for which we've picked different modes. */