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/vkms: Fix bad matrix offset component multiplication Date: Fri, 13 Feb 2026 06:36:24 +1000 Message-ID: In-Reply-To: <20260210-vkms-composer-fix-matrix-v2-1-d1ed09cb23e8@collabora.com> References: <20260210-vkms-composer-fix-matrix-v2-0-d1ed09cb23e8@collabora.com> <20260210-vkms-composer-fix-matrix-v2-1-d1ed09cb23e8@collabora.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Mailer: Claude Code Patch Reviewer Patch Review > +#define UNORM_16BIT_ONE (1ULL << 16) Pekka Paalanen's review comment on the mailing list is correct here. The UNORM-16 representation of 1.0 is `(2^n - 1)` where n=16, which is `0xffff = 65535`, not `(1 << 16) = 65536`. The value 65536 doesn't even fit in a u16. This should be `0xffffULL` instead. In practice the off-by-one (65536 vs 65535) will produce a very small error -- at most ~1 LSB in the final result -- so the tests probably still pass with either value. But mathematically and by UNORM convention, this is wrong. > + /* > + * Pixels values are packed as 16-bit UNORM values, so the matrix offset > + * components must be multiplied properly by the idempotent element -i.e. > + * number 1 encoded as 16-bit UNORM-. > + */ The comment is helpful but uses the word "idempotent element" when the correct term would be "multiplicative identity" (or simply "the value 1.0 in UNORM representation"). "Idempotent" means `f(f(x)) = f(x)`, which is a different concept. Minor nit, but worth correcting since the comment is trying to explain the mathematical reasoning. > + drm_fixp_mul(drm_sm2fixp(matrix->matrix[3]), drm_int2fixp(UNORM_16BIT_ONE)); The fix itself is structurally correct. The offset terms need to be multiplied by the pixel-scale representation of 1.0, matching how the other matrix coefficients are multiplied against `r`, `g`, `b` (which are also pixel-scale integers converted via `drm_int2fixp()`). Before the fix, the offset was just `drm_sm2fixp(matrix->matrix[3])`, which left it as a raw S31.32 fixed-point value without the pixel scaling -- effectively treating it as if pixel values were normalized to [0.0, 1.0] rather than [0, 65535]. The same fix is applied consistently to all three rows (rf, gf, bf), which is correct. The commit message could benefit from a Fixes: tag since this is a bugfix to existing code. --- Generated by Claude Code Patch Reviewer