[Intel-gfx] [RFC] dpms handling on atomic drivers

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch