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