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: add support for cursor planes
Date: Wed, 25 Mar 2026 07:03:51 +1000
Message-ID: <review-patch2-20260324060222.2029061-3-zhengxingda@iscas.ac.cn>
In-Reply-To: <20260324060222.2029061-3-zhengxingda@iscas.ac.cn>
References: <20260324060222.2029061-1-zhengxingda@iscas.ac.cn> <20260324060222.2029061-3-zhengxingda@iscas.ac.cn>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Bug: Missing `FIELD_PREP`/shift for X_OFF and Y_OFF values**

When writing negative offsets to the CONFIG register:

```c
regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
                   VSDC_CURSOR_CONFIG_X_OFF_MASK,
                   -state->crtc_x);
```

The X_OFF field is at bits [20:16] (`GENMASK(20, 16)`), but the raw value `=
-state->crtc_x` is not shifted. It should use the defined `VSDC_CURSOR_CONF=
IG_X_OFF(v)` macro:

```c
VSDC_CURSOR_CONFIG_X_OFF(-state->crtc_x)
```

Same issue for Y_OFF at bits [12:8]:

```c
regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output),
                   VSDC_CURSOR_CONFIG_Y_OFF_MASK,
                   -state->crtc_y);
```

Should be `VSDC_CURSOR_CONFIG_Y_OFF(-state->crtc_y)`. Without these shifts,=
 the offset value lands in the wrong bit positions (bits [4:0] instead of [=
20:16] for X, and [4:0] instead of [12:8] for Y), corrupting other fields i=
n the CONFIG register.

**Issue: `mode_config.cursor_width/height` not set**

The DRM core exposes cursor size limits to userspace via `drm_mode_config.c=
ursor_width` and `cursor_height`. These are not set anywhere in the driver =
(checked `vs_drm.c`). Without this, userspace won't know the maximum cursor=
 size. This should be set in the mode_config init, e.g.:

```c
drm_dev->mode_config.cursor_width =3D dc->identity.max_cursor_size;
drm_dev->mode_config.cursor_height =3D dc->identity.max_cursor_size;
```

**Nit: Typos in register header**

```c
#define VSDC_CURSOR_BACKGRUOND_DEFAULT  0x00FFFFFF
#define VSDC_CURSOR_FOREGRUOND_DEFAULT  0x00AAAAAA
```

`BACKGRUOND` and `FOREGRUOND` should be `BACKGROUND` and `FOREGROUND`. Thes=
e macros are currently unused but should be fixed before they get used.

**Nit: Comment typo**

```c
 * To for positive left-top  coordinates the
```

Should read "For positive left-top coordinates the" (remove "To", fix doubl=
e space).

**Nit: `atomic_update` calls `atomic_disable` directly**

```c
if (!state->visible) {
    vs_cursor_plane_atomic_disable(plane, atomic_state);
    return;
}
```

This works but is unusual =E2=80=94 normally `atomic_disable` is only calle=
d by the DRM core. The DRM atomic helper should call `atomic_disable` when =
the plane becomes invisible, so this explicit call may be redundant. Worth =
verifying that the DRM helpers handle this transition correctly for cursor =
planes, or if this is an intentional workaround.

**Style: `ERR_PTR(PTR_ERR(primary))` vs `ERR_CAST`**

In the existing code at `vs_crtc.c:175`:
```c
return ERR_PTR(PTR_ERR(primary));
```

The new cursor error path correctly uses `ERR_CAST(cursor)`. The pre-existi=
ng primary path should also use `ERR_CAST` for consistency, though that's o=
utside the scope of this series.

---
Generated by Claude Code Patch Reviewer