[RFC] dpms handling on atomic drivers

[Sean Paul]

On Thu, Nov 6, 2014 at 5:06 PM, Rob Clark <robdclark at gmail.com> wrote:
> 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..
>

The only plausible reason that I can come up with is that it might be
useful to communicate whether the driver is going to be suspended vs
off to panels & bridges.

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

Maybe we could just fail the atomic check in this case?

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

This seems very reasonable to me. Addition by subtraction.

Sean


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