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

[PATCH 1/3] 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.

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] 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.5.0

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

[PATCH 3/3] drm/atomic: refuse changing CRTC for planes while active

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

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) {
-- 
2.5.0

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

Re: [PATCH 3/3] drm/atomic: refuse changing CRTC for planes while active

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6889
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  297/297              297/297
SNB                                  315/315              315/315
IVB                                  342/342              342/342
BYT                                  282/282              282/282
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

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


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/atomic: refuse changing CRTC for planes directly

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

Re: [Intel-gfx] [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Rob Clark]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Rob Clark]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Daniel Stone]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Rob Clark]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Ville Syrjälä]

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?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Rob Clark]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/atomic: refuse changing CRTC for planes directly

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

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

Re: [Intel-gfx] [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Rob Clark]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Maarten Lankhorst]

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;
>  }
>  

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

Re: [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: refuse changing CRTC for planes directly

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7264
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -2              302/302              300/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                 -1              283/283              282/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@flip-vs-dpms-interruptible      PASS(1)      DMESG_WARN(1)
*ILK  igt@kms_flip@wf_vblank-vs-modeset-interruptible      PASS(1)      DMESG_WARN(1)
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: refuse changing CRTC for planes directly

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7266
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -1              302/302              301/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                 -1              283/283              282/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@wf_vblank-vs-modeset-interruptible      PASS(1)      DMESG_WARN(1)
*BYT  igt@gem_persistent_relocs@interruptible      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

_______________________________________________
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: 2075 bytes --]

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>

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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

_______________________________________________
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 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
-- 
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