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/rockchip: vop2: Support setting custom background color Date: Wed, 04 Mar 2026 06:54:47 +1000 Message-ID: In-Reply-To: <20260303-rk3588-bgcolor-v8-4-fee377037ad1@collabora.com> References: <20260303-rk3588-bgcolor-v8-0-fee377037ad1@collabora.com> <20260303-rk3588-bgcolor-v8-4-fee377037ad1@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Status: Mostly good, one concern** The register programming using `FIELD_PREP`/`FIELD_MODIFY` with the BPCS (s= hift-based) helpers is clean: ```c + val =3D FIELD_PREP(RK3568_VP_DSP_BG__DSP_BG_RED, DRM_ARGB64_GETR_BPCS(bgc= olor, 10)); + FIELD_MODIFY(RK3568_VP_DSP_BG__DSP_BG_GREEN, &val, DRM_ARGB64_GETG_BPCS(b= gcolor, 10)); + FIELD_MODIFY(RK3568_VP_DSP_BG__DSP_BG_BLUE, &val, DRM_ARGB64_GETB_BPCS(bg= color, 10)); ``` The register field definitions (three 10-bit fields in bits 29-0) look corr= ect for VOP2 documentation. Using BPCS (shift instead of divide) is justifi= ed for a hot path like `vop2_post_config()`. **Concern =E2=80=94 alpha-blending rejection check:** ```c + if ((cstate->background_color << 16) && + (fb->format->has_alpha || pstate->alpha !=3D 0xffff)) { + drm_dbg_kms(vop2->drm, + "Alpha-blending with background color is unsupported\n"); + return -EINVAL; + } ``` The `<< 16` trick to strip the alpha and check if RGB is non-zero is clever= but somewhat obscure =E2=80=94 a named helper or a mask like `bgcolor & 0x= 0000FFFFFFFFFFFF` would be clearer. More substantively, this check may be **overly conservative**: it rejects a= ny plane that has alpha (format or property) whenever the background is non= -black, regardless of whether that plane's pixels actually interact with th= e background. For example, if plane A is a fully opaque bottom layer coveri= ng the entire screen, and plane B above it has `has_alpha`, the check would= reject plane B even though plane B blends with plane A, not the background= . I understand this is a hardware limitation being enforced conservatively,= but it could break valid compositing scenarios with non-black backgrounds.= Is the hardware truly incapable of handling this, or is it just that the b= ackground-to-first-plane blending is problematic? If the latter, this could= be refined to only reject when the lowest z-order plane has alpha. The state dump addition is a nice touch for debugging: ```c + seq_printf(s, "\tbackground color (10bpc): r=3D0x%x g=3D0x%x b=3D0x%x\n", ``` R-b from Andy Yan (Rockchip maintainer) is appropriate. --- Generated by Claude Code Patch Reviewer