Re: [PATCH v4 35/41] drm/rcar-du: Convert to atomic_create_state

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Maxime,

Thank you for the patch.

On Tue, Sep 08, 2026 at 04:47:06PM +0200, Maxime Ripard wrote:
> The plane reset implementation creates a custom state
> subclass, but only initializes a pristine state without resetting any
> hardware. This is equivalent to what atomic_create_state expects.
> Convert to it.
> 
> The conversion was done using the following Coccinelle semantic patch:
> 
> @@
> identifier funcs;
> symbol drm_atomic_helper_plane_reset;
> symbol drm_atomic_helper_plane_create_state;
> @@
> 
> struct drm_plane_funcs funcs = {
>   ...,
> - .reset = drm_atomic_helper_plane_reset,
> + .atomic_create_state = drm_atomic_helper_plane_create_state,
>   ...,
> };
> 
> @match_struct_reset@
> identifier funcs, reset_func;
> @@
> struct drm_plane_funcs funcs = {
>     ...,
>     .reset = reset_func,
>     ...,
> };
> 
> @reset_uses_helpers depends on match_struct_reset@
> identifier match_struct_reset.reset_func;
> @@
> 
>  void reset_func(...)
>  {
>  	<+...
> (
>  	__drm_atomic_helper_plane_reset(...);
> |
> 	__drm_gem_reset_shadow_plane(...);
> )
>  	...+>
>  }
> 
> @match_struct_destroy@
> identifier funcs, destroy_func;
> @@
> struct drm_plane_funcs funcs = {
>     ...,
>     .atomic_destroy_state = destroy_func,
>     ...,
> };
> 
> @script:python renamed_func@
> old_name << match_struct_reset.reset_func;
> new_name;
> @@
> if old_name.endswith("_reset"):
>     coccinelle.new_name = old_name.replace("_reset", "_create_state")
> else:
>     coccinelle.new_name = old_name
> 
> @update_struct depends on match_struct_reset && reset_uses_helpers@
> identifier match_struct_reset.funcs, match_struct_reset.reset_func;
> identifier renamed_func.new_name;
> @@
> struct drm_plane_funcs funcs = {
>     ...,
> -   .reset = reset_func,
> +   .atomic_create_state = new_name,
>     ...,
> };
> 
> @drop_destroy depends on update_struct && match_struct_destroy@
> identifier match_struct_reset.reset_func;
> identifier match_struct_destroy.destroy_func;
> identifier container_func;
> identifier P;
> symbol drm_atomic_helper_plane_destroy_state;
> symbol __drm_atomic_helper_plane_destroy_state;
> @@
> 
>  void reset_func(struct drm_plane *P)
>  {
>  	...
> (
> -	if (P->state) {
> - 		<+...
> (
> -		drm_atomic_helper_plane_destroy_state(P, P->state);
> |
> -		__drm_atomic_helper_plane_destroy_state(P->state);
> |
> -		P->funcs->atomic_destroy_state(P, P->state);
> |
> -		destroy_func(P, P->state);
> )
> - 		...+>
> - 	}
> |
> -	drm_WARN_ON_ONCE(P->dev, P->state);
> |
> -	WARN_ON(P->state);
> )
>  	...
> (
> -	kfree(P->state);
> |
> -	kfree(container_func(P->state));
> |
>  	// kfree is optional
> )
> (
> -	P->state = NULL;
> |
>  	// plane->state clearing is optional
> )
>  	...
>  }
> 
> @drop_destroy_mtk depends on update_struct@
> identifier P;
> symbol __drm_atomic_helper_plane_destroy_state;
> symbol to_mtk_plane_state;
> @@
> 
>  void mtk_plane_reset(struct drm_plane *P)
>  {
>  	...
> -	if (P->state) {
> -		__drm_atomic_helper_plane_destroy_state(P->state);
> -		...
> -	} else {
>  		...
> -	}
>  	...
>  }
> 
> @transform_nv50_wndw depends on update_struct@
> identifier S;
> @@
> 
>  void nv50_wndw_reset(...)
>  {
>  	...
> -	if (WARN_ON(!(S = kzalloc_obj(*S))))
> +	S = kzalloc_obj(*S);
> +	if (WARN_ON(!S))
>  		return;
>  	...
>  }
> 
> @transform_kzalloc depends on update_struct@
> identifier match_struct_reset.reset_func;
> identifier P, S;
> statement ST;
> statement list STL;
> @@
> 
>  void reset_func(struct drm_plane *P)
>  {
>  	<...
>  	S = kzalloc_obj(*S);
> (
> -	if (S)
> -	{
> -		STL
> -	}
> +	if (!S) return;
> +
> +	STL
> |
> -	if (S) ST
> +	if (!S) return;
> +
> +	ST
> )
> 	...>
>  }
> 
> @transform_body depends on update_struct@
> identifier match_struct_reset.reset_func;
> identifier renamed_func.new_name;
> identifier S, P;
> expression PS;
> @@
> - void reset_func(struct drm_plane *P)
> + struct drm_plane_state *new_name(struct drm_plane *P)
> {
> 	...
>  	S = kzalloc_obj(*S);
> 	...
> (
>  	if (!S) {
> 		...
> -		return;
> +		return ERR_PTR(-ENOMEM);
>  	}
> |
>  	if (WARN_ON(!S)) {
> 		...
> -		return;
> +		return ERR_PTR(-ENOMEM);
>  	}
> |
>  	if (S == NULL) {
>  		...
> -		return;
> +		return ERR_PTR(-ENOMEM);
>  	}
> )
> 	...
> (
> -	__drm_atomic_helper_plane_reset(P, PS);
> +	__drm_atomic_helper_plane_state_init(PS, P);
> |
> -	__drm_gem_reset_shadow_plane(P, PS);
> +	__drm_gem_shadow_plane_state_init(P, PS);
> )
> 	...
> }
> 
> @update_early_return depends on update_struct@
> identifier match_struct_reset.reset_func;
> identifier renamed_func.new_name;
> identifier P;
> expression PS;
> @@
>  struct drm_plane_state *new_name(struct drm_plane *P)
> {
> 	<+...
> -	return;
> +	return ERR_PTR(-EINVAL);
> 	...+>
> }
> 
> @update_return_plane depends on update_struct@
> identifier match_struct_reset.reset_func;
> identifier renamed_func.new_name;
> identifier P;
> expression PS;
> @@
>  struct drm_plane_state *new_name(struct drm_plane *P)
> {
> 	...
>  	__drm_atomic_helper_plane_state_init(PS, P);
> 	...
> +
> +	return PS;
> }
> 
> @update_return_shadow depends on update_struct@
> identifier renamed_func.new_name;
> identifier P;
> expression PS;
> @@
>  struct drm_plane_state *new_name(struct drm_plane *P)
> {
> 	...
>  	__drm_gem_shadow_plane_state_init(P, PS);
> 	...
> +
> +	return &PS->base;
> }
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

An impressive semantic patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
> Cc: geert+renesas@glider.be
> Cc: kieran.bingham+renesas@ideasonboard.com
> Cc: laurent.pinchart+renesas@ideasonboard.com
> Cc: linux-renesas-soc@vger.kernel.org
> Cc: magnus.damm@gmail.com
> Cc: tomi.valkeinen+renesas@ideasonboard.com
> ---
>  drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c | 15 ++++++---------
>  drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.c   | 15 ++++++---------
>  2 files changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c
> index 8870766b9e54..da2dff9bb317 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c
> @@ -710,28 +710,25 @@ static void rcar_du_plane_atomic_destroy_state(struct drm_plane *plane,
>  {
>  	__drm_atomic_helper_plane_destroy_state(state);
>  	kfree(to_rcar_plane_state(state));
>  }
>  
> -static void rcar_du_plane_reset(struct drm_plane *plane)
> +static struct drm_plane_state *rcar_du_plane_create_state(struct drm_plane *plane)
>  {
>  	struct rcar_du_plane_state *state;
>  
> -	if (plane->state) {
> -		rcar_du_plane_atomic_destroy_state(plane, plane->state);
> -		plane->state = NULL;
> -	}
> -
>  	state = kzalloc_obj(*state);
>  	if (state == NULL)
> -		return;
> +		return ERR_PTR(-ENOMEM);
>  
> -	__drm_atomic_helper_plane_reset(plane, &state->state);
> +	__drm_atomic_helper_plane_state_init(&state->state, plane);
>  
>  	state->hwindex = -1;
>  	state->source = RCAR_DU_PLANE_MEMORY;
>  	state->colorkey = RCAR_DU_COLORKEY_NONE;
> +
> +	return &state->state;
>  }
>  
>  static int rcar_du_plane_atomic_set_property(struct drm_plane *plane,
>  					     struct drm_plane_state *state,
>  					     struct drm_property *property,
> @@ -765,11 +762,11 @@ static int rcar_du_plane_atomic_get_property(struct drm_plane *plane,
>  }
>  
>  static const struct drm_plane_funcs rcar_du_plane_funcs = {
>  	.update_plane = drm_atomic_helper_update_plane,
>  	.disable_plane = drm_atomic_helper_disable_plane,
> -	.reset = rcar_du_plane_reset,
> +	.atomic_create_state = rcar_du_plane_create_state,
>  	.destroy = drm_plane_cleanup,
>  	.atomic_duplicate_state = rcar_du_plane_atomic_duplicate_state,
>  	.atomic_destroy_state = rcar_du_plane_atomic_destroy_state,
>  	.atomic_set_property = rcar_du_plane_atomic_set_property,
>  	.atomic_get_property = rcar_du_plane_atomic_get_property,
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.c
> index ae9f381b03c8..4293e792afdb 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.c
> @@ -419,30 +419,27 @@ static void rcar_du_vsp_plane_atomic_destroy_state(struct drm_plane *plane,
>  {
>  	__drm_atomic_helper_plane_destroy_state(state);
>  	kfree(to_rcar_vsp_plane_state(state));
>  }
>  
> -static void rcar_du_vsp_plane_reset(struct drm_plane *plane)
> +static struct drm_plane_state *rcar_du_vsp_plane_create_state(struct drm_plane *plane)
>  {
>  	struct rcar_du_vsp_plane_state *state;
>  
> -	if (plane->state) {
> -		rcar_du_vsp_plane_atomic_destroy_state(plane, plane->state);
> -		plane->state = NULL;
> -	}
> -
>  	state = kzalloc_obj(*state);
>  	if (state == NULL)
> -		return;
> +		return ERR_PTR(-ENOMEM);
>  
> -	__drm_atomic_helper_plane_reset(plane, &state->state);
> +	__drm_atomic_helper_plane_state_init(&state->state, plane);
> +
> +	return &state->state;
>  }
>  
>  static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = {
>  	.update_plane = drm_atomic_helper_update_plane,
>  	.disable_plane = drm_atomic_helper_disable_plane,
> -	.reset = rcar_du_vsp_plane_reset,
> +	.atomic_create_state = rcar_du_vsp_plane_create_state,
>  	.destroy = drm_plane_cleanup,
>  	.atomic_duplicate_state = rcar_du_vsp_plane_atomic_duplicate_state,
>  	.atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state,
>  };
>  

-- 
Regards,

Laurent Pinchart