[PATCH v2] drm/atomic_helper: Add drm_atomic_helper_disable_planes_on_crtc()

[PATCH v2] drm/atomic_helper: Add drm_atomic_helper_disable_planes_on_crtc()

[Jyri Sarha]

Add drm_atomic_helper_disable_planes_on_crtc() for disabling all
planes associated with the given CRTC. This can be used for instance
in the CRTC helper disable callback to disable all planes before
shutting down the display pipeline.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
v2:
- Address Daniels review comments [1]
  - Do atomic_begin() and atomic_flush() always if they are defined and
    atomic knob is set
  - update kerneldoc
- Put drm_atomic_helper_disable_planes_on_crtc() after
  drm_atomic_helper_commit_planes_on_crtc() in drm_atomic_helper.c 
  to have functions in the same order as in drm_atomic_helper.h
 
Here the patch is again with latest review comment addressed, however
I am not sure if we are going use the drm_atomic_helper_disable_planes_on_crtc()
in omapdrm.

BTW, the latest change to this patch makes me wonder a how the
atomic_begin() and atomic_flush() are supposed to work. 

1. Should it be allowed to call atomic_begin() and atomic_flush()
multiple times in a recursive manner?

If yes, how should the driver cope with that? Keep refcount of
atomic_begins and do the magic only after the final matching
atomic_flush comes and the refcount reaches zero?

2. What about calling them consecutively (but not recursively)
multiple times without vblank happening in the middle?

I am afraid that the current omapdrm implementation does not cope too
well in either situation.

Cheers,
Jyri

