Skip to content

Conversation

@Drakulix
Copy link
Member

Needs some testing in cosmic-comp, but I believe this is necessary to provide race-free state changes.

Note: We could potentially provide two variants of this api, the old one and the new one. I don't see a particular advantage to that approach other than preserving a relatively new and barely used api.

@Drakulix Drakulix requested a review from cmeissl July 15, 2025 15:49
@Drakulix Drakulix force-pushed the feature/explicit-lock-drm-manager branch 2 times, most recently from fd25a9f to 9c3b69e Compare July 15, 2025 16:31
@Drakulix Drakulix force-pushed the feature/explicit-lock-drm-manager branch from 9c3b69e to c28604c Compare July 16, 2025 15:23
@Drakulix Drakulix marked this pull request as ready for review July 17, 2025 16:35
@Drakulix
Copy link
Member Author

Drakulix commented Jul 17, 2025

Confirmed this does end up fixing our issues in cosmic.

Especially changing the Output, when used as an OutputModeSource can be particularly brittle, as that is used for CRTC_W and CRTC_H, which might cause the commit to be rejected, if the hardware cannot do the necessary scaling with a new mode.

So without locks and careful order of the operations any thread might use the wrong resolution temporarily or any tests by the DrmOutput/DrmOutputManager using the standard code paths can be rejected causing the new valid configuration to fail being applied.

This is sufficient to get rid of the races, though this is still easier to shoot yourself in the foot than I would like it to be.

@cmeissl
Copy link
Collaborator

cmeissl commented Jul 21, 2025

This is sufficient to get rid of the races, though this is still easier to shoot yourself in the foot than I would like it to be.

Yeah, not sure how to resolve this. I guess at some point it would be great to have some kind of output abstraction across backends.

@Drakulix Drakulix merged commit 700941b into master Jul 21, 2025
13 checks passed
Drakulix added a commit that referenced this pull request Sep 10, 2025
Since #1778 we provide an api to explicitly lock the `DrmOutputManager`
to apply multiple changes atomically without `DrmOutput`s being able to
observe any inconsistent state.

However as a side-effect of this, it has become impossible to re-assign
a crtc to a different connector in one atomic opertion, as the lock on
the `DrmOutputManager` prevents the `Drop`-implementation of `DrmOutput`
to free any crtcs for re-use.

This commit introduces a channel, if locking fails to deferr freeing
the crtc to the locked manager.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants