Create a subclass of drm_plane_state to store hardware-specific state
information (e.g. hardware plane format settings) in the future.

Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
Changes in v2:
- Add the #include clause for atomic state helpers, which was wrongly
  placed in the previous patch in v1.
- Switch to kzalloc_obj helper for allocating the state.

 drivers/gpu/drm/verisilicon/vs_plane.c        | 41 +++++++++++++++++++
 drivers/gpu/drm/verisilicon/vs_plane.h        | 14 +++++++
 .../gpu/drm/verisilicon/vs_primary_plane.c    |  6 +--
 3 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c b/drivers/gpu/drm/verisilicon/vs_plane.c
index fa88ed14e41d7..2fd4fb2a9591c 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_plane.c
@@ -6,6 +6,7 @@
 #include <linux/errno.h>
 #include <linux/printk.h>
 
+#include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_fb_dma_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_dma_helper.h>
@@ -124,3 +125,43 @@ dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
 
 	return dma_addr;
 }
+
+struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane *plane)
+{
+	struct vs_plane_state *vs_state;
+
+	if (WARN_ON(!plane->state))
+		return NULL;
+
+	vs_state = kmemdup(plane->state, sizeof(*vs_state), GFP_KERNEL);
+	if (!vs_state)
+		return NULL;
+
+	__drm_atomic_helper_plane_duplicate_state(plane, &vs_state->base);
+
+	return &vs_state->base;
+}
+
+void vs_plane_destroy_state(struct drm_plane *plane,
+			    struct drm_plane_state *state)
+{
+	__drm_atomic_helper_plane_destroy_state(state);
+	kfree(state);
+}
+
+/* Called during init to allocate the plane's atomic state. */
+void vs_plane_reset(struct drm_plane *plane)
+{
+	struct vs_plane_state *vs_state;
+
+	if (plane->state)
+		__drm_atomic_helper_plane_destroy_state(plane->state);
+
+	kfree(plane->state);
+
+	vs_state = kzalloc_obj(*vs_state, GFP_KERNEL);
+	if (!vs_state)
+		return;
+
+	__drm_atomic_helper_plane_reset(plane, &vs_state->base);
+}
diff --git a/drivers/gpu/drm/verisilicon/vs_plane.h b/drivers/gpu/drm/verisilicon/vs_plane.h
index a88cc19f2202e..12848a72af576 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.h
+++ b/drivers/gpu/drm/verisilicon/vs_plane.h
@@ -63,10 +63,24 @@ struct vs_format {
 	bool uv_swizzle;
 };
 
+struct vs_plane_state {
+	struct drm_plane_state base;
+};
+
+static inline struct vs_plane_state *state_to_vs_plane_state(struct drm_plane_state *state)
+{
+	return container_of(state, struct vs_plane_state, base);
+}
+
 int drm_format_to_vs_format(u32 drm_format, struct vs_format *vs_format);
 dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
 			      const struct drm_rect *src_rect);
 
+struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane *plane);
+void vs_plane_destroy_state(struct drm_plane *plane,
+			    struct drm_plane_state *state);
+void vs_plane_reset(struct drm_plane *plane);
+
 struct drm_plane *vs_primary_plane_init(struct drm_device *dev, struct vs_dc *dc);
 
 #endif /* _VS_PLANE_H_ */
diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
index e8fcb5958615c..bad0bc5e3242d 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
@@ -145,10 +145,10 @@ static const struct drm_plane_helper_funcs vs_primary_plane_helper_funcs = {
 };
 
 static const struct drm_plane_funcs vs_primary_plane_funcs = {
-	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
-	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state	= vs_plane_destroy_state,
+	.atomic_duplicate_state	= vs_plane_duplicate_state,
 	.disable_plane		= drm_atomic_helper_disable_plane,
-	.reset			= drm_atomic_helper_plane_reset,
+	.reset			= vs_plane_reset,
 	.update_plane		= drm_atomic_helper_update_plane,
 };
 
-- 
2.52.0

Hi

Am 05.03.26 um 08:59 schrieb Icenowy Zheng:
> Create a subclass of drm_plane_state to store hardware-specific state
> information (e.g. hardware plane format settings) in the future.
>
> Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> ---
> Changes in v2:
> - Add the #include clause for atomic state helpers, which was wrongly
>    placed in the previous patch in v1.
> - Switch to kzalloc_obj helper for allocating the state.
>
>   drivers/gpu/drm/verisilicon/vs_plane.c        | 41 +++++++++++++++++++
>   drivers/gpu/drm/verisilicon/vs_plane.h        | 14 +++++++
>   .../gpu/drm/verisilicon/vs_primary_plane.c    |  6 +--
>   3 files changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c b/drivers/gpu/drm/verisilicon/vs_plane.c
> index fa88ed14e41d7..2fd4fb2a9591c 100644
> --- a/drivers/gpu/drm/verisilicon/vs_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_plane.c
> @@ -6,6 +6,7 @@
>   #include <linux/errno.h>
>   #include <linux/printk.h>
>   
> +#include <drm/drm_atomic_state_helper.h>
>   #include <drm/drm_fb_dma_helper.h>
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_gem_dma_helper.h>
> @@ -124,3 +125,43 @@ dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
>   
>   	return dma_addr;
>   }
> +
> +struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane *plane)
> +{
> +	struct vs_plane_state *vs_state;
> +
> +	if (WARN_ON(!plane->state))

drm_WARN_ON() here. You can use plane->dev for the first argument.

> +		return NULL;
> +
> +	vs_state = kmemdup(plane->state, sizeof(*vs_state), GFP_KERNEL);

Not wrong, but I'd advise against memdup() because it doesn't understand 
copy-ing semantics of the data structures. And some fields should maybe 
not be copied.

I'd rather use kzalloc_obj() and then ...

> +	if (!vs_state)
> +		return NULL;
> +
> +	__drm_atomic_helper_plane_duplicate_state(plane, &vs_state->base);

... assign the fields manually after this call. That makes is clear 
what's copied and sets the remaining bits to zero.

> +
> +	return &vs_state->base;
> +}
> +
> +void vs_plane_destroy_state(struct drm_plane *plane,
> +			    struct drm_plane_state *state)
> +{
> +	__drm_atomic_helper_plane_destroy_state(state);
> +	kfree(state);
> +}
> +
> +/* Called during init to allocate the plane's atomic state. */
> +void vs_plane_reset(struct drm_plane *plane)
> +{
> +	struct vs_plane_state *vs_state;
> +
> +	if (plane->state)
> +		__drm_atomic_helper_plane_destroy_state(plane->state);
> +
> +	kfree(plane->state);

You can move this line into the conditional branch.

> +
> +	vs_state = kzalloc_obj(*vs_state, GFP_KERNEL);
> +	if (!vs_state)
> +		return;
> +
> +	__drm_atomic_helper_plane_reset(plane, &vs_state->base);
> +}
> diff --git a/drivers/gpu/drm/verisilicon/vs_plane.h b/drivers/gpu/drm/verisilicon/vs_plane.h
> index a88cc19f2202e..12848a72af576 100644
> --- a/drivers/gpu/drm/verisilicon/vs_plane.h
> +++ b/drivers/gpu/drm/verisilicon/vs_plane.h
> @@ -63,10 +63,24 @@ struct vs_format {
>   	bool uv_swizzle;
>   };
>   
> +struct vs_plane_state {
> +	struct drm_plane_state base;
> +};
> +
> +static inline struct vs_plane_state *state_to_vs_plane_state(struct drm_plane_state *state)

Maybe 'plane_state_to_vs_plane_state' oj just 'to_vs_plane_state'.

Best regards
Thomas

> +{
> +	return container_of(state, struct vs_plane_state, base);
> +}
> +
>   int drm_format_to_vs_format(u32 drm_format, struct vs_format *vs_format);
>   dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
>   			      const struct drm_rect *src_rect);
>   
> +struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane *plane);
> +void vs_plane_destroy_state(struct drm_plane *plane,
> +			    struct drm_plane_state *state);
> +void vs_plane_reset(struct drm_plane *plane);
> +
>   struct drm_plane *vs_primary_plane_init(struct drm_device *dev, struct vs_dc *dc);
>   
>   #endif /* _VS_PLANE_H_ */
> diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> index e8fcb5958615c..bad0bc5e3242d 100644
> --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> @@ -145,10 +145,10 @@ static const struct drm_plane_helper_funcs vs_primary_plane_helper_funcs = {
>   };
>   
>   static const struct drm_plane_funcs vs_primary_plane_funcs = {
> -	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> -	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
> +	.atomic_destroy_state	= vs_plane_destroy_state,
> +	.atomic_duplicate_state	= vs_plane_duplicate_state,
>   	.disable_plane		= drm_atomic_helper_disable_plane,
> -	.reset			= drm_atomic_helper_plane_reset,
> +	.reset			= vs_plane_reset,
>   	.update_plane		= drm_atomic_helper_update_plane,
>   };
>   

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

在 2026-03-05四的 11:34 +0100，Thomas Zimmermann写道：
> Hi
> 
> Am 05.03.26 um 08:59 schrieb Icenowy Zheng:
> > Create a subclass of drm_plane_state to store hardware-specific
> > state
> > information (e.g. hardware plane format settings) in the future.
> > 
> > Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> > ---
> > Changes in v2:
> > - Add the #include clause for atomic state helpers, which was
> > wrongly
> >    placed in the previous patch in v1.
> > - Switch to kzalloc_obj helper for allocating the state.
> > 
> >   drivers/gpu/drm/verisilicon/vs_plane.c        | 41
> > +++++++++++++++++++
> >   drivers/gpu/drm/verisilicon/vs_plane.h        | 14 +++++++
> >   .../gpu/drm/verisilicon/vs_primary_plane.c    |  6 +--
> >   3 files changed, 58 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c
> > b/drivers/gpu/drm/verisilicon/vs_plane.c
> > index fa88ed14e41d7..2fd4fb2a9591c 100644
> > --- a/drivers/gpu/drm/verisilicon/vs_plane.c
> > +++ b/drivers/gpu/drm/verisilicon/vs_plane.c
> > @@ -6,6 +6,7 @@
> >   #include <linux/errno.h>
> >   #include <linux/printk.h>
> >   
> > +#include <drm/drm_atomic_state_helper.h>
> >   #include <drm/drm_fb_dma_helper.h>
> >   #include <drm/drm_fourcc.h>
> >   #include <drm/drm_gem_dma_helper.h>
> > @@ -124,3 +125,43 @@ dma_addr_t vs_fb_get_dma_addr(struct
> > drm_framebuffer *fb,
> >   
> >   	return dma_addr;
> >   }
> > +
> > +struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane
> > *plane)
> > +{
> > +	struct vs_plane_state *vs_state;
> > +
> > +	if (WARN_ON(!plane->state))
> 
> drm_WARN_ON() here. You can use plane->dev for the first argument.

Sounds right.

> 
> > +		return NULL;
> > +
> > +	vs_state = kmemdup(plane->state, sizeof(*vs_state),
> > GFP_KERNEL);
> 
> Not wrong, but I'd advise against memdup() because it doesn't
> understand 
> copy-ing semantics of the data structures. And some fields should
> maybe 
> not be copied.
> 
> I'd rather use kzalloc_obj() and then ...
> 
> > +	if (!vs_state)
> > +		return NULL;
> > +
> > +	__drm_atomic_helper_plane_duplicate_state(plane,
> > &vs_state->base);
> 
> ... assign the fields manually after this call. That makes is clear 
> what's copied and sets the remaining bits to zero.

Okay. The AST demo you showed me in PATCH 3 seems to be quite
followable.

> > +
> > +	return &vs_state->base;
> > +}
> > +
> > +void vs_plane_destroy_state(struct drm_plane *plane,
> > +			    struct drm_plane_state *state)
> > +{
> > +	__drm_atomic_helper_plane_destroy_state(state);
> > +	kfree(state);
> > +}
> > +
> > +/* Called during init to allocate the plane's atomic state. */
> > +void vs_plane_reset(struct drm_plane *plane)
> > +{
> > +	struct vs_plane_state *vs_state;
> > +
> > +	if (plane->state)
> > +		__drm_atomic_helper_plane_destroy_state(plane-
> > >state);
> > +
> > +	kfree(plane->state);
> 
> You can move this line into the conditional branch.

Yes, this sounds more natural (although I followed what vc4 driver does
here).

> 
> > +
> > +	vs_state = kzalloc_obj(*vs_state, GFP_KERNEL);
> > +	if (!vs_state)
> > +		return;
> > +
> > +	__drm_atomic_helper_plane_reset(plane, &vs_state->base);
> > +}
> > diff --git a/drivers/gpu/drm/verisilicon/vs_plane.h
> > b/drivers/gpu/drm/verisilicon/vs_plane.h
> > index a88cc19f2202e..12848a72af576 100644
> > --- a/drivers/gpu/drm/verisilicon/vs_plane.h
> > +++ b/drivers/gpu/drm/verisilicon/vs_plane.h
> > @@ -63,10 +63,24 @@ struct vs_format {
> >   	bool uv_swizzle;
> >   };
> >   
> > +struct vs_plane_state {
> > +	struct drm_plane_state base;
> > +};
> > +
> > +static inline struct vs_plane_state
> > *state_to_vs_plane_state(struct drm_plane_state *state)
> 
> Maybe 'plane_state_to_vs_plane_state' oj just 'to_vs_plane_state'.

I will pick the latter. I think I removed `plane_` because of the
function name being too long.

Thanks,
Icenowy

> 
> Best regards
> Thomas