Re: [PATCH v4 26/41] drm/amdgpu_dm: Convert to atomic_create_state

[Leo Li <sunpeng.li@amd.com>]

On 2026-09-08 10:46, 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>

Reviewed-by: Leo Li <sunpeng.li@amd.com>

Thanks!
Leo

> ---
> Cc: alexander.deucher@amd.com
> Cc: amd-gfx@lists.freedesktop.org
> Cc: christian.koenig@amd.com
> Cc: harry.wentland@amd.com
> Cc: siqueira@igalia.com
> Cc: sunpeng.li@amd.com
> ---
>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c    | 28 ++++++++++++--------
>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_plane.h    |  2 +-
>  .../display/amdgpu_dm/tests/amdgpu_dm_plane_test.c | 30 ++++++++--------------
>  3 files changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index 824ef3ce5de0..f9655f534d3b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -1786,28 +1786,27 @@ static const struct drm_plane_helper_funcs dm_primary_plane_helper_funcs = {
>  	.atomic_async_update = amdgpu_dm_plane_atomic_async_update,
>  	.get_scanout_buffer = amdgpu_display_get_scanout_buffer,
>  	.panic_flush = amdgpu_dm_plane_panic_flush,
>  };
>  
> -STATIC_IFN_KUNIT void amdgpu_dm_plane_drm_plane_reset(struct drm_plane *plane)
> +STATIC_IFN_KUNIT struct drm_plane_state *amdgpu_dm_plane_drm_plane_create_state(struct drm_plane *plane)
>  {
>  	struct dm_plane_state *amdgpu_state;
>  
>  	amdgpu_state = kzalloc_obj(*amdgpu_state);
>  	if (!amdgpu_state)
> -		return;
> +		return ERR_PTR(-ENOMEM);
>  
> -	if (plane->state)
> -		plane->funcs->atomic_destroy_state(plane, plane->state);
> -
> -	__drm_atomic_helper_plane_reset(plane, &amdgpu_state->base);
> +	__drm_atomic_helper_plane_state_init(&amdgpu_state->base, plane);
>  	amdgpu_state->degamma_tf = AMDGPU_TRANSFER_FUNCTION_DEFAULT;
>  	amdgpu_state->hdr_mult = AMDGPU_HDR_MULT_DEFAULT;
>  	amdgpu_state->shaper_tf = AMDGPU_TRANSFER_FUNCTION_DEFAULT;
>  	amdgpu_state->blend_tf = AMDGPU_TRANSFER_FUNCTION_DEFAULT;
> +
> +	return &amdgpu_state->base;
>  }
> -EXPORT_IF_KUNIT(amdgpu_dm_plane_drm_plane_reset);
> +EXPORT_IF_KUNIT(amdgpu_dm_plane_drm_plane_create_state);
>  
>  STATIC_IFN_KUNIT struct drm_plane_state *
>  amdgpu_dm_plane_drm_plane_duplicate_state(struct drm_plane *plane)
>  {
>  	struct dm_plane_state *dm_plane_state, *old_dm_plane_state;
> @@ -2166,11 +2165,11 @@ dm_plane_init_colorops(struct drm_plane *plane)
>  
>  static const struct drm_plane_funcs dm_plane_funcs = {
>  	.update_plane	= drm_atomic_helper_update_plane,
>  	.disable_plane	= drm_atomic_helper_disable_plane,
>  	.destroy	= drm_plane_helper_destroy,
> -	.reset = amdgpu_dm_plane_drm_plane_reset,
> +	.atomic_create_state = amdgpu_dm_plane_drm_plane_create_state,
>  	.atomic_duplicate_state = amdgpu_dm_plane_drm_plane_duplicate_state,
>  	.atomic_destroy_state = amdgpu_dm_plane_drm_plane_destroy_state,
>  	.format_mod_supported = amdgpu_dm_plane_format_mod_supported,
>  	.format_mod_supported_async = amdgpu_dm_plane_format_mod_supported,
>  #ifdef AMD_PRIVATE_COLOR
> @@ -2276,13 +2275,20 @@ int amdgpu_dm_plane_init(struct amdgpu_display_manager *dm,
>  	res = dm_plane_init_colorops(plane);
>  	if (res)
>  		return res;
>  #endif
>  
> -	/* Create (reset) the plane state */
> -	if (plane->funcs->reset)
> -		plane->funcs->reset(plane);
> +	/* Create the plane state */
> +	if (plane->funcs->atomic_create_state) {
> +		struct drm_plane_state *plane_state;
> +
> +		plane_state = plane->funcs->atomic_create_state(plane);
> +		if (IS_ERR(plane_state))
> +			return PTR_ERR(plane_state);
> +
> +		plane->state = plane_state;
> +	}
>  
>  	return 0;
>  }
>  
>  bool amdgpu_dm_plane_is_video_format(uint32_t format)
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.h
> index 092ade738ce9..6aaea290206d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.h
> @@ -115,11 +115,11 @@ void amdgpu_dm_plane_get_min_max_dc_plane_scaling(struct drm_device *dev,
>  int amdgpu_dm_plane_atomic_async_check(struct drm_plane *plane,
>  				       struct drm_atomic_commit *state, bool flip);
>  int amdgpu_dm_plane_atomic_check(struct drm_plane *plane,
>  				 struct drm_atomic_commit *state);
>  void amdgpu_dm_plane_panic_flush(struct drm_plane *plane);
> -void amdgpu_dm_plane_drm_plane_reset(struct drm_plane *plane);
> +struct drm_plane_state *amdgpu_dm_plane_drm_plane_create_state(struct drm_plane *plane);
>  struct drm_plane_state *amdgpu_dm_plane_drm_plane_duplicate_state(struct drm_plane *plane);
>  void amdgpu_dm_plane_drm_plane_destroy_state(struct drm_plane *plane,
>  					     struct drm_plane_state *state);
>  void amdgpu_dm_plane_add_modifier_dedup(uint64_t **mods, uint64_t *size,
>  					uint64_t *cap, uint64_t mod);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_plane_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_plane_test.c
> index ba97092c7bb8..ab425ee14f30 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_plane_test.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_plane_test.c
> @@ -2922,40 +2922,32 @@ static void dm_test_panic_flush_no_dc_state(struct kunit *test)
>  static const struct drm_plane_funcs dm_test_plane_reset_funcs = {
>  	.atomic_destroy_state = amdgpu_dm_plane_drm_plane_destroy_state,
>  };
>  
>  /**
> - * dm_test_plane_reset_initializes_state() - Verify reset installs default state.
> + * dm_test_plane_create_state_initializes_state() - Verify create_state allocates default state.
>   * @test: KUnit test context.
>   *
> - * Verify amdgpu_dm_plane_drm_plane_reset() destroys the existing plane state,
> - * allocates a fresh dm_plane_state, and initializes the AMD-specific transfer
> - * function and HDR multiplier defaults.
> + * Verify amdgpu_dm_plane_drm_plane_create_state() allocates a fresh
> + * dm_plane_state, and initializes the AMD-specific transfer function and HDR
> + * multiplier defaults.
>   */
> -static void dm_test_plane_reset_initializes_state(struct kunit *test)
> +static void dm_test_plane_create_state_initializes_state(struct kunit *test)
>  {
> -	struct dm_plane_state *old_state;
> +	struct drm_plane_state *plane_state;
>  	struct dm_plane_state *new_state;
>  	struct drm_plane *plane;
>  
>  	plane = kunit_kzalloc(test, sizeof(*plane), GFP_KERNEL);
>  	KUNIT_ASSERT_NOT_NULL(test, plane);
>  
> -	/*
> -	 * Provide an existing state plus a funcs table so reset exercises the
> -	 * destroy-existing-state path. The destroy hook frees this state, so it
> -	 * must be a plain (non-KUnit-managed) allocation.
> -	 */
> -	old_state = kzalloc(sizeof(*old_state), GFP_KERNEL);
> -	KUNIT_ASSERT_NOT_NULL(test, old_state);
>  	plane->funcs = &dm_test_plane_reset_funcs;
> -	plane->state = &old_state->base;
>  
> -	amdgpu_dm_plane_drm_plane_reset(plane);
> +	plane_state = amdgpu_dm_plane_drm_plane_create_state(plane);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, plane_state);
>  
> -	KUNIT_ASSERT_NOT_NULL(test, plane->state);
> -	new_state = to_dm_plane_state(plane->state);
> +	new_state = to_dm_plane_state(plane_state);
>  	KUNIT_EXPECT_EQ(test, new_state->degamma_tf, AMDGPU_TRANSFER_FUNCTION_DEFAULT);
>  	KUNIT_EXPECT_EQ(test, new_state->hdr_mult, AMDGPU_HDR_MULT_DEFAULT);
>  	KUNIT_EXPECT_EQ(test, new_state->shaper_tf, AMDGPU_TRANSFER_FUNCTION_DEFAULT);
>  	KUNIT_EXPECT_EQ(test, new_state->blend_tf, AMDGPU_TRANSFER_FUNCTION_DEFAULT);
>  
> @@ -3096,12 +3088,12 @@ static struct kunit_case amdgpu_dm_plane_test_cases[] = {
>  	KUNIT_CASE(dm_test_atomic_check_helper_failure),
>  	KUNIT_CASE(dm_test_atomic_check_color_pipeline_conflict),
>  	KUNIT_CASE(dm_test_atomic_check_scaling_failure),
>  	/* amdgpu_dm_plane_panic_flush() */
>  	KUNIT_CASE(dm_test_panic_flush_no_dc_state),
> -	/* amdgpu_dm_plane_drm_plane_reset() */
> -	KUNIT_CASE(dm_test_plane_reset_initializes_state),
> +	/* amdgpu_dm_plane_drm_plane_create_state() */
> +	KUNIT_CASE(dm_test_plane_create_state_initializes_state),
>  	/* amdgpu_dm_plane_drm_plane_duplicate_state() */
>  	KUNIT_CASE(dm_test_plane_duplicate_state_copies_fields),
>  	/* amdgpu_dm_plane_drm_plane_destroy_state() */
>  	KUNIT_CASE(dm_test_plane_destroy_state_minimal),
>  	/* amdgpu_dm_plane_add_modifier() */
>