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

[Pekka Paalanen <pekka.paalanen@collabora.com>]

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