Re: [PATCH v2] drm/rockchip: vop2: Scale the AXI clock to the bandwidth the mode needs

[sashiko-bot@kernel.org]

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 <royalnet026@gmail.com>

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/20260808104240.13776-1-royalnet026@gmail.com/
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/20260813094614.9072-1-royalnet026@gmail.com?part=1