Re: [PATCH] drm/format-helper: Add XRGB8888 to UYVY conversion

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 12.10.26 um 19:08 schrieb Ho Jie Feng:
> Add a helper for devices that use packed UYVY but accept XRGB8888
> framebuffers.

Is there a description of UYVY available somewhere?

>
> Signed-off-by: Ho Jie Feng <hjf3108@gmail.com>
> ---
> This is used for a driver, ms912x that I will be submitting. It only
> supports uyvy framebuffers.

Please resubmit as part of the driver series. We're not going to merge 
it without callers anyway.

>
>   drivers/gpu/drm/drm_format_helper.c   | 73 +++++++++++++++++++++++++++
>   drivers/gpu/drm/drm_format_internal.h | 21 ++++++++
>   include/drm/drm_format_helper.h       |  4 ++
>   3 files changed, 98 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 6cddf05c493bd..606733ca43fb4 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -1379,3 +1379,76 @@ void drm_fb_xrgb8888_to_gray2(struct iosys_map *dst, const unsigned int *dst_pit
>   }
>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_gray2);
>   
> +static void drm_fb_xrgb8888_to_uyvy_line(void *dbuf, const void *sbuf, unsigned int width)
> +{
> +	u8 *transfer_buffer = dbuf;
> +	const __le32 *temp_buffer = sbuf;

For naming, please follow the conventions used by the other helpers in 
this file.

> +	unsigned int i, dst_offset = 0;
> +	unsigned int pixel1, pixel2;
> +	unsigned int r1, g1, b1, r2, g2, b2;
> +	unsigned int r, g, b;
> +	unsigned int v, y1, u, y2;
> +
> +	for (i = 0; i < width; i += 2) {
> +		pixel1 = le32_to_cpup(&temp_buffer[i]);
> +		pixel2 = le32_to_cpup(&temp_buffer[i + 1]);
> +
> +		r1 = (pixel1 & 0x00ff0000) >> 16;
> +		g1 = (pixel1 & 0x0000ff00) >> 8;
> +		b1 =  pixel1 & 0x000000ff;
> +		r2 = (pixel2 & 0x00ff0000) >> 16;
> +		g2 = (pixel2 & 0x0000ff00) >> 8;
> +		b2 =  pixel2 & 0x000000ff;
> +
> +		y1 = drm_pixel_rgb_to_y_bt601(r1, g1, b1);
> +		y2 = drm_pixel_rgb_to_y_bt601(r2, g2, b2);
> +
> +		r = r1 + r2;
> +		g = g1 + g2;
> +		b = b1 + b2;
> +
> +		v = drm_pixel_rgb2_to_v_bt601(r, g, b);
> +		u = drm_pixel_rgb2_to_u_bt601(r, g, b);
> +
> +		transfer_buffer[dst_offset++] = u;
> +		transfer_buffer[dst_offset++] = y1;
> +		transfer_buffer[dst_offset++] = v;
> +		transfer_buffer[dst_offset++] = y2;

Rather that writing bytes individually, use put_unaligned_le32() or a 
similar helper.  Byte order within a pixel is little endian, although 
I'm not sure what this means for UYVY.

> +	}
> +}
> +
> +/**
> + * drm_fb_xrgb8888_to_uyvy - Convert XRGB8888 to UYVY clip buffer
> + * @dst: Array of UYVY destination buffers
> + * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
> + *             within @dst; can be NULL if scanlines are stored next to each other.
> + * @src: Array of XRGB8888 source buffers
> + * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy; x1 and x2 must be even
> + * @state: Transform and conversion state
> + *
> + * This function copies parts of a framebuffer to display memory and converts the
> + * color format during the process. The parameters @dst, @dst_pitch and @src refer
> + * to arrays. Each array must have at least as many entries as there are planes in
> + * @fb's format. Each entry stores the value for the format's respective color plane
> + * at the same index.
> + *
> + * This function does not apply clipping on @dst (i.e. the destination is at the
> + * top-left corner).
> + *
> + * Drivers can use this function for UYVY devices that don't support XRGB8888
> + * natively. ITU BT.601 is being used for the RGB -> YUV conversion.
> + */
> +void drm_fb_xrgb8888_to_uyvy(struct iosys_map *dst, const unsigned int *dst_pitch,
> +			     const struct iosys_map *src, const struct drm_framebuffer *fb,
> +			     const struct drm_rect *clip, struct drm_format_conv_state *state)
> +{
> +	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
> +		2,
> +	};
> +
> +	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false, state,
> +		    drm_fb_xrgb8888_to_uyvy_line);
> +}
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_uyvy);
> +
> diff --git a/drivers/gpu/drm/drm_format_internal.h b/drivers/gpu/drm/drm_format_internal.h
> index ce29dd05bcc55..7a4cfb0d188cb 100644
> --- a/drivers/gpu/drm/drm_format_internal.h
> +++ b/drivers/gpu/drm/drm_format_internal.h
> @@ -159,6 +159,27 @@ static inline u32 drm_pixel_xrgb8888_to_abgr2101010(u32 pix)
>   	       drm_pixel_xrgb8888_to_xbgr2101010(pix);
>   }
>   
> +static inline u32 drm_pixel_rgb_to_y_bt601(u32 r, u32 g, u32 b)
> +{
> +	const u32 luma = (16U << 16) + 16829 * r + 33039 * g + 6416 * b + 32768;
> +
> +	return luma >> 16;
> +}
> +
> +static inline u32 drm_pixel_rgb2_to_u_bt601(u32 r2, u32 g2, u32 b2)
> +{
> +	const u32 u = (128U << 17) - 9714 * r2 - 19071 * g2 + 28784 * b2 + 65536;
> +
> +	return u >> 17;
> +}
> +
> +static inline u32 drm_pixel_rgb2_to_v_bt601(u32 r2, u32 g2, u32 b2)
> +{
> +	const u32 v = (128U << 17) + 28784 * r2 - 24103 * g2 - 4681 * b2 + 65536;
> +
> +	return v >> 17;
> +}
> +

These helpers seem too fine-grained. IMHO it would be better to have a 
helper like  drm_pixel_rgb_to_uyvy() that takes two RGB values and 
returns the assembled 16-bit UYVY value.

Best regards
Thomas

>   /*
>    * Conversion from ARGB8888
>    */
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index 2b5c1aef80b0e..e50f96e07484c 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -136,4 +136,8 @@ void drm_fb_xrgb8888_to_gray2(struct iosys_map *dst, const unsigned int *dst_pit
>   			      const struct iosys_map *src, const struct drm_framebuffer *fb,
>   			      const struct drm_rect *clip, struct drm_format_conv_state *state);
>   
> +void drm_fb_xrgb8888_to_uyvy(struct iosys_map *dst, const unsigned int *dst_pitch,
> +			     const struct iosys_map *src, const struct drm_framebuffer *fb,
> +			     const struct drm_rect *clip, struct drm_format_conv_state *state);
> +
>   #endif /* __LINUX_DRM_FORMAT_HELPER_H */

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)