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

[Ariel D'Alessandro <ariel.dalessandro@collabora.com>]

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