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

[Daniel Vetter <daniel@ffwll.ch>]

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