From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm: verisilicon: add support for cursor planes Date: Thu, 07 May 2026 13:14:48 +1000 Message-ID: In-Reply-To: <20260506175610.2542888-3-zhengxingda@iscas.ac.cn> References: <20260506175610.2542888-1-zhengxingda@iscas.ac.cn> <20260506175610.2542888-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 (critical) =E2=80=94 Missing field shift macros for negative coordina= te offsets:** In `vs_cursor_plane_atomic_update()`, when writing negative X/Y offsets, th= e 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)` =E2=80=94 it covers bi= ts 16 through 20. The value `-state->crtc_x` is a small positive integer (1= =E2=80=9331), which has no bits set in the 16=E2=80=9320 range. `regmap_upd= ate_bits` applies `val & mask`, so the offset will always be written as **z= ero**. The cursor will jump to position 0 instead of being partially offscr= een. 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)` a= nd `VSDC_CURSOR_CONFIG_Y_OFF(v)` as `((v) << 8)`, but they are never used. **Bug (off-by-one) =E2=80=94 Coordinate range checks allow values that over= flow 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)` =3D `1UL << 15` =3D 32768, but `VSDC_CURSOR_LOCATION_X_MASK`= is `GENMASK(14, 0)` which is 15 bits holding values 0=E2=80=9332767. The c= heck `coord <=3D 32768` accepts coord=3D32768, which cannot be represented = in the field and would silently wrap to 0. Similarly, `BIT_MASK(5)` =3D 32, but the 5-bit offset fields (`GENMASK(12, = 8)` / `GENMASK(20, 16)`) hold values 0=E2=80=9331. The check `(-coord) <=3D= 32` accepts 32, which overflows. Both comparisons should use `<` instead of `<=3D`: ```c return coord < VSDC_CURSOR_LOCATION_MAX_POSITIVE; return (-coord) < VSDC_CURSOR_LOCATION_MAX_NEGATIVE; ``` **Issue =E2=80=94 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_C= AP_CURSOR_HEIGHT`; they default to 64, which happens to match the current H= WDB entries. But if a DC variant with `max_cursor_size =3D 256` is added la= ter, userspace would still see 64. Consider setting these in `vs_mode_confi= g_init()` or after the CRTCs are created. **Nit =E2=80=94 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 unused now but will rot if not fixed before someone references= them. **Nit =E2=80=94 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 ha= s a double space before "coordinates". **Nit =E2=80=94 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 =E2=80=94 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