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: Tue, 10 Mar 2026 12:36:01 +1000 Message-ID: In-Reply-To: <20260309085302.3132732-3-zhengxingda@iscas.ac.cn> References: <20260309085302.3132732-1-zhengxingda@iscas.ac.cn> <20260309085302.3132732-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 Negative coordinate offset values not shifted to= bit field positions:** In `vs_cursor_plane_atomic_update()`, when handling negative `crtc_x`: ```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)` (bits 20:16), but `-st= ate->crtc_x` is not shifted to that position. Since `regmap_update_bits` pe= rforms `(old & ~mask) | (val & mask)`, the unshifted value will be ANDed wi= th `0x1F0000`, yielding zero for any small coordinate value. The X offset w= ill always be programmed as 0. The fix is to use the existing shift macro: ```c VSDC_CURSOR_CONFIG_X_OFF(-state->crtc_x) ``` The same bug exists for negative `crtc_y`: ```c regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output), VSDC_CURSOR_CONFIG_Y_OFF_MASK, -state->crtc_y); ``` `VSDC_CURSOR_CONFIG_Y_OFF_MASK` is `GENMASK(12, 8)`, but the value is not s= hifted. Should use `VSDC_CURSOR_CONFIG_Y_OFF(-state->crtc_y)`. **Bug =E2=80=94 Off-by-one errors in coordinate range checks:** ```c #define VSDC_CURSOR_LOCATION_MAX_POSITIVE BIT_MASK(15) #define VSDC_CURSOR_LOCATION_MAX_NEGATIVE BIT_MASK(5) ``` `BIT_MASK(n)` in the kernel is `(1UL << n)`, so `BIT_MASK(15)` =3D 32768 an= d `BIT_MASK(5)` =3D 32. But the LOCATION X/Y fields are 15 bits wide (`GENM= ASK(14, 0)` / `GENMASK(30, 16)`), which can hold 0=E2=80=9332767 max. And t= he offset fields are 5 bits wide, which can hold 0=E2=80=9331 max. The chec= ks `coord <=3D BIT_MASK(15)` and `(-coord) <=3D BIT_MASK(5)` each allow one= value too many. These should use `BIT_MASK(15) - 1` and `BIT_MASK(5) - 1`,= or equivalently `GENMASK(14, 0)` and `GENMASK(4, 0)`. **Comment typo:** ```c * To for positive left-top coordinates the ``` Should be "For positive left-top coordinates the" (extra word "To" and doub= le space). **Typos in register defines** (`vs_cursor_plane_regs.h`): ```c #define VSDC_CURSOR_BACKGRUOND_DEFAULT 0x00FFFFFF #define VSDC_CURSOR_FOREGRUOND_DEFAULT 0x00AAAAAA ``` `BACKGRUOND` =E2=86=92 `BACKGROUND`, `FOREGRUOND` =E2=86=92 `FOREGROUND`. T= hese are unused currently but will cause confusion if referenced later. **Unrelated copyright change** in `vs_primary_plane_regs.h`: This patch cha= nges the copyright header of `vs_primary_plane_regs.h` from Icenowy Zheng's= personal email to the ISCAS institutional copyright and updates the year. = This is unrelated to cursor plane support and should be a separate patch if= desired. **Missing `.prepare_fb` helper:** The cursor plane helper funcs don't inclu= de `.prepare_fb =3D drm_gem_plane_helper_prepare_fb` (or the shadow-plane e= quivalent). The primary plane appears to also omit this, so it may be handl= ed elsewhere, but it's worth verifying that GEM buffer pinning/mapping is h= andled correctly for cursor buffers. **`drmm_universal_plane_alloc` usage:** ```c plane =3D drmm_universal_plane_alloc(drm_dev, struct drm_plane, dev, 0, ...= ); ``` The `member` argument `dev` refers to the `struct drm_device *dev` pointer = within `struct drm_plane`. This is unusual =E2=80=94 normally the macro is = used with a wrapping struct where `member` names the embedded `struct drm_p= lane`. However, this matches the primary plane's existing usage and appears= to be an existing pattern in this driver, so not an issue introduced by th= is series. --- Generated by Claude Code Patch Reviewer