[PATCH 1/2] drm/atomic-helper: Add option to update planes only on active crtc

[PATCH 1/2] drm/atomic-helper: Add option to update planes only on active crtc

[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.

Cc: Thierry Reding <treding@nvidia.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c    | 15 +++++++++++++--
 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, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 91ad6bd13734..192412277586 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -989,7 +989,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);
 
@@ -1108,6 +1108,7 @@ EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
  * 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
@@ -1122,7 +1123,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;
@@ -1138,6 +1140,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);
 	}
 
@@ -1149,6 +1154,9 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (!funcs)
 			continue;
 
+		if (active_only && !plane->state->crtc->state->active)
+			continue;
+
 		/*
 		 * Special-case disabling the plane if drivers support it.
 		 */
@@ -1168,6 +1176,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);
 	}
 }
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 cc1fee8a12d0..c7b6aa54be37 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);
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/rcar: Only update planes on active crtc

[Daniel Vetter]

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);
 
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/atomic-helper: Add option to update planes only on active crtc

[Maarten Lankhorst]

Op 22-07-15 om 16:30 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.
>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c    | 15 +++++++++++++--
>  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, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 91ad6bd13734..192412277586 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -989,7 +989,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);
>  
> @@ -1108,6 +1108,7 @@ EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
>   * 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
> @@ -1122,7 +1123,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;
> @@ -1138,6 +1140,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);
>  	}
>  
> @@ -1149,6 +1154,9 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  		if (!funcs)
>  			continue;
>  
> +		if (active_only && !plane->state->crtc->state->active)
> +			continue;
>
Find the null pointer deref. ;-)

Otherwise r-b.

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/atomic-helper: Add option to update planes only on active crtc

[Daniel Vetter]

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

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

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

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c    | 29 +++++++++++++++++++++++++++--
 drivers/gpu/drm/exynos/exynos_drm_fb.c |  2 +-
 drivers/gpu/drm/msm/msm_atomic.c       |  2 +-
 drivers/gpu/drm/omapdrm/omap_drv.c     |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
 drivers/gpu/drm/sti/sti_drm_drv.c      |  2 +-
 drivers/gpu/drm/tegra/drm.c            |  2 +-
 include/drm/drm_atomic_helper.h        |  3 ++-
 8 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 99656815641d..2122c2b844da 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -471,6 +471,14 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
 		if (!funcs || !funcs->atomic_check)
 			continue;
 
+		if (plane->state->crtc && plane_state->crtc &&
+		    plane->state->crtc != plane_state->crtc &&
+		    plane->state->crtc->state->active) {
+			DRM_DEBUG_ATOMIC("[PLANE:%d] changing crtc while still active\n",
+					 plane->base.id);
+			return -EINVAL;
+		}
+
 		ret = funcs->atomic_check(plane, plane_state);
 		if (ret) {
 			DRM_DEBUG_ATOMIC("[PLANE:%d] atomic driver check failed\n",
@@ -995,7 +1003,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 
 	drm_atomic_helper_commit_modeset_disables(dev, state);
 
-	drm_atomic_helper_commit_planes(dev, state);
+	drm_atomic_helper_commit_planes(dev, state, false);
 
 	drm_atomic_helper_commit_modeset_enables(dev, state);
 
@@ -1110,10 +1118,16 @@ fail:
 }
 EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
 
+bool plane_crtc_active(struct drm_plane_state *state)
+{
+	return state->crtc && state->crtc->state->active;
+}
+
 /**
  * drm_atomic_helper_commit_planes - commit plane state
  * @dev: DRM device
  * @old_state: atomic state object with old state structures
+ * @active_only: Only commit on active CRTC if set
  *
  * This function commits the new plane state using the plane and atomic helper
  * functions for planes and crtcs. It assumes that the atomic state has already
@@ -1128,7 +1142,8 @@ EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
  * drm_atomic_helper_commit_planes_on_crtc() instead.
  */
 void drm_atomic_helper_commit_planes(struct drm_device *dev,
-				     struct drm_atomic_state *old_state)
+				     struct drm_atomic_state *old_state,
+				     bool active_only)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
@@ -1144,6 +1159,9 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (!funcs || !funcs->atomic_begin)
 			continue;
 
+		if (active_only && !crtc->state->active)
+			continue;
+
 		funcs->atomic_begin(crtc);
 	}
 
@@ -1155,6 +1173,10 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (!funcs)
 			continue;
 
+		if (active_only && !plane_crtc_active(plane->state) &&
+		    !plane_crtc_active(old_plane_state))
+			continue;
+
 		/*
 		 * Special-case disabling the plane if drivers support it.
 		 */
@@ -1174,6 +1196,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);
 	}
 }
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 cc1fee8a12d0..c7b6aa54be37 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);
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic-helper: Add option to update planes only on active crtc

