Smithay / smithay

A smithy for rusty wayland compositors
MIT License
1.9k stars 168 forks source link

DPMS Support #1553

Open ids1024 opened 1 month ago

ids1024 commented 1 month ago

https://github.com/pop-os/cosmic-comp/pull/741 has code setting the DPMS connector property to disable monitors. But the DPMS property is deprecated, not supported on atomic mode-setting, and for some reason setting it seems to take .5 to 1 second.

Legacy property for setting the power state of the connector. For atomic drivers this is only provided for backwards compatibility with existing drivers, it remaps to controlling the “ACTIVE” property on the CRTC the connector is linked to. Drivers should never set this property directly, it is handled by the DRM core by calling the drm_connector_funcs.dpms callback. For atomic drivers the remapping to the “ACTIVE” property is implemented in the DRM core.

Note that this property cannot be set through the MODE_ATOMIC ioctl, userspace must use “ACTIVE” on the CRTC instead.

https://www.kernel.org/doc/html/latest/gpu/drm-kms.html

I think to handle DPMS properly, it should be handled as part of the atomic commit within Smithay.

Apparently toggling the ACTIVE property of a CRTC should always succeed, if no other properties are changed. We can simply set ACTIVE to false, and then set it on again when we want to show things on screen again. (But not try to do more page flips with ACTIVE off, which errors.)

I think though that we also want to release buffers used in direct scanout? So probably also a commit removing planes...

The compositor will also want to stop sending frame callbacks or throttle their rate, however it normally handles surfaces that aren't visible. But that doesn't require Smithay changes.

I guess if I right about how this should work, and there aren't other considerations I'm missing, the question is how to fit this in the API of DrmCompositor...

ids1024 commented 1 month ago

https://gitlab.freedesktop.org/wlroots/wlroots/-/blob/master/backend/drm/atomic.c?ref_type=heads#L412-472

Wlroots does seem to disable planes when it sets ACTIVE to false.

https://gitlab.gnome.org/GNOME/mutter/-/blob/main/src/backends/native/meta-kms-impl-device-atomic.c?ref_type=heads#L390-470

Mutter seems to set MODE_ID to 0 when it sets ACTIVE to 0? I don't see anything handling active in process_plane_assignment, so I don't know if it's doing anything different with planes when active is false.

Drakulix commented 1 month ago

I wonder if the easiest solution would be to just drop the DrmSurface.

Looking at DrmSurface (or rather LegacyDrmSurface's and AtomicDrmSurface's code) it already handles setting ACTIVE or DPMS (depending on the underlying api) by forcing the crtc to be enabled on first commit and disable it again on drop.

(Slightly off-topic: This might contribute to some of the black screens we see on switching from cosmic-greeter to cosmic-session, perhaps we should make this behaviour configurable just like disabling all connectors on DrmDevice creation...)

With the current api this obviously means also to drop DrmCompositor and thus invalidating all of it's state. This is problematic because of Plane-Claims, which we might want to keep active while only temporarily disabling a display. In generally this seems undesirable from an api perspective.

So naively we might want to additionally introduce an api to "turn off" the underlying DrmSurface, that just internally drops the DrmSurface inside DrmCompositor + invalidating all the state necessary?

The annoying part of this proposal would be adding a new DrmSurface, as fundamentally the compositor needs to provide that, given you need access to the DrmDevice to create it.

So I would propose to introduce api for DrmSurface to do these same steps internally and just pass that through the DrmCompositor, re-enabling the crtc automatically on the next commit.

ids1024 commented 1 month ago

So naively we might want to additionally introduce an api to "turn off" the underlying DrmSurface, that just internally drops the DrmSurface inside DrmCompositor + invalidating all the state necessary?

Yeah, something like that is probably best. Something that sets ACTIVE to 0 and removes planes (so we're not referencing direct scanout buffers in DRM that may be released in Wayland). The wrapper in DrmCompositor can also clear some of it's own state, and make sure it's not holding references to the WlBuffer.

I think that should work...