Re: [PATCH v3 05/14] drm/rockchip: vop2: Avoid division by zero when computing max_dclk

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The expression `VOP2_MAX_DCLK_RATE * 8` silently overflows `unsigned long` on 32-bit architectures, resulting in a drastically lower `max_dclk` value.
- [High] The CRTC atomic enable callback invokes multiple sleeping operations while executing in an atomic context.
--

--- Patch [5]: [PATCH v3 05/14] drm/rockchip: vop2: Avoid division by zero when computing max_dclk ---
commit ba0b89bfb2234e515bb1a3e786beceb00c9813f0
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: vop2: Avoid division by zero when computing max_dclk

The commit modifies vop2_crtc_atomic_enable() to avoid division by zero
when vcstate->output_bpc is 0 by defaulting bpc to 8. This happens when
the CRTC drives a non-HDMI output.

Link: https://lore.kernel.org/all/20260623203310.9F8C81F000E9@smtp.kernel.org/

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 4bb1985b87267..e9f5d015b6458 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -1931,8 +1931,9 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>  	 * 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);
> +		unsigned int bpc = vcstate->output_bpc ?: 8;
> +		unsigned long max_dclk = DIV_ROUND_CLOSEST_ULL(VOP2_MAX_DCLK_RATE * 8, bpc);

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

Since VOP2_MAX_DCLK_RATE is 600000000UL, multiplying by 8 mathematically 
yields 4,800,000,000. On 32-bit architectures, this wraps around to 
505,032,704 before being passed to DIV_ROUND_CLOSEST_ULL(), which would
result in max_dclk being ~63 MHz instead of 600 MHz.

Could this prevent higher resolutions like 1080p and 4K from being 
configured properly on 32-bit kernels?

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

[Severity: High]
This is a pre-existing issue, but does vop2_crtc_atomic_enable() execute 
sleeping operations within an atomic context?

Looking earlier in vop2_crtc_atomic_enable():

	vop2_lock(vop2);

	ret = clk_prepare_enable(vp->dclk);
	if (ret < 0) {

The function unconditionally acquires a mutex via vop2_lock() and calls 
clk_prepare_enable(), which can sleep. Since CRTC atomic enable callbacks
execute in an atomic context during non-blocking commits, could this cause
system instability or deadlocks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-dw-hdmi-qp-yuv-v3-0-a4a982a9f2e7@collabora.com?part=5