Re: [PATCH 43/60] drm/atmel-hlcdc: Convert to atomic_create_state

[<Manikandan.M@microchip.com>]

Hi Maxime Ripard,

On 7/9/26 17:21, Maxime Ripard wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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>
Tested the patch on sama5 platforms

Tested-by: Manikandan Muralidharan <manikandan.m@microchip.com>
> ---
> Cc: alexandre.belloni@bootlin.com
> Cc: claudiu.beznea@tuxon.dev
> Cc: dharma.b@microchip.com
> Cc: manikandan.m@microchip.com
> Cc: nicolas.ferre@microchip.com
> ---
>   drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 27 +++++++++++--------------
>   1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> index f17a832351e9..ff35251a0184 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> @@ -1181,40 +1181,37 @@ static void atmel_hlcdc_plane_atomic_destroy_state(struct drm_plane *p,
>          __drm_atomic_helper_plane_destroy_state(s);
> 
>          kfree(state);
>   }
> 
> -static void atmel_hlcdc_plane_reset(struct drm_plane *p)
> +static struct drm_plane_state *atmel_hlcdc_plane_create_state(struct drm_plane *p)
>   {
>          struct atmel_hlcdc_plane_state *state;
>          struct atmel_hlcdc_dc *dc = p->dev->dev_private;
>          struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p);
> 
> -       if (p->state) {
> -               atmel_hlcdc_plane_atomic_destroy_state(p, p->state);
> -               p->state = NULL;
> -       }
> -
>          state = kzalloc_obj(*state);
> -       if (state) {
> -               if (atmel_hlcdc_plane_alloc_dscrs(p, state)) {
> -                       kfree(state);
> -                       drm_err(p->dev,
> -                               "Failed to allocate initial plane state\n");
> -                       return;
> -               }
> -               __drm_atomic_helper_plane_reset(p, &state->base);
> +       if (!state)
> +               return ERR_PTR(-ENOMEM);
> +
> +       if (atmel_hlcdc_plane_alloc_dscrs(p, state)) {
> +               kfree(state);
> +               drm_err(p->dev, "Failed to allocate initial plane state\n");
> +               return ERR_PTR(-EINVAL);
>          }
> +       __drm_atomic_helper_plane_state_init(&state->base, p);
> 
>          if (plane->layer.desc->layout.csc)
>                  dc->desc->ops->lcdc_csc_init(plane, plane->layer.desc);
> +
> +       return &state->base;
>   }
> 
>   static const struct drm_plane_funcs layer_plane_funcs = {
>          .update_plane = drm_atomic_helper_update_plane,
>          .disable_plane = drm_atomic_helper_disable_plane,
> -       .reset = atmel_hlcdc_plane_reset,
> +       .atomic_create_state = atmel_hlcdc_plane_create_state,
>          .atomic_duplicate_state = atmel_hlcdc_plane_atomic_duplicate_state,
>          .atomic_destroy_state = atmel_hlcdc_plane_atomic_destroy_state,
>   };
> 
>   static int atmel_hlcdc_plane_create(struct drm_device *dev,
> 
> --
> 2.54.0
> 


-- 
Thanks and Regards,
Manikandan M.