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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Nov 25, 2014 at 12:09:46PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@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.
> 
> v2: pass old state and detect edges to avoid calling ->atomic_disable on
> already disabled planes, remove redundant comment (Daniel Vetter)
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Some minor bikesheds about consistency and clarity below.
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++++--
>  drivers/gpu/drm/drm_plane_helper.c  | 10 +++++++++-
>  include/drm/drm_crtc.h              | 26 ++++++++++++++++++++++++++
>  include/drm/drm_plane_helper.h      |  3 +++
>  4 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 7f020403ffe0..a1c7d1b73f86 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1020,12 +1020,23 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>  			continue;
>  
>  		funcs = plane->helper_private;
> -
> -		if (!funcs || !funcs->atomic_update)
> +		if (!funcs)
>  			continue;
>  
>  		old_plane_state = old_state->plane_states[i];
>  
> +		/*
> +		 * Special-case disabling the plane if drivers support it.
> +		 */
> +		if (drm_plane_disabled(plane, old_plane_state) &&
> +		    funcs->atomic_disable) {
> +			funcs->atomic_disable(plane, old_plane_state);
> +			continue;
> +		}
> +
> +		if (!funcs->atomic_update)
> +			continue;

This looks dangerous - we really should either have the atomic_disable or
at least atomic_update. And plane transitional helpers exactly require
that. On the bikeshed front I also like the plane helper structure more
with the if () atomic_disalbel else atomic_update instead of the continue.

> +
>  		funcs->atomic_update(plane, old_plane_state);
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index aa399db5d36d..8c81d3a9e625 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -443,7 +443,15 @@ 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_state) &&
> +	    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..80d0f1c2b265 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -795,6 +795,32 @@ struct drm_plane {
>  	struct drm_plane_state *state;
>  };
>  
> +/*
> + * drm_plane_disabled - check whether a plane is being disabled
> + * @plane: plane object
> + * @old_state: previous atomic state
> + *
> + * 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,
> +				      struct drm_plane_state *old_state)
> +{
> +	/*
> +	 * 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 (!old_state || old_state->crtc) && !plane->state->crtc;

The !old_state check here confused me a bit, until I've realized that you
use this for transitional helpers too. What about adding

	/* Cope with NULL old_state for transitional helpers. */

right above?

> +}

Hm, given that this is used by helpers maybe place it into
drm_atomic_helper.h? I'm also not too happy about the name, disabled
doesn't clearly only mean the enable->disable transition. What about
drm_atomic_plane_disabling instead, to make it clear we only care about
the transition? After all your kerneldoc also uses continuous ("being
disabled").

> +
>  /**
>   * 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel