Hi all,
After a few atomic irc chats I've shockingly realized that I've completely ignored dpms handling in my helper series. Oops.
But there's a few things which are seriously wrong with DPMS, so I've figured I'll start a discussion about them first - converting to atomic looks like a good time to fix up past mistakes, simplify drivers and make the interface exposed to userspace more consistent.
1. Intermediate dpms levels
DPMS standby/suspend essentially ceased to be useful years ago: Intel doesn't ship hardware which supports them since a few years, and analog CRTs (the only thing that cares really) are also dying. And these intermediate levels cause quite a bit of pain in drivers since depending upon the level and chip/output the pipe needs to be shut off or kept running, but with black output. That means more possible state transitions and invariable more bugs.
Proposal: Just clamp dpms standy/suspend to off in the atomic implementation of dpms.
2. Cloned configurations
Currently dpms is set per-connector, and the crtc helpers only shut down the specific encoder. Only when all connectors are off will it shut down the crtc, too. That pretty much defeats the point of the new helpers of always enabling/disabling a given output pipeline holesale and in the same order. i915 modeset tries to have the cake and eat it with some clever bit-frobbing on the encoder (to select black output when needed) but otherwise only enable/disable the entire pipe. Imo that was stupid and not worth the complexity at all, furthermore it needs changes in all drivers to implement. Especially since X doesn't care about per-connector dpms. And even for multi-seat dpms will switch only per-crtc.
Proposal: Only shut down anything (and then the hole output pipe with all cloned outputs) when all connector's dpms property is set to off. And enable it again as soon as one property goes to on.
3. No internal representation of dpms state
Because of the above nonsense it's essentially not possible to find out in a generic way whether the crtc is actually on. Which means that no generic (core or helper code) can figure out whether e.g. vblanks still happen. In the atomic helpers I just do a drm_vblank_get and look at the return value to figure out whether the crtc is on or not. This is one giant mess.
Proposal. Add a new boolean ->active to the crtc state, which will track the dpms state of the crtc. Add a helper to the atomic core functions which will compute ->active from the state update according to the proposals for issue 1&2. Require that all callers of ->atomich_check/commit update ->active properly and require implementations to obey it. That means the atomic helpers will switch from looking at ->enable to looking at ->active to decided whether a crtc an all its outputs should be enabled or not.
4. Modesets always force dpms on
This is essentially a side-effect of how the crtc helpers have been implemented. Except that for a very long time the sw side tracking wasnt ever updated, resulting in lots of hilarity in all the drivers that checked the dpms state somewhere and got confused. Or userspace which also somehow believed the dpms state has any match with reality.
Proposal: Enforce the informal rule that after a legacy ->set_config call all connectors should be in dpms on. We should probably do this in the drm core right after having called ->set_config to catch all possible implementations. For the actual atomic interface the solution for issue 3. will allow us to separate these two things and userspace to always know what's going on.
5. Inconsistent return values for vblank waits/page flips
Becuase of issues 1-3 the core can't reject vblank waits or async page flips. Which means there's no consistency at all about when such an ioctl will work and when it will be rejected (because the pipe is off).
Proposal: Use the new ->active state from issue 3 and implement in the core what i915 currently does. Which means reject vblank waits and page flip ioctls when ->active == false.
For the atomic interface I think we should reject it if userspace asks for an event or async commit and neither the old nor new ->active state for that crtc (if it's an event) or any crtc (async commit) is true. Updating a plane on a disabled pipe really tends to be a programming bug.
6. Existing userspace might rely on driver specific behaviour
That would be a bummer, but we can work around this by doing the new checks in the atomic helpers instead of the core. And since the atomic helpers allow drivers to augment them or completely replace them for specific legacy entry points we should always be able to keep perfect backwards compatibility. And of course drivers not yet converted to atomic will stay unchanged - the proposed solution for issue 3 needs the new crtc state to be there and properly in sync.
Proposal: Add the checks to the core for now, to aim for maximal consistency. But if there's a problem with existing userspace (most likely driver-specific X drivers) we can push them into helpers where needed.
That's it I think, with the above changes we should have fairly sane dpms handling for atomic drivers. And a lot less accidental complexity to deal with in driver backends - they can essentially rip out all their ->dmps callbacks and go with the much simpler ->enable/disable hooks only.
Comments, ideas and feedback highly welcome. Most important is there anything I've missed?
Cheers, Daniel