Re: [PATCH 3/9] drm/rockchip: vop2: Avoid DCLK source switch for 10-bit YUV422 output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Division by zero in max_dclk calculation due to uninitialized output_bpc.
- [High] Integer overflow in max_dclk calculation on 32-bit architectures.
- [High] Atomic context violation due to calling sleeping functions.
--

commit cdb8cbbc3fd201db3157d6704b3856b8565e10e2
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: vop2: Avoid DCLK source switch for 10-bit YUV422 output
    
The commit modifies the DCLK source decision logic for HDMI output in VOP2.
It prevents unnecessary switching of the DCLK source to the system CRU for
10-bit YUV422 output by forcing the bandwidth check to 8 bpc.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 2833fb49ad81a..17d21e08ad977 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -1922,8 +1922,17 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,

[Severity: High]
This is a pre-existing issue, but does this function violate atomic context
rules?

According to the DRM subsystem guidelines, CRTC atomic enable callbacks execute
in atomic context where sleeping is forbidden. However, vop2_crtc_atomic_enable()
acquires a mutex via vop2_lock() and later calls clk_prepare_enable(), both of
which can sleep.

Can this cause scheduling while atomic warnings or deadlocks during atomic
commits?

>  	 * to 4K@60Hz, if available, otherwise keep using the system CRU.
>  	 */
>  	if (vop2->pll_hdmiphy0 || vop2->pll_hdmiphy1) {
> -		unsigned long max_dclk = DIV_ROUND_CLOSEST_ULL(VOP2_MAX_DCLK_RATE * 8,
> -							       vcstate->output_bpc);
> +		/*
> +		 * YUV422 always transmits two 12-bit components per clock
> +		 * cycle, regardless of the color depth, which from a rate
> +		 * perspective is equivalent to three 8-bit RGB components.
> +		 * Force 8 bpc here so the bandwidth check reflects the actual
> +		 * TMDS rate and avoids an unnecessary DCLK source switch.
> +		 */
> +		unsigned int bpc = vcstate->output_mode == ROCKCHIP_OUT_MODE_YUV422 ?
> +					8 : vcstate->output_bpc;
> +		unsigned long max_dclk = DIV_ROUND_CLOSEST_ULL(VOP2_MAX_DCLK_RATE * 8, bpc);

[Severity: High]
This isn't a bug introduced by this patch, but can bpc be 0 here and cause a
division by zero?

If the VOP2 driver is used with a non-HDMI output like DSI or LVDS on a board
that also has an HDMI PHY, vop2->pll_hdmiphy0 or vop2->pll_hdmiphy1 will be
non-NULL. Since DSI/LVDS encoders do not set vcstate->output_bpc, it defaults
to 0.

This would lead to DIV_ROUND_CLOSEST_ULL(..., 0) before the loop checks if
the endpoint is HDMI.

[Severity: High]
This is also a pre-existing issue, but does VOP2_MAX_DCLK_RATE * 8 overflow on
32-bit architectures?

VOP2_MAX_DCLK_RATE is defined as 600000000UL. On architectures where
unsigned long is 32 bits, 600000000UL * 8 equals 4,800,000,000, which exceeds
ULONG_MAX.

This truncates to 505,032,704 before being promoted to unsigned long long inside
DIV_ROUND_CLOSEST_ULL, improperly forcing higher resolutions to fall back to the
system CRU.

Should this be cast to a 64-bit type first, such as VOP2_MAX_DCLK_RATE * 8ULL?

>  		if (clock <= max_dclk) {
>  			drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
>  				struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-dw-hdmi-qp-yuv-v1-0-a665cfd06d7d@collabora.com?part=3