config/output: reconfigure on commit failure #5521
Conversation
If the wlr_output_commit fails and the output was previously enabled, we re-enable the output. If the output was previously enabled, then it was also configured. Without reenabling or reconfiguring the output, the output is left in an invalid state. This adds the missing reconfiguring step. This also adds safe guards in output_disable to prevent removing an output index that is not in the list.
|
Hmm, this is getting a little bit complicated. I'm not a huge fan of this approach because we need to save/restore more output properties than before. This logic would be nicer if we only marked an output as enabled once the commit was successful. What was the reason why we don't do this? Because of |
Yeah. Unfortunately.
The case where an output is disabled and the commit fails for enabling it is easy. We just flip the enabled boolean back, which does need to be enabled for the output event handlers. The issue and complication is actually the other way - the output was enabled and the commit to disable it fails. This case should be much more rare, but still should be handled. If we don't re-enable and reconfigure the output, then we are left in a state where wlroots thinks the output is enabled, but sway doesn't. This leads to the output being frozen on the last frame or just the default swaybg gray, which can cascade to more problems due to the invalid state if the output gets disconnected or the user issues an enable for the output. |
| bool was_enabled = output->enabled; | ||
| bool was_configured = output->configured; | ||
| double old_x = 0, old_y = 0; | ||
|
|
||
| if (oc && !oc->enabled) { | ||
| // Output is configured to be disabled | ||
| sway_log(SWAY_DEBUG, "Disabling output %s", oc->name); | ||
| if (output->enabled) { | ||
| output_disable(output); |
There was a problem hiding this comment.
What about only calling output_disable once the output commit succeeds?
Sure. I was mainly asking why we need to flip the It turns out we have a |
Previously, we called output_disable prior to wlr_output_commit. This mutates Sway's output state before the output commit actually succeeds. This results in Sway's state getting out-of-sync with wlroots'. An alternative fix [1] was to revert the changes made by output_disable in case of failure. This is a little complicated. Instead, this patch makes it so Sway's internal state is never changed before a successful wlr_output commit. We had two output flags: enabled and configured. However enabled was set prior to the output becoming enabled, and was used to prevent the output event handlers (specifically, the mode handler) from calling apply_output_config again (infinite loop). Rename enabled to enabling and use it exclusively for this purpose. Rename configure to enabled, because that's what it really means. [1]: swaywm#5521 Closes: swaywm#5483
Previously, we called output_disable prior to wlr_output_commit. This mutates Sway's output state before the output commit actually succeeds. This results in Sway's state getting out-of-sync with wlroots'. An alternative fix [1] was to revert the changes made by output_disable in case of failure. This is a little complicated. Instead, this patch makes it so Sway's internal state is never changed before a successful wlr_output commit. We had two output flags: enabled and configured. However enabled was set prior to the output becoming enabled, and was used to prevent the output event handlers (specifically, the mode handler) from calling apply_output_config again (infinite loop). Rename enabled to enabling and use it exclusively for this purpose. Rename configure to enabled, because that's what it really means. [1]: #5521 Closes: #5483
Previously, we called output_disable prior to wlr_output_commit. This mutates Sway's output state before the output commit actually succeeds. This results in Sway's state getting out-of-sync with wlroots'. An alternative fix [1] was to revert the changes made by output_disable in case of failure. This is a little complicated. Instead, this patch makes it so Sway's internal state is never changed before a successful wlr_output commit. We had two output flags: enabled and configured. However enabled was set prior to the output becoming enabled, and was used to prevent the output event handlers (specifically, the mode handler) from calling apply_output_config again (infinite loop). Rename enabled to enabling and use it exclusively for this purpose. Rename configure to enabled, because that's what it really means. [1]: swaywm#5521 Closes: swaywm#5483 (cherry picked from commit 5432f00)
Previously, we called output_disable prior to wlr_output_commit. This mutates Sway's output state before the output commit actually succeeds. This results in Sway's state getting out-of-sync with wlroots'. An alternative fix [1] was to revert the changes made by output_disable in case of failure. This is a little complicated. Instead, this patch makes it so Sway's internal state is never changed before a successful wlr_output commit. We had two output flags: enabled and configured. However enabled was set prior to the output becoming enabled, and was used to prevent the output event handlers (specifically, the mode handler) from calling apply_output_config again (infinite loop). Rename enabled to enabling and use it exclusively for this purpose. Rename configure to enabled, because that's what it really means. [1]: swaywm#5521 Closes: swaywm#5483
Fixes #5483
Note that this only addresses the crash. I opened #5522 to provide a
way to destroy the headless output.
If the wlr_output_commit fails and the output was previously enabled, we
re-enable the output. If the output was previously enabled, then it was
also configured. Without reenabling or reconfiguring the output, the
output is left in an invalid state. This adds the missing reconfiguring
step.
This also adds safe guards in output_disable to prevent removing an
output index that is not in the list.