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.
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.
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
On Thu, Nov 6, 2014 at 4:43 AM, Daniel Vetter daniel@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.
- 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..
- 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..
- 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
- 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.
- 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.
- 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Nov 6, 2014 at 5:06 PM, Rob Clark robdclark@gmail.com wrote:
On Thu, Nov 6, 2014 at 4:43 AM, Daniel Vetter daniel@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.
- 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.
- 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..
- 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:
- valid mode is set
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
- 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.
- 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.
- 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Nov 6, 2014 at 11:35 PM, Sean Paul seanpaul@chromium.org wrote:
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?
Oh, I missed this bit a bit. We can't disable the output on the first dpms off for a connector, since if there's 1 connector left on stuff like vblank waits or page flips need to keep working. And maybe there's crazy userspace out there which needs that to not die in a racecondition. But keeping the screen running for a bit longer will not be noticeable since all compositors always do dpms on all outputs anyway. So from a "what does the user see" pov disabling when the first or last cloned connector get dpms off set doesn't matter, but from an userspace pov there's a difference.
All this only matters for cloned configs anyway only, which is about i915 for gen2, roughly. Well Ville implemented cloning for some more recent stuff too. -Daniel
On Fri, Nov 7, 2014 at 12:33 AM, Daniel Vetter daniel@ffwll.ch wrote:
All this only matters for cloned configs anyway only, which is about i915 for gen2, roughly. Well Ville implemented cloning for some more recent stuff too.
Ok, I've grepped: Beside i915 there's only radeon which has a possible_clones setting which isn't totally bogus. Everyone else either sets nonsense, 0 (which is also nonsense, the own encoder bit should be in there), nothing (which means 0 because kzalloc) or a complete lie like exynos claiming to be able to clone encoders between fimd and mixer.
So for everyone else: I simply propose to clamp dpms to on/off and move it to the crtc as a new boolean called "active". All the other talk is only interesting if you have cloned configs in hw (i.e. 1 crtc, 2 outputs from the same crtc).
Another (rather quick chase, haven't followed call-chains fully really) audit shows that besides i915 no one has a disdinction between crtc enable and active tracking. At least I haven't found how radeon tracks shared dplls even, that one should have some checks though since they have 5+ output cards with just 2-3 dplls. So at least right now not a lot of drivers which bother with even handling this. -Daniel
- 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..
I guess I need to extend a bit here. My idea is to track the dpms state (all 4 states) in the connector state. But that is only to keep up the illusion for the legacy dpms interface. The real state would be the active boolean in the crtc. And I think that would also be the one we'd expose in the atomic ioctl.
And yeah that would all be in an atomic helper to implement legacy dpms on top of atomic, so drivers could overwrite this if needed.
- 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:
- valid mode is set
seems like the existing problem w/ enabled is just to do with how current helpers use it.. but maybe I'm overlooking something..
Well 3) is imo wrong, since black screen with no fbs is imo a valid state (hopefully only transitional). And the core/helper will ensure that 1&2 never get out of sync (you won't get an -EINVAL though since existing userspace does stupid stuff that violates this, the core setCrtc ioctl cleans that up though).
That aside I guess I need to elaborate on what makes dpms special in i915, and why there's a real difference between crtc->enable == true && ->active == false and crtc->enable == false in i915. For complex configs we do resource checking (shared dplls) and that's done in the modeset. For a pipe which has been disabled just with dpms we then guarantee that we'll keep these resources reserve and so will always be able to enable the pipe again. If you disable the pipe completely (i.e. set crtc->enable to false) we'll release these resources. E.g. in the i915 share dpll code we have both an active refcount (does the ppl need to be running) and a reference mask (which crtc is referencing this pll, even when the crtc is disabled with dpms).
Now atomic has less of an issue with that since a given config should always keep working. Well, until you have multi-seat and the other compositor might have stolen some shared resources meanwhile. So I still think a separate knob to set the hw state and make the hw state independent from reserving required resources is rather useful.
Note that even with the old crtc helper code there's a difference between dpms and set_config(NULL): crtc->mode_set can fail and is only called from set-config, not from the dpms code. Before the rewrite i915 used that to handle shared dplls and failing the modeset if we couldn't find a free dpll. I don't know whether there's any other driver though that uses this facility.
I hope that explains why I want to keep separate enable and active crtc states and don't just want to completely remove dpms from drivers. Note that for the actual driver backend there will be no difference at all, since my plan is that the atomic helpers would implement exactly your idea. So drivers using the crtc+plane helper vfuncs wouldn't care at all about all this. The overall point is to enforce consistency, and that's best done with no driver specific code at all ;-) -Daniel
On Thu, Nov 6, 2014 at 6:29 PM, Daniel Vetter daniel@ffwll.ch wrote:
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:
- valid mode is set
seems like the existing problem w/ enabled is just to do with how current helpers use it.. but maybe I'm overlooking something..
Well 3) is imo wrong, since black screen with no fbs is imo a valid state (hopefully only transitional). And the core/helper will ensure that 1&2 never get out of sync (you won't get an -EINVAL though since existing userspace does stupid stuff that violates this, the core setCrtc ioctl cleans that up though).
(hmm, I wonder whether that is even possible on all hw..)
That aside I guess I need to elaborate on what makes dpms special in i915, and why there's a real difference between crtc->enable == true && ->active == false and crtc->enable == false in i915. For complex configs we do resource checking (shared dplls) and that's done in the modeset. For a pipe which has been disabled just with dpms we then guarantee that we'll keep these resources reserve and so will always be able to enable the pipe again. If you disable the pipe completely (i.e. set crtc->enable to false) we'll release these resources. E.g. in the i915 share dpll code we have both an active refcount (does the ppl need to be running) and a reference mask (which crtc is referencing this pll, even when the crtc is disabled with dpms).
ahh, ok, "reserved but not enabled" makes a lot more sense.. that was the distinction that I was missing. That probably deserves to be in headerdoc somewhere..
BR, -R
On Thu, 6 Nov 2014 19:35:51 -0500 Rob Clark robdclark@gmail.com wrote:
On Thu, Nov 6, 2014 at 6:29 PM, Daniel Vetter daniel@ffwll.ch wrote:
That aside I guess I need to elaborate on what makes dpms special in i915, and why there's a real difference between crtc->enable == true && ->active == false and crtc->enable == false in i915. For complex configs we do resource checking (shared dplls) and that's done in the modeset. For a pipe which has been disabled just with dpms we then guarantee that we'll keep these resources reserve and so will always be able to enable the pipe again. If you disable the pipe completely (i.e. set crtc->enable to false) we'll release these resources. E.g. in the i915 share dpll code we have both an active refcount (does the ppl need to be running) and a reference mask (which crtc is referencing this pll, even when the crtc is disabled with dpms).
ahh, ok, "reserved but not enabled" makes a lot more sense.. that was the distinction that I was missing. That probably deserves to be in headerdoc somewhere..
A rename would be nice too; it's very misleading. Though with a move to a boolean DPMS internal state, it should be possible to drop it and just re-alloc all the resources on DPMS on (iow treat both DPMS off and on as mode sets). But that's not something that should block these changes by any means.
Jesse
On Thu, Nov 6, 2014 at 1:43 AM, Daniel Vetter daniel@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.
- 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.
- 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.
- 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.
- 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.
- 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
- 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Nov 7, 2014 at 12:31 AM, Stéphane Marchesin stephane.marchesin@gmail.com wrote:
On Thu, Nov 6, 2014 at 1:43 AM, Daniel Vetter daniel@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.
- 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.
- 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.
- 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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jesse Barnes -
Rob Clark -
Sean Paul -
Stéphane Marchesin