[PATCH v2 0/2] drm/vkms: Fix bad matrix offset component multiplication

[PATCH v2 0/2] drm/vkms: Fix bad matrix offset component multiplication

[Ariel D'Alessandro]

This patch series fixes a bug in the 3x4 matrix multiplication for BT709
enconding/decoding, where the offset component if not handled properly.

Currently, BT.709 encoding matrix kunit tests don't cover the offset
component multiplication, so a new set of tests is added.

The bug was found while working on the IGT tools CRTC (post-blend) color
pipelines, see submitted patchset [0] (patch [1] in particular) for the
full context.

[0] https://lore.kernel.org/igt-dev/20251223-post-blend-colorops-v2-0-f40aca9795bd@collabora.com/
[1] https://lore.kernel.org/igt-dev/20251223-post-blend-colorops-v2-7-f40aca9795bd@collabora.com/

Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
---
Changes in v2:
- Added kunit tests for alternative BT709 encoding matrix.
- Link to v1: https://lore.kernel.org/r/20251223-vkms-composer-fix-matrix-v1-1-10a979e06209@collabora.com

---
Ariel D'Alessandro (2):
      drm/vkms: Fix bad matrix offset component multiplication
      drm/vkms: Add kunit tests for alternative BT709 encoding matrix

 drivers/gpu/drm/vkms/tests/vkms_color_test.c | 146 +++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_composer.c         |  13 ++-
 2 files changed, 156 insertions(+), 3 deletions(-)
---
base-commit: b96bcfba104c65db41378a04f5ccac186f79578f
change-id: 20251223-vkms-composer-fix-matrix-aa2c593f4515

Best regards,
-- 
Ariel D'Alessandro <ariel.dalessandro@collabora.com>

[PATCH v2 1/2] drm/vkms: Fix bad matrix offset component multiplication

[Ariel D'Alessandro]

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-.

Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index cd85de4ffd03d..d53ea4189c97b 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -17,6 +17,8 @@
 #include "vkms_composer.h"
 #include "vkms_luts.h"
 
+#define UNORM_16BIT_ONE			(1ULL << 16)
+
 static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
 {
 	u32 new_color;
@@ -139,20 +141,25 @@ VISIBLE_IF_KUNIT void apply_3x4_matrix(struct pixel_argb_s32 *pixel,
 	g = drm_int2fixp(pixel->g);
 	b = drm_int2fixp(pixel->b);
 
+	/*
+	 * 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-.
+	 */
 	rf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[0]), r) +
 	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[1]), g) +
 	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[2]), b) +
-	     drm_sm2fixp(matrix->matrix[3]);
+	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[3]), drm_int2fixp(UNORM_16BIT_ONE));
 
 	gf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[4]), r) +
 	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[5]), g) +
 	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[6]), b) +
-	     drm_sm2fixp(matrix->matrix[7]);
+	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[7]), drm_int2fixp(UNORM_16BIT_ONE));
 
 	bf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[8]), r) +
 	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[9]), g) +
 	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[10]), b) +
-	     drm_sm2fixp(matrix->matrix[11]);
+	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[11]), drm_int2fixp(UNORM_16BIT_ONE));
 
 	pixel->r = drm_fixp2int_round(rf);
 	pixel->g = drm_fixp2int_round(gf);

-- 
2.51.0

[PATCH v2 2/2] drm/vkms: Add kunit tests for alternative BT709 encoding matrix

[Ariel D'Alessandro]

Currently, BT.709 encoding matrix kunit tests don't cover the offset
component multiplication.

This commit adds another kunit tests using the BT.709 standard [0]
alternative representation of colors by three coordinates Y'Cb'Cr',
which are linear combinations of the (non-linear) RGB coordinates.

According to these formulas, if RGB varies between [0.0, 1.0], then Y'
will vary between [0.0, 1.0], while Cb' and Cr' will vary between
[-0.5, 0.5].

The 0.5 offset is added to the Cb' and Cr' components in the 3x4
encoding matrix, so the resulting pixel values then fits the 16-bit
UNORM.

[0] https://www.itu.int/rec/R-REC-BT.709-6-201506-I/en

Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
---
 drivers/gpu/drm/vkms/tests/vkms_color_test.c | 146 +++++++++++++++++++++++++++
 1 file changed, 146 insertions(+)

diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_test.c b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
index 1a1c7cac2f158..53ab88e9e2f9e 100644
--- a/drivers/gpu/drm/vkms/tests/vkms_color_test.c
+++ b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
@@ -393,6 +393,151 @@ static void vkms_color_ctm_3x4_bt709(struct kunit *test)
 	KUNIT_EXPECT_LT(test, out.b, 0x100);
 }
 
+/*
+ * BT.709 encoding matrix: Y'Cb'Cr' coordinates
+ *
+ * BT.709 standard [0] alternative representation of colors by three coordinates
+ * Y'Cb'Cr', which are linear combinations of the (non-linear) RGB coordinates.
+ *
+ * According to these formulas, if RGB varies between [0.0, 1.0], then Y'
+ * will vary between [0.0, 1.0], while Cb' and Cr' will vary between
+ * [-0.5, 0.5].
+ *
+ * The 0.5 offset is added to the Cb' and Cr' components in the 3x4 encoding
+ * matrix, so the resulting pixel values then fits the 16-bit UNORM.
+ *
+ * [0] https://www.itu.int/rec/R-REC-BT.709-6-201506-I/en
+ */
+static const struct drm_color_ctm_3x4 test_matrix_3x4_bt709_alt_enc = { {
+	0x00000000366cf400ull, 0x00000000b7175900ull, 0x0000000127bb300ull, 0,
+	0x800000001d5475a0ull, 0x8000000062ab8a80ull, 0x0000000080000000ull, 0x0000000080000000ull,
+	0x0000000080000000ull, 0x8000000074432c80ull, 0x800000000bbcd360ull, 0x0000000080000000ull,
+} };
+
+static void vkms_color_ctm_3x4_bt709_alt(struct kunit *test)
+{
+	struct pixel_argb_s32 out;
+
+	/* full white to bt709 */
+	out.a = 0xffff;
+	out.r = 0xffff;
+	out.g = 0xffff;
+	out.b = 0xffff;
+
+	apply_3x4_matrix(&out, &test_matrix_3x4_bt709_alt_enc);
+
+	/* Y' 255 */
+	KUNIT_EXPECT_GT(test, out.r, 0x7F00);
+	KUNIT_EXPECT_LT(test, out.r, 0x11000);
+
+	/* Cb' 127 */
+	KUNIT_EXPECT_GT(test, out.g, 0x7F00);
+	KUNIT_EXPECT_LT(test, out.g, 0x8100);
+
+	/* Cr' 127 */
+	KUNIT_EXPECT_GT(test, out.b, 0x7F00);
+	KUNIT_EXPECT_LT(test, out.b, 0x8100);
+
+	/* full black to bt709 */
+	out.a = 0xffff;
+	out.r = 0x0;
+	out.g = 0x0;
+	out.b = 0x0;
+
+	apply_3x4_matrix(&out, &test_matrix_3x4_bt709_alt_enc);
+
+	/* Y' 0 */
+	KUNIT_EXPECT_LT(test, out.r, 0x100);
+
+	/* Cb' 127 */
+	KUNIT_EXPECT_GT(test, out.g, 0x7F00);
+	KUNIT_EXPECT_LT(test, out.g, 0x8100);
+
+	/* Cr' 127 */
+	KUNIT_EXPECT_GT(test, out.b, 0x7F00);
+	KUNIT_EXPECT_LT(test, out.b, 0x8100);
+
+	/* gray to bt709 */
+	out.a = 0xffff;
+	out.r = 0x7fff;
+	out.g = 0x7fff;
+	out.b = 0x7fff;
+
+	apply_3x4_matrix(&out, &test_matrix_3x4_bt709_alt_enc);
+
+	/* Y' 127 */
+	KUNIT_EXPECT_GT(test, out.r, 0x7F00);
+	KUNIT_EXPECT_LT(test, out.r, 0x8100);
+
+	/* Cb' 127 */
+	KUNIT_EXPECT_GT(test, out.g, 0x7F00);
+	KUNIT_EXPECT_LT(test, out.g, 0x8100);
+
+	/* Cr' 127 */
+	KUNIT_EXPECT_GT(test, out.b, 0x7F00);
+	KUNIT_EXPECT_LT(test, out.b, 0x8100);
+
+	/* == red 255 - bt709 enc == */
+	out.a = 0xffff;
+	out.r = 0xffff;
+	out.g = 0x0;
+	out.b = 0x0;
+
+	apply_3x4_matrix(&out, &test_matrix_3x4_bt709_alt_enc);
+
+	/* Y' 54 */
+	KUNIT_EXPECT_GT(test, out.r, 0x3500);
+	KUNIT_EXPECT_LT(test, out.r, 0x3700);
+
+	/* Cb' 99 */
+	KUNIT_EXPECT_GT(test, out.g, 0x6200);
+	KUNIT_EXPECT_LT(test, out.g, 0x6400);
+
+	/* Cr' 255 */
+	KUNIT_EXPECT_GT(test, out.b, 0x7F00);
+	KUNIT_EXPECT_LT(test, out.b, 0x11000);
+
+	/* == green 255 - bt709 enc == */
+	out.a = 0xffff;
+	out.r = 0x0;
+	out.g = 0xffff;
+	out.b = 0x0;
+
+	apply_3x4_matrix(&out, &test_matrix_3x4_bt709_alt_enc);
+
+	/* Y' 182 */
+	KUNIT_EXPECT_GT(test, out.r, 0xB500);
+	KUNIT_EXPECT_LT(test, out.r, 0xB780); /* laxed by half*/
+
+	/* Cb' 29 */
+	KUNIT_EXPECT_GT(test, out.g, 0x1C00);
+	KUNIT_EXPECT_LT(test, out.g, 0x1E00);
+
+	/* Cr' 12 */
+	KUNIT_EXPECT_GT(test, out.b, 0x0B00);
+	KUNIT_EXPECT_LT(test, out.b, 0x0D00);
+
+	/* == blue 255 - bt709 enc == */
+	out.a = 0xffff;
+	out.r = 0x0;
+	out.g = 0x0;
+	out.b = 0xffff;
+
+	apply_3x4_matrix(&out, &test_matrix_3x4_bt709_alt_enc);
+
+	/* Y' 18 */
+	KUNIT_EXPECT_GT(test, out.r, 0x1100);
+	KUNIT_EXPECT_LT(test, out.r, 0x1300);
+
+	/* Cb' 255 */
+	KUNIT_EXPECT_GT(test, out.g, 0x7F00);
+	KUNIT_EXPECT_LT(test, out.g, 0x11000);
+
+	/* Cr' 116 */
+	KUNIT_EXPECT_GT(test, out.b, 0x7300);
+	KUNIT_EXPECT_LT(test, out.b, 0x7500);
+}
+
 static struct kunit_case vkms_color_test_cases[] = {
 	KUNIT_CASE(vkms_color_test_get_lut_index),
 	KUNIT_CASE(vkms_color_test_lerp),
@@ -400,6 +545,7 @@ static struct kunit_case vkms_color_test_cases[] = {
 	KUNIT_CASE(vkms_color_srgb_inv_srgb),
 	KUNIT_CASE(vkms_color_ctm_3x4_50_desat),
 	KUNIT_CASE(vkms_color_ctm_3x4_bt709),
+	KUNIT_CASE(vkms_color_ctm_3x4_bt709_alt),
 	{}
 };
 

-- 
2.51.0

Claude review: drm/vkms: Fix bad matrix offset component multiplication

[Claude Code Review Bot]

Overall Series Review

Subject: drm/vkms: Fix bad matrix offset component
 multiplication
Author: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
Patches: 3
Reviewed: 2026-02-11T16:19:41.698292

---



---
Generated by Claude Code Patch Reviewer

Re: [PATCH v2 1/2] drm/vkms: Fix bad matrix offset component multiplication

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2811 bytes --]

On Tue, 10 Feb 2026 14:44:00 -0300
Ariel D'Alessandro <ariel.dalessandro@collabora.com> wrote:

> 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-.
> 
> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index cd85de4ffd03d..d53ea4189c97b 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -17,6 +17,8 @@
>  #include "vkms_composer.h"
>  #include "vkms_luts.h"
>  
> +#define UNORM_16BIT_ONE			(1ULL << 16)

Hi,

shouldn't this be 0xffff instead?

> +
>  static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
>  {
>  	u32 new_color;
> @@ -139,20 +141,25 @@ VISIBLE_IF_KUNIT void apply_3x4_matrix(struct pixel_argb_s32 *pixel,
>  	g = drm_int2fixp(pixel->g);
>  	b = drm_int2fixp(pixel->b);
>  
> +	/*
> +	 * 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-.
> +	 */
>  	rf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[0]), r) +
>  	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[1]), g) +
>  	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[2]), b) +
> -	     drm_sm2fixp(matrix->matrix[3]);
> +	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[3]), drm_int2fixp(UNORM_16BIT_ONE));
>  
>  	gf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[4]), r) +
>  	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[5]), g) +
>  	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[6]), b) +
> -	     drm_sm2fixp(matrix->matrix[7]);
> +	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[7]), drm_int2fixp(UNORM_16BIT_ONE));
>  
>  	bf = drm_fixp_mul(drm_sm2fixp(matrix->matrix[8]), r) +
>  	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[9]), g) +
>  	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[10]), b) +
> -	     drm_sm2fixp(matrix->matrix[11]);
> +	     drm_fixp_mul(drm_sm2fixp(matrix->matrix[11]), drm_int2fixp(UNORM_16BIT_ONE));
>  
>  	pixel->r = drm_fixp2int_round(rf);
>  	pixel->g = drm_fixp2int_round(gf);
> 

Ok, so this is because r, g, b have the integer pixel value [0, 65535].
A casual reader would expect them to be normalized [0.0, 1.0] (as is
possible without any loss of precision). But since they are not
normalized, multiplication by normalized 1.0 must be carried out
explicitly.

If UNORM_16BIT_ONE was 0xffff, you would have my
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Claude review: drm/vkms: Fix bad matrix offset component multiplication

[Claude Code Review Bot]

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

Claude review: drm/vkms: Add kunit tests for alternative BT709 encoding matrix

[Claude Code Review Bot]

Patch Review

> +static const struct drm_color_ctm_3x4 test_matrix_3x4_bt709_alt_enc = { {
> +	0x00000000366cf400ull, 0x00000000b7175900ull, 0x0000000127bb300ull, 0,
> +	0x800000001d5475a0ull, 0x8000000062ab8a80ull, 0x0000000080000000ull, 0x0000000080000000ull,
> +	0x0000000080000000ull, 0x8000000074432c80ull, 0x800000000bbcd360ull, 0x0000000080000000ull,
> +} };

This is the key test matrix. The first row (Y' coefficients) has the same coefficients as the existing BT.709 test matrix with a zero offset, which is fine -- Y' doesn't get an offset. The second and third rows (Cb' and Cr') have non-zero offsets (`0x0000000080000000`), which in S31.32 represents 0.5. This exercises the offset multiplication path that patch 1 fixes. This is a good test addition that directly validates the fix.

One thing worth noting: the first three coefficients in row 1 (`0x00000000366cf400`, `0x00000000b7175900`, `0x0000000127bb300`) are the same as in `test_matrix_3x4_bt709_enc`. The Cb' and Cr' coefficients differ from the existing matrix because the existing test uses a different variant of BT.709 (separate U/V vs. Cb'/Cr' formulations). This all looks correct for the standard.

> +	/* Y' 255 */
> +	KUNIT_EXPECT_GT(test, out.r, 0x7F00);
> +	KUNIT_EXPECT_LT(test, out.r, 0x11000);

For full white input (R=G=B=0xffff), Y' should sum to approximately 1.0, which in UNORM-16 is 0xffff. The test range 0x7F00 to 0x11000 (32512 to 69632) is quite wide but covers the expected value. This is acceptable given fixed-point rounding imprecision.

> +	/* Cb' 127 */
> +	KUNIT_EXPECT_GT(test, out.g, 0x7F00);
> +	KUNIT_EXPECT_LT(test, out.g, 0x8100);

For full white, Cb' should be 0.5 (since the chrominance components cancel out and only the 0.5 offset remains). In UNORM-16 terms, 0.5 maps to approximately 0x7FFF (32767), so the range 0x7F00 to 0x8100 is reasonable.

> +	KUNIT_EXPECT_LT(test, out.r, 0xB780); /* laxed by half*/

Minor: "laxed" should be "relaxed".

> +	/* Cr' 255 */
> +	KUNIT_EXPECT_GT(test, out.b, 0x7F00);
> +	KUNIT_EXPECT_LT(test, out.b, 0x11000);

The comment says "Cr' 255" but the upper bound 0x11000 = 69632, which is slightly above 0xffff = 65535. For pure red input, Cr' = 0.5 + 0.5 = 1.0, which maps to 0xffff in UNORM-16. The wide bound accommodates fixed-point rounding. This is fine as a sanity check test.

The test structure is thorough, covering white, black, gray, and the three primary colors (red, green, blue). Each test case verifies Y', Cb', and Cr' output values against expected ranges derived from the BT.709 standard. The comments labeling expected 8-bit-equivalent values (e.g., "Y' 54", "Cb' 99") help readability, though they don't precisely match the 16-bit ranges -- they appear to be approximate 8-bit equivalents for orientation.

No structural issues with the test. The test is properly registered in `vkms_color_test_cases[]`.

**Summary of issues to address:**
1. `UNORM_16BIT_ONE` should be `0xffffULL` (65535), not `(1ULL << 16)` (65536) -- the UNORM representation of 1.0.
2. Consider adding a `Fixes:` tag to patch 1.
3. Minor: "idempotent element" should be "multiplicative identity" in the comment.
4. Minor: "laxed" typo in patch 2.

---
Generated by Claude Code Patch Reviewer