On 2023-10-24 14:41, Andy Yan wrote: > Hi: > > On 10/24/23 16:49, Christopher Obbard wrote: >> Hi Jonas, >> >> On Mon, 2023-10-23 at 21:11 +0000, Jonas Karlman wrote: >>> Use of DRM_FORMAT_RGB888 and DRM_FORMAT_BGR888 on e.g. RK3288, RK3328 >>> and RK3399 result in wrong colors being displayed. >>> >>> The issue can be observed using modetest: >>> >>> modetest -s <connector_id>@<crtc_id>:1920x1080-60@RG24 >>> modetest -s <connector_id>@<crtc_id>:1920x1080-60@BG24 >>> >>> Vendor 4.4 kernel apply an inverted rb swap for these formats on VOP >>> full (major = 3). >>> >>> Fix colors by applying rb swap similar to vendor 4.4 kernel. >>> >>> Signed-off-by: Jonas Karlman <jonas@xxxxxxxxx> >> How about a fixes tag? Seems like this was introduced in commit 85a359f25388 >> ("drm/rockchip: Add BGR formats to VOP") That commit added BGR888 format, the RGB888 format goes back to from when driver was initially added. This patch depend on a macro that was introduced later, in commit eb5cb6aa9a72 ("drm/rockchip: vop: add a series of vop support"). Still think commit 85a359f25388 is best commit to use in a fixes tag. Will include a fixes tag for 85a359f25388 in v2. >> >>> --- >>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>> index b3d0b6ae9294..a1ce09a22f83 100644 >>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >>> @@ -247,14 +247,17 @@ static inline void vop_cfg_done(struct vop *vop) >>> VOP_REG_SET(vop, common, cfg_done, 1); >>> } >>> >>> -static bool has_rb_swapped(uint32_t format) >>> +static bool has_rb_swapped(uint32_t version, uint32_t format) >>> { >>> switch (format) { >>> case DRM_FORMAT_XBGR8888: >>> case DRM_FORMAT_ABGR8888: >>> - case DRM_FORMAT_BGR888: >>> case DRM_FORMAT_BGR565: >>> return true; >>> + case DRM_FORMAT_RGB888: >>> + return VOP_MAJOR(version) == 3; >>> + case DRM_FORMAT_BGR888: >>> + return VOP_MAJOR(version) != 3; >> The hardcoded bits are quite scary as it applies to all hardware variants ;-). >> Is it worth adding an inline comment to describe what is going on and how it >> relates to VOPL and VOPH? Or would it be even better to add this as a quirk in >> the various vop_data structs? > I will add a comment in v2. It would be a quirk that apply for all VOP full framework, IP version 3.x, SoCs so checking VOP_MAJOR seem like a good option. See commit eb5cb6aa9a72 ("drm/rockchip: vop: add a series of vop support") for a list of SoCs that use VOP full framework: Vop Full framework now has following vops: IP version chipname 3.1 rk3288 3.2 rk3368 3.4 rk3366 3.5 rk3399 big 3.6 rk3399 lit 3.7 rk3228 3.8 rk3328 major version: used for IP structure, Vop full framework is 3, vop little framework is 2. There are currently three struct vop_data that is missing version declaration: - rk3036_vop should use VOP_VERSION(2, 2) - rk3126_vop should use VOP_VERSION(2, 4) - rk3188_vop guessing is 2.0/2.1 (same/similar as rk3066) Since none of them are 3.x / VOP full framework, this patch should only fix/change behavior for affected 3.x / VOP full framework SoCs. Regards, Jonas > > Every vop hardware has a version(including VOPB/VOPL), so I think use > VOP_MAJOR to distinguish is ok. Of course it's better > > to add some comments. As for adding some quirk in vop_data, I have the > idea of adding a table to describe the drm format and it's (rb/uv swap, > afbc) > > capability, but I think this is can be done in the future. > >> >> >> Cheers! >> >> Chris >> >>> default: >>> return false; >>> } >>> @@ -1035,7 +1038,7 @@ static void vop_plane_atomic_update(struct drm_plane >>> *plane, >>> VOP_WIN_SET(vop, win, dsp_info, dsp_info); >>> VOP_WIN_SET(vop, win, dsp_st, dsp_st); >>> >>> - rb_swap = has_rb_swapped(fb->format->format); >>> + rb_swap = has_rb_swapped(vop->data->version, fb->format->format); >>> VOP_WIN_SET(vop, win, rb_swap, rb_swap); >>> >>> /*