Claude review: drm: verisilicon: add support for cursor planes

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Bug (critical) — Missing field shift macros for negative coordinate offsets:**

In `vs_cursor_plane_atomic_update()`, when writing negative X/Y offsets, the raw value is passed without shifting it to the register field position:

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

`VSDC_CURSOR_CONFIG_X_OFF_MASK` is `GENMASK(20, 16)` — it covers bits 16 through 20. The value `-state->crtc_x` is a small positive integer (1–31), which has no bits set in the 16–20 range. `regmap_update_bits` applies `val & mask`, so the offset will always be written as **zero**. The cursor will jump to position 0 instead of being partially offscreen.

The same bug applies to Y_OFF at `GENMASK(12, 8)`.

Fix: use the defined shift macros:
```c
VSDC_CURSOR_CONFIG_X_OFF(-state->crtc_x)
VSDC_CURSOR_CONFIG_Y_OFF(-state->crtc_y)
```

The header already defines `VSDC_CURSOR_CONFIG_X_OFF(v)` as `((v) << 16)` and `VSDC_CURSOR_CONFIG_Y_OFF(v)` as `((v) << 8)`, but they are never used.

**Bug (off-by-one) — Coordinate range checks allow values that overflow the register fields:**

```c
#define VSDC_CURSOR_LOCATION_MAX_POSITIVE BIT_MASK(15)
#define VSDC_CURSOR_LOCATION_MAX_NEGATIVE BIT_MASK(5)
```

`BIT_MASK(15)` = `1UL << 15` = 32768, but `VSDC_CURSOR_LOCATION_X_MASK` is `GENMASK(14, 0)` which is 15 bits holding values 0–32767. The check `coord <= 32768` accepts coord=32768, which cannot be represented in the field and would silently wrap to 0.

Similarly, `BIT_MASK(5)` = 32, but the 5-bit offset fields (`GENMASK(12, 8)` / `GENMASK(20, 16)`) hold values 0–31. The check `(-coord) <= 32` accepts 32, which overflows.

Both comparisons should use `<` instead of `<=`:
```c
return coord < VSDC_CURSOR_LOCATION_MAX_POSITIVE;
return (-coord) < VSDC_CURSOR_LOCATION_MAX_NEGATIVE;
```

**Issue — Missing `cursor_width`/`cursor_height` in mode_config:**

The driver never sets `drm->mode_config.cursor_width` or `drm->mode_config.cursor_height`. Userspace queries these via `DRM_CAP_CURSOR_WIDTH` / `DRM_CAP_CURSOR_HEIGHT`; they default to 64, which happens to match the current HWDB entries. But if a DC variant with `max_cursor_size = 256` is added later, userspace would still see 64. Consider setting these in `vs_mode_config_init()` or after the CRTCs are created.

**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". These macros are unused now but will rot if not fixed before someone references them.

**Nit — Garbled comment:**

```c
 * values are unsigned. To for positive left-top  coordinates the
```

"To for positive" doesn't parse. Probably should be "For positive". Also has a double space before "coordinates".

**Nit — License inconsistency:**

`vs_cursor_plane.c` uses `GPL-2.0-only` while `vs_cursor_plane_regs.h` uses `GPL-2.0-or-later`. Both are valid choices but the inconsistency within a single feature is worth noting.

**Minor — Unused `fb` variable when invisible:**

In `vs_cursor_plane_atomic_check()`, `fb` is assigned from `new_plane_state->fb` at the top but only used after the visibility check. This is harmless (no dereference on the invisible path) but could be moved down for clarity.

---
Generated by Claude Code Patch Reviewer