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

[PATCH] 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 3cf3f26e0d8ea..efdaf665435d9 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -16,6 +16,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;
@@ -138,20 +140,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);

---
base-commit: b96bcfba104c65db41378a04f5ccac186f79578f
change-id: 20251223-vkms-composer-fix-matrix-aa2c593f4515

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

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

[Louis Chauvet]

On 12/23/25 18:26, Ariel D'Alessandro 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>

Hi,

Thanks for this patch!

How did you catch the issue? If you have an example of failing color 
conversion, can you add a test case in vkms_color_test?

Thanks,
Louis Chauvet

> ---
>   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 3cf3f26e0d8ea..efdaf665435d9 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -16,6 +16,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;
> @@ -138,20 +140,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);
> 
> ---
> base-commit: b96bcfba104c65db41378a04f5ccac186f79578f
> change-id: 20251223-vkms-composer-fix-matrix-aa2c593f4515
> 
> Best regards,

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

[Ariel D'Alessandro]

Hi Louis,

Sorry for the delay.

On 12/29/25 6:33 AM, Louis Chauvet wrote:
> 
> 
> On 12/23/25 18:26, Ariel D'Alessandro 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>
> 
> Hi,
> 
> Thanks for this patch!
> 
> How did you catch the issue? If you have an example of failing color 
> conversion, can you add a test case in vkms_color_test?
I found it while working on this IGT tools patch series [0], this patch 
in particular [1].

I've just added the related kunit test, so I'll send it as a v2 patchset.

[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/

Regards,

-- 
Ariel D'Alessandro
Software Engineer

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