Add a disable method to DrmSurface/DrmCompositor for implementing DPMS
#1561
Conversation
This field is currently set, but never read. This logically doesn't need to be part of `state`/`pending` either, since a full commit is not needed to change the DPMS state.
| .for_each(|(_, state)| *state = Default::default()); | ||
| self.pending_frame = None; | ||
| self.queued_frame = None; | ||
| self.next_frame = None; |
There was a problem hiding this comment.
Ping @cmeissl
Does this look fine to you?
There was a problem hiding this comment.
This depends on how it should behave, this should be fine for disabling alone, yes.
We could also reset the element states to drop all framebuffers we imported, this might allow to lower
the memory footprint. Same for the swapchain which could be reset.
|
I will do a review Sunday, latest Monday when I am back home. |
There was a problem hiding this comment.
The whole thing looks more like a reset/clear to me. Disable suggests that the surface will be disabled until some non-existing enable imo.
I would expect it to track and handle the disable state internally, potentially returning errors for some functions like render_frame.
In its current state tracking still has to be done externally, which is fine imo, but maybe not expected from the name disable.
src/backend/drm/surface/mod.rs
Outdated
| @@ -441,6 +441,17 @@ impl DrmSurface { | |||
| DrmSurfaceInternal::Legacy(surf) => &surf.span, | |||
| } | |||
| } | |||
|
|
|||
| /// Disable the output, setting DPMS state to off. | |||
There was a problem hiding this comment.
imo the name disable might be misleading, or at lest the comment might be. it does not only set the dpms state, but also clears the whole state including all planes.
| .for_each(|(_, state)| *state = Default::default()); | ||
| self.pending_frame = None; | ||
| self.queued_frame = None; | ||
| self.next_frame = None; |
There was a problem hiding this comment.
This depends on how it should behave, this should be fine for disabling alone, yes.
We could also reset the element states to drop all framebuffers we imported, this might allow to lower
the memory footprint. Same for the swapchain which could be reset.
|
Yeah, I wasn't really sure what to name it. For disabling the screen with DPMS when the session is inactive, saving memory isn't particularly the goal (unless something also saves power). Though different implementations seem a little inconsistent in what they set when setting the DPMS Perhaps there are other use cases for something like this, but I don't know exactly what function(s) would be useful for them. |
This sets DPMS state to off. Submitting a new frame will re-enable the output.
|
I've renamed the function to |
As discussed in #1553.
This seems to be working okay, on atomic or legacy DRM. I'm still finishing the cosmic-comp support.
Not sure if
DrmCompositor::disableshould clear any other state. Or ifAtomicDrmSurface::clear_statedoes anything that is unneeded here. Other compositors appear to vary slightly in which properties their atomic backends set/unset for DPMS.