Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic wlr-output-management support #263

Merged
merged 3 commits into from
Jul 28, 2023

Conversation

joggee-fr
Copy link
Collaborator

I have tried to resume work from @dimkr in PR #213.

  • Rebased on current HEAD
  • Implements output manager test (wlr-randr --dryrun)
  • Add output layout change monitor to ensure views are well positioned according to new layout

For the moment, I have only played with Wayland as backend.
I am still unsure about current_mode test, see discussion.
Furthermore, trying to disable an output returns an error. I don't know if it specific to Wayland backend. Here are the logs from wlroots (0.16.2):

00:00:04.247 [types/output/output.c:656] Tried to modeset a disabled output
00:00:04.247 [types/output/output.c:734] Basic output test failed for WL-2

This is now handled through wlr_output_management
@joggee-fr joggee-fr force-pushed the jg/wlr-output-management-2 branch 2 times, most recently from d091a94 to c6da86e Compare July 24, 2023 14:30
@joggee-fr
Copy link
Collaborator Author

When disabling an output, we must avoid to set other parameters (scale, mode, etc.) as it will cause the error described in my first comment. Now, with a Wayland backend, we still observe an error but a kind of expected one:
[backend/wayland/output.c:254] Unsupported output state fields: 0x8.

I have also tried with DRM backend. Disabling output is OK but re-enabling next is not yet very stable.

@joggee-fr
Copy link
Collaborator Author

I have also tried with DRM backend. Disabling output is OK but re-enabling next is not yet very stable.

Simply because I forgot Cage terminates when no more output is available.

@joggee-fr joggee-fr marked this pull request as ready for review July 25, 2023 08:22
@emersion
Copy link
Contributor

with a Wayland backend, we still observe an error but a kind of expected one

Right. With wlroots 0.16, only the DRM backend supports disabling outputs. With wlroots 0.17, all backends support disabling outputs.

output.c Show resolved Hide resolved
output.c Outdated Show resolved Hide resolved
output.c Outdated Show resolved Hide resolved
output.c Outdated Show resolved Hide resolved
output.c Show resolved Hide resolved
@joggee-fr joggee-fr requested a review from emersion July 27, 2023 15:12
@joggee-fr
Copy link
Collaborator Author

I have added commits (to be squashed if OK) to answer @emersion comments.
By the way, I have also modified the access to previous or next item in output list causing segfaults when using -m last option.

output.c Outdated Show resolved Hide resolved
@emersion
Copy link
Contributor

Yeah, the changes you made LGTM.

@emersion
Copy link
Contributor

I'm still not super sure about "output: do not terminate when last output is destroyed". It sounds like this is useful mainly when running nested under the X11 and Wayland backends, to stop running when the last cage window is closed.

@joggee-fr
Copy link
Collaborator Author

Squashed some commits and removed extra parentheses.

@joggee-fr
Copy link
Collaborator Author

I'm still not super sure about "output: do not terminate when last output is destroyed". It sounds like this is useful mainly when running nested under the X11 and Wayland backends, to stop running when the last cage window is closed.

I don't have done any tests with X11 backend for now.
When using Wayland backend, how can we destroy output? I understand with next version of wlroots, we will be able to disable outputs, but how about output destruction?

Below a short description of the issue I observed with terminate when last output available is destroyed:

  • Cage started with a monitor plugged in HDMI output. DRM backend is selected, GPU is integrated Intel Iris with i915 kernel driver ;
  • HDMI-A-1 output is disabled using wlr-randr --output HDMI-A-1 --off ;
  • Monitor lost signal and automatically enters a standby mode ;
  • HDMI output is re-enabled using wlr-randr --output HDMI-A-1 --on ;
  • I think it is due to monitor exiting "slowly" from standby but Cage receives a destroy event just before handling again a new HDMI output. Consequently, things are broken if terminating here.

@emersion
Copy link
Contributor

I don't have done any tests with X11 backend for now.

It's fine, it should behave exactly just like the Wayland backend.

When using Wayland backend, how can we destroy output? I understand with next version of wlroots, we will be able to disable outputs, but how about output destruction?

Closing windows should destroy the output, regardless of wlroots version.

Below a short description of the issue I observed with terminate when last output available is destroyed

Yeah. Maybe it would make sense to special-case Wayland and X11.

@joggee-fr
Copy link
Collaborator Author

Closing windows should destroy the output, regardless of wlroots version.

As I understand it (and according to tests), in such a case, whatever the selected backend, Cage receives sigchld_handler() as the started/forked child process has been closed and terminates. Last available output patch does not modify this behavior.

@emersion
Copy link
Contributor

No, I'm talking about closing the wlroots Wayland/X11 backend window. The nested child process is not involved here.

I don't think Cage ever sends a xdg_toplevel.close event to the nested Wayland client.

@emersion
Copy link
Contributor

Come to think of it, maybe it would be better to not force-terminate when the wlroots Wayland/X11 backend window is closed by the user. Instead we could just relay the close request to the nested Wayland client. That way "do you want to save your work before closing?" kind of dialogs would work properly. But this requires some wlroots changes: https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/3589

@joggee-fr
Copy link
Collaborator Author

No, I'm talking about closing the wlroots Wayland/X11 backend window. The nested child process is not involved here.

Ok, now I get it. I have just given it a try with Wayland backend and indeed, Cage is now running without any output available. Kind of weird 😅 .

@emersion
Copy link
Contributor

If you want to get this PR merged, maybe we can leave out that commit for now and fix it in a separate PR?

joggee-fr and others added 2 commits July 28, 2023 15:23
Check output list length before accessing previous or next item to avoid
segfault.
@joggee-fr
Copy link
Collaborator Author

If you want to get this PR merged, maybe we can leave out that commit for now and fix it in a separate PR?

Just dropped the commit from this PR.
Who get rights to merge it (if OK)? @Hjdskes does not seem to be (publicly) active here since several months.

@joggee-fr joggee-fr requested a review from emersion July 28, 2023 13:33
@emersion emersion merged commit b1129ca into cage-kiosk:master Jul 28, 2023
10 checks passed
@emersion
Copy link
Contributor

Thanks!

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