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

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

diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c b/drivers/gpu/drm/verisilicon/vs_plane.c
index 2f3953e588a34..df406f565143a 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_plane.c
@@ -6,13 +6,14 @@
 #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 "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 +63,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 +102,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

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

[PATCH drm-misc-next 3/3] 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>
---
 drivers/gpu/drm/verisilicon/vs_plane.h         |  2 ++
 drivers/gpu/drm/verisilicon/vs_primary_plane.c | 18 ++++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_plane.h b/drivers/gpu/drm/verisilicon/vs_plane.h
index 12848a72af576..df4b248b52954 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 *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 bad0bc5e3242d..b5bc6b0078fc3 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
@@ -25,12 +25,19 @@ 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 = state_to_vs_plane_state(new_plane_state);
 	struct drm_crtc *crtc = new_plane_state->crtc;
 	struct drm_crtc_state *crtc_state;
+	int ret;
 
 	if (!crtc)
 		return 0;
 
+	ret = drm_format_to_vs_format(new_plane_state->fb->format->format,
+				      &new_vs_plane_state->format);
+	if (ret)
+		return ret;
+
 	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	if (WARN_ON(!crtc_state))
 		return -EINVAL;
@@ -88,11 +95,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 = 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;
 
@@ -105,16 +112,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

**Purpose:** Move format conversion to atomic_check and store result in plane state.

**Code Analysis:**

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

Good - adds the format field to store the validated format.

```c
 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 = state_to_vs_plane_state(new_plane_state);
 	struct drm_crtc *crtc = new_plane_state->crtc;
 	struct drm_crtc_state *crtc_state;
+	int ret;
 
 	if (!crtc)
 		return 0;
 
+	ret = drm_format_to_vs_format(new_plane_state->fb->format->format,
+				      &new_vs_plane_state->format);
```

**CRITICAL Issue 8 - NULL pointer dereference:**
This code dereferences `new_plane_state->fb->format->format` but doesn't check if `fb` is NULL. In atomic_check, the fb can legitimately be NULL (for example, when disabling the plane). The check for `!crtc` doesn't protect against NULL fb.

The check should be:
```c
if (!crtc || !new_plane_state->fb)
	return 0;
```

Or more explicitly:
```c
if (!new_plane_state->fb || !crtc)
	return 0;
```

This is a critical bug that will cause kernel crashes.

**Issue 9 - State validation completeness:**
After this patch, the format is converted and stored in atomic_check. However, we should verify the existing `drm_atomic_helper_check_plane_state()` call (if any) happens after our format validation. Looking at the patch, I don't see where plane state validation happens. The series should ensure:
1. Format conversion happens first
2. Then drm_atomic_helper_check_plane_state() validates scaling, etc.

**Atomic update side:**
```c
 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 = 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;
```

Good - removes the local format conversion.

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

**Issue 10 - Assumption about state validity:**
The atomic_update function now assumes `vs_state->format` is valid. This is correct IF atomic_check always runs before atomic_update and IF the format field is properly duplicated during state copying. 

We should verify:
1. Patch 2's duplicate_state uses kmemdup, which will copy the format field - ✓ OK
2. There's no path where atomic_update runs without atomic_check - this is guaranteed by DRM atomic framework - ✓ OK

**Issue 11 - Incomplete conversion:**
This patch only modifies vs_primary_plane.c. If there are other plane types (overlay, cursor), they also need similar changes. The commit message should note whether this is complete or if overlay planes need separate patches.

**Verdict:** Correct approach but has critical NULL pointer dereference bug and potential incompleteness.

---

## SUMMARY OF REQUIRED CHANGES

**Critical (must fix):**
1. **Patch 3**: Add NULL check for `new_plane_state->fb` before dereferencing
2. **Patch 2**: Fix the plane->state cleanup pattern in vs_plane_reset()
3. **Patch 2**: Set plane->state = NULL after kfree and before allocation

**Important (should fix):**
4. **Patch 1**: Move drm_atomic_state_helper.h include to appropriate patch
5. **Patch 1**: Add format value to error message for unsupported formats
6. **Patch 2**: Use proper type casting in kmemdup (use state_to_vs_plane_state)

**Suggestions:**
7. Document whether overlay/cursor planes need similar treatment
8. Consider adding a comment explaining the relationship between the two switch statements in drm_format_to_vs_format()

**Testing recommendations:**
- Test plane disable operations (NULL fb case)
- Test invalid format rejection
- Test state duplication preserves format correctly
- Verify no memory leaks under error conditions

---
Generated by Claude Code Patch Reviewer

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: 4
Reviewed: 2026-02-11T16:24:25.294866

---

This 3-patch series refactors the Verisilicon DRM driver to move DRM-to-hardware format conversion from `atomic_commit` to `atomic_check`, improving error handling by allowing validation to occur before the point of no return.

**Series Architecture:**
The refactoring follows a logical progression:
1. Patch 1: Makes format conversion function return error codes
2. Patch 2: Introduces plane state subclassing infrastructure
3. Patch 3: Moves format conversion to atomic_check and stores result in plane state

**Overall Assessment:**
- **Conceptually sound**: Moving validation to atomic_check is the correct DRM atomic design pattern
- **Good incremental approach**: Breaking changes into logical steps aids review and bisectability
- **Implementation concerns**: Several issues with state management and error handling (detailed below)

**Key Issues:**
1. Missing null pointer checks in patch 3 (fb can be NULL in atomic_check)
2. Patch 2's state management has a use-after-free pattern
3. Incomplete header includes across the series
4. Need verification that format values are properly preserved across state duplications

---

---
Generated by Claude Code Patch Reviewer