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

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

Hi Chaoyi,

On 11/28/25 10:46 AM, Chaoyi Chen wrote:
> Hello Cristian,
> 
> On 11/18/2025 7:52 AM, Cristian Ciocaltea 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..87110beba366 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_BPC(bgcolor, 10));
>> +	val |= FIELD_PREP(RK3568_VP_DSP_BG__DSP_BG_GREEN, DRM_ARGB64_GETG_BPC(bgcolor, 10));
>> +	val |= FIELD_PREP(RK3568_VP_DSP_BG__DSP_BG_BLUE, DRM_ARGB64_GETB_BPC(bgcolor, 10));
> 
> Division is expensive. If we convert a 16 bpc value to 10 bpc using
> direct bit-shifts, that is "DRM_ARGB64_GETX(bgcolor) >> 6" will
> keep the relative error within 1 compared to DIV_ROUND_CLOSEST().
> 
> Should we be concerned about the precision problem here?

The precision was something I initially looked into for CRC verification, in the
context of the related IGT test.  But since I've added the VKMS support, I think
we should not worry about that anymore. 

Moreover, as already pointed out in [1], only RK3576 supports CRC generation at
display controller level, and that is not particularly useful because it doesn't
take the background color into account.  Therefore I had to capture the frame
CRCs at DisplayPort AUX channel level, by using the USB-C DP AltMode capable
port of my RK3588-based board.  However, that solution is not yet available
upstream, as it requires further work for cleanup and improving the overall
USB-C reliability. 

Hence I'll move on with your suggestion and switch to the simple bit-shifting
approach for the next revision.

Thanks,
Cristian

[1] https://lore.kernel.org/all/20251118-crtc-bgcolor-v2-1-dce4063f85a9@collabora.com/