Re:Re: [PATCH v4 4/4] drm/rockchip: vop2: Support setting custom background color

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

Hello Cristian，

At 2026-01-16 23:22:11, "Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> wrote:
>On 1/10/26 11:58 AM, Cristian Ciocaltea wrote:
>> Hi Andy,
>> 
>> On 1/10/26 6:00 AM, Andy Yan wrote:
>>>
>>>
>>> Hello Cristian,
>>> At 2025-12-20 05:47:01, "Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> wrote:
>>>> The Rockchip VOP2 display controller allows configuring the background
>>>> color of each video output port.
>>>>
>>>> Since a previous patch introduced the BACKGROUND_COLOR CRTC property,
>>>> which defaults to solid black, make use of it when programming the
>>>> hardware.
>>>>
>>>> Note the maximum precision allowed by the display controller is 10bpc,
>>>> while the alpha component is not supported, hence ignored.
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 13 ++++++++++++-
>>>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  4 ++++
>>>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>> index 498df0ce4680..3a232d0d4acb 100644
>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>> @@ -1554,6 +1554,7 @@ static void vop2_post_config(struct drm_crtc *crtc)
>>>> 	struct vop2_video_port *vp = to_vop2_video_port(crtc);
>>>> 	struct vop2 *vop2 = vp->vop2;
>>>> 	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>>>> +	u64 bgcolor = crtc->state->background_color;
>>>> 	u16 vtotal = mode->crtc_vtotal;
>>>> 	u16 hdisplay = mode->crtc_hdisplay;
>>>> 	u16 hact_st = mode->crtc_htotal - mode->crtc_hsync_start;
>>>> @@ -1599,7 +1600,11 @@ static void vop2_post_config(struct drm_crtc *crtc)
>>>> 		vop2_vp_write(vp, RK3568_VP_POST_DSP_VACT_INFO_F1, val);
>>>> 	}
>>>>
>>>> -	vop2_vp_write(vp, RK3568_VP_DSP_BG, 0);
>>>> +	/* Background color is programmed with 10 bits of precision */
>>>> +	val = FIELD_PREP(RK3568_VP_DSP_BG__DSP_BG_RED, DRM_ARGB64_GETR(bgcolor) >> 6);
>>>> +	val |= FIELD_PREP(RK3568_VP_DSP_BG__DSP_BG_GREEN, DRM_ARGB64_GETG(bgcolor) >> 6);
>>>
>>>> +	val |= FIELD_PREP(RK3568_VP_DSP_BG__DSP_BG_BLUE, DRM_ARGB64_GETB(bgcolor) >> 6);
>>>
>>>
>>> the bit31 of  RK3568_VP_DSP_BG  is bg_display_en， that means when we set a 
>>> background color, we should set this bg_display_en bit.
>
>I performed several tests on my ROCK 3A (RK3568), ROCK 4D (RK3576) and ROCK 5B
>(RK3588) boards and noticed that by setting bg_display_en bit to 1 or 0 doesn't
>have any influence on RK3568 and RK3576, the background color is always active
>and cannot be disabled.
>
>However, flipping the bit to 1 on RK3588 has the unexpected effect of covering
>the whole screen with the configured color, even when there's an active plane
>displayed on top. Switching back to 0 makes it work as expected.
>
>Therefore I think we should keep this patch as is, unless there's something else
>we're missing here.
>
>>> The default value of this bit is 1, which explains why the patch currently works 
>>> properly even though it doesn't set bit31.
>> 
>> For some reason, the RK3588 TRM indicates 0x0 for the reset value.  I assume
>> that's a mistake, as RK3576 TRM shows 0x1.
>
>Considering the observation above, it kinda makes sense now for RK3588 to
>default to 0.

   I further confirmed with our IC team: for RK3588, RK3528, and RK3562, if the display_enbit is set, the background color will indeed cover all layers. 
   For other chips, this bit has no effect.
   So ACK

>