On Thu, Nov 6, 2014 at 5:06 PM, Rob Clark <robdclark@xxxxxxxxx> wrote: > On Thu, Nov 6, 2014 at 4:43 AM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
