Re: [PATCH v2 5/9] drm/rockchip: vop2: Balance window fetches across both AXI ports on RK3588

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/rockchip: vop2: wrong AXI clash checks in
  vop2_win_may_fetch()
--

commit 73d9c526a345979a18aa8e0e398a482c59cd3a4f
Author: Arun Iyengar <aiyengar1990@gmail.com>

drm/rockchip: vop2: Balance window fetches across both AXI ports on RK3588

This commit dynamically assigns AXI read ports to VOP2 windows on RK3588
during atomic check to balance memory bandwidth across both ports while
avoiding AXI read ID collisions.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 7c157f5c17346..684fa46924e7f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
[ ... ]
> +/*
> + * Whether a window that doesn't fetch in the new state may still fetch while
> + * a window starts on @crtc. The window registers are latched at the frame
> + * start of their video port: a window switched off in this commit stops
> + * together with the new one only on the same video port, and one switched
> + * off in an earlier commit once that commit is done.
> + */
> +static int vop2_win_may_fetch(struct drm_atomic_commit *state,
> +			      struct vop2_win *win, const struct drm_crtc *crtc,
> +			      bool *fetching)
> +{
> +	const struct drm_plane_state *old = drm_atomic_get_old_plane_state(state, &win->base);
> +	struct drm_crtc_commit *commit;
> +	int ret;
> +
> +	if (old) {
> +		*fetching = old->crtc && old->crtc != crtc;
> +		return 0;
> +	}
> +
> +	ret = drm_modeset_lock(&win->base.mutex, state->acquire_ctx);
> +	if (ret)
> +		return ret;
> +
> +	commit = win->base.state->commit;
> +	*fetching = commit && !try_wait_for_completion(&commit->flip_done);
> +
> +	return 0;
> +}
[ ... ]
> +			if (!fetching) {
> +				ret = vop2_win_may_fetch(state, w, pstate->crtc, &fetching);
> +				if (ret)
> +					return ret;
> +			}
> +
> +			if (fetching)
> +				clash[new->win[j].bus] = true;

[Severity: Medium]
Does vop2_win_may_fetch() misidentify whether a clashing window w is still
fetching by branching on whether w is in @state (old != NULL) instead of
whether w was active in the old AXI state (old->win[j].active)?

When old != NULL, *fetching is set to old->crtc && old->crtc != crtc without
checking old->win[j].active. If w is attached to another CRTC that is in
DPMS off, or w is clipped off-screen (!old->visible), w is already disabled
in hardware, yet pulling w into @state (for instance via
drm_atomic_normalize_zpos()) marks its AXI port as clashing and forces the
new window onto the other, potentially saturated port. Conversely, if w was
disabled in an earlier non-blocking commit that has not completed flip_done
and w is included in @state with old->crtc == NULL, the old != NULL branch
returns *fetching = false without checking old->commit->flip_done.

When old == NULL, the fallback checks win->base.state->commit->flip_done
without checking commit->crtc != crtc, so an earlier non-blocking disable on
the same video port is treated as clashing even though its registers latch at
the same frame start. In addition, drm_atomic_helper_setup_commit() sets
plane_state->commit on every plane in a commit's state, so an in-flight
commit on another CRTC that included an already-inactive plane w causes
vop2_win_may_fetch() to report w as fetching.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-vop2-fixes-v2-0-8218c1cf8ac8@gmail.com?part=5