[PATCH drm-misc-next v4 0/4] drm: verisilicon: convert drm_format to vs_format in atomic_check

[PATCH drm-misc-next v4 0/4] drm: verisilicon: convert drm_format to vs_format in atomic_check

[Icenowy Zheng]

This patchset tries to move the process of converting drm_format to
vs_format into plane's atomic_check callback (instead of the
atomic_commit one, which the process currently sits) for more proper
error handling (because atomic_commit cannot fail).

In addition, because of the original flow of primary plane's
atomic_check seems to be flawed, it's also updated before filling
vs_format is added there.

Icenowy Zheng (4):
  drm: verisilicon: make vs_format conversion function return int
  drm: verisilicon: subclass drm_plane_state
  drm: verisilicon: call atomic helper's plane state check even if no
    CRTC
  drm: verisilicon: fill plane's vs_format in atomic_check

 drivers/gpu/drm/verisilicon/vs_plane.c        | 54 ++++++++++++++++++-
 drivers/gpu/drm/verisilicon/vs_plane.h        | 18 ++++++-
 .../gpu/drm/verisilicon/vs_primary_plane.c    | 48 ++++++++++-------
 3 files changed, 98 insertions(+), 22 deletions(-)

-- 
2.52.0

[PATCH drm-misc-next v4 1/4] drm: verisilicon: make vs_format conversion function return int

[Icenowy Zheng]

This is for further proper invalid drm_format handling before committing
the plane state change.

The return value is not yet checked yet, and will be checked in
atomic_check in the future.

Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
Changes in v4:
- Reposition the R-b tag (in v3 it's wrongly placed before SoB).
Changes in v3:
- Add Thomas's R-b.
Changes in v2:
- Move a bogus #include clause to the next patch.

 drivers/gpu/drm/verisilicon/vs_plane.c | 6 ++++--
 drivers/gpu/drm/verisilicon/vs_plane.h | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c b/drivers/gpu/drm/verisilicon/vs_plane.c
index 2f3953e588a34..fa88ed14e41d7 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_plane.c
@@ -12,7 +12,7 @@
 
 #include "vs_plane.h"
 
-void drm_format_to_vs_format(u32 drm_format, struct vs_format *vs_format)
+int drm_format_to_vs_format(u32 drm_format, struct vs_format *vs_format)
 {
 	switch (drm_format) {
 	case DRM_FORMAT_XRGB4444:
@@ -62,7 +62,7 @@ void drm_format_to_vs_format(u32 drm_format, struct vs_format *vs_format)
 		vs_format->color = VSDC_COLOR_FORMAT_A2R10G10B10;
 		break;
 	default:
-		pr_warn("Unexpected drm format!\n");
+		return -EINVAL;
 	}
 
 	switch (drm_format) {
@@ -101,6 +101,8 @@ void drm_format_to_vs_format(u32 drm_format, struct vs_format *vs_format)
 
 	/* N/A for non-YUV formats */
 	vs_format->uv_swizzle = false;
+
+	return 0;
 }
 
 dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/verisilicon/vs_plane.h b/drivers/gpu/drm/verisilicon/vs_plane.h
index 41875ea3d66a5..a88cc19f2202e 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.h
+++ b/drivers/gpu/drm/verisilicon/vs_plane.h
@@ -63,7 +63,7 @@ struct vs_format {
 	bool uv_swizzle;
 };
 
-void drm_format_to_vs_format(u32 drm_format, struct vs_format *vs_format);
+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);
 
-- 
2.52.0

Claude review: drm: verisilicon: make vs_format conversion function return int

[Claude Code Review Bot]

Patch Review

**Status: Good**

Clean mechanical change. Converts `drm_format_to_vs_format()` from `void` to `int`, replacing the `pr_warn()` on unknown format with `return -EINVAL`, and adding `return 0` on the success path. The commit message correctly notes the return value is not yet checked — that comes in patch 4.

The `pr_warn()` removal is fine since callers will now check the return code. No issues.

---
Generated by Claude Code Patch Reviewer

[PATCH drm-misc-next v4 2/4] drm: verisilicon: subclass drm_plane_state

[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>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
Changes in v4:
- Add code clearing plane->state pointer after freeing it.
- Add Thomas's R-b.

Changes in v3:
- Switch to drm_WARN_ON().
- Stop to memdup the state.
- Move the code freeing the existing plane state to the branch checking
  whether it's not NULL.
- Rename the typecast function to `to_vs_plane_state()`.

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        | 43 +++++++++++++++++++
 drivers/gpu/drm/verisilicon/vs_plane.h        | 14 ++++++
 .../gpu/drm/verisilicon/vs_primary_plane.c    |  6 +--
 3 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c b/drivers/gpu/drm/verisilicon/vs_plane.c
index fa88ed14e41d7..7c6b905c9e1fa 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_plane.c
@@ -6,9 +6,11 @@
 #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>
+#include <drm/drm_print.h>
 
 #include "vs_plane.h"
 
@@ -124,3 +126,44 @@ 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 (drm_WARN_ON(plane->dev, !plane->state))
+		return NULL;
+
+	vs_state = kzalloc_obj(*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);
+		plane->state = NULL;
+	}
+
+	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..48ed8fc754d18 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 *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

Claude review: drm: verisilicon: subclass drm_plane_state

[Claude Code Review Bot]

Patch Review

**Status: Good**

Standard DRM plane state subclassing pattern. The implementation is correct:

- `vs_plane_reset()` properly frees any existing state, sets `plane->state = NULL`, then allocates and calls `__drm_atomic_helper_plane_reset()`.
- `vs_plane_duplicate_state()` uses `drm_WARN_ON` (device-aware) instead of bare `WARN_ON`.
- `vs_plane_destroy_state()` calls `__drm_atomic_helper_plane_destroy_state()` then `kfree()`.

The `vs_plane_state` struct starts empty (just `base`) — the `format` field is added in patch 4. This is fine for bisectability.

One note: `vs_plane_destroy_state()` does `kfree(state)` which frees the `drm_plane_state` pointer, not the `vs_plane_state` container. This works because `base` is the first member of `vs_plane_state`, so the addresses are identical. This is standard kernel container_of pattern and is correct.

---
Generated by Claude Code Patch Reviewer

[PATCH drm-misc-next v4 3/4] drm: verisilicon: call atomic helper's plane state check even if no CRTC

[Icenowy Zheng]

The `drm_atomic_helper_check_plane_state()` helper function needs to be
called even if the plane is bound to no CRTCs.

Remove the early return in the primary plane's atomic_check, and use
NULL for crtc_state in this situation.

Fixes: dbf21777caa8 ("drm: verisilicon: add a driver for Verisilicon display controllers")
Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
Changes in v4:
- Add Fixed tag.
- Add Thomas's R-b.

New patch in v3.

 drivers/gpu/drm/verisilicon/vs_primary_plane.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
index bad0bc5e3242d..421d6f9dc547b 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
@@ -26,14 +26,10 @@ static int vs_primary_plane_atomic_check(struct drm_plane *plane,
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
 	struct drm_crtc *crtc = new_plane_state->crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state = NULL;
 
-	if (!crtc)
-		return 0;
-
-	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
-	if (WARN_ON(!crtc_state))
-		return -EINVAL;
+	if (crtc)
+		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 
 	return drm_atomic_helper_check_plane_state(new_plane_state,
 						   crtc_state,
-- 
2.52.0

Claude review: drm: verisilicon: call atomic helper's plane state check even if no CRTC

[Claude Code Review Bot]

Patch Review

**Status: Good, with note**

Removes the early `return 0` when `!crtc` and instead passes `NULL` as `crtc_state` to `drm_atomic_helper_check_plane_state()`. Looking at the helper implementation, when `crtc` is NULL:
- Line 907: `WARN_ON(plane_state->crtc && ...)` — short-circuits since `plane_state->crtc` is NULL, no NULL deref.
- Line 912-915: If `!fb`, sets `visible = false` and returns 0.
- Line 918-921: If `!plane_state->crtc` (and fb is set), fires `WARN_ON` and sets `visible = false`.

So this is safe. The key benefit is that `plane_state->visible` and `plane_state->src`/`dst` get properly initialized even when the plane is being disabled, which is important for correctness. The `Fixes` tag is appropriate.

Also removes the `WARN_ON(!crtc_state)` after `drm_atomic_get_new_crtc_state()` — this is fine since that condition would indicate a framework bug and the helper already has its own warnings.

---
Generated by Claude Code Patch Reviewer

[PATCH drm-misc-next v4 4/4] drm: verisilicon: fill plane's vs_format in atomic_check

[Icenowy Zheng]

Move the conversion from drm_format to vs_format to atomic_check, which
is before the point of no return and can properly bail out.

Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
Changes in v4:
- Make atomic_check early return if the plane isn't visible.
- Added Thomas's R-b.

Changes in v3:
- Move the code filling vs_format to after drm_atomic_helper_check_plane_state()
  call.
- Add drm_WARN_ON_ONCE when filling vs_format fails.
- Add code copying vs_format in vs_plane_duplicate_state (because it no
  longer memdup's the whole struct vs_plane_state).

Changes in v2:
- Add non-NULL check for drm_plane_state->fb.

 drivers/gpu/drm/verisilicon/vs_plane.c        |  7 +++-
 drivers/gpu/drm/verisilicon/vs_plane.h        |  2 ++
 .../gpu/drm/verisilicon/vs_primary_plane.c    | 36 +++++++++++++------
 3 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c b/drivers/gpu/drm/verisilicon/vs_plane.c
index 7c6b905c9e1fa..d81f7b8f4c650 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_plane.c
@@ -129,17 +129,22 @@ dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb,
 
 struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane *plane)
 {
-	struct vs_plane_state *vs_state;
+	struct vs_plane_state *vs_state, *vs_state_old;
 
 	if (drm_WARN_ON(plane->dev, !plane->state))
 		return NULL;
 
+	vs_state_old = to_vs_plane_state(plane->state);
+
 	vs_state = kzalloc_obj(*vs_state, GFP_KERNEL);
 	if (!vs_state)
 		return NULL;
 
 	__drm_atomic_helper_plane_duplicate_state(plane, &vs_state->base);
 
+	memcpy(&vs_state->format, &vs_state_old->format,
+	       sizeof(struct vs_format));
+
 	return &vs_state->base;
 }
 
diff --git a/drivers/gpu/drm/verisilicon/vs_plane.h b/drivers/gpu/drm/verisilicon/vs_plane.h
index 48ed8fc754d18..f18c36ef4cd90 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.h
+++ b/drivers/gpu/drm/verisilicon/vs_plane.h
@@ -65,6 +65,8 @@ struct vs_format {
 
 struct vs_plane_state {
 	struct drm_plane_state base;
+
+	struct vs_format format;
 };
 
 static inline struct vs_plane_state *to_vs_plane_state(struct drm_plane_state *state)
diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
index 421d6f9dc547b..3576dd524f5c4 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
@@ -25,17 +25,32 @@ static int vs_primary_plane_atomic_check(struct drm_plane *plane,
 {
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
+	struct vs_plane_state *new_vs_plane_state = to_vs_plane_state(new_plane_state);
+	struct drm_framebuffer *fb = new_plane_state->fb;
 	struct drm_crtc *crtc = new_plane_state->crtc;
 	struct drm_crtc_state *crtc_state = NULL;
+	int ret;
 
 	if (crtc)
 		crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 
-	return drm_atomic_helper_check_plane_state(new_plane_state,
-						   crtc_state,
-						   DRM_PLANE_NO_SCALING,
-						   DRM_PLANE_NO_SCALING,
-						   false, true);
+	ret = drm_atomic_helper_check_plane_state(new_plane_state,
+						  crtc_state,
+						  DRM_PLANE_NO_SCALING,
+						  DRM_PLANE_NO_SCALING,
+						  false, true);
+	if (ret)
+		return ret;
+
+	if (!new_plane_state->visible)
+		return 0;
+
+	ret = drm_format_to_vs_format(fb->format->format,
+				      &new_vs_plane_state->format);
+	if (drm_WARN_ON_ONCE(plane->dev, ret))
+		return ret;
+
+	return 0;
 }
 
 static void vs_primary_plane_commit(struct vs_dc *dc, unsigned int output)
@@ -84,11 +99,11 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
 {
 	struct drm_plane_state *state = drm_atomic_get_new_plane_state(atomic_state,
 								       plane);
+	struct vs_plane_state *vs_state = to_vs_plane_state(state);
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_crtc *crtc = state->crtc;
 	struct vs_dc *dc;
 	struct vs_crtc *vcrtc;
-	struct vs_format fmt;
 	unsigned int output;
 	dma_addr_t dma_addr;
 
@@ -101,16 +116,15 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
 	output = vcrtc->id;
 	dc = vcrtc->dc;
 
-	drm_format_to_vs_format(state->fb->format->format, &fmt);
-
 	regmap_update_bits(dc->regs, VSDC_FB_CONFIG(output),
 			   VSDC_FB_CONFIG_FMT_MASK,
-			   VSDC_FB_CONFIG_FMT(fmt.color));
+			   VSDC_FB_CONFIG_FMT(vs_state->format.color));
 	regmap_update_bits(dc->regs, VSDC_FB_CONFIG(output),
 			   VSDC_FB_CONFIG_SWIZZLE_MASK,
-			   VSDC_FB_CONFIG_SWIZZLE(fmt.swizzle));
+			   VSDC_FB_CONFIG_SWIZZLE(vs_state->format.swizzle));
 	regmap_assign_bits(dc->regs, VSDC_FB_CONFIG(output),
-			   VSDC_FB_CONFIG_UV_SWIZZLE_EN, fmt.uv_swizzle);
+			   VSDC_FB_CONFIG_UV_SWIZZLE_EN,
+			   vs_state->format.uv_swizzle);
 
 	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
 
-- 
2.52.0

Claude review: drm: verisilicon: fill plane's vs_format in atomic_check

[Claude Code Review Bot]

Patch Review

**Status: Good**

This is the payoff patch. It:

1. Adds `struct vs_format format` to `vs_plane_state`.
2. Copies the format in `vs_plane_duplicate_state()` so it persists across state duplication.
3. In `atomic_check`, after `drm_atomic_helper_check_plane_state()` succeeds and confirms visibility, converts the format and stores it in the vs_plane_state.
4. In `atomic_update`, reads from `vs_state->format` instead of calling `drm_format_to_vs_format()` inline.

The `drm_WARN_ON_ONCE` around the format conversion failure in `atomic_check` is appropriate — since the format list is controlled by the driver, an unsupported format reaching this point indicates a driver bug (the format should have been rejected earlier), so a warning + error return is the right response.

The early return on `!new_plane_state->visible` before the format conversion is correct — if the plane isn't visible, there's no framebuffer to convert, and `fb` could be NULL.

The `memcpy` of `vs_format` in `vs_plane_duplicate_state()` could alternatively just assign `vs_state->format = vs_state_old->format` (struct assignment), but `memcpy` is fine and common in kernel code.

---
Generated by Claude Code Patch Reviewer

Re: [PATCH drm-misc-next v4 0/4] drm: verisilicon: convert drm_format to vs_format in atomic_check

[Thomas Zimmermann]

Hi,

I only looked briefly over the update, but it seems good now.

Best regards
Thomas


Am 31.03.26 um 08:01 schrieb Icenowy Zheng:
> This patchset tries to move the process of converting drm_format to
> vs_format into plane's atomic_check callback (instead of the
> atomic_commit one, which the process currently sits) for more proper
> error handling (because atomic_commit cannot fail).
>
> In addition, because of the original flow of primary plane's
> atomic_check seems to be flawed, it's also updated before filling
> vs_format is added there.
>
> Icenowy Zheng (4):
>    drm: verisilicon: make vs_format conversion function return int
>    drm: verisilicon: subclass drm_plane_state
>    drm: verisilicon: call atomic helper's plane state check even if no
>      CRTC
>    drm: verisilicon: fill plane's vs_format in atomic_check
>
>   drivers/gpu/drm/verisilicon/vs_plane.c        | 54 ++++++++++++++++++-
>   drivers/gpu/drm/verisilicon/vs_plane.h        | 18 ++++++-
>   .../gpu/drm/verisilicon/vs_primary_plane.c    | 48 ++++++++++-------
>   3 files changed, 98 insertions(+), 22 deletions(-)
>

-- 
--
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)

Claude review: drm: verisilicon: convert drm_format to vs_format in atomic_check

[Claude Code Review Bot]

Overall Series Review

Subject: drm: verisilicon: convert drm_format to vs_format in atomic_check
Author: Icenowy Zheng <zhengxingda@iscas.ac.cn>
Patches: 6
Reviewed: 2026-03-31T16:44:29.302372

---

This is a well-structured 4-patch series that moves the `drm_format` to `vs_format` conversion from `atomic_update` (which cannot fail) to `atomic_check` (which can properly return errors). The series is on v4 and already has Reviewed-by tags from Thomas Zimmermann on all patches. The approach is correct and follows standard DRM patterns. The patches are logically ordered and each patch is self-contained.

One minor issue: in patch 3, passing `NULL` for `crtc_state` to `drm_atomic_helper_check_plane_state()` will trigger a `WARN_ON` inside that helper (line 907: `WARN_ON(plane_state->crtc && plane_state->crtc != crtc_state->crtc)`). However, since `crtc` is also NULL in that case, the dereference of `crtc_state->crtc` is guarded by the short-circuit evaluation of `plane_state->crtc` being NULL. And line 918 has `WARN_ON(!plane_state->crtc)` which fires but harmlessly returns `visible = false`. This is a known/accepted pattern — the helper itself handles the NULL crtc case (with a warning that `fb` without `crtc` is odd), and the real fix is ensuring `drm_atomic_helper_check_plane_state()` gets called at all for cleanup purposes.

Overall: **Looks good.** Series is ready for merge.

---
Generated by Claude Code Patch Reviewer

[PATCH drm-misc-next 0/3] drm: verisilicon: convert drm_format to vs_format in atomic_check

[Icenowy Zheng]

This patchset tries to move the process of converting drm_format to
vs_format into plane's atomic_check callback (instead of the
atomic_commit one, which the process currently sits) for more proper
error handling (because atomic_commit cannot fail).

Icenowy Zheng (3):
  drm: verisilicon: make vs_format conversion function return int
  drm: verisilicon: subclass drm_plane_state
  drm: verisilicon: fill plane's vs_format in atomic_check

 drivers/gpu/drm/verisilicon/vs_plane.c        | 47 ++++++++++++++++++-
 drivers/gpu/drm/verisilicon/vs_plane.h        | 18 ++++++-
 .../gpu/drm/verisilicon/vs_primary_plane.c    | 24 ++++++----
 3 files changed, 77 insertions(+), 12 deletions(-)

-- 
2.52.0

[PATCH drm-misc-next 2/3] drm: verisilicon: subclass drm_plane_state

[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>
---
 drivers/gpu/drm/verisilicon/vs_plane.c        | 40 +++++++++++++++++++
 drivers/gpu/drm/verisilicon/vs_plane.h        | 14 +++++++
 .../gpu/drm/verisilicon/vs_primary_plane.c    |  6 +--
 3 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c b/drivers/gpu/drm/verisilicon/vs_plane.c
index df406f565143a..f9d38e0e583f6 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_plane.c
@@ -125,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(sizeof(*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

Claude review: drm: verisilicon: subclass drm_plane_state

[Claude Code Review Bot]

Patch Review

**Purpose:** Introduce custom plane state structure to hold driver-specific state.

**Code Analysis:**

```c
+struct vs_plane_state {
+	struct drm_plane_state base;
+};
```

Empty struct is fine as a placeholder - patch 3 will add the format field.

```c
+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);
```

**Issue 4 - Type confusion in kmemdup:**
This is copying `plane->state` (a `drm_plane_state*`) but using `sizeof(*vs_state)` (size of `vs_plane_state`). While this works because we're early in the transition, it's fragile. The correct pattern is:
```c
vs_state = kmemdup(state_to_vs_plane_state(plane->state), sizeof(*vs_state), GFP_KERNEL);
```
Or more idiomatically:
```c
struct vs_plane_state *old_vs_state = state_to_vs_plane_state(plane->state);
vs_state = kmemdup(old_vs_state, sizeof(*vs_state), GFP_KERNEL);
```

```c
+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);
```

**CRITICAL Issue 5 - Use-after-free:**
This code calls `__drm_atomic_helper_plane_destroy_state(plane->state)` which may access the memory, then immediately frees it with `kfree(plane->state)`. If `__drm_atomic_helper_plane_destroy_state` holds any references or the memory is accessed after the kfree, this is a use-after-free bug.

Looking at DRM core, `__drm_atomic_helper_plane_destroy_state` decrements refcounts on the framebuffer and fence, but doesn't access the state structure itself after that. So this is technically safe BUT violates common patterns.

The standard pattern in DRM drivers is:
```c
if (plane->state) {
	vs_plane_destroy_state(plane, plane->state);
	plane->state = NULL;
}
```

Or if you want to inline it:
```c
if (plane->state) {
	__drm_atomic_helper_plane_destroy_state(plane->state);
	kfree(state_to_vs_plane_state(plane->state));
	plane->state = NULL;
}
```

**Issue 6 - Missing plane->state = NULL:**
After freeing plane->state, you should set it to NULL before allocating the new state. Otherwise if kzalloc fails, plane->state points to freed memory.

**Issue 7 - Memory leak on allocation failure:**
```c
+	vs_state = kzalloc(sizeof(*vs_state), GFP_KERNEL);
+	if (!vs_state)
+		return;
```
If allocation fails, we've just freed plane->state but don't restore it. The plane is left in an invalid state. This is probably acceptable for reset during init, but should be documented or handled more gracefully.

**Verdict:** Core approach is correct, but implementation has memory management issues that need fixing.

---

---
Generated by Claude Code Patch Reviewer