[PATCH] drm/atomic_helper: Add drm_atomic_helper_disable_planes_on_crtc()

[Daniel Vetter]

On Mon, Nov 23, 2015 at 12:44:35PM +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 at ti.com>
> ---
> This a follow version to "[PATCH RFC] drm/crtc_helper: Add
> drm_crtc_helper_disable_planes()"-patch [1], with Daniel Vetter's
> review comments [2] implemented. Hope I got everything right this
> time. Thanks a lot for the prompt review.

When resending a patch please add a revision log to remind reviewers of
what you changed. Otherwise they have to dig out the old thread again to
figure that out. E.g.

v2 per Daniel's feedback:
- Improve kerneldoc.
- WARN_ON when atomic_disable is missing.
- Also use crtc_funcs->atomic_begin/atomic_flush optionally.
> 
> Best regards,
> Jyri
> 
> [1] http://www.spinics.net/lists/dri-devel/msg94720.html
> [2] http://www.spinics.net/lists/dri-devel/msg94722.html
> 
>  drivers/gpu/drm/drm_atomic_helper.c | 52 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  2 ++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a53ed7a..229c810 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1281,6 +1281,58 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_atomic_helper_commit_planes);
>  
>  /**
> + * 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 there are planes to disable and 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;
> +	bool flush = false;
> +
> +	if (!crtc_funcs || !crtc_funcs->atomic_begin)
> +		atomic = false;
> +
> +	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;
> +
> +		if (atomic && !flush) {
> +			crtc_funcs->atomic_begin(crtc, NULL);
> +			flush = true;

I think it's clearer if you do that upfront with a simple

	if (crtc_funcs->atomic_begin && atomic)
		crtc_funcs->atomic_begin();

> +		}
> +
> +		WARN_ON(!plane_funcs->atomic_disable);
> +		if (plane_funcs->atomic_disable)
> +			plane_funcs->atomic_disable(plane, NULL);
> +	}
> +
> +	if (flush) {
> +		WARN_ON(!crtc_funcs->atomic_flush);
> +		if (crtc_funcs->atomic_flush)
> +			crtc_funcs->atomic_flush(crtc, NULL);
> +	}

Same here below. Imo the tracking you do in flush isn't required, since
drivers can opt to not implement either atomic_begin or atomic_flush (on
some hw you only need atomic_flush, and your code would break such a
driver here).

In short the code flow for atomic_begin/flush should look exactly like in
drm_atomic_helper_commit_planes_on_crtc, except for the added && atomic
check.

Otherwise looks good.

Cheers, Daniel

> +}
> +EXPORT_SYMBOL(drm_atomic_helper_disable_planes_on_crtc);
> +
> +/**
>   * drm_atomic_helper_commit_planes_on_crtc - commit plane state for a crtc
>   * @old_crtc_state: atomic state object with the old crtc state
>   *
> 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