[RFC] dpms handling on atomic drivers

[Rob Clark]

On Thu, Nov 6, 2014 at 4:43 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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.

given that I haven't seen any hw that actually has any sort of analog
output (without some external bridge chip) in quite some time, this
seems perfectly ok with me.  So I might be the wrong one to ask.. the
person out there using CRTs to warm there office in the winter might
be upset... http://xkcd.com/1172/

If we do try to preserve the intermediate states, then helpers clamp
to on/off and if someone wants to keep intermediate states in some
driver can skip the helpers?  But having a hard time convincing myself
it is worth keeping that dinosaur alive..

> 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.

Well, it isn't quite the behavior I would expect..  at least not if
atomic ioctl has a "preserve properties not explicitly set" mode.  I
would expect one of the displays to turn off.  Or at least go black
(but preferably off..)

we could move the property to the encoder with only two states
(on/off) and somehow emulate old dpms w/ some helpers to do a full
modeset if/when needed?  Not really sure if that helps anything..

> 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.

hmm.. then I'm starting to get a bit fuzzy on the distinction between
enabled and active..  I guess I should go back and look more closely
at the differentiation between 'enabled' and 'active' in current i915
helpers..

Seems like we should be able to just repurpose enabled in the new
helpers, to not automatically be true when a mode is set, but instead
be true iff:
  1) >=1 attached connector is ON
  2) valid mode is set
  3) >=1 attached plane has an fb

seems like the existing problem w/ enabled is just to do with how
current helpers use it.. but maybe I'm overlooking something..

BR,
-R

> 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
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel