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.
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_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, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0b475fae067d..6be0adb5a0e9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1029,7 +1029,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);
@@ -1144,10 +1144,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 @@ -1162,7 +1168,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; @@ -1178,6 +1185,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); }
@@ -1189,6 +1199,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. */ @@ -1208,6 +1221,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_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 2b6320e6eae2..7b383acbb5af 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -293,7 +293,7 @@ static int exynos_atomic_commit(struct drm_device *dev, * have the relevant clocks enabled to perform the update. */
- drm_atomic_helper_commit_planes(dev, state); + drm_atomic_helper_commit_planes(dev, state, false);
drm_atomic_helper_cleanup_planes(dev, state);
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 1b22d8bfe142..0709b97251bf 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_drm_drv.c b/drivers/gpu/drm/sti/sti_drm_drv.c index 59d558b400b3..123f1408d508 100644 --- a/drivers/gpu/drm/sti/sti_drm_drv.c +++ b/drivers/gpu/drm/sti/sti_drm_drv.c @@ -59,7 +59,7 @@ static void sti_drm_atomic_complete(struct sti_drm_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 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); 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);
R-car does runtime pm (that's why it's committing plane state only at the end). Therefore better to only update planes on active crtc. Note that since the helpers always add all enabled planes when doing a modeset change on a crtc we are guaranteed to update plane hw state to the latest requested state each time the crtc is enabled.
Cc: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index ca12e8ca5552..20813582fbf1 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, false); + drm_atomic_helper_commit_planes(dev, old_state, true);
drm_atomic_helper_wait_for_vblanks(dev, old_state);
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
The exception is when both CRTC do a full modeset, then it should be no problem at all to move the planes around (presuming a correct implementation) so allow that case.
I've put this into the core since I really couldn't come up with a case where we don't want to enforce that. But if that ever happens it would be easy to move this check into helpers.
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic.c | 38 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 1 + 2 files changed, 39 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 434915448ea0..422183e7ee7d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,38 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+/* checks whether a plane has its CRTC switched while being in active use. */ +static bool +active_plane_switching(struct drm_atomic_state *state, + struct drm_plane *plane, + struct drm_plane_state *plane_state) +{ + struct drm_crtc_state *crtc_state, *curr_crtc_state; + + if (!plane->state->crtc || !plane_state->crtc) + return false; + + if (plane->state->crtc == plane_state->crtc) + return false; + + curr_crtc_state = plane->state->crtc->state; + if (!curr_crtc_state->active) + return false; + + crtc_state = drm_atomic_get_existing_crtc_state(state, + plane_state->crtc); + if (!crtc_state->active) + return false; + + /* plane switching CRTC and both CRTC are active. This is only ok if + * both CRTC do a full modeset. */ + if (drm_atomic_crtc_needs_modeset(curr_crtc_state) && + drm_atomic_crtc_needs_modeset(crtc_state)) + return false; + + return true; +} + /** * drm_atomic_plane_check - check plane state * @plane: plane to check @@ -734,6 +766,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
+ if (active_plane_switching(state->state, plane, state)) { + DRM_DEBUG_ATOMIC("[PLANE:%d] switching active CRTC without modeset\n", + plane->base.id); + return -EINVAL; + } + return 0; }
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 6be0adb5a0e9..54c59ddc59a5 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -497,6 +497,7 @@ drm_atomic_helper_check_planes(struct drm_device *dev, plane->base.id); return ret; } + }
for_each_crtc_in_state(state, crtc, crtc_state, i) {
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
I've put this into the core since I really couldn't come up with a case where we don't want to enforce that. But if that ever happens it would be easy to move this check into helpers.
v2: don't bother with complexity and just outright disallow plane switching without the intermediate OFF state. Simplifies drivers, we don't have any hw that could do it anyway and current atomic userspace (weston) works like this already anyway.
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 434915448ea0..f27aae3fa765 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+static bool +plane_switching(struct drm_atomic_state *state, + struct drm_plane *plane, + struct drm_plane_state *plane_state) +{ + struct drm_crtc_state *crtc_state, *curr_crtc_state; + + if (!plane->state->crtc || !plane_state->crtc) + return false; + + if (plane->state->crtc == plane_state->crtc) + return false; + + return true; +} + /** * drm_atomic_plane_check - check plane state * @plane: plane to check @@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
+ if (plane_switching(state->state, plane, state)) { + DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n", + plane->base.id); + return -EINVAL; + } + return 0; }
On Wed, Aug 26, 2015 at 05:41:23PM +0200, Daniel Vetter wrote:
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
I've put this into the core since I really couldn't come up with a case where we don't want to enforce that. But if that ever happens it would be easy to move this check into helpers.
v2: don't bother with complexity and just outright disallow plane switching without the intermediate OFF state. Simplifies drivers, we don't have any hw that could do it anyway and current atomic userspace (weston) works like this already anyway.
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 434915448ea0..f27aae3fa765 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+static bool +plane_switching(struct drm_atomic_state *state,
struct drm_plane *plane,struct drm_plane_state *plane_state)
plane_switching_crtc()?
+{
- struct drm_crtc_state *crtc_state, *curr_crtc_state;
- if (!plane->state->crtc || !plane_state->crtc)
return false;- if (plane->state->crtc == plane_state->crtc)
return false;- return true;
+}
/**
- drm_atomic_plane_check - check plane state
- @plane: plane to check
@@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
- if (plane_switching(state->state, plane, state)) {
DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",plane->base.id);return -EINVAL;- }
- return 0;
}
-- 2.5.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2015-08-26 at 18:53 +0300, Ville Syrjälä wrote:
On Wed, Aug 26, 2015 at 05:41:23PM +0200, Daniel Vetter wrote:
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
I've put this into the core since I really couldn't come up with a case where we don't want to enforce that. But if that ever happens it would be easy to move this check into helpers.
v2: don't bother with complexity and just outright disallow plane switching without the intermediate OFF state. Simplifies drivers, we don't have any hw that could do it anyway and current atomic userspace (weston) works like this already anyway.
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 434915448ea0..f27aae3fa765 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+static bool +plane_switching(struct drm_atomic_state *state,
struct drm_plane *plane,struct drm_plane_state *plane_state)plane_switching_crtc()?
With Ville's shed colour: Acked-by: Daniel Stone daniels@collabora.com
As this won't work in the general case, I'd prefer to see a hint to userspace that two CRTCs are genlocked before we enable it and people start to rely on it.
Cheers, Daniel
On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
So, I expect msm should actually be able to do this w/ dual-dsi (where we are using two CRTC's, w/ synchronized flushes)..
Probably someone who has a dual-dsi panel should actually test that to confirm. But it seems like it should work. Maybe we need something in 'struct drm_crtc' so core can realize that two CRTC's are locked together..
BR, -R
I've put this into the core since I really couldn't come up with a case where we don't want to enforce that. But if that ever happens it would be easy to move this check into helpers.
v2: don't bother with complexity and just outright disallow plane switching without the intermediate OFF state. Simplifies drivers, we don't have any hw that could do it anyway and current atomic userspace (weston) works like this already anyway.
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 434915448ea0..f27aae3fa765 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+static bool +plane_switching(struct drm_atomic_state *state,
struct drm_plane *plane,struct drm_plane_state *plane_state)+{
struct drm_crtc_state *crtc_state, *curr_crtc_state;if (!plane->state->crtc || !plane_state->crtc)return false;if (plane->state->crtc == plane_state->crtc)return false;return true;+}
/**
- drm_atomic_plane_check - check plane state
- @plane: plane to check
@@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
if (plane_switching(state->state, plane, state)) {DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",plane->base.id);return -EINVAL;}return 0;}
-- 2.5.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Aug 26, 2015 at 12:03 PM, Rob Clark robdclark@gmail.com wrote:
On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
So, I expect msm should actually be able to do this w/ dual-dsi (where we are using two CRTC's, w/ synchronized flushes)..
Probably someone who has a dual-dsi panel should actually test that to confirm. But it seems like it should work. Maybe we need something in 'struct drm_crtc' so core can realize that two CRTC's are locked together..
oh, and for most drivers, switching plane between CRTCs without an off-cycle would probably also work for DSI cmd mode..
BR, -R
I've put this into the core since I really couldn't come up with a case where we don't want to enforce that. But if that ever happens it would be easy to move this check into helpers.
v2: don't bother with complexity and just outright disallow plane switching without the intermediate OFF state. Simplifies drivers, we don't have any hw that could do it anyway and current atomic userspace (weston) works like this already anyway.
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 434915448ea0..f27aae3fa765 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+static bool +plane_switching(struct drm_atomic_state *state,
struct drm_plane *plane,struct drm_plane_state *plane_state)+{
struct drm_crtc_state *crtc_state, *curr_crtc_state;if (!plane->state->crtc || !plane_state->crtc)return false;if (plane->state->crtc == plane_state->crtc)return false;return true;+}
/**
- drm_atomic_plane_check - check plane state
- @plane: plane to check
@@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
if (plane_switching(state->state, plane, state)) {DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",plane->base.id);return -EINVAL;}return 0;}
-- 2.5.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Aug 26, 2015 at 12:07:30PM -0400, Rob Clark wrote:
On Wed, Aug 26, 2015 at 12:03 PM, Rob Clark robdclark@gmail.com wrote:
On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
So, I expect msm should actually be able to do this w/ dual-dsi (where we are using two CRTC's, w/ synchronized flushes)..
Probably someone who has a dual-dsi panel should actually test that to confirm. But it seems like it should work. Maybe we need something in 'struct drm_crtc' so core can realize that two CRTC's are locked together..
oh, and for most drivers, switching plane between CRTCs without an off-cycle would probably also work for DSI cmd mode..
You'd need to wait for any ongoing transfer on the old crtc to finish before moving the plane. So that's not really any different than the driver doing the dance with a vblank wait on a video mode display.
BR, -R
I've put this into the core since I really couldn't come up with a case where we don't want to enforce that. But if that ever happens it would be easy to move this check into helpers.
v2: don't bother with complexity and just outright disallow plane switching without the intermediate OFF state. Simplifies drivers, we don't have any hw that could do it anyway and current atomic userspace (weston) works like this already anyway.
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 434915448ea0..f27aae3fa765 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+static bool +plane_switching(struct drm_atomic_state *state,
struct drm_plane *plane,struct drm_plane_state *plane_state)+{
struct drm_crtc_state *crtc_state, *curr_crtc_state;if (!plane->state->crtc || !plane_state->crtc)return false;if (plane->state->crtc == plane_state->crtc)return false;return true;+}
/**
- drm_atomic_plane_check - check plane state
- @plane: plane to check
@@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
if (plane_switching(state->state, plane, state)) {DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",plane->base.id);return -EINVAL;}return 0;}
-- 2.5.0
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 Wed, Aug 26, 2015 at 12:30 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Aug 26, 2015 at 12:07:30PM -0400, Rob Clark wrote:
On Wed, Aug 26, 2015 at 12:03 PM, Rob Clark robdclark@gmail.com wrote:
On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
So, I expect msm should actually be able to do this w/ dual-dsi (where we are using two CRTC's, w/ synchronized flushes)..
Probably someone who has a dual-dsi panel should actually test that to confirm. But it seems like it should work. Maybe we need something in 'struct drm_crtc' so core can realize that two CRTC's are locked together..
oh, and for most drivers, switching plane between CRTCs without an off-cycle would probably also work for DSI cmd mode..
You'd need to wait for any ongoing transfer on the old crtc to finish before moving the plane. So that's not really any different than the driver doing the dance with a vblank wait on a video mode display.
except that update would need to block from previous xfer anyways, so there isn't really a race w/ frame n+1 like there would be with video mode..
I do think that *somehow* we need to expose whether or not the display is cmd-mode to userspace, since that factors into decisions about whether it can immediately re-use a plane on another CRTC, and I think it factors into discussion about some differences between ADF fencing and upstream fencing as we add explicit fencing support to atomic.. but that is probably a different topic..
BR, -R
BR, -R
I've put this into the core since I really couldn't come up with a case where we don't want to enforce that. But if that ever happens it would be easy to move this check into helpers.
v2: don't bother with complexity and just outright disallow plane switching without the intermediate OFF state. Simplifies drivers, we don't have any hw that could do it anyway and current atomic userspace (weston) works like this already anyway.
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 434915448ea0..f27aae3fa765 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+static bool +plane_switching(struct drm_atomic_state *state,
struct drm_plane *plane,struct drm_plane_state *plane_state)+{
struct drm_crtc_state *crtc_state, *curr_crtc_state;if (!plane->state->crtc || !plane_state->crtc)return false;if (plane->state->crtc == plane_state->crtc)return false;return true;+}
/**
- drm_atomic_plane_check - check plane state
- @plane: plane to check
@@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
if (plane_switching(state->state, plane, state)) {DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",plane->base.id);return -EINVAL;}return 0;}
-- 2.5.0
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
-- Ville Syrjälä Intel OTC
On Wed, Aug 26, 2015 at 01:38:36PM -0400, Rob Clark wrote:
On Wed, Aug 26, 2015 at 12:30 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Aug 26, 2015 at 12:07:30PM -0400, Rob Clark wrote:
On Wed, Aug 26, 2015 at 12:03 PM, Rob Clark robdclark@gmail.com wrote:
On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
So, I expect msm should actually be able to do this w/ dual-dsi (where we are using two CRTC's, w/ synchronized flushes)..
Probably someone who has a dual-dsi panel should actually test that to confirm. But it seems like it should work. Maybe we need something in 'struct drm_crtc' so core can realize that two CRTC's are locked together..
oh, and for most drivers, switching plane between CRTCs without an off-cycle would probably also work for DSI cmd mode..
You'd need to wait for any ongoing transfer on the old crtc to finish before moving the plane. So that's not really any different than the driver doing the dance with a vblank wait on a video mode display.
except that update would need to block from previous xfer anyways, so there isn't really a race w/ frame n+1 like there would be with video mode..
Why would it block if it's on a separate crtc?
On Wed, Aug 26, 2015 at 1:53 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Aug 26, 2015 at 01:38:36PM -0400, Rob Clark wrote:
On Wed, Aug 26, 2015 at 12:30 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Aug 26, 2015 at 12:07:30PM -0400, Rob Clark wrote:
On Wed, Aug 26, 2015 at 12:03 PM, Rob Clark robdclark@gmail.com wrote:
On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
So, I expect msm should actually be able to do this w/ dual-dsi (where we are using two CRTC's, w/ synchronized flushes)..
Probably someone who has a dual-dsi panel should actually test that to confirm. But it seems like it should work. Maybe we need something in 'struct drm_crtc' so core can realize that two CRTC's are locked together..
oh, and for most drivers, switching plane between CRTCs without an off-cycle would probably also work for DSI cmd mode..
You'd need to wait for any ongoing transfer on the old crtc to finish before moving the plane. So that's not really any different than the driver doing the dance with a vblank wait on a video mode display.
except that update would need to block from previous xfer anyways, so there isn't really a race w/ frame n+1 like there would be with video mode..
Why would it block if it's on a separate crtc?
or, well, could block.. but anyways, the more realistic scenario for 2x dsi is dual-dsi to single panel w/ the two CRTC's locked in sync..
BR, -R
-- Ville Syrjälä Intel OTC
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
I've put this into the core since right now no helper or driver copes with it, no userspace has code for it and no one asks for it. Yes there's piles of corner-cases where this would be possible to do this like: - switch from inactive crtc to active crtc - swithc from active crtc to inactive crtc - genlocked display - invisible plane (to do whatever) - idle plane hw due to dsi cmd mode/psr - whatever but looking at details it's not that easy to implement this correctly. Hence just put it into the core and add a comment, since the only userspace we have right now for atomic (weston) doesn't want to use direct plane switching either.
v2: don't bother with complexity and just outright disallow plane switching without the intermediate OFF state. Simplifies drivers, we don't have any hw that could do it anyway and current atomic userspace (weston) works like this already anyway.
v3: Bikeshed function name (Ville) and add comment (Rob).
v4: Also bikeshed commit message (Rob).
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Stone daniels@collabora.com Acked-by: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
---
Imo this is bikeshedded enough, so either someone takes this or someone else can mangle this patch more. -Daniel --- drivers/gpu/drm/drm_atomic.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1066e4b658cf..40ddd6aa100f 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,27 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+static bool +plane_switching_crtc(struct drm_atomic_state *state, + struct drm_plane *plane, + struct drm_plane_state *plane_state) +{ + struct drm_crtc_state *crtc_state, *curr_crtc_state; + + if (!plane->state->crtc || !plane_state->crtc) + return false; + + if (plane->state->crtc == plane_state->crtc) + return false; + + /* This could be refined, but currently there's no helper or driver code + * to implement direct switching of active planes nor userspace to take + * advantage of more direct plane switching without the intermediate + * full OFF state. + */ + return true; +} + /** * drm_atomic_plane_check - check plane state * @plane: plane to check @@ -734,6 +755,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
+ if (plane_switching_crtc(state->state, plane, state)) { + DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n", + plane->base.id); + return -EINVAL; + } + return 0; }
On Wed, Aug 26, 2015 at 3:49 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
I've put this into the core since right now no helper or driver copes with it, no userspace has code for it and no one asks for it. Yes there's piles of corner-cases where this would be possible to do this like:
- switch from inactive crtc to active crtc
- swithc from active crtc to inactive crtc
- genlocked display
- invisible plane (to do whatever)
- idle plane hw due to dsi cmd mode/psr
- whatever
but looking at details it's not that easy to implement this correctly. Hence just put it into the core and add a comment, since the only userspace we have right now for atomic (weston) doesn't want to use direct plane switching either.
I suspect that eventually we'll want to make this a helper exposed to drivers and push this check down into the drivers.. perhaps that is not until weston (and/or atomic based hwc) grows driver specific userspace plugin type API to make smarter hw specific decisions.. until then, this is probably the most reasonable thing to do.. so
(w/ s/swihc/switch/ fix smashed in above)
Reviewed-by: Rob Clark robdclark@gmail.com
v2: don't bother with complexity and just outright disallow plane switching without the intermediate OFF state. Simplifies drivers, we don't have any hw that could do it anyway and current atomic userspace (weston) works like this already anyway.
v3: Bikeshed function name (Ville) and add comment (Rob).
v4: Also bikeshed commit message (Rob).
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Stone daniels@collabora.com Acked-by: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Imo this is bikeshedded enough, so either someone takes this or someone else can mangle this patch more.
-Daniel
drivers/gpu/drm/drm_atomic.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1066e4b658cf..40ddd6aa100f 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,27 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+static bool +plane_switching_crtc(struct drm_atomic_state *state,
struct drm_plane *plane,struct drm_plane_state *plane_state)+{
struct drm_crtc_state *crtc_state, *curr_crtc_state;if (!plane->state->crtc || !plane_state->crtc)return false;if (plane->state->crtc == plane_state->crtc)return false;/* This could be refined, but currently there's no helper or driver code* to implement direct switching of active planes nor userspace to take* advantage of more direct plane switching without the intermediate* full OFF state.*/return true;+}
/**
- drm_atomic_plane_check - check plane state
- @plane: plane to check
@@ -734,6 +755,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
if (plane_switching_crtc(state->state, plane, state)) {DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",plane->base.id);return -EINVAL;}return 0;}
-- 2.5.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Aug 26, 2015 at 05:51:46PM -0400, Rob Clark wrote:
On Wed, Aug 26, 2015 at 3:49 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
I've put this into the core since right now no helper or driver copes with it, no userspace has code for it and no one asks for it. Yes there's piles of corner-cases where this would be possible to do this like:
- switch from inactive crtc to active crtc
- swithc from active crtc to inactive crtc
- genlocked display
- invisible plane (to do whatever)
- idle plane hw due to dsi cmd mode/psr
- whatever
but looking at details it's not that easy to implement this correctly. Hence just put it into the core and add a comment, since the only userspace we have right now for atomic (weston) doesn't want to use direct plane switching either.
I suspect that eventually we'll want to make this a helper exposed to drivers and push this check down into the drivers.. perhaps that is not until weston (and/or atomic based hwc) grows driver specific userspace plugin type API to make smarter hw specific decisions.. until then, this is probably the most reasonable thing to do.. so
Yeah this really is just to plug a "undefined behaviour" gap in the abi until we know what exactly we want and have some userspace needing this.
(w/ s/swihc/switch/ fix smashed in above)
Reviewed-by: Rob Clark robdclark@gmail.com
Fixed&merged to drm-misc, thanks for the review. -Daniel
v2: don't bother with complexity and just outright disallow plane switching without the intermediate OFF state. Simplifies drivers, we don't have any hw that could do it anyway and current atomic userspace (weston) works like this already anyway.
v3: Bikeshed function name (Ville) and add comment (Rob).
v4: Also bikeshed commit message (Rob).
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Daniel Stone daniels@collabora.com Acked-by: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Imo this is bikeshedded enough, so either someone takes this or someone else can mangle this patch more.
-Daniel
drivers/gpu/drm/drm_atomic.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1066e4b658cf..40ddd6aa100f 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,27 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+static bool +plane_switching_crtc(struct drm_atomic_state *state,
struct drm_plane *plane,struct drm_plane_state *plane_state)+{
struct drm_crtc_state *crtc_state, *curr_crtc_state;if (!plane->state->crtc || !plane_state->crtc)return false;if (plane->state->crtc == plane_state->crtc)return false;/* This could be refined, but currently there's no helper or driver code* to implement direct switching of active planes nor userspace to take* advantage of more direct plane switching without the intermediate* full OFF state.*/return true;+}
/**
- drm_atomic_plane_check - check plane state
- @plane: plane to check
@@ -734,6 +755,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
if (plane_switching_crtc(state->state, plane, state)) {DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",plane->base.id);return -EINVAL;}return 0;}
-- 2.5.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 26-08-15 om 21:49 schreef Daniel Vetter:
Very strictly speaking this is possible if you have special hw and genlocked CRTCs. In general switching a plane between two active CRTC just won't work so well and is probably not tested at all. Just forbid it.
I've put this into the core since right now no helper or driver copes with it, no userspace has code for it and no one asks for it. Yes there's piles of corner-cases where this would be possible to do this like:
- switch from inactive crtc to active crtc
- swithc from active crtc to inactive crtc
- genlocked display
- invisible plane (to do whatever)
- idle plane hw due to dsi cmd mode/psr
- whatever
but looking at details it's not that easy to implement this correctly. Hence just put it into the core and add a comment, since the only userspace we have right now for atomic (weston) doesn't want to use direct plane switching either.
v2: don't bother with complexity and just outright disallow plane switching without the intermediate OFF state. Simplifies drivers, we don't have any hw that could do it anyway and current atomic userspace (weston) works like this already anyway.
v3: Bikeshed function name (Ville) and add comment (Rob).
v4: Also bikeshed commit message (Rob).
Cc: Thierry Reding thierry.reding@gmail.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Acked-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Cc: Daniel Stone daniels@collabora.com Acked-by: Daniel Stone daniels@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Imo this is bikeshedded enough, so either someone takes this or someone else can mangle this patch more.
-Daniel
drivers/gpu/drm/drm_atomic.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1066e4b658cf..40ddd6aa100f 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -663,6 +663,27 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }
+static bool +plane_switching_crtc(struct drm_atomic_state *state,
struct drm_plane *plane,struct drm_plane_state *plane_state)+{
- struct drm_crtc_state *crtc_state, *curr_crtc_state;
- if (!plane->state->crtc || !plane_state->crtc)
return false;- if (plane->state->crtc == plane_state->crtc)
return false;- /* This could be refined, but currently there's no helper or driver code
* to implement direct switching of active planes nor userspace to take* advantage of more direct plane switching without the intermediate* full OFF state.*/- return true;
+}
/**
- drm_atomic_plane_check - check plane state
- @plane: plane to check
@@ -734,6 +755,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
- if (plane_switching_crtc(state->state, plane, state)) {
DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",plane->base.id);return -EINVAL;- }
- return 0;
}
Op 29-07-15 om 14:01 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.
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.
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_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, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0b475fae067d..6be0adb5a0e9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1029,7 +1029,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);
@@ -1144,10 +1144,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
@@ -1162,7 +1168,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; @@ -1178,6 +1185,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); }
@@ -1189,6 +1199,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.
This would leave a plane active if it was moved from a active crtc to a inactive crtc, you would still need to call the atomic_disable(plane) callback for that one. ;-)
~Maarten
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.
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_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_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 e0b99302c667..8bfd64f71bc4 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);
@@ -1152,10 +1152,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 @@ -1170,7 +1176,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; @@ -1186,6 +1193,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); }
@@ -1197,6 +1207,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. */ @@ -1216,6 +1229,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_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 9738f4e0c6eb..2e1f7143174c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -293,7 +293,7 @@ static int exynos_atomic_commit(struct drm_device *dev, * have the relevant clocks enabled to perform the update. */
- drm_atomic_helper_commit_planes(dev, state); + drm_atomic_helper_commit_planes(dev, state, false);
drm_atomic_helper_cleanup_planes(dev, 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(-)
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);
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
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
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);
dri-devel@lists.freedesktop.org
-
Daniel Stone -
Daniel Vetter -
Daniel Vetter -
Laurent Pinchart -
Maarten Lankhorst -
Rob Clark -
Thierry Reding -
Ville Syrjälä