Re: [PATCH v4 7/8] drm/vkms: Create helper macro for YUV formats

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

Hi Louis,

On 5/30/25 11:06, Louis Chauvet wrote:
> The callback functions for line conversion are almost identical for
> semi-planar formats. The generic READ_LINE_YUV_SEMIPLANAR macro
> generate all the required boilerplate to process a line from a
> semi-planar format.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/gpu/drm/vkms/vkms_formats.c | 75 ++++++++++++++++++++++++-------------
>   1 file changed, 48 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 261e822e9618..8ecd75d063f4 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -485,35 +485,56 @@ READ_LINE(R8_read_line, px, u8, argb_u16_from_gray8, *px)
>    * - Convert YUV and YVU with the same function (a column swap is needed when setting up
>    * plane->conversion_matrix)
>    */
> -static void semi_planar_yuv_read_line(const struct vkms_plane_state *plane, int x_start,
> -				      int y_start, enum pixel_read_direction direction, int count,
> -				      struct pixel_argb_u16 out_pixel[])
> -{
> -	u8 *y_plane;
> -	u8 *uv_plane;
> -
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0,
> -			       &y_plane);
> -	packed_pixels_addr_1x1(plane->frame_info,
> -			       x_start / plane->frame_info->fb->format->hsub,
> -			       y_start / plane->frame_info->fb->format->vsub, 1,
> -			       &uv_plane);
> -	int step_y = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> -	int step_uv = get_block_step_bytes(plane->frame_info->fb, direction, 1);
> -	int subsampling = get_subsampling(plane->frame_info->fb->format, direction);
> -	int subsampling_offset = get_subsampling_offset(direction, x_start, y_start);
> -	const struct conversion_matrix *conversion_matrix = &plane->conversion_matrix;
>   
> -	for (int i = 0; i < count; i++) {
> -		*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)
> -			uv_plane += step_uv;
> -	}
> +/**
> + * READ_LINE_YUV_SEMIPLANAR() - Generic generator for a read_line function which can be used for yuv
> + * formats with two planes and block_w == block_h == 1.
> + *
> + * @function_name: Function name to generate
> + * @pixel_1_name: temporary pixel name for the first plane used in the @__VA_ARGS__ parameters

s/temporary/Temporary

> + * @pixel_2_name: temporary pixel name for the second plane used in the @__VA_ARGS__ parameters
> + * @pixel_1_type: Used to specify the type you want to cast the pixel pointer on the plane 1
> + * @pixel_2_type: Used to specify the type you want to cast the pixel pointer on the plane 2
> + * @callback: Callback to call for each pixels. This function should take
> + *            (struct conversion_matrix*, @__VA_ARGS__) as parameter and return a pixel_argb_u16

s/struct conversion_matrix*/struct conversion_matrix *

> + * @__VA_ARGS__: Argument to pass inside the callback. You can use @pixel_1_name and @pixel_2_name
> + *               to access current pixel values
> + */
> +#define READ_LINE_YUV_SEMIPLANAR(function_name, pixel_1_name, pixel_2_name, pixel_1_type,	\
> +				 pixel_2_type, callback, ...)					\
> +static void function_name(const struct vkms_plane_state *plane, int x_start,			\
> +		 int y_start, enum pixel_read_direction direction, int count,			\
> +		 struct pixel_argb_u16 out_pixel[])						\
> +{												\
> +	u8 *plane_1;										\
> +	u8 *plane_2;										\

For now, how do you feel about keeping it as `y_plane` and `uv_plane`.
For me, it has more semantic information and eases the understanding.

With those nits,

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

Best Regards,
- Maíra

> +												\
> +	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0,				\
> +			       &plane_1);							\
> +	packed_pixels_addr_1x1(plane->frame_info,						\
> +			       x_start / plane->frame_info->fb->format->hsub,			\
> +			       y_start / plane->frame_info->fb->format->vsub, 1,		\
> +			       &plane_2);							\
> +	int step_1 = get_block_step_bytes(plane->frame_info->fb, direction, 0);			\
> +	int step_2 = get_block_step_bytes(plane->frame_info->fb, direction, 1);			\
> +	int subsampling = get_subsampling(plane->frame_info->fb->format, direction);		\
> +	int subsampling_offset = get_subsampling_offset(direction, x_start, y_start);		\
> +	const struct conversion_matrix *conversion_matrix = &plane->conversion_matrix;		\
> +												\
> +	for (int i = 0; i < count; i++) {							\
> +		pixel_1_type *(pixel_1_name) = (pixel_1_type *)plane_1;				\
> +		pixel_2_type *(pixel_2_name) = (pixel_2_type *)plane_2;				\
> +		*out_pixel = (callback)(conversion_matrix, __VA_ARGS__);			\
> +		out_pixel += 1;									\
> +		plane_1 += step_1;								\
> +		if ((i + subsampling_offset + 1) % subsampling == 0)				\
> +			plane_2 += step_2;							\
> +	}											\
>   }
>   
> +READ_LINE_YUV_SEMIPLANAR(YUV888_semiplanar_read_line, y, uv, u8, u8, argb_u16_from_yuv161616,
> +			 y[0] * 257, uv[0] * 257, uv[1] * 257)
> +
>   /*
>    * This callback can be used for YUV format where each color component is
>    * stored in a different plane (often called planar formats). It will
> @@ -713,7 +734,7 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
>   	case DRM_FORMAT_NV21:
>   	case DRM_FORMAT_NV61:
>   	case DRM_FORMAT_NV42:
> -		return &semi_planar_yuv_read_line;
> +		return &YUV888_semiplanar_read_line;
>   	case DRM_FORMAT_YUV420:
>   	case DRM_FORMAT_YUV422:
>   	case DRM_FORMAT_YUV444:
>