On Thu, Jul 23, 2015 at 05:53:43AM +0200, Maarten Lankhorst wrote:

Op 22-07-15 om 18:02 schreef Daniel Vetter:

...

With drivers supporting runtime pm it's generally not a good idea to touch the hardware when it's off. Add an option to the commit_planes helper to support this case.

Note that the helpers already add all planes on a crtc when a modeset happens, hence plane updates will not be lost if drivers set this to true.

v2: Check for NULL state->crtc before chasing the pointer. Also check both old and new crtc if there's a switch. Finally just outright disallow switching crtcs for a plane if the plane is in active use, on most hardware that doesn't make sense.

Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Thierry Reding treding@nvidia.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/drm_atomic_helper.c | 29 +++++++++++++++++++++++++++-- drivers/gpu/drm/exynos/exynos_drm_fb.c | 2 +- drivers/gpu/drm/msm/msm_atomic.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +- drivers/gpu/drm/sti/sti_drm_drv.c | 2 +- drivers/gpu/drm/tegra/drm.c | 2 +- include/drm/drm_atomic_helper.h | 3 ++- 8 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 99656815641d..2122c2b844da 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -471,6 +471,14 @@ drm_atomic_helper_check_planes(struct drm_device *dev, if (!funcs || !funcs->atomic_check) continue;

• if (plane->state->crtc && plane_state->crtc &&
  

•     plane->state->crtc != plane_state->crtc &&
  

•     plane->state->crtc->state->active) {
  

• 	DRM_DEBUG_ATOMIC("[PLANE:%d] changing crtc while still active\n",
  

• 			 plane->base.id);
  

• 	return -EINVAL;
  

• }
  

• ret = funcs->atomic_check(plane, plane_state); if (ret) { DRM_DEBUG_ATOMIC("[PLANE:%d] atomic driver check failed\n",

@@ -995,7 +1003,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,

drm_atomic_helper_commit_modeset_disables(dev, state);

• drm_atomic_helper_commit_planes(dev, state);

• drm_atomic_helper_commit_planes(dev, state, false);

  drm_atomic_helper_commit_modeset_enables(dev, state);

@@ -1110,10 +1118,16 @@ fail: } EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);

+bool plane_crtc_active(struct drm_plane_state *state) +{

• return state->crtc && state->crtc->state->active;

+}

/**

• drm_atomic_helper_commit_planes - commit plane state
• @dev: DRM device
• @old_state: atomic state object with old state structures

• • @active_only: Only commit on active CRTC if set
  • This function commits the new plane state using the plane and atomic helper
  • functions for planes and crtcs. It assumes that the atomic state has already

@@ -1128,7 +1142,8 @@ EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);

• drm_atomic_helper_commit_planes_on_crtc() instead.

*/ void drm_atomic_helper_commit_planes(struct drm_device *dev,

• 		     struct drm_atomic_state *old_state)
  

• 		     struct drm_atomic_state *old_state,
  

• 		     bool active_only)
  

{ struct drm_crtc *crtc; struct drm_crtc_state *old_crtc_state; @@ -1144,6 +1159,9 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, if (!funcs || !funcs->atomic_begin) continue;

• if (active_only && !crtc->state->active)
  

• 	continue;
  

• funcs->atomic_begin(crtc); }

@@ -1155,6 +1173,10 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, if (!funcs) continue;

• if (active_only && !plane_crtc_active(plane->state) &&
  

•     !plane_crtc_active(old_plane_state))
  

• 	continue;
  

• /*
  • Special-case disabling the plane if drivers support it.
  */

The check is still wrong. It should only check if the new plane_state->crtc is active, if it's not call the disable_plane hook, but only when get_existing_crtc_state(old_plane_state->crtc)->active && !needs_modeset(old_plane_state->crtc->state).

That's why I'm simply disallowing reassigning the plane if it's still in use. If I understand you correct your case is: BEFORE: plane active on CRTC A and CRTC A is active. AFTER: plane active on CRTC B, but CRTC B is not active.

I agree that the correct thing would be to only disable the plane on CRTC A in that case. But if we make it the rule that generic drivers don't support reassigning the plane if it's active (and generic userspace can't rely on that working) then the above just can't happen.

Or is there another case where the above falls short?

The other helper should be a disable_planes helper that blindly calls disable_plane for planes that are on crtc's and require modeset for the crtc on the previous plane_state.

Yeah, I'll try to stitch that helper together. -Daniel

On Mon, Jul 27, 2015 at 02:44:31PM +0200, Thierry Reding wrote:

On Wed, Jul 22, 2015 at 06:02:27PM +0200, Daniel Vetter wrote: [...]

...

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index c6276aebfec3..783edc242648 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -56,7 +56,7 @@ static void tegra_atomic_complete(struct tegra_drm *tegra, */

drm_atomic_helper_commit_modeset_disables(drm, state);

• drm_atomic_helper_commit_planes(drm, state);

• drm_atomic_helper_commit_planes(drm, state, false); drm_atomic_helper_commit_modeset_enables(drm, state);

  drm_atomic_helper_wait_for_vblanks(drm, state);

I tried to give this a go with my ongoing DPMS work and I can't make this work. I think we'll need something more involved to fix this properly with runtime PM enabled drivers.

The reason why this isn't working on Tegra is because with the rework done for DPMS, the CRTC will be put into reset in ->disable() and taken out of reset in ->enable() (eventually I suspect that it'd also be powergated in ->disable() and unpowergated in ->enable(), but that should have no further side-effects than the reset).

There are two cases where this leads to problems, though as far as I can tell they are the same problem: with legacy fbdev enabled, the initial modeset works fine (though that's only because the mode is actually set twice due to the monitor pulsing HPD). If I then run modetest with the same mode set as fbdev, I also see planes updated properly. Now after I exit modetest it will do a modeset (it's what modetest does, no matter if the mode it did set matches fbdev, not sure if that's really desired) and that ends up with no primary plane being set.

The second case where I've seen this happen is with legacy fbdev disabled. In that case, running modetest won't ever display the correct plane. I'm somewhat surprised that it even works, given that the CRTC to which the plane's registers belong is in reset and has clocks disabled at the time of the ->atomic_update() call. I suspect this could be related to the fact that we're accessing only plane registers, and they go through some sort of muxing machinery that may not underly the same clocking and reset restrictions as the other registers. Anyway, the result is that all of the changes done in ->atomic_update() will be lost when the CRTC is enabled, and therefore the only case where the drm_atomic_helper_commit_planes() call works is when the CRTC stays on (essentially what used to be ->mode_set_base()).

Moreover, I'd say with active_only set to true, the core shouldn't even call ->atomic_update() for planes associated with the CRTC because at this point it's still off. drm_atomic_helper_commit_modeset_enables() is what will turn the CRTC on.

So all in all, I think what we need is actually five steps:

1. disable all planes that will no longer be used in the new state

2. disable all CRTCs that will no longer be used in the new state

3. update all planes that have changed from old to new state

4. enable CRTCs that were not used in the old state but are enabled in the new state

5. enable all planes that were not used in the old state but are enabled in the new state

6. and 5) I think could be helpers that are called from drivers directly

in their ->enable() functions.

The downside of the above is that there are a bunch of cases where we'd need to be very careful to preserve atomicity across updates. For planes that are enabled on a CRTC that remains active between two states, they would need to be updated in the same ->atomic_begin()/->atomic_flush() sequence as planes that move or change framebuffer, otherwise the frames won't be perfect.

Similarly I think we'd want a way to allow drivers to atomically enable updates. That is, enabling the output and setting the planes on them should be an atomic operation. Currently depending on how fast the mode is being set you could have a short period where the mode has been applied, but plane updates aren't visible yet. That would essentially mean that 1) & 2) as well as 4) & 5) in the above would need to be collapsed into single steps, at the end of which the CRTC hardware needs to commit all changes at once. I suspect that for most (all?) cases this may not even matter, though. If the CRTC is set up to scan out black if no planes are active, then this should not be noticable.

On i915 we do a slightly different sequence: 1) kill all the planes for crtcs which will be shut down in step 2). Those are all the ones for which needs_modeset is true. A helper to do this could be implemented using the plane_funcs->atomic_disable hook. To make this nicely atomic we should also wrap them with ->atomic_begin and ->atomic_flush. 2) shut down crtcs/encoders using drm_atomic_helper_commit_modeset_disables 3) enable crtcs/encoders with new config using drm_atomic_helper_commit_modeset_enables 4) do atomic plane updates on _all_ active crtc. This is important since if you combine a mode change (which forces the crtc to be disabled in 2 and enabled again in 3) you want to first disable all the planes in 1 and then enable them with the new config in 4. Also if you do a dpms off->on change your hw likely has lost plane state too, so that needs to be restored.

One consequence is that step 3 only enables the pipe itself with no planes displaying at all, even if there are planes enabled on a given crtc. That's only done in step 4.

Another consequence is that if you want to switch planes between CRTCs and there's no modeset involved then that would need to be done in step 4. But since CRTCs usually aren't gen-locked you can't do that atomically (in general at least). I'd just forbid CRTC switching for planes when the original CRTC is still active completely, probably not a corner-case we'll ever care about. I tried to implement that in the revised patch version but I think I failed.

In that sequence there's no step 3 from your sequence, that's folded in with the atomic updates in step 4. That means if you do a flip-only on one crtc and a full modeset on another the flip will be unecessarily stalled, but that's what you get for piling things up ;-)

Note that my patch only tries to implement step 4) in the above sequence, step 1 is still tbd. And as Maarten pointed out I failed at implementing step 4 correctly for all corner cases. -Daniel