[PATCH 3/5] drm/plane: Add optional ->atomic_disable() callback

[Daniel Vetter]

On Fri, Nov 21, 2014 at 05:23:51PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding at nvidia.com>
> 
> In order to prevent drivers from having to perform the same checks over
> and over again, add an optional ->atomic_disable callback which the core
> calls under the right circumstances.
> 
> Signed-off-by: Thierry Reding <treding at nvidia.com>

One comment below, looks good otherwise.
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++++++++-
>  drivers/gpu/drm/drm_plane_helper.c  |  9 ++++++++-
>  include/drm/drm_crtc.h              | 24 ++++++++++++++++++++++++
>  include/drm/drm_plane_helper.h      |  3 +++
>  4 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d55f4d0ce990..810818fe8fdf 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1017,8 +1017,19 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  			continue;
>  
>  		funcs = plane->helper_private;
> +		if (!funcs)
> +			continue;
> +
> +		/*
> +		 * Special-case disabling the plane if drivers support it.
> +		 */
> +		if (drm_plane_disabled(plane) && funcs->atomic_disable) {
> +			/* NOTE: state is the old state here. */
> +			funcs->atomic_disable(plane, state);
> +			continue;
> +		}
>  
> -		if (!funcs || !funcs->atomic_update)
> +		if (!funcs->atomic_update)
>  			continue;
>  
>  		/* NOTE: state is the old state here. */
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index aa399db5d36d..881e0100fa49 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -443,7 +443,14 @@ int drm_plane_helper_commit(struct drm_plane *plane,
>  			crtc_funcs[i]->atomic_begin(crtc[i]);
>  	}
>  
> -	plane_funcs->atomic_update(plane, plane_state);
> +	/*
> +	 * Drivers may optionally implement the ->atomic_disable callback, so
> +	 * special-case that here.
> +	 */
> +	if (drm_plane_disabled(plane) && plane_funcs->atomic_disable)
> +		plane_funcs->atomic_disable(plane, plane_state);
> +	else
> +		plane_funcs->atomic_update(plane, plane_state);
>  
>  	for (i = 0; i < 2; i++) {
>  		if (crtc_funcs[i] && crtc_funcs[i]->atomic_flush)
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 6da67dfcb6fc..7b8750e79383 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -795,6 +795,30 @@ struct drm_plane {
>  	struct drm_plane_state *state;
>  };
>  
> +/*
> + * drm_plane_disabled - check whether a plane is being disabled
> + * @plane: plane object
> + *
> + * Checks the atomic state of a plane to determine whether it's being disabled
> + * or not. This also WARNs if it detects an invalid state (both CRTC and FB
> + * need to either both be NULL or both be non-NULL).
> + *
> + * RETURNS:
> + * True if the plane is being disabled, false otherwise.
> + */
> +static inline bool drm_plane_disabled(struct drm_plane *plane)
> +{
> +	/*
> +	 * When disabling a plane, CRTC and FB should always be NULL together.
> +	 * Anything else should be considered a bug in the atomic core, so we
> +	 * gently warn about it.
> +	 */
> +	WARN_ON((plane->state->crtc == NULL && plane->state->fb != NULL) ||
> +		(plane->state->crtc != NULL && plane->state->fb == NULL));
> +
> +	return plane->state->crtc == NULL || plane->state->fb == NULL;

I think we should only detect disabling edges here so that multiple
disables (userspace or drm core might do stupid things) don't result in
multiple disable calls. So would amount to passing the old state and
changing the check to

	return old_state->crtc && !plane->state->crtc;

Imo keeping the || here is redudant, the WARN_ON is good enough.
-Daniel

> +}
> +
>  /**
>   * struct drm_bridge_funcs - drm_bridge control functions
>   * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index 0f2230311aa8..680be61ef20a 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -52,6 +52,7 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>   * @cleanup_fb: cleanup a framebuffer when it's no longer used by the plane
>   * @atomic_check: check that a given atomic state is valid and can be applied
>   * @atomic_update: apply an atomic state to the plane
> + * @atomic_disable: disable the plane
>   *
>   * The helper operations are called by the mid-layer CRTC helper.
>   */
> @@ -65,6 +66,8 @@ struct drm_plane_helper_funcs {
>  			    struct drm_plane_state *state);
>  	void (*atomic_update)(struct drm_plane *plane,
>  			      struct drm_plane_state *old_state);
> +	void (*atomic_disable)(struct drm_plane *plane,
> +			       struct drm_plane_state *old_state);
>  };
>  
>  static inline void drm_plane_helper_add(struct drm_plane *plane,
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch