在 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 <[email protected]>
> > ---
> > 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
