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: Add YUV support to background color Date: Mon, 25 May 2026 17:00:23 +1000 Message-ID: In-Reply-To: <20260524-vop2-bg-yuv-v1-2-dcb6a52923f5@collabora.com> References: <20260524-vop2-bg-yuv-v1-0-dcb6a52923f5@collabora.com> <20260524-vop2-bg-yuv-v1-2-dcb6a52923f5@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 **Bug: Wrong color space domain passed to conversion function** At `rockchip_drm_vop2.c:1659`: ```c vop2_rgb16_to_yuv16(new_vcstate->color_space, DRM_ARGB64_GETR(bgcolor), ...); ``` `new_vcstate->color_space` is a `V4L2_COLORSPACE_*` value (e.g., `V4L2_COLO= RSPACE_DEFAULT=3D0`, `V4L2_COLORSPACE_REC709=3D3`). The function `vop2_rgb1= 6_to_yuv16()` expects an `enum vop_csc_format` (`CSC_BT601L=3D0`, `CSC_BT70= 9L=3D1`, `CSC_BT601F=3D2`, `CSC_BT2020L=3D3`). These are different numeric = domains. For the default case (`V4L2_COLORSPACE_DEFAULT=3D0`), this would select `CS= C_BT601L` instead of the intended `CSC_BT709L` (which is what `vop2_convert= _csc_mode()` returns for `V4L2_COLORSPACE_DEFAULT`). For `V4L2_COLORSPACE_R= EC709=3D3`, it would select `CSC_BT2020L` instead of `CSC_BT709L`. The fix is to convert first, matching how `vop2_setup_csc_mode()` at line 8= 01 does it: ```c vop2_rgb16_to_yuv16(vop2_convert_csc_mode(new_vcstate->color_space), ...); ``` **Issue: Skip condition doesn't account for `yuv_overlay` changes** At `rockchip_drm_vop2.c:1650-1652`: ```c if (!force && old_crtc_state->background_color =3D=3D bgcolor && old_vcstate->color_space =3D=3D new_vcstate->color_space) return; ``` This compares `background_color` and `color_space`, but does not compare `o= ld_vcstate->yuv_overlay` vs `new_vcstate->yuv_overlay`. If the overlay mode= switches between RGB and YUV while the background color and color space re= main the same, the register will not be reprogrammed. In practice this may = be unlikely without an accompanying modeset (which would go through `atomic= _enable` with `force=3Dtrue`), but the condition is incomplete and should b= e checked or documented. If this transition can only happen during modeset,= a comment explaining why would be valuable. **Minor: Inconsistent use of `crtc->state` vs `crtc_state` in `vop2_crtc_at= omic_enable`** At line 1754, after the patch: ```c struct drm_display_mode *mode =3D &crtc->state->adjusted_mode; ``` The patch correctly updates `vcstate` to use the new `crtc_state` obtained = from `drm_atomic_get_new_crtc_state()` (line 1753), but the `mode` variable= on the very next line still reads from `crtc->state->adjusted_mode`. While= `crtc->state` should point to the new state by this point in the atomic co= mmit, using `crtc_state->adjusted_mode` would be more consistent and less f= ragile. This is pre-existing, but since the patch already touches this func= tion and fixes the analogous `vcstate` issue, it would be a good cleanup to= include. **Positive notes:** - The conversion matrices are well-sourced and the coefficients match the v= 4l2-tpg reference implementation. - Using `s64` for intermediates avoids overflow =E2=80=94 the maximum produ= ct is `~256*219*65535 =E2=89=88 3.6 billion`, which exceeds `s32` range but= fits comfortably in `s64`. - The `force` parameter split between `atomic_enable` (always recompute) an= d `atomic_flush` (skip if unchanged) is a clean optimization strategy. - The refactoring of `vop2_post_config` to take explicit state pointers ins= tead of reading `crtc->state` is a good improvement. --- Generated by Claude Code Patch Reviewer