From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm: verisilicon: fill plane's vs_format in
 atomic_check
Date: Wed, 11 Feb 2026 16:24:25 +1000
Message-ID: <review-patch3-20260210141300.749013-4-zhengxingda@iscas.ac.cn>
In-Reply-To: <20260210141300.749013-4-zhengxingda@iscas.ac.cn>
References: <20260210141300.749013-1-zhengxingda@iscas.ac.cn> <20260210141300.749013-4-zhengxingda@iscas.ac.cn>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
X-Mailer: Claude Code Patch Reviewer

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