[1] http://www.spinics.net/lists/dri-devel/msg94942.html

 drivers/gpu/drm/drm_atomic_helper.c | 43 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic_helper.h     |  2 ++
 2 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0c6f621..1ab6821 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1334,6 +1334,49 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
 EXPORT_SYMBOL(drm_atomic_helper_commit_planes_on_crtc);
 
 /**
+ * drm_atomic_helper_disable_planes_on_crtc - helper to disable CRTC's planes
+ * @crtc: CRTC
+ * @atomic: if set, synchronize with CRTC's atomic_begin/flush hooks
+ *
+ * Disables all planes associated with the given CRTC. This can be
+ * used for instance in the CRTC helper disable callback to disable
+ * all planes before shutting down the display pipeline.
+ *
+ * If the atomic-parameter is set the function calls the CRTC's
+ * atomic_begin hook before and atomic_flush hook after disabling the
+ * planes.
+ *
+ * It is a bug to call this function without having implemented the
+ * ->atomic_disable() plane hook.
+ */
+void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc,
+					      bool atomic)
+{
+	const struct drm_crtc_helper_funcs *crtc_funcs =
+		crtc->helper_private;
+	struct drm_plane *plane;
+
+	if (atomic && crtc_funcs && crtc_funcs->atomic_begin)
+		crtc_funcs->atomic_begin(crtc, NULL);
+
+	drm_for_each_plane(plane, crtc->dev) {
+		const struct drm_plane_helper_funcs *plane_funcs =
+			plane->helper_private;
+
+		if (plane->state->crtc != crtc || !plane_funcs)
+			continue;
+
+		WARN_ON(!plane_funcs->atomic_disable);
+		if (plane_funcs->atomic_disable)
+			plane_funcs->atomic_disable(plane, NULL);
+	}
+
+	if (atomic && crtc_funcs && crtc_funcs->atomic_flush)
+		crtc_funcs->atomic_flush(crtc, NULL);
+}
+EXPORT_SYMBOL(drm_atomic_helper_disable_planes_on_crtc);
+
+/**
  * drm_atomic_helper_cleanup_planes - cleanup plane resources after commit
  * @dev: DRM device
  * @old_state: atomic state object with old state structures
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 8cba54a..b7d4237 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -62,6 +62,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 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);
+void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc,
+					      bool atomic);
 
 void drm_atomic_helper_swap_state(struct drm_device *dev,
 				  struct drm_atomic_state *state);
-- 
1.9.1

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

Re: [PATCH v2] drm/atomic_helper: Add drm_atomic_helper_disable_planes_on_crtc()

[Daniel Vetter]

On Fri, Nov 27, 2015 at 04:14:01PM +0200, Jyri Sarha wrote:
> Add drm_atomic_helper_disable_planes_on_crtc() for disabling all
> planes associated with the given CRTC. This can be used for instance
> in the CRTC helper disable callback to disable all planes before
> shutting down the display pipeline.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
> v2:
> - Address Daniels review comments [1]
>   - Do atomic_begin() and atomic_flush() always if they are defined and
>     atomic knob is set
>   - update kerneldoc
> - Put drm_atomic_helper_disable_planes_on_crtc() after
>   drm_atomic_helper_commit_planes_on_crtc() in drm_atomic_helper.c 
>   to have functions in the same order as in drm_atomic_helper.h
>  
> Here the patch is again with latest review comment addressed, however
> I am not sure if we are going use the drm_atomic_helper_disable_planes_on_crtc()
> in omapdrm.
> 
> BTW, the latest change to this patch makes me wonder a how the
> atomic_begin() and atomic_flush() are supposed to work. 
> 
> 1. Should it be allowed to call atomic_begin() and atomic_flush()
> multiple times in a recursive manner?

No. Since the idea is to call this new disable_planes_on_crtc from
crtc_funcs->disable, which is called from commit_disables() there's not
problem. atomic_begin/end is just for plane updates (not for
enabling/disabling the entire display pipeline), and that's done with the
various commit_planes()/on_crtc() functions.

> If yes, how should the driver cope with that? Keep refcount of
> atomic_begins and do the magic only after the final matching
> atomic_flush comes and the refcount reaches zero?
> 
> 2. What about calling them consecutively (but not recursively)
> multiple times without vblank happening in the middle?
> 
> I am afraid that the current omapdrm implementation does not cope too
> well in either situation.

Currently atomic updates should only happen once per vblank (there's
planes for benchmark mode where we want to flip faster). Maybe just add a
drm_crtc_wait_one_vblank() call after calling this in your crtc_disable
functions?

Anyway, patch looks great, applied to drm-misc.

Thanks, Daniel

> 
> Cheers,
> Jyri
> 
> [1] http://www.spinics.net/lists/dri-devel/msg94942.html
> 
>  drivers/gpu/drm/drm_atomic_helper.c | 43 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  2 ++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 0c6f621..1ab6821 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1334,6 +1334,49 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
>  EXPORT_SYMBOL(drm_atomic_helper_commit_planes_on_crtc);
>  
>  /**
> + * drm_atomic_helper_disable_planes_on_crtc - helper to disable CRTC's planes
> + * @crtc: CRTC
> + * @atomic: if set, synchronize with CRTC's atomic_begin/flush hooks
> + *
> + * Disables all planes associated with the given CRTC. This can be
> + * used for instance in the CRTC helper disable callback to disable
> + * all planes before shutting down the display pipeline.
> + *
> + * If the atomic-parameter is set the function calls the CRTC's
> + * atomic_begin hook before and atomic_flush hook after disabling the
> + * planes.
> + *
> + * It is a bug to call this function without having implemented the
> + * ->atomic_disable() plane hook.
> + */
> +void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc,
> +					      bool atomic)
> +{
> +	const struct drm_crtc_helper_funcs *crtc_funcs =
> +		crtc->helper_private;
> +	struct drm_plane *plane;
> +
> +	if (atomic && crtc_funcs && crtc_funcs->atomic_begin)
> +		crtc_funcs->atomic_begin(crtc, NULL);
> +
> +	drm_for_each_plane(plane, crtc->dev) {
> +		const struct drm_plane_helper_funcs *plane_funcs =
> +			plane->helper_private;
> +
> +		if (plane->state->crtc != crtc || !plane_funcs)
> +			continue;
> +
> +		WARN_ON(!plane_funcs->atomic_disable);
> +		if (plane_funcs->atomic_disable)
> +			plane_funcs->atomic_disable(plane, NULL);
> +	}
> +
> +	if (atomic && crtc_funcs && crtc_funcs->atomic_flush)
> +		crtc_funcs->atomic_flush(crtc, NULL);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_disable_planes_on_crtc);
> +
> +/**
>   * drm_atomic_helper_cleanup_planes - cleanup plane resources after commit
>   * @dev: DRM device
>   * @old_state: atomic state object with old state structures
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 8cba54a..b7d4237 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -62,6 +62,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  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);
> +void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc,
> +					      bool atomic);
>  
>  void drm_atomic_helper_swap_state(struct drm_device *dev,
>  				  struct drm_atomic_state *state);
> -- 
> 1.9.1
> 

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