Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Make wlr_output_enable idempotent #3306

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

DemiMarie
Copy link

Enabling or disabling an output should be idempotent. Previously, this
was not the case, causing bugs.

@emersion
Copy link
Member

10:05:29 <emersion> output->pending.enabled only has a meaningful value if WLR_OUTPUT_STATE_ENABLED is set
10:05:36 <emersion> same for the rest of the output state
10:07:00 <emersion> that's the status quo -- i'm not against changing it, but there are gotchas
10:07:53 <emersion> e.g. if we keep the previous state in pending, there has to be an exception for the wlr_buffer field and the gamma tables -- we don't want to keep these locked/allocated after we're done with the commit

@DemiMarie
Copy link
Author

10:05:29 <emersion> output->pending.enabled only has a meaningful value if WLR_OUTPUT_STATE_ENABLED is set
10:05:36 <emersion> same for the rest of the output state
10:07:00 <emersion> that's the status quo -- i'm not against changing it, but there are gotchas
10:07:53 <emersion> e.g. if we keep the previous state in pending, there has to be an exception for the wlr_buffer field and the gamma tables -- we don't want to keep these locked/allocated after we're done with the commit

The only time this change will have any effect is if wlr_output_enable is called twice on the same output, with different values of the parameter, before the output commits. Needless to say, this will basically never happen for typical rootful compositors. For rootless compositors such as the one I am working on, however, this happens a LOT. The result is that the output is left with the wrong value of output->pending.enabled, which causes problems later.

@emersion
Copy link
Member

I don't understand which kind of issues you're running into. If WLR_OUTPUT_STATE_ENABLED is unset in the flags, you shouldn't even look at pending.enabled.

@DemiMarie
Copy link
Author

I don't understand which kind of issues you're running into. If WLR_OUTPUT_STATE_ENABLED is unset in the flags, you shouldn't even look at pending.enabled.

I will do some testing locally.

@DemiMarie
Copy link
Author

Without this commit my compositor trips over this assertion.

This is a glue file to allow integration with builds.sr.ht.
@emersion
Copy link
Member

emersion commented Nov 1, 2021

wlroots has migrated to gitlab.freedesktop.org. This pull request has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3306

emersion and others added 23 commits November 1, 2021 18:54
For `required` to disable search the value needs to be of `feature` type.
Checking `gles2` via `in` keyword returns a `bool` but `required: false`
makes the dependency optional instead of disabled.
This allows compositors to handle touch pointer emulation manually,
instead of having Xwayland do it [1].

[1]: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/691
This struct contains additional information for session device
change events, such as the DRM connector ID that has changed.
When a connector ID is specified in a hotplug event, don't scan all
connectors. Only scan the connector that has changed.
Callers can access output->front_buffer instead.
The protocol uses a signed integer here, which is also what the
wlr_input_method_v2_preedit_string struct provides to compositors from
the input method protocol. Sway currently just passes those int32_t
values directly to this function leading to an implicit conversion.
Removing an input device requires unlinking it from the list of all headless
input devices. For that implement a destroy function.
We were send a protocol error if INTERLACED or BOTTOM_FIRST was
set. This is incorrect for the zwp_linux_dmabuf_params.create
code-path because this kills the client without allowing it to
gracefully handle the error.

We should only send a protocol error if the client provides a bit
not listed in the protocol definition.
They are never used in practice, which makes all of our flag
handling effectively dead code. Also, APIs such as KMS don't
provide a good way to deal with the flags. Let's just fail the
DMA-BUF import when clients provide flags.
A wlroots user can easily get confused and think that `cap` refers to
wlroots buffer capabilities, not array capacity.
@DemiMarie DemiMarie force-pushed the idempotent-enable-disable branch from ab53f5e to 7375cfa Compare November 18, 2021 17:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants