| Message ID | 1441706527-22048-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 08, 2015 at 12:02:07PM +0200, Daniel Vetter wrote: > 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. > > v3: Since commit_planes(active_only = true) is for enabling things > only after all the crtc are on we should only look at the new crtc to > decide whether to call the plane hooks - if the current CRTC isn't on > then skip. If the old crtc (when moving a plane) went down then the > plane should have been disabled as part of the pipe shutdown work > already. For which there's currently no helper really unfortunately. > Also move the check for wether a plane gets a new CRTC assigned while > still in active use out of this patch. > > v4: Rebase over exynos changes. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Thierry Reding <treding@nvidia.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 | 20 ++++++++++++++++++-- > drivers/gpu/drm/exynos/exynos_drm_drv.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_drv.c | 2 +- > drivers/gpu/drm/tegra/drm.c | 2 +- > include/drm/drm_atomic_helper.h | 3 ++- > 8 files changed, 26 insertions(+), 9 deletions(-) Reviewed-by: Thierry Reding <treding@nvidia.com> Tested-by: Thierry Reding <treding@nvidia.com>
Hi Daniel, Thank you for the patch. On Tuesday 08 September 2015 12:02:07 Daniel Vetter wrote: > 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. > > v3: Since commit_planes(active_only = true) is for enabling things > only after all the crtc are on we should only look at the new crtc to > decide whether to call the plane hooks - if the current CRTC isn't on > then skip. If the old crtc (when moving a plane) went down then the > plane should have been disabled as part of the pipe shutdown work > already. For which there's currently no helper really unfortunately. > Also move the check for wether a plane gets a new CRTC assigned while > still in active use out of this patch. > > v4: Rebase over exynos changes. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Thierry Reding <treding@nvidia.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 | 20 ++++++++++++++++++-- > drivers/gpu/drm/exynos/exynos_drm_drv.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_drv.c | 2 +- > drivers/gpu/drm/tegra/drm.c | 2 +- > include/drm/drm_atomic_helper.h | 3 ++- > 8 files changed, 26 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c index 9b0c47690ae8..12c25c54309f > 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1037,7 +1037,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); > > @@ -1146,10 +1146,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 Following our recent discussion on the subject, I believe some more documentation would be helpful in the form of guidelines for driver developers. Most drivers using the atomic helpers shouldn't need to be notified of plane state update for disabled CRTC, as the DRM core will record the plane state and apply it when the CRTC gets enabled. > @@ -1164,7 +1170,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; > @@ -1180,6 +1187,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, old_crtc_state); > } > > @@ -1191,6 +1201,9 @@ void drm_atomic_helper_commit_planes(struct drm_device > *dev, if (!funcs) > continue; > > + if (active_only && !plane_crtc_active(plane->state)) > + continue; > + > /* > * Special-case disabling the plane if drivers support it. > */ > @@ -1210,6 +1223,9 @@ void drm_atomic_helper_commit_planes(struct drm_device > *dev, if (!funcs || !funcs->atomic_flush) > continue; > > + if (active_only && !crtc->state->active) > + continue; > + > funcs->atomic_flush(crtc, old_crtc_state); > } > } > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c > b/drivers/gpu/drm/exynos/exynos_drm_drv.c index c1ed308496a2..632e21e0d1be > 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > @@ -105,7 +105,7 @@ static void exynos_atomic_commit_complete(struct > exynos_atomic_commit *commit) atomic_inc(&exynos_crtc->pending_update); > } > > - drm_atomic_helper_commit_planes(dev, state); > + drm_atomic_helper_commit_planes(dev, state, false); > > exynos_atomic_wait_for_commit(state); > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c > b/drivers/gpu/drm/msm/msm_atomic.c index 1ceb4f22dd89..7eb253bc24df 100644 > --- a/drivers/gpu/drm/msm/msm_atomic.c > +++ b/drivers/gpu/drm/msm/msm_atomic.c > @@ -125,7 +125,7 @@ static void complete_commit(struct msm_commit *c) > > 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); > > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c > b/drivers/gpu/drm/omapdrm/omap_drv.c index e888a831dd3c..0ecce79fc782 > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.c > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c > @@ -96,7 +96,7 @@ static void omap_atomic_complete(struct > omap_atomic_state_commit *commit) dispc_runtime_get(); > > drm_atomic_helper_commit_modeset_disables(dev, old_state); > - drm_atomic_helper_commit_planes(dev, old_state); > + drm_atomic_helper_commit_planes(dev, old_state, false); > drm_atomic_helper_commit_modeset_enables(dev, old_state); > > omap_atomic_wait_for_completion(dev, old_state); > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 56518eb1269a..ca12e8ca5552 > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > @@ -456,7 +456,7 @@ static void rcar_du_atomic_complete(struct > rcar_du_commit *commit) /* Apply the atomic update. */ > drm_atomic_helper_commit_modeset_disables(dev, old_state); > drm_atomic_helper_commit_modeset_enables(dev, old_state); > - drm_atomic_helper_commit_planes(dev, old_state); > + drm_atomic_helper_commit_planes(dev, old_state, false); > > drm_atomic_helper_wait_for_vblanks(dev, old_state); > > diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c > index 6f4af6a8ba1b..9f85988a43ce 100644 > --- a/drivers/gpu/drm/sti/sti_drv.c > +++ b/drivers/gpu/drm/sti/sti_drv.c > @@ -59,7 +59,7 @@ static void sti_atomic_complete(struct sti_private > *private, */ > > 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); > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c > index e7759a7b9f2d..9ba8a988a030 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); > diff --git a/include/drm/drm_atomic_helper.h > b/include/drm/drm_atomic_helper.h index 11266d147a29..4ffe9dca07c4 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -55,7 +55,8 @@ void drm_atomic_helper_commit_modeset_enables(struct > drm_device *dev, int drm_atomic_helper_prepare_planes(struct drm_device > *dev, > struct drm_atomic_state *state); > void drm_atomic_helper_commit_planes(struct drm_device *dev, > - struct drm_atomic_state *state); > + struct drm_atomic_state *state, > + bool active_only); > void drm_atomic_helper_cleanup_planes(struct drm_device *dev, > struct drm_atomic_state *old_state); > void drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state > *old_crtc_state);
On Tue, Sep 08, 2015 at 01:10:58PM +0200, Thierry Reding wrote: > On Tue, Sep 08, 2015 at 12:02:07PM +0200, Daniel Vetter wrote: > > 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. > > > > v3: Since commit_planes(active_only = true) is for enabling things > > only after all the crtc are on we should only look at the new crtc to > > decide whether to call the plane hooks - if the current CRTC isn't on > > then skip. If the old crtc (when moving a plane) went down then the > > plane should have been disabled as part of the pipe shutdown work > > already. For which there's currently no helper really unfortunately. > > Also move the check for wether a plane gets a new CRTC assigned while > > still in active use out of this patch. > > > > v4: Rebase over exynos changes. > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Thierry Reding <treding@nvidia.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 | 20 ++++++++++++++++++-- > > drivers/gpu/drm/exynos/exynos_drm_drv.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_drv.c | 2 +- > > drivers/gpu/drm/tegra/drm.c | 2 +- > > include/drm/drm_atomic_helper.h | 3 ++- > > 8 files changed, 26 insertions(+), 9 deletions(-) > > Reviewed-by: Thierry Reding <treding@nvidia.com> > Tested-by: Thierry Reding <treding@nvidia.com> Applied to drm-misc, thanks for the review. -Daniel
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 9b0c47690ae8..12c25c54309f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1037,7 +1037,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); @@ -1146,10 +1146,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 @@ -1164,7 +1170,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; @@ -1180,6 +1187,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, old_crtc_state); } @@ -1191,6 +1201,9 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, if (!funcs) continue; + if (active_only && !plane_crtc_active(plane->state)) + continue; + /* * Special-case disabling the plane if drivers support it. */ @@ -1210,6 +1223,9 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, if (!funcs || !funcs->atomic_flush) continue; + if (active_only && !crtc->state->active) + continue; + funcs->atomic_flush(crtc, old_crtc_state); } } diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index c1ed308496a2..632e21e0d1be 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -105,7 +105,7 @@ static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit) atomic_inc(&exynos_crtc->pending_update); } - drm_atomic_helper_commit_planes(dev, state); + drm_atomic_helper_commit_planes(dev, state, false); exynos_atomic_wait_for_commit(state); diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 1ceb4f22dd89..7eb253bc24df 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -125,7 +125,7 @@ static void complete_commit(struct msm_commit *c) 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); diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index e888a831dd3c..0ecce79fc782 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -96,7 +96,7 @@ static void omap_atomic_complete(struct omap_atomic_state_commit *commit) dispc_runtime_get(); drm_atomic_helper_commit_modeset_disables(dev, old_state); - drm_atomic_helper_commit_planes(dev, old_state); + drm_atomic_helper_commit_planes(dev, old_state, false); drm_atomic_helper_commit_modeset_enables(dev, old_state); omap_atomic_wait_for_completion(dev, old_state); diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 56518eb1269a..ca12e8ca5552 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -456,7 +456,7 @@ static void rcar_du_atomic_complete(struct rcar_du_commit *commit) /* Apply the atomic update. */ drm_atomic_helper_commit_modeset_disables(dev, old_state); drm_atomic_helper_commit_modeset_enables(dev, old_state); - drm_atomic_helper_commit_planes(dev, old_state); + drm_atomic_helper_commit_planes(dev, old_state, false); drm_atomic_helper_wait_for_vblanks(dev, old_state); diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c index 6f4af6a8ba1b..9f85988a43ce 100644 --- a/drivers/gpu/drm/sti/sti_drv.c +++ b/drivers/gpu/drm/sti/sti_drv.c @@ -59,7 +59,7 @@ static void sti_atomic_complete(struct sti_private *private, */ 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); diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index e7759a7b9f2d..9ba8a988a030 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); diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 11266d147a29..4ffe9dca07c4 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -55,7 +55,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, int drm_atomic_helper_prepare_planes(struct drm_device *dev, struct drm_atomic_state *state); void drm_atomic_helper_commit_planes(struct drm_device *dev, - struct drm_atomic_state *state); + struct drm_atomic_state *state, + bool active_only); void drm_atomic_helper_cleanup_planes(struct drm_device *dev, struct drm_atomic_state *old_state); void drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state);
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. v3: Since commit_planes(active_only = true) is for enabling things only after all the crtc are on we should only look at the new crtc to decide whether to call the plane hooks - if the current CRTC isn't on then skip. If the old crtc (when moving a plane) went down then the plane should have been disabled as part of the pipe shutdown work already. For which there's currently no helper really unfortunately. Also move the check for wether a plane gets a new CRTC assigned while still in active use out of this patch. v4: Rebase over exynos changes. Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Thierry Reding <treding@nvidia.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 | 20 ++++++++++++++++++-- drivers/gpu/drm/exynos/exynos_drm_drv.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_drv.c | 2 +- drivers/gpu/drm/tegra/drm.c | 2 +- include/drm/drm_atomic_helper.h | 3 ++- 8 files changed, 26 insertions(+), 9 deletions(-)