[Maarten Lankhorst]

Op 22-07-15 om 18:02 schreef Daniel Vetter:
> With drivers supporting runtime pm it's generally not a good idea to
> touch the hardware when it's off. Add an option to the commit_planes
> helper to support this case.
>
> Note that the helpers already add all planes on a crtc when a modeset
> happens, hence plane updates will not be lost if drivers set this to
> true.
>
> v2: Check for NULL state->crtc before chasing the pointer. Also check
> both old and new crtc if there's a switch. Finally just outright
> disallow switching crtcs for a plane if the plane is in active use, on
> most hardware that doesn't make sense.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c    | 29 +++++++++++++++++++++++++++--
>  drivers/gpu/drm/exynos/exynos_drm_fb.c |  2 +-
>  drivers/gpu/drm/msm/msm_atomic.c       |  2 +-
>  drivers/gpu/drm/omapdrm/omap_drv.c     |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>  drivers/gpu/drm/sti/sti_drm_drv.c      |  2 +-
>  drivers/gpu/drm/tegra/drm.c            |  2 +-
>  include/drm/drm_atomic_helper.h        |  3 ++-
>  8 files changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 99656815641d..2122c2b844da 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -471,6 +471,14 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  		if (!funcs || !funcs->atomic_check)
>  			continue;
>  
> +		if (plane->state->crtc && plane_state->crtc &&
> +		    plane->state->crtc != plane_state->crtc &&
> +		    plane->state->crtc->state->active) {
> +			DRM_DEBUG_ATOMIC("[PLANE:%d] changing crtc while still active\n",
> +					 plane->base.id);
> +			return -EINVAL;
> +		}
> +
>  		ret = funcs->atomic_check(plane, plane_state);
>  		if (ret) {
>  			DRM_DEBUG_ATOMIC("[PLANE:%d] atomic driver check failed\n",
> @@ -995,7 +1003,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>  
>  	drm_atomic_helper_commit_modeset_disables(dev, state);
>  
> -	drm_atomic_helper_commit_planes(dev, state);
> +	drm_atomic_helper_commit_planes(dev, state, false);
>  
>  	drm_atomic_helper_commit_modeset_enables(dev, state);
>  
> @@ -1110,10 +1118,16 @@ fail:
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
>  
> +bool plane_crtc_active(struct drm_plane_state *state)
> +{
> +	return state->crtc && state->crtc->state->active;
> +}
> +
>  /**
>   * drm_atomic_helper_commit_planes - commit plane state
>   * @dev: DRM device
>   * @old_state: atomic state object with old state structures
> + * @active_only: Only commit on active CRTC if set
>   *
>   * This function commits the new plane state using the plane and atomic helper
>   * functions for planes and crtcs. It assumes that the atomic state has already
> @@ -1128,7 +1142,8 @@ EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
>   * drm_atomic_helper_commit_planes_on_crtc() instead.
>   */
>  void drm_atomic_helper_commit_planes(struct drm_device *dev,
> -				     struct drm_atomic_state *old_state)
> +				     struct drm_atomic_state *old_state,
> +				     bool active_only)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state;
> @@ -1144,6 +1159,9 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  		if (!funcs || !funcs->atomic_begin)
>  			continue;
>  
> +		if (active_only && !crtc->state->active)
> +			continue;
> +
>  		funcs->atomic_begin(crtc);
>  	}
>  
> @@ -1155,6 +1173,10 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  		if (!funcs)
>  			continue;
>  
> +		if (active_only && !plane_crtc_active(plane->state) &&
> +		    !plane_crtc_active(old_plane_state))
> +			continue;
> +
>  		/*
>  		 * Special-case disabling the plane if drivers support it.
>  		 */
>
The check is still wrong. It should only check if the new plane_state->crtc is active, if it's not call the disable_plane hook, but only when get_existing_crtc_state(old_plane_state->crtc)->active && !needs_modeset(old_plane_state->crtc->state).

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

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic-helper: Add option to update planes only on active crtc

[Daniel Vetter]

On Thu, Jul 23, 2015 at 05:53:43AM +0200, Maarten Lankhorst wrote:
> Op 22-07-15 om 18:02 schreef Daniel Vetter:
> > With drivers supporting runtime pm it's generally not a good idea to
> > touch the hardware when it's off. Add an option to the commit_planes
> > helper to support this case.
> >
> > Note that the helpers already add all planes on a crtc when a modeset
> > happens, hence plane updates will not be lost if drivers set this to
> > true.
> >
> > v2: Check for NULL state->crtc before chasing the pointer. Also check
> > both old and new crtc if there's a switch. Finally just outright
> > disallow switching crtcs for a plane if the plane is in active use, on
> > most hardware that doesn't make sense.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Thierry Reding <treding@nvidia.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c    | 29 +++++++++++++++++++++++++++--
> >  drivers/gpu/drm/exynos/exynos_drm_fb.c |  2 +-
> >  drivers/gpu/drm/msm/msm_atomic.c       |  2 +-
> >  drivers/gpu/drm/omapdrm/omap_drv.c     |  2 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
> >  drivers/gpu/drm/sti/sti_drm_drv.c      |  2 +-
> >  drivers/gpu/drm/tegra/drm.c            |  2 +-
> >  include/drm/drm_atomic_helper.h        |  3 ++-
> >  8 files changed, 35 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 99656815641d..2122c2b844da 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -471,6 +471,14 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> >  		if (!funcs || !funcs->atomic_check)
> >  			continue;
> >  
> > +		if (plane->state->crtc && plane_state->crtc &&
> > +		    plane->state->crtc != plane_state->crtc &&
> > +		    plane->state->crtc->state->active) {
> > +			DRM_DEBUG_ATOMIC("[PLANE:%d] changing crtc while still active\n",
> > +					 plane->base.id);
> > +			return -EINVAL;
> > +		}
> > +
> >  		ret = funcs->atomic_check(plane, plane_state);
> >  		if (ret) {
> >  			DRM_DEBUG_ATOMIC("[PLANE:%d] atomic driver check failed\n",
> > @@ -995,7 +1003,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> >  
> >  	drm_atomic_helper_commit_modeset_disables(dev, state);
> >  
> > -	drm_atomic_helper_commit_planes(dev, state);
> > +	drm_atomic_helper_commit_planes(dev, state, false);
> >  
> >  	drm_atomic_helper_commit_modeset_enables(dev, state);
> >  
> > @@ -1110,10 +1118,16 @@ fail:
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
> >  
> > +bool plane_crtc_active(struct drm_plane_state *state)
> > +{
> > +	return state->crtc && state->crtc->state->active;
> > +}
> > +
> >  /**
> >   * drm_atomic_helper_commit_planes - commit plane state
> >   * @dev: DRM device
> >   * @old_state: atomic state object with old state structures
> > + * @active_only: Only commit on active CRTC if set
> >   *
> >   * This function commits the new plane state using the plane and atomic helper
> >   * functions for planes and crtcs. It assumes that the atomic state has already
> > @@ -1128,7 +1142,8 @@ EXPORT_SYMBOL(drm_atomic_helper_prepare_planes);
> >   * drm_atomic_helper_commit_planes_on_crtc() instead.
> >   */
> >  void drm_atomic_helper_commit_planes(struct drm_device *dev,
> > -				     struct drm_atomic_state *old_state)
> > +				     struct drm_atomic_state *old_state,
> > +				     bool active_only)
> >  {
> >  	struct drm_crtc *crtc;
> >  	struct drm_crtc_state *old_crtc_state;
> > @@ -1144,6 +1159,9 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
> >  		if (!funcs || !funcs->atomic_begin)
> >  			continue;
> >  
> > +		if (active_only && !crtc->state->active)
> > +			continue;
> > +
> >  		funcs->atomic_begin(crtc);
> >  	}
> >  
> > @@ -1155,6 +1173,10 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
> >  		if (!funcs)
> >  			continue;
> >  
> > +		if (active_only && !plane_crtc_active(plane->state) &&
> > +		    !plane_crtc_active(old_plane_state))
> > +			continue;
> > +
> >  		/*
> >  		 * Special-case disabling the plane if drivers support it.
> >  		 */
> >
> The check is still wrong. It should only check if the new
> plane_state->crtc is active, if it's not call the disable_plane hook,
> but only when get_existing_crtc_state(old_plane_state->crtc)->active &&
> !needs_modeset(old_plane_state->crtc->state).

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

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

Or is there another case where the above falls short?

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

Yeah, I'll try to stitch that helper together.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic-helper: Add option to update planes only on active crtc

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4474 bytes --]

On Wed, Jul 22, 2015 at 06:02:27PM +0200, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index c6276aebfec3..783edc242648 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -56,7 +56,7 @@ static void tegra_atomic_complete(struct tegra_drm *tegra,
>  	 */
>  
>  	drm_atomic_helper_commit_modeset_disables(drm, state);
> -	drm_atomic_helper_commit_planes(drm, state);
> +	drm_atomic_helper_commit_planes(drm, state, false);
>  	drm_atomic_helper_commit_modeset_enables(drm, state);
>  
>  	drm_atomic_helper_wait_for_vblanks(drm, state);

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

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

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

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

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

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

	1) disable all planes that will no longer be used in the new
	   state
	2) disable all CRTCs that will no longer be used in the new
	   state
	3) update all planes that have changed from old to new state
	4) enable CRTCs that were not used in the old state but are
	   enabled in the new state
	5) enable all planes that were not used in the old state but
	   are enabled in the new state

