Re: [PATCH v3 03/11] drm/atomic: Create function to insert plane state into a commit

[Thomas Zimmermann <tzimmermann@suse.de>]

Am 08.09.26 um 16:35 schrieb Maxime Ripard:
> drm_atomic_get_plane_state() allocates a new plane state by
> duplicating the current one and inserts it into the atomic commit as
> a single operation.
>
> However, a later change will need to insert a plane state into a
> commit without going through the full allocation and duplication path
> in drm_atomic_get_plane_state().
>
> Extract the state insertion logic into a new static
> drm_atomic_commit_set_plane_state() helper, and convert
> drm_atomic_get_plane_state() to use it.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

But there's a question below.

> ---
>   drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++------
>   1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 153d959bd4ff..f62323679f44 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -624,10 +624,27 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
>   	}
>   
>   	return 0;
>   }
>   
> +static int drm_atomic_commit_set_plane_state(struct drm_atomic_commit *commit,
> +					     struct drm_plane *plane,
> +					     struct drm_plane_state *plane_state)
> +{
> +	int index = drm_plane_index(plane);
> +
> +	drm_modeset_lock_assert_held(&plane->mutex);
> +
> +	commit->planes[index].state_to_destroy = plane_state;
> +	commit->planes[index].old_state = plane->state;
> +	commit->planes[index].new_state = plane_state;
> +	commit->planes[index].ptr = plane;
> +	plane_state->state = commit;
> +
> +	return 0;

I've notices that in other commits as well: why does it have a return 
value when the function cannot fail?  If there isn't a need later on, it 
should preferably by removed.

Best regards
Thomas

> +}
> +
>   /**
>    * drm_atomic_get_plane_state - get plane state
>    * @state: global atomic state object
>    * @plane: plane to get state object for
>    *
> @@ -642,11 +659,11 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
>    */
>   struct drm_plane_state *
>   drm_atomic_get_plane_state(struct drm_atomic_commit *state,
>   			  struct drm_plane *plane)
>   {
> -	int ret, index = drm_plane_index(plane);
> +	int ret;
>   	struct drm_plane_state *plane_state;
>   
>   	WARN_ON(!state->acquire_ctx);
>   	drm_WARN_ON(state->dev, state->checked);
>   
> @@ -665,15 +682,15 @@ drm_atomic_get_plane_state(struct drm_atomic_commit *state,
>   
>   	plane_state = plane->funcs->atomic_duplicate_state(plane);
>   	if (!plane_state)
>   		return ERR_PTR(-ENOMEM);
>   
> -	state->planes[index].state_to_destroy = plane_state;
> -	state->planes[index].ptr = plane;
> -	state->planes[index].old_state = plane->state;
> -	state->planes[index].new_state = plane_state;
> -	plane_state->state = state;
> +	ret = drm_atomic_commit_set_plane_state(state, plane, plane_state);
> +	if (ret) {
> +		plane->funcs->atomic_destroy_state(plane, plane_state);
> +		return ERR_PTR(ret);
> +	}
>   
>   	drm_dbg_atomic(plane->dev, "Added [PLANE:%d:%s] %p state to %p\n",
>   		       plane->base.id, plane->name, plane_state, state);
>   
>   	if (plane_state->crtc) {
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)