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 <[email protected]> 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/[email protected]/ > 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/[email protected]?part=5
