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

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

[Claude Code Review Bot]

Patch Review

**Purpose:** Convert `drm_format_to_vs_format()` from void to int return type to enable error reporting.

**Code Analysis:**

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

This is a straightforward API change. The function signature update is correct.

```c
 	default:
-		pr_warn("Unexpected drm format!\n");
+		return -EINVAL;
 	}
```

**Issue 1 - Error handling improvement:**
Good: Replacing a warning with proper error propagation is correct. However, it would be better to log which format was unexpected for debugging:
```c
default:
	pr_err("Unsupported drm format: 0x%08x\n", drm_format);
	return -EINVAL;
```

```c
 	/* N/A for non-YUV formats */
 	vs_format->uv_swizzle = false;
+
+	return 0;
 }
```

**Issue 2 - Success path:**
The success return is only at the very end. This means if the first switch statement matches a format, but execution continues through the second switch, we always return 0. This appears correct since the default case in the first switch would have returned -EINVAL already. However, this creates implicit coupling between the two switch statements that should be documented.

**Issue 3 - Header includes:**
```c
+#include <drm/drm_atomic_state_helper.h>
```
This include appears unrelated to this patch's functionality (making the function return int). It's likely needed for patch 2 or 3 but was mistakenly added here. This breaks the logical separation of changes and could cause bisection issues.

**Verdict:** The core change is correct, but has header inclusion and error message issues.

---

---
Generated by Claude Code Patch Reviewer

[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

[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

[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

[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

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

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

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

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

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