| Message ID | 20231023211158.424489-1-jonas@kwiboo.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/rockchip: vop: Fix color for RGB888/BGR888 format on VOP full | expand |
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? 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); > > /*
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
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@kwiboo.se> >> 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); >>> >>> /*
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; 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); /*
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> --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)