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 at linux.intel.com>
> > Cc: Thierry Reding <treding at nvidia.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart+renesas at ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
