Messages in this thread | | | Date | Tue, 24 Oct 2023 20:41:33 +0800 | Subject | Re: [PATCH] drm/rockchip: vop: Fix color for RGB888/BGR888 format on VOP full | From | Andy Yan <> |
| |
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@kwiboo.se> > How about a fixes tag? Seems like this was introduced in commit 85a359f25388 > ("drm/rockchip: Add BGR formats to VOP") > >> --- >> 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?
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); >> >> /* > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
| |