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

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 11/06/2025 à 22:24, Maíra Canal a écrit :
> Hi Louis,
> 
> On 5/30/25 11:06, Louis Chauvet wrote:
>> Some YUV format uses 16 bit values, so change the helper function for
>> conversion to support those new formats.
>>
>> Add support for the YUV format P010
> 
> Hum, I don't think this patch added support for P010.

Obviously not... It only adds support for 16 bits yuv conversions.

>>
>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>> ---
>>    drivers/gpu/drm/vkms/tests/vkms_format_test.c | 103 +++++++++++++-------------
>>    drivers/gpu/drm/vkms/vkms_formats.c           |  26 ++++---
>>    drivers/gpu/drm/vkms/vkms_formats.h           |   4 +-
>>    3 files changed, 68 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
>> index 2e1daef94831..272e18a82f9c 100644
>> --- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
>> +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
>> @@ -23,7 +23,7 @@ MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
>>     *     machine endianness
>>     */
>>    struct pixel_yuv_u8 {
>> -	u8 y, u, v;
>> +	u16 y, u, v;
>>    };
>>    
>>    /*
>> @@ -64,7 +64,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
>>    	 *                     in_bits = 16,
>>    	 *                     in_legal = False,
>>    	 *                     in_int = True,
>> -	 *                     out_bits = 8,
>> +	 *                     out_bits = 16,
>>    	 *                     out_legal = False,
>>    	 *                     out_int = True)
>>    	 *
>> @@ -76,13 +76,13 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
>>    		.range = DRM_COLOR_YCBCR_FULL_RANGE,
>>    		.n_colors = 6,
>>    		.colors = {
>> -			{ "white", { 0xff, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
>> -			{ "gray",  { 0x80, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
>> -			{ "black", { 0x00, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
>> -			{ "red",   { 0x4c, 0x55, 0xff }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
>> -			{ "green", { 0x96, 0x2c, 0x15 }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
>> -			{ "blue",  { 0x1d, 0xff, 0x6b }, { 0xffff, 0x0000, 0x0000, 0xffff }},
>> -		},
>> +			{ "white",	{ 0xffff, 0x8000, 0x8000 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
>> +			{ "gray",	{ 0x8080, 0x8000, 0x8000 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
>> +			{ "black",	{ 0x0000, 0x8000, 0x8000 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
>> +			{ "red",	{ 0x4c8b, 0x54ce, 0xffff }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
>> +			{ "green",	{ 0x9645, 0x2b33, 0x14d1 }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
>> +			{ "blue",	{ 0x1d2f, 0xffff, 0x6b2f }, { 0xffff, 0x0000, 0x0000, 0xffff }},
> 
> Is there an explicit need of those tabs? They make the line length
> exceed 100 columns.

No explicit need, I don't know how I missed the checkpatch warning...
Fixed for v2.

> [...]
> 
>> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
>> index 5106441f916b..261e822e9618 100644
>> --- a/drivers/gpu/drm/vkms/vkms_formats.c
>> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
>> @@ -279,16 +279,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) +
>> @@ -310,7 +311,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
>> @@ -504,8 +505,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)
>> @@ -549,8 +550,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) {
>> @@ -690,9 +692,9 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
>>    	case DRM_FORMAT_BGRX8888:
>>    		return &BGRX8888_read_line;
>>    	case DRM_FORMAT_RGB888:
>> -		return RGB888_read_line;
>> +		return &RGB888_read_line;
>>    	case DRM_FORMAT_BGR888:
>> -		return BGR888_read_line;
>> +		return &BGR888_read_line;
> 
> This should be in the previous patch.

Fixed for v5.

> Best Regards,
> - Maíra
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com