[RFC] dpms handling on atomic drivers

[Daniel Vetter]

On Fri, Nov 7, 2014 at 12:31 AM, Stéphane Marchesin
<stephane.marchesin at gmail.com> wrote:
> On Thu, Nov 6, 2014 at 1: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.
>
>
> One thing that has been discussed in the past is how to send hdmi
> audio while in dpms off state. On some platforms we've been abusing
> these extra dpms levels. Overall it would be useful to have a way to
> do that.

At least intel hw can't actually do it any more (i.e. we simply don't
have these dpms levels). But with universal planes and atomic you
could set a mode with no planes. That's about as little power as you
can get to shovel the audio over the wire in the vblanks.

>> 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.
>
>
> That pretty much means completely automated crtc enable/disable in the
> drm core then? That sounds great, but we should be careful with
> drivers relying on vblank interrupts coming in (for example waiting
> for vblank during modeset would fail if the crtc is disabled). I guess
> if that's an issue you can work around it on the driver side.

Not in the core so that drivers can still overwrite it, but in the
atomic helper code. But yeah, driver backends would not need to care
at all. Wrt the vblank stuff the other point for having crtc->active
is to finally know when a vblank wait should be possible - atm my
helpers have to test that by simplying trying a drm_vblank_get and
skipping the vblank wait if that call fails. So yeah I also want to
lock down and unify the rules about that while at it.

> Another thing which could potentially break is that X tracks (and
> caches) the crtc dpms state.

See my other mail, but the entire point of keeping dpms at the
connector is to have a 100% faithful lie for old userspace ready to
return when it asks. And to make sure we don't get out of sync with
whatever userspace thinks it asked the kernel for. Note that there's
still the issue that set_config does an implicit dpms on, and we've
had lots of hilarity in the past with userspace getting confused by
that. But that's always been like that, so that mess can't be fixed
any more. Robust non-atomic userspace simply must re-read dpms state
after each setCrtc.
_Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch