Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Out-of-order execution of non-blocking atomic commits causes AXI clock rate inversion, leading to hardware starvation. - [High] Premature AXI clock lowering during simultaneous CRTC disables causes FIFO underruns. - [Medium] Missing cleanup of `rockchip_rgb` resources on `drm_atomic_private_obj_init` failure in `vop2_bind`. --
--- Patch [1]: [PATCH v2] drm/rockchip: vop2: Scale the AXI clock to the bandwidth the mode needs --- commit e3a0edc8d8cda30a6d024161b316416c1d467e17 Author: Igor Paunovic <[email protected]> drm/rockchip: vop2: Scale the AXI clock to the bandwidth the mode needs VOP2 fetches the scanout data for all of its video ports over a single AXI clock. On RK3588 that clock is pinned to 500 MHz by the device tree and nothing ever raises it, so a mode whose pixel rate outruns what the AXI clock can deliver underruns the internal scanout FIFO. Link: https://lore.kernel.org/all/[email protected]/ Link: https://lore.kernel.org/all/20767137.geO5KgaWL5@diego/ [1] > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > index 4cce3e336f5b3..fdee08042ac74 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c [ ... ] > @@ -1008,6 +1022,72 @@ static bool vop2_gamma_lut_in_use(struct vop2 *vop2, > struct vop2_video_port *vp) [ ... ] > +static void vop2_apply_aclk_rate(struct vop2 *vop2, struct drm_atomic_commit > *state) > +{ > + struct drm_private_state *priv_state; > + > + if (vop2->version != VOP_VERSION_RK3588) > + return; > + > + priv_state = drm_atomic_get_new_private_obj_state(state, > &vop2->aclk_obj); > + if (!priv_state) > + return; > + > + clk_set_rate(vop2->aclk, > + vop2_aclk_rate(vop2, to_vop2_aclk_state(priv_state))); [Severity: High] Does this allow older state snapshots to overwrite newer ones when non-blocking atomic commits execute out of order? Since the driver doesn't implement the atomic_commit_setup hook, DRM core does not enforce execution ordering for commits that only share a drm_private_obj. If a compositor submits a non-blocking modeset on CRTC A (C1), followed closely by a non-blocking modeset on CRTC B (C2), C1's atomic state captures a snapshot of aclk_obj before C2's requirements exist. If C1 is delayed and C2 applies its high clock rate first, when C1 finally executes, it might apply its stale state snapshot here. Could this override C2's higher clock requirement, dropping the AXI clock and causing hardware starvation for the active video ports? > +} > + > static void vop2_crtc_atomic_disable(struct drm_crtc *crtc, > struct drm_atomic_commit *state) > { > @@ -1053,6 +1133,8 @@ static void vop2_crtc_atomic_disable(struct drm_crtc > *crtc, > if (!vop2->enable_count) > vop2_disable(vop2); > > + vop2_apply_aclk_rate(vop2, state); [Severity: High] Can this cause premature AXI clock lowering when a single atomic commit disables multiple active CRTCs? During a multi-CRTC disable commit, drm_atomic_helper_commit_crtc_disable() disables them sequentially. For the first CRTC disabled, this call applies the new atomic state. Since the new state reflects that all participating CRTCs will be disabled, the AXI clock is instantly dropped to its baseline. However, the subsequent CRTCs have not yet been disabled and are still scanning out pixels while waiting for dsp_hold_completion. During this time window, could the lowered AXI clock be insufficient for the remaining active CRTCs, resulting in FIFO underruns? > + > vop2_unlock(vop2); > > if (crtc->state->event && !crtc->state->active) { [ ... ] > @@ -2944,6 +3044,11 @@ static int vop2_bind(struct device *dev, struct device > *master, void *data) > > rockchip_drm_dma_init_device(vop2->drm, vop2->dev); > > + ret = drm_atomic_private_obj_init(vop2->drm, &vop2->aclk_obj, > + &vop2_aclk_state_funcs); > + if (ret) > + goto err_crtcs; [Severity: Medium] Does this error path leak rockchip_rgb resources? If drm_atomic_private_obj_init() fails, the code jumps to the err_crtcs label. However, the err_crtcs block only calls vop2_destroy_crtcs(vop2), bypassing the rockchip_rgb_fini(vop2->rgb) cleanup for the RGB encoder resources allocated earlier in vop2_bind(). > + > pm_runtime_enable(&pdev->dev); > > return 0; > > err_crtcs: > vop2_destroy_crtcs(vop2); > > return ret; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