1) and 5) I think could be helpers that are called from drivers directly
in their ->enable() functions.

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

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

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic-helper: Add option to update planes only on active crtc

[Daniel Vetter]

On Mon, Jul 27, 2015 at 02:44:31PM +0200, Thierry Reding wrote:
> On Wed, Jul 22, 2015 at 06:02:27PM +0200, Daniel Vetter wrote:
> [...]
> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > index c6276aebfec3..783edc242648 100644
> > --- a/drivers/gpu/drm/tegra/drm.c
> > +++ b/drivers/gpu/drm/tegra/drm.c
> > @@ -56,7 +56,7 @@ static void tegra_atomic_complete(struct tegra_drm *tegra,
> >  	 */
> >  
> >  	drm_atomic_helper_commit_modeset_disables(drm, state);
> > -	drm_atomic_helper_commit_planes(drm, state);
> > +	drm_atomic_helper_commit_planes(drm, state, false);
> >  	drm_atomic_helper_commit_modeset_enables(drm, state);
> >  
> >  	drm_atomic_helper_wait_for_vblanks(drm, state);
> 
> I tried to give this a go with my ongoing DPMS work and I can't make
> this work. I think we'll need something more involved to fix this
> properly with runtime PM enabled drivers.
> 
> The reason why this isn't working on Tegra is because with the rework
> done for DPMS, the CRTC will be put into reset in ->disable() and taken
> out of reset in ->enable() (eventually I suspect that it'd also be
> powergated in ->disable() and unpowergated in ->enable(), but that
> should have no further side-effects than the reset).
> 
> There are two cases where this leads to problems, though as far as I can
> tell they are the same problem: with legacy fbdev enabled, the initial
> modeset works fine (though that's only because the mode is actually set
> twice due to the monitor pulsing HPD). If I then run modetest with the
> same mode set as fbdev, I also see planes updated properly. Now after I
> exit modetest it will do a modeset (it's what modetest does, no matter
> if the mode it did set matches fbdev, not sure if that's really desired)
> and that ends up with no primary plane being set.
> 
> The second case where I've seen this happen is with legacy fbdev
> disabled. In that case, running modetest won't ever display the correct
> plane. I'm somewhat surprised that it even works, given that the CRTC to
> which the plane's registers belong is in reset and has clocks disabled
> at the time of the ->atomic_update() call. I suspect this could be
> related to the fact that we're accessing only plane registers, and they
> go through some sort of muxing machinery that may not underly the same
> clocking and reset restrictions as the other registers. Anyway, the
> result is that all of the changes done in ->atomic_update() will be lost
> when the CRTC is enabled, and therefore the only case where the
> drm_atomic_helper_commit_planes() call works is when the CRTC stays on
> (essentially what used to be ->mode_set_base()).
> 
> Moreover, I'd say with active_only set to true, the core shouldn't even
> call ->atomic_update() for planes associated with the CRTC because at
> this point it's still off. drm_atomic_helper_commit_modeset_enables() is
> what will turn the CRTC on.
> 
> So all in all, I think what we need is actually five steps:
> 
> 	1) disable all planes that will no longer be used in the new
> 	   state
> 	2) disable all CRTCs that will no longer be used in the new
> 	   state
> 	3) update all planes that have changed from old to new state
> 	4) enable CRTCs that were not used in the old state but are
> 	   enabled in the new state
> 	5) enable all planes that were not used in the old state but
> 	   are enabled in the new state
> 
> 1) and 5) I think could be helpers that are called from drivers directly
> in their ->enable() functions.
> 
> The downside of the above is that there are a bunch of cases where we'd
> need to be very careful to preserve atomicity across updates. For planes
> that are enabled on a CRTC that remains active between two states, they
> would need to be updated in the same ->atomic_begin()/->atomic_flush()
> sequence as planes that move or change framebuffer, otherwise the frames
> won't be perfect.
> 
> Similarly I think we'd want a way to allow drivers to atomically enable
> updates. That is, enabling the output and setting the planes on them
> should be an atomic operation. Currently depending on how fast the mode
> is being set you could have a short period where the mode has been
> applied, but plane updates aren't visible yet. That would essentially
> mean that 1) & 2) as well as 4) & 5) in the above would need to be
> collapsed into single steps, at the end of which the CRTC hardware needs
> to commit all changes at once. I suspect that for most (all?) cases this
> may not even matter, though. If the CRTC is set up to scan out black if
> no planes are active, then this should not be noticable.

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

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

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

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

Note that my patch only tries to implement step 4) in the above sequence,
step 1 is still tbd. And as Maarten pointed out I failed at implementing
step 4 correctly for all corner cases.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel