swaywm / wlroots

A modular Wayland compositor library
https://gitlab.freedesktop.org/wlroots/wlroots/
MIT License
2.15k stars 343 forks source link

Decouple DPMS from wl_output globals #568

Closed Ongy closed 6 years ago

Ongy commented 6 years ago

Sorry, missed it before, but https://github.com/swaywm/wlroots/pull/549 does a something I feel like it shouldn't.

So this PR couples the wl_outupt globals to the enabled/disabled state of the output. This state also determines the DPMS state on DRM backend. IMO this isn't the best idea. The general hierarchy of output states I can think of is roughly:

IIRC the KDE protocol for setting this up had a custom information protocol attached as well, which could be used for the disabled state. Then the wl_output could be bound to the wlr_output_layout and have the disabled state controlled by the compositor removing the output from the layout.

emersion commented 6 years ago

I don't see why clients should be aware of sleeping outputs. That would break the screenshooter. Also, the wlr_output is not aware weather it's sleeping or disabled.

KDE has another interface (outputdevice) for outputs not exported as wl_outputs.

Ongy commented 6 years ago

I'd consider the screenshooter more of an edgecase here. I don't know the protocol, is there no way to tell the application it cant get this output? I'd prefer (and expect) other things that interact with wl_output not to react to powersave features. And there are more things that interact with wl_output e.g. xdg-shell fullscreen, I would expect surface_layers and probably others. (pointer constraints? Mapping drawing tablets to areas?)

And sure, I would bind the sleep/disabled states here (I just made the names up) to whether the output is in the wlr_output_layout or not. If that doesn't make sense/wouldn't work with wlroots, that's nothing important, just my current idea.

Ongy commented 6 years ago

One interaction that can be observed with what we currently have: Set an output-scale >1 Let the surface enter the output. Cycle DPMS (on->off->on). This destroys the output and creates a new one Set the output-scale to something different.

The surface will not associate the event on the new output with the output it originally entered. Neither will it associate any leaves that may occure later on with said output.

This could be fixed on the compositor side by going through all surfaces on the output and sending enter/leave events.

ddevault commented 6 years ago

I think we need a detailed spec of the entire output state machine and should change the codebase to be consistent with it.

Ongy commented 6 years ago

This is the current state of things. I did propose to change the wl_output handling. That would either give the compositor authority over the global or bind it to the output_layout (expecting the compositor to set up the output before it's added).

Going through this, I think it's also created too early. At this point position/scale/transform/mode can all be bogus.

output created/discoverd (wlroots)

1) output is enabled (wl_output global created) 2) backend.events.output_add is emitted Compositor is expected to set up output for IPC (swaymsg/keybinds etc.)

output livecycle (compositor)

wlroots will emit events to clients for any of these

output destroyed

The compositor should hook the frame callback either on create or hook/unhook on enable events.

There's also a bit more around rendering/cursors but I think https://github.com/swaywm/wlroots/pull/571 changes that a little (damage events/tracking damage is new) should I add that?

ddevault commented 6 years ago

Also want to know how hotplugging is involved.

DPMS disable should not remove the wl_global

Ongy commented 6 years ago

Hotplug triggers transition from limbo (unplugged/non-existant) to/from livecycle over the respective way.

Hotpluginin will create a wlr_output and go over what I called created/discoverd. Then it's up to the compositor.

Hotunplug will clean up internal state emit the output_remove signal on the backend.

ddevault commented 6 years ago

Sounds correct to me.

emersion commented 6 years ago

So the only change to do is to expose functions to create/destroy the output global right? (And don't do it when enabling/disabling the output)