Re: [PATCH v2 51/61] drm/nouveau: Convert to atomic_create_state

[lyude@redhat.com]

On Fri, 2026-08-14 at 16:57 +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>
> ---
> Cc: dakr@kernel.org
> Cc: lyude@redhat.com
> Cc: nouveau@lists.freedesktop.org
> ---
>  drivers/gpu/drm/nouveau/dispnv50/wndw.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> index 15a322422f4e..378095138d12 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> @@ -750,22 +750,21 @@ nv50_wndw_zpos_default(struct drm_plane *plane)
>  {
>  	return (plane->type == DRM_PLANE_TYPE_PRIMARY) ? 0 :
>  	       (plane->type == DRM_PLANE_TYPE_OVERLAY) ? 1 : 255;
>  }
>  
> -static void
> -nv50_wndw_reset(struct drm_plane *plane)
> +static struct drm_plane_state *nv50_wndw_create_state(struct
> drm_plane *plane)
>  {
>  	struct nv50_wndw_atom *asyw;
>  
> -	if (WARN_ON(!(asyw = kzalloc_obj(*asyw))))
> -		return;
> +	asyw = kzalloc_obj(*asyw);
> +	if (WARN_ON(!asyw))
> +		return ERR_PTR(-ENOMEM);

Should this be drm_WARN_ON() while we're at it?

Otherwise, this looks good to me:

Reviewed-by: Lyude Paul <lyude@redhat.com>

>  
> -	if (plane->state)
> -		plane->funcs->atomic_destroy_state(plane, plane-
> >state);
> +	__drm_atomic_helper_plane_state_init(&asyw->state, plane);
>  
> -	__drm_atomic_helper_plane_reset(plane, &asyw->state);
> +	return &asyw->state;
>  }
>  
>  static void
>  nv50_wndw_destroy(struct drm_plane *plane)
>  {
> @@ -835,11 +834,11 @@ static bool
> nv50_plane_format_mod_supported(struct drm_plane *plane,
>  const struct drm_plane_funcs
>  nv50_wndw = {
>  	.update_plane = drm_atomic_helper_update_plane,
>  	.disable_plane = drm_atomic_helper_disable_plane,
>  	.destroy = nv50_wndw_destroy,
> -	.reset = nv50_wndw_reset,
> +	.atomic_create_state = nv50_wndw_create_state,
>  	.atomic_duplicate_state = nv50_wndw_atomic_duplicate_state,
>  	.atomic_destroy_state = nv50_wndw_atomic_destroy_state,
>  	.format_mod_supported = nv50_plane_format_mod_supported,
>  };
>