Re: [PATCH v5 6/8] drm/vkms: Change YUV helpers to support u16 inputs for conversion

["Maíra Canal" <mcanal@igalia.com>]

Hi Louis,

On 27/06/25 06:12, Louis Chauvet wrote:
> Some YUV format uses 16 bit values, so change the helper function for
> conversion to support those new formats.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>> ---
>   drivers/gpu/drm/vkms/tests/vkms_format_test.c | 139 +++++++++++++-------------
>   drivers/gpu/drm/vkms/vkms_formats.c           |  22 ++--
>   drivers/gpu/drm/vkms/vkms_formats.h           |   4 +-
>   3 files changed, 84 insertions(+), 81 deletions(-)
> 

[...]

>   
>   /*
> - * vkms_format_test_yuv_u8_to_argb_u16 - Testing the conversion between YUV
> + * vkms_format_test_yuv_u16_to_argb_u16 - Testing the conversion between YUV
>    * colors to ARGB colors in VKMS
>    *
>    * This test will use the functions get_conversion_matrix_to_argb_u16 and
>    * argb_u16_from_yuv888 to convert YUV colors (stored in

s/argb_u16_from_yuv888/argb_u16_from_yuv161616

> - * yuv_u8_to_argb_u16_cases) into ARGB colors.
> + * yuv_u16_to_argb_u16_cases) into ARGB colors.
>    *
>    * The conversion between YUV and RGB is not totally reversible, so there may be
>    * some difference between the expected value and the result.
>    * In addition, there may be some rounding error as the input color is 8 bits

 From what I understand the input color now is 16 bits as well. Please,
update the comment.

Sorry, I missed those nits in the first review. Apart from that,

Reviewed-by: Maíra Canal <mcanal@igalia.com>

Best Regards,
- Maíra

>    * and output color is 16 bits.
>    */
> -static void vkms_format_test_yuv_u8_to_argb_u16(struct kunit *test)
> +static void vkms_format_test_yuv_u16_to_argb_u16(struct kunit *test)
>   {
> -	const struct yuv_u8_to_argb_u16_case *param = test->param_value;
> +	const struct yuv_u16_to_argb_u16_case *param = test->param_value;
>   	struct pixel_argb_u16 argb;
>   
>   	for (size_t i = 0; i < param->n_colors; i++) {
> @@ -236,7 +236,8 @@ static void vkms_format_test_yuv_u8_to_argb_u16(struct kunit *test)
>   		get_conversion_matrix_to_argb_u16
>   			(DRM_FORMAT_NV12, param->encoding, param->range, &matrix);
>   
> -		argb = argb_u16_from_yuv888(color->yuv.y, color->yuv.u, color->yuv.v, &matrix);
> +		argb = argb_u16_from_yuv161616(&matrix, color->yuv.y, color->yuv.u,
> +					       color->yuv.v);
>   
>   		KUNIT_EXPECT_LE_MSG(test, abs_diff(argb.a, color->argb.a), 0x1ff,
>   				    "On the A channel of the color %s expected 0x%04x, got 0x%04x",
> @@ -253,19 +254,19 @@ static void vkms_format_test_yuv_u8_to_argb_u16(struct kunit *test)
>   	}
>   }
>   
> -static void vkms_format_test_yuv_u8_to_argb_u16_case_desc(struct yuv_u8_to_argb_u16_case *t,
> -							  char *desc)
> +static void vkms_format_test_yuv_u16_to_argb_u16_case_desc(struct yuv_u16_to_argb_u16_case *t,
> +							   char *desc)
>   {
>   	snprintf(desc, KUNIT_PARAM_DESC_SIZE, "%s - %s",
>   		 drm_get_color_encoding_name(t->encoding), drm_get_color_range_name(t->range));
>   }
>   
> -KUNIT_ARRAY_PARAM(yuv_u8_to_argb_u16, yuv_u8_to_argb_u16_cases,
> -		  vkms_format_test_yuv_u8_to_argb_u16_case_desc
> +KUNIT_ARRAY_PARAM(yuv_u16_to_argb_u16, yuv_u16_to_argb_u16_cases,
> +		  vkms_format_test_yuv_u16_to_argb_u16_case_desc
>   );
>   
>   static struct kunit_case vkms_format_test_cases[] = {
> -	KUNIT_CASE_PARAM(vkms_format_test_yuv_u8_to_argb_u16, yuv_u8_to_argb_u16_gen_params),
> +	KUNIT_CASE_PARAM(vkms_format_test_yuv_u16_to_argb_u16, yuv_u16_to_argb_u16_gen_params),
>   	{}
>   };
>   
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 5b50e8622521..03eb73f4caef 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -269,16 +269,17 @@ static struct pixel_argb_u16 argb_u16_from_BGR565(const __le16 *pixel)
>   	return out_pixel;
>   }
>   
> -VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
> -							    const struct conversion_matrix *matrix)
> +VISIBLE_IF_KUNIT
> +struct pixel_argb_u16 argb_u16_from_yuv161616(const struct conversion_matrix *matrix,
> +					      u16 y, u16 channel_1, u16 channel_2)
>   {
>   	u16 r, g, b;
>   	s64 fp_y, fp_channel_1, fp_channel_2;
>   	s64 fp_r, fp_g, fp_b;
>   
> -	fp_y = drm_int2fixp(((int)y - matrix->y_offset) * 257);
> -	fp_channel_1 = drm_int2fixp(((int)channel_1 - 128) * 257);
> -	fp_channel_2 = drm_int2fixp(((int)channel_2 - 128) * 257);
> +	fp_y = drm_int2fixp((int)y - matrix->y_offset * 257);
> +	fp_channel_1 = drm_int2fixp((int)channel_1 - 128 * 257);
> +	fp_channel_2 = drm_int2fixp((int)channel_2 - 128 * 257);
>   
>   	fp_r = drm_fixp_mul(matrix->matrix[0][0], fp_y) +
>   	       drm_fixp_mul(matrix->matrix[0][1], fp_channel_1) +
> @@ -300,7 +301,7 @@ VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1,
>   
>   	return argb_u16_from_u16161616(0xffff, r, g, b);
>   }
> -EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
> +EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv161616);
>   
>   /**
>    * READ_LINE() - Generic generator for a read_line function which can be used for format with one
> @@ -492,8 +493,8 @@ static void semi_planar_yuv_read_line(const struct vkms_plane_state *plane, int
>   	const struct conversion_matrix *conversion_matrix = &plane->conversion_matrix;
>   
>   	for (int i = 0; i < count; i++) {
> -		*out_pixel = argb_u16_from_yuv888(y_plane[0], uv_plane[0], uv_plane[1],
> -						  conversion_matrix);
> +		*out_pixel = argb_u16_from_yuv161616(conversion_matrix, y_plane[0] * 257,
> +						     uv_plane[0] * 257, uv_plane[1] * 257);
>   		out_pixel += 1;
>   		y_plane += step_y;
>   		if ((i + subsampling_offset + 1) % subsampling == 0)
> @@ -537,8 +538,9 @@ static void planar_yuv_read_line(const struct vkms_plane_state *plane, int x_sta
>   	const struct conversion_matrix *conversion_matrix = &plane->conversion_matrix;
>   
>   	for (int i = 0; i < count; i++) {
> -		*out_pixel = argb_u16_from_yuv888(*y_plane, *channel_1_plane, *channel_2_plane,
> -						  conversion_matrix);
> +		*out_pixel = argb_u16_from_yuv161616(conversion_matrix,
> +						     *y_plane * 257, *channel_1_plane * 257,
> +						     *channel_2_plane * 257);
>   		out_pixel += 1;
>   		y_plane += step_y;
>   		if ((i + subsampling_offset + 1) % subsampling == 0) {
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
> index b4fe62ab9c65..eeb208cdd6b1 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.h
> +++ b/drivers/gpu/drm/vkms/vkms_formats.h
> @@ -14,8 +14,8 @@ void get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encod
>   				       struct conversion_matrix *matrix);
>   
>   #if IS_ENABLED(CONFIG_KUNIT)
> -struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
> -					   const struct conversion_matrix *matrix);
> +struct pixel_argb_u16 argb_u16_from_yuv161616(const struct conversion_matrix *matrix,
> +					      u16 y, u16 channel_1, u16 channel_2);
>   #endif
>   
>   #endif /* _VKMS_FORMATS_H_ */
>