[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

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

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

[Ariel D'Alessandro]

Hi Pekka,

On 2/12/26 8:37 AM, Pekka Paalanen wrote:
> 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?

Indeed, you're right, 1.0 is encoded as 0xffff.

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

Ack, will fix in v3 and add your Reviewed-by tag then.

Thanks a lot!

-- 
Ariel D'Alessandro
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK 
Registered in England & Wales, no. 5513718

[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