Re:Re: [PATCH v7 10/22] drm/rockchip: vop2: Fix YUV444 output

["Andy Yan" <andyshrk@163.com>]

Hello Nicolas,

在 2026-01-22 20:59:41，"Nicolas Frattaroli" <nicolas.frattaroli@collabora.com> 写道：
>On Thursday, 22 January 2026 09:28:54 Central European Standard Time Andy Yan wrote:
>> 
>> Hello Nicolas，
>> 
>> At 2026-01-21 22:45:17, "Nicolas Frattaroli" <nicolas.frattaroli@collabora.com> wrote:
>> >YUV444 (aka YCbCr444) output isn't working quite right on RK3588. The
>> >resulting image on the display, while identifying itself as YUV444, has
>> >some components swapped, even after adding the necessary DRM formats to
>> >the conversion functions.
>> >
>> >Judging by downstream, this is because YUV444 also needs an rb swap
>> >performed in the AFBC case.
>> >
>> >Add the DRM formats to the appropriate switch statements, and add a
>> >function for checking whether an rb swap needs to be performed in the
>> >AFBC case.
>> >
>> >Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")
>> >Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>> >---
>> > drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 19 +++++++++++++++++++
>> > 1 file changed, 19 insertions(+)
>> >
>> >diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> >index ec3b4fde10db..469c63dd97d5 100644
>> >--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> >+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> >@@ -176,6 +176,7 @@ static enum vop2_data_format vop2_convert_format(u32 format)
>> > 	case DRM_FORMAT_ARGB2101010:
>> > 	case DRM_FORMAT_XBGR2101010:
>> > 	case DRM_FORMAT_ABGR2101010:
>> >+	case DRM_FORMAT_VUY101010:
>> > 		return VOP2_FMT_XRGB101010;
>> > 	case DRM_FORMAT_XRGB8888:
>> > 	case DRM_FORMAT_ARGB8888:
>> >@@ -184,6 +185,7 @@ static enum vop2_data_format vop2_convert_format(u32 format)
>> > 		return VOP2_FMT_ARGB8888;
>> > 	case DRM_FORMAT_RGB888:
>> > 	case DRM_FORMAT_BGR888:
>> >+	case DRM_FORMAT_VUY888:
>> > 		return VOP2_FMT_RGB888;
>> > 	case DRM_FORMAT_RGB565:
>> > 	case DRM_FORMAT_BGR565:
>> >@@ -225,6 +227,7 @@ static enum vop2_afbc_format vop2_convert_afbc_format(u32 format)
>> > 	case DRM_FORMAT_ARGB2101010:
>> > 	case DRM_FORMAT_XBGR2101010:
>> > 	case DRM_FORMAT_ABGR2101010:
>> >+	case DRM_FORMAT_VUY101010:
>> > 		return VOP2_AFBC_FMT_ARGB2101010;
>> > 	case DRM_FORMAT_XRGB8888:
>> > 	case DRM_FORMAT_ARGB8888:
>> >@@ -233,6 +236,7 @@ static enum vop2_afbc_format vop2_convert_afbc_format(u32 format)
>> > 		return VOP2_AFBC_FMT_ARGB8888;
>> > 	case DRM_FORMAT_RGB888:
>> > 	case DRM_FORMAT_BGR888:
>> >+	case DRM_FORMAT_VUY888:
>> 
>> How did you test this format? It seems tools like modetest don’t support testing this pattern.
>> 
>
>Hi Andy,
>
>using the rest of this series, which implements the "color format"
>DRM property, and the corresponding weston MR that makes use of it[1].
>
>I create a ~/.config/weston.ini with the following contents:
>
>    [output]
>    name=HDMI-A-1
>    color-format=yuv444
>
>This will make Weston try to set the output format to 10-bit YUV444. To
>limit it to 8-bit, you can add `max-bpc=8`. The monitor's EDID needs to
>report YUV444 support, otherwise that Weston version won't let you set
>this property.
>


This looks a bit strange. Your commit message and the Weston configuration here both target the output format, 
but the patch modifies the functions vop2_convert_format and vop2_convert_afbc_format, which are responsible for
converting the data formats of planes/framebuffers (fb)—these have nothing to do with the output format.


>Link: https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/1859 [1]
>
>Kind regards,
>Nicolas Frattaroli
>
>> 
>> 
>> > 		return VOP2_AFBC_FMT_RGB888;
>> > 	case DRM_FORMAT_RGB565:
>> > 	case DRM_FORMAT_BGR565:
>> >@@ -270,6 +274,19 @@ static bool vop2_win_rb_swap(u32 format)
>> > 	}
>> > }
>> > 
>> >+static bool vop2_afbc_rb_swap(u32 format)
>> >+{
>> >+	switch (format) {
>> >+	case DRM_FORMAT_NV24:
>> >+	case DRM_FORMAT_NV30:
>> >+	case DRM_FORMAT_VUY888:
>> >+	case DRM_FORMAT_VUY101010:
>> >+		return true;
>> >+	default:
>> >+		return false;
>> >+	}
>> >+}
>> >+
>> > static bool vop2_afbc_uv_swap(u32 format)
>> > {
>> > 	switch (format) {
>> >@@ -1291,6 +1308,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>> > 		 /* It's for head stride, each head size is 16 byte */
>> > 		stride = ALIGN(stride, block_w) / block_w * 16;
>> > 
>> >+		rb_swap = vop2_afbc_rb_swap(fb->format->format);
>> > 		uv_swap = vop2_afbc_uv_swap(fb->format->format);
>> > 		/*
>> > 		 * This is a workaround for crazy IC design, Cluster
>> >@@ -1308,6 +1326,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>> > 			vop2_win_write(win, VOP2_WIN_AFBC_ENABLE, 1);
>> > 		vop2_win_write(win, VOP2_WIN_AFBC_FORMAT, afbc_format);
>> > 		vop2_win_write(win, VOP2_WIN_AFBC_UV_SWAP, uv_swap);
>> >+		vop2_win_write(win, VOP2_WIN_AFBC_RB_SWAP, rb_swap);
>> > 		/*
>> > 		 * On rk3566/8, this bit is auto gating enable,
>> > 		 * but this function is not work well so we need
>> >
>> 
>
>
>
>