Re: [PATCH] drm/atomic-helper: Don't allocated plane state in CRTC check

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 29, 2022 at 04:07:14PM +0200, Thomas Zimmermann wrote:
> In drm_atomic_helper_check_crtc_state(), do not add a new plane state
> to the global state if it does not exist already. Adding a new plane
> state will results in overhead for the plane during the atomic-commit
> step.
> 
> For the test in drm_atomic_helper_check_crtc_state() to succeed, it is
> important that the CRTC has an enabled primary plane after the commit.
> This can be a newly enabled plane or an already enabled plane. So if a
> plane is not part of the commit, use the plane's existing state. The new
> helper drm_atomic_get_next_plane_state() returns the correct instance.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: d6b9af1097fe ("drm/atomic-helper: Add helper drm_atomic_helper_check_crtc_state()")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Jocelyn Falempe <jfalempe@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  4 +---
>  include/drm/drm_atomic.h            | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 98cc3137c062..463d4f3fa443 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -960,9 +960,7 @@ int drm_atomic_helper_check_crtc_state(struct drm_crtc_state *crtc_state,
>  
>  			if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>  				continue;
> -			plane_state = drm_atomic_get_plane_state(state, plane);
> -			if (IS_ERR(plane_state))
> -				return PTR_ERR(plane_state);
> +			plane_state = drm_atomic_get_next_plane_state(state, plane);
>  			if (plane_state->fb && plane_state->crtc) {

Hmm. Why this is even looking at these. If the plane is in the crtc's
plane_mask then surely plane_state->crtc must already point to this
crtc? And I don't think ->fb and ->crtc are allowed to disagree, so
if one is set then surely the other one must be as well or we'd
return an error at some point somewhere?

>  				has_primary_plane = true;
>  				break;
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 10b1990bc1f6..4006f2d459e3 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -623,6 +623,26 @@ drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
>  	return state->planes[drm_plane_index(plane)].new_state;
>  }
>  
> +/**
> + * drm_atomic_get_next_plane_state - get plane state after atomic commit
> + * @state: global atomic state object
> + * @plane: plane to grab
> + *
> + * This function returns the plane state that the given plane will have
> + * after the atomic commit. This will be the new plane state if the plane
> + * is part of the global atomic state, or the current state otherwise.
> + */
> +static inline struct drm_plane_state *
> +drm_atomic_get_next_plane_state(struct drm_atomic_state *state,
> +				struct drm_plane *plane)
> +{
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +
> +	if (plane_state)
> +		return plane_state;
> +	return plane->state;

You're not holding the appropriate lock so that does not look safe. Even
if you know somewhere, somehow this sort of thing to be safe I don't
think it's a good idea to promote this with any kind of official
function because someone will shoot themselves in the foot with it.

> +}
> +
>  /**
>   * drm_atomic_get_existing_connector_state - get connector state, if it exists
>   * @state: global atomic state object
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel