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

[Stéphane Marchesin]

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.


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

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


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


+1, that's making the de facto standard explicit.

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


+1, here too it makes sense to make the de facto behavior a standard

Stéphane

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