Re: [PATCH 3/3] drm/rockchip: vop2: Support 8K output by splicing two video ports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] drm/rockchip: vop2: unconditional left_win lookup in vop2_win_get_pstate()
- [High] drm/rockchip: vop2: unreserved splice_win planes conflict across CRTCs
- [High] drm/rockchip: vop2: unlocked and non-atomic splice_vp check in atomic_check
- [High] drm/rockchip: vop2: NULL crtc->state deref on vop2_crtc_atomic_enable() error
- [High] drm/rockchip: vop2: unvalidated split rects in vop2_plane_splice_check()
- [High] drm/rockchip: vop2: wrong VP ID and cfg_done wait in setup_layer_mixer()
- [High] drm/rockchip: vop2: unlatched splice_win disable in vop2_crtc_atomic_disable()
- [Medium] drm/rockchip: vop2: wrong window target in vop2_plane_setup_color_key() call
--

commit 5afbbbf195591a121c509707a5216215ec7b5279
Author: Chaoyi Chen <chaoyi.chen@rock-chips.com>

drm/rockchip: vop2: Support 8K output by splicing two video ports

This commit adds 8K display mode support on RK3588 VOP2 by splicing two video
ports (VP0 and VP1) and pairing windows horizontally across the left and right
halves of the screen. It updates CRTC, plane, and overlay/post-processing
configuration to split planes and program both video ports.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 71312bbeabeb1..f5a52554eaae8 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c

[ ... ]

> @@ -1012,13 +1013,33 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
>  				     struct drm_atomic_commit *state)
>  {
>  	struct vop2_video_port *vp = to_vop2_video_port(crtc);
> +	struct rockchip_crtc_state *old_vcstate;
> +	struct vop2_video_port *splice_vp = NULL;
>  	struct vop2 *vop2 = vp->vop2;
>  	struct drm_crtc_state *old_crtc_state;
> +	struct drm_plane *plane;
>  	int ret;
>  
>  	vop2_lock(vop2);
>  
>  	old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> +	old_vcstate = to_rockchip_crtc_state(old_crtc_state);
> +	if (old_vcstate->splice_mode)
> +		splice_vp = &vop2->vps[vop2->data->vp[vp->id].splice_vp_id];
> +
> +	/*
> +	 * The right windows are not tracked by the plane states, disable
> +	 * them before the video ports are put into standby.
> +	 */
> +	if (splice_vp) {
> +		drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) {
> +			struct vop2_win *win = to_vop2_win(plane)->splice_win;
> +
> +			if (win)
> +				vop2_win_disable(win);
> +		}
> +	}
> +
>  	drm_atomic_helper_disable_planes_on_crtc(old_crtc_state, false);
>  
>  	drm_crtc_vblank_off(crtc);
> @@ -1034,6 +1055,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
>  
>  	vop2_crtc_enable_irq(vp, VP_INT_DSP_HOLD_VALID);
>  
> +	if (splice_vp)
> +		vop2->ops->setup_splice(vp, false);
> +
>  	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, RK3568_VP_DSP_CTRL__STANDBY);

[Severity: High]
When old_vcstate->splice_mode is true, vop2_crtc_atomic_disable() disables
each plane's splice_win via vop2_win_disable(win) and clears
RK3568_LUT_PORT_SEL__SPLICE_EN, but never calls vop2_cfg_done().

Furthermore, vop2_cfg_done(vp) checks vcstate->splice_mode on vp->crtc.state
(the new CRTC state):

    if (vcstate->splice_mode) {
        int splice_vp_id = vop2->data->vp[vp->id].splice_vp_id;

        val |= BIT(splice_vp_id) | (BIT(splice_vp_id) << 16);
    }

If a CRTC transitions from an 8K splice mode to a non-splice mode (or is
disabled while another CRTC keeps VOP2 powered on), the new vcstate->splice_mode
is false, so subsequent vop2_cfg_done(vp) calls only set BIT(vp->id) and never
set BIT(splice_vp_id).

Won't this leave the shadowed VOP2_WIN_ENABLE and VOP2_WIN_CLUSTER_ENABLE
register writes on splice_win unlatched in hardware?

> @@ -1064,6 +1093,152 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,

[ ... ]

> +	/*
> +	 * Make sure the yrgb/uv mst of the right window are byte aligned
> +	 * with full pixel.
> +	 */
> +	if (right_dst_w) {
> +		if (fb->format->format == DRM_FORMAT_NV15)
> +			left_src_w &= ~0x7;
> +		else if (fb->format->is_yuv)
> +			left_src_w &= ~0x1;
> +	}
> +
> +	if (left_src_w > src_w)
> +		left_src_w = src_w;
> +
> +	left_src->x1 = src->x1;
> +	left_src->x2 = src->x1 + (left_src_w << 16);
> +	left_dst->x1 = dst->x1;
> +	left_dst->x2 = dst->x1 + left_dst_w;
> +
> +	right_src->x1 = left_src->x2;
> +	right_src->x2 = src->x2;
> +	right_dst->x1 = dst->x1 + left_dst_w - half_hdisplay;
> +	if (right_dst->x1 < 0)
> +		right_dst->x1 = 0;
> +	right_dst->x2 = right_dst->x1 + right_dst_w;
> +
> +	left_src->y1 = right_src->y1 = src->y1;
> +	left_src->y2 = right_src->y2 = src->y2;
> +	left_dst->y1 = right_dst->y1 = dst->y1;
> +	left_dst->y2 = right_dst->y2 = dst->y2;
> +
> +	return 0;
> +}
> +
> +static int vop2_cluster_splice_scale_check(struct vop2_win *win,
> +					   struct drm_plane_state *pstate,
> +					   int hdisplay)
> +{
> +	struct drm_rect *src = &pstate->src;
> +	struct drm_rect *dst = &pstate->dst;
> +	int half_hdisplay = hdisplay >> 1;
> +
> +	/* scale up is ok */
> +	if ((drm_rect_width(src) >> 16) <= drm_rect_width(dst))
> +		return 0;
> +
> +	if ((drm_rect_width(src) >> 16) <= VOP2_MAX_VP_OUTPUT_WIDTH)
> +		return 0;
> +	/*
> +	 * Cluster scale down limitation in splice mode:
> +	 * If scale down, must display at horizontal center
> +	 */
> +	if (dst->x1 < half_hdisplay && dst->x2 > half_hdisplay) {
> +		if (dst->x2 + dst->x1 != hdisplay) {
> +			drm_dbg_kms(win->vop2->drm,
> +				    "%s must scale down at center in splice mode\n",
> +				    win->data->name);
> +			return -EINVAL;
> +		}
> +
> +		if (drm_rect_calc_hscale(src, dst, 1, FRAC_16_16(6, 5)) < 0) {
> +			drm_dbg_kms(win->vop2->drm,
> +				    "%s scale down factor should be < 1.2 in splice mode\n",
> +				    win->data->name);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int vop2_plane_splice_check(struct drm_plane_state *pstate,
> +				   struct drm_crtc_state *cstate)
> +{
> +	struct vop2_win *win = to_vop2_win(pstate->plane);
> +	struct vop2 *vop2 = win->vop2;
> +	struct vop2_win *splice_win = win->splice_win;
> +	struct drm_rect left_src, left_dst, right_src, right_dst;
> +	int ret;
> +
> +	if (!(win->data->feature & WIN_FEATURE_SPLICE_LEFT) || !splice_win) {
> +		drm_dbg_kms(vop2->drm, "%s cannot be used in splice mode\n",
> +			    win->data->name);
> +		return -EINVAL;
> +	}
> +
> +	if (pstate->rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270 |
> +				DRM_MODE_REFLECT_X)) {
> +		drm_dbg_kms(vop2->drm, "%s can't rotate 270/90 and xmirror in splice mode\n",
> +			    win->data->name);
> +		return -EINVAL;
> +	}
> +
> +	if (vop2_cluster_window(win)) {
> +		ret = vop2_cluster_splice_scale_check(win, pstate,
> +						      cstate->adjusted_mode.hdisplay);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = vop2_calc_splice_rects(pstate, cstate, &left_src, &left_dst,
> +				     &right_src, &right_dst);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

[Severity: High]
In vop2_plane_atomic_check(), the minimum size check (src_w >= 4, dest_w >= 4)
is only performed on the combined plane, and max_src_w is doubled to
2 * vop2_data->max_input.width. vop2_plane_splice_check() then computes
left_src, left_dst, right_src, and right_dst via vop2_calc_splice_rects(),
but does not validate the resulting split rectangles:

1. If a plane straddles half_hdisplay such that either split half has a
   non-zero width smaller than 4 pixels (or if left_src_w is rounded down to
   0 by the NV15/YUV alignment mask while left_dst_w > 0),
   vop2_plane_atomic_check() succeeds. During commit, vop2_win_atomic_update()
   will then trigger WARN_ON(dsp_w < 4) || WARN_ON(src_w < 4) and return
   early without updating VOP2_WIN_YRGB_MST or disabling the window, leaving
   a previously active window scanning out a stale DMA address.

2. If a plane with src_w > 4096 is placed wholly or mostly on one half of the
   screen (for example, dst->x2 <= half_hdisplay or dst->x1 >= half_hdisplay,
   which also bypasses the dst->x1 < half_hdisplay && dst->x2 > half_hdisplay
   check in vop2_cluster_splice_scale_check(), or on an Esmart window),
   left_src_w or right_src_w will exceed the single-window maximum input
   width of 4096.

3. In vop2_calc_splice_rects(), the 8-pixel alignment mask (left_src_w &= ~0x7)
   only checks DRM_FORMAT_NV15, missing DRM_FORMAT_NV20 and DRM_FORMAT_NV30
   which also use packed 10-bit YUV blocks on Esmart windows.

Should vop2_plane_splice_check() validate that each visible half satisfies
the per-window minimum (4) and maximum (vop2_data->max_input.width) width
constraints, and should vop2_calc_splice_rects() also align DRM_FORMAT_NV20
and DRM_FORMAT_NV30 to 8 pixels?

[ ... ]

> @@ -1468,7 +1659,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>  
>  	vop2_setup_scale(vop2, win, src_w, src_h, dsp_w, dsp_h, fb->format->format);
>  	if (!vop2_cluster_window(win))
> -		vop2_plane_setup_color_key(plane, 0);
> +		vop2_plane_setup_color_key(pstate->plane, 0);
>  	vop2_win_write(win, VOP2_WIN_ACT_INFO, act_info);
>  	vop2_win_write(win, VOP2_WIN_DSP_INFO, dsp_info);
>  	vop2_win_write(win, VOP2_WIN_DSP_ST, dest->y1 << 16 | (dest->x1 & 0xffff));

[Severity: Medium]
When vop2_plane_atomic_update() calls vop2_win_atomic_update() for
win->splice_win, pstate->plane still points to the left window's drm_plane.
Inside vop2_plane_setup_color_key(), the target window is resolved via
to_vop2_win(plane):

    struct vop2_win *win = to_vop2_win(plane);

Doesn't this program VOP2_WIN_COLOR_KEY_EN and VOP2_WIN_COLOR_KEY on the left
window a second time instead of programming win->splice_win when updating an
Esmart window in splice mode?

> @@ -1488,6 +1679,45 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>  	}
>  }
>  
> +static void vop2_plane_atomic_update(struct drm_plane *plane,
> +				     struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *pstate = plane->state;
> +	struct drm_crtc *crtc = pstate->crtc;
> +	struct vop2_win *win = to_vop2_win(plane);
> +	struct drm_crtc_state *cstate;
> +	struct rockchip_crtc_state *vcstate;
> +	struct drm_rect left_src, left_dst, right_src, right_dst;
> +
> +	if (WARN_ON(!crtc))
> +		return;
> +
> +	if (!pstate->visible) {
> +		vop2_plane_atomic_disable(plane, state);
> +		return;
> +	}
> +
> +	cstate = drm_atomic_get_new_crtc_state(state, crtc);
> +	if (!cstate)
> +		cstate = crtc->state;
> +	vcstate = to_rockchip_crtc_state(cstate);
> +
> +	if (!vcstate->splice_mode) {
> +		vop2_win_atomic_update(win, &pstate->src, &pstate->dst, pstate);
> +		return;
> +	}
> +
> +	if (!win->splice_win)
> +		return;
> +
> +	if (WARN_ON(vop2_calc_splice_rects(pstate, cstate, &left_src, &left_dst,
> +					  &right_src, &right_dst)))
> +		return;
> +
> +	vop2_win_atomic_update(win, &left_src, &left_dst, pstate);
> +	vop2_win_atomic_update(win->splice_win, &right_src, &right_dst, pstate);
> +}

[Severity: High]
In vop2_create_crtcs(), all right-half windows (Cluster1-win0, Cluster3-win0,
Esmart1-win0, Esmart3-win0) are still registered as independent DRM primary or
overlay planes. For example, when VP1 has no DT port, the primary plane loop
in vop2_create_crtcs() skips VP1 and assigns Cluster1-win0
(Cluster0-win0->splice_win) as VP2's primary plane, while the remaining
right-half windows are registered as overlay planes with possible_crtcs
covering all CRTCs.

However, neither vop2_plane_splice_check() nor vop2_plane_atomic_check()
verifies whether win->splice_win is currently active as a DRM plane on another
CRTC, or prevents another CRTC from enabling a plane whose win->left_win is
active on a spliced CRTC:

1. If VP0 is in 8K splice mode using Cluster0 or Esmart0 while VP2 uses
   Cluster1 or Esmart1, both CRTCs will concurrently overwrite the same
   physical window registers, win->delay, and RK3568_OVL_PORT_SEL.

2. Because every splice_win has a higher plane index than its left_win, if a
   right-half DRM plane is disabled in the same atomic commit that updates
   its left_win in splice mode, drm_atomic_helper_commit_planes() calls
   vop2_plane_atomic_update(left_win) first (enabling win->splice_win) and
   then calls vop2_plane_atomic_disable(splice_win) second, which calls
   vop2_win_disable(splice_win) and turns off the right half of the spliced
   plane.

How is exclusive ownership of win->splice_win enforced between the spliced
CRTC and other CRTCs?

[ ... ]

> @@ -1941,6 +2203,14 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>  
>  	clk_set_rate(vp->dclk, clock);
>  
> +	if (vcstate->splice_mode) {
> +		splice_vp = &vop2->vps[vp_data->splice_vp_id];
> +		splice_vp->splice_mode_right = true;
> +		splice_vp->left_vp = vp;
> +
> +		vop2->ops->setup_splice(vp, true);
> +	}
> +
>  	vop2_post_config(crtc, true, crtc_state, old_crtc_state);

[Severity: High]
What happens if vop2_crtc_atomic_enable() returns early on an error path
(such as clk_prepare_enable(vp->dclk) failing or !clock) before
splice_vp->splice_mode_right and splice_vp->left_vp are initialized here?

Because vcstate->splice_mode was already set to true in
vop2_crtc_atomic_check(), the subsequent calls in the same commit to
vop2_crtc_atomic_begin() -> rk3568_vop2_setup_overlay() ->
rk3568_vop2_setup_layer_mixer(splice_vp) and vop2_crtc_atomic_flush() ->
vop2_post_config() -> rk3568_vop2_setup_bg_dly(splice_vp) will still execute
for splice_vp.

In both rk3568_vop2_setup_layer_mixer() and rk3568_vop2_setup_bg_dly():

    if (vp->splice_mode_right)
        crtc = &vp->left_vp->crtc;
    vcstate = to_rockchip_crtc_state(crtc->state);

Since splice_vp->splice_mode_right is still false, crtc remains
&splice_vp->crtc, and when splice_vp is reserved without a DT port,
splice_vp->crtc.state is NULL, causing a NULL pointer dereference.

> @@ -1985,21 +2255,59 @@ static int vop2_crtc_atomic_check(struct drm_crtc *crtc,
>  				  struct drm_atomic_commit *state)
>  {
>  	struct vop2_video_port *vp = to_vop2_video_port(crtc);
> +	struct vop2 *vop2 = vp->vop2;
> +	const struct vop2_video_port_data *vp_data = &vop2->data->vp[vp->id];
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(crtc_state);
> +	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> +	struct vop2_video_port *splice_vp;
>  	struct drm_plane *plane;
>  	int nplanes = 0;
> -	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>  	int ret;
>  
>  	ret = vop2_crtc_atomic_check_gamma(vp, crtc, state, crtc_state);
>  	if (ret)
>  		return ret;
>  
> +	if (vp->splice_mode_right && crtc_state->active)
> +		return -EBUSY;
> +
>  	drm_atomic_crtc_state_for_each_plane(plane, crtc_state)
>  		nplanes++;
>  
>  	if (nplanes > vp->nlayers)
>  		return -EINVAL;
>  
> +	vcstate->splice_mode = false;
> +	if (crtc_state->enable && mode->hdisplay > VOP2_MAX_VP_OUTPUT_WIDTH) {
> +		const struct drm_crtc_state *splice_state;
> +
> +		if (!(vop2->data->feature & VOP2_FEATURE_SPLICE) ||
> +		    vp_data->max_output.width <= VOP2_MAX_VP_OUTPUT_WIDTH)
> +			return -EINVAL;
> +
> +		splice_vp = &vop2->vps[vp_data->splice_vp_id];
> +		if (splice_vp->crtc.state) {
> +			splice_state = drm_atomic_get_new_crtc_state(state, &splice_vp->crtc);
> +			if (!splice_state)
> +				splice_state = splice_vp->crtc.state;
> +			if (splice_state->active) {
> +				drm_dbg_kms(vop2->drm, "vp%d is already in use\n",
> +					    splice_vp->id);
> +				return -EBUSY;
> +			}
> +		}
> +
> +		/*
> +		 * The right half uses its own set of OVL layers, both video
> +		 * ports need enough layers for the planes of the left CRTC.
> +		 */
> +		if (nplanes > min(vp->nlayers, splice_vp->nlayers))
> +			return -EINVAL;
> +
> +		vcstate->splice_mode = true;
> +	}
> +
>  	return 0;
>  }

[Severity: High]
There appear to be several synchronization and atomic state issues with how
splice_vp is checked and reserved here:

1. vop2_crtc_atomic_check() runs on every atomic check involving crtc,
   including plane updates where splice_vp->crtc.mutex is not held. When
   splice_vp->crtc is not part of state, falling back to reading
   splice_vp->crtc.state directly without holding splice_vp->crtc.mutex races
   with concurrent commits on splice_vp->crtc swapping and freeing its old
   crtc_state in drm_atomic_helper_swap_state() -> vop2_crtc_destroy_state().
   Not adding splice_vp->crtc to state also means drm_atomic_helper_setup_commit()
   will not serialize against in-flight non-blocking commits on splice_vp.

2. vp->splice_mode_right is mutated during commit_tail in
   vop2_crtc_atomic_enable() and vop2_crtc_atomic_disable() rather than being
   derived from the atomic state being checked. If userspace submits a single
   atomic commit (or DRM_MODE_ATOMIC_TEST_ONLY check) that switches VP0 from
   an 8K mode to a 4K mode while enabling VP1, vop2_crtc_atomic_check() for
   VP1 will still see vp->splice_mode_right == true and reject the commit with
   -EBUSY (and vop2_crtc_mode_valid() will similarly reject all modes on VP1
   with MODE_BAD while VP0 is in splice mode). Conversely, after a
   non-blocking commit enabling splice mode on VP0 swaps state and drops
   locks, vp1->splice_mode_right remains false until commit_work runs
   vop2_crtc_atomic_enable(), allowing a racing commit on VP1 to pass
   vop2_crtc_atomic_check().

3. Checking crtc_state->active and splice_state->active instead of ->enable
   allows VP0 to steal VP1 while VP1 is in DPMS OFF (enable == true,
   active == false), and because vop2_crtc_atomic_disable() clears
   splice_vp->splice_mode_right on DPMS OFF, VP1 can be enabled while VP0 is
   in DPMS OFF, causing VP0's subsequent DPMS ON to fail with -EBUSY.

Could vop2_crtc_atomic_check() instead acquire the partner CRTC state via
drm_atomic_get_crtc_state() when modesetting and check enable rather than
active and commit-time splice_mode_right?

> diff --git a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
> index c44648e1bc0d6..8d97dcd10ec10 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c

[ ... ]

> @@ -1387,6 +1394,18 @@ static struct vop2_win *vop2_find_win_by_phys_id(struct vop2 *vop2, uint8_t phys
>  	return NULL;
>  }
>  
> +/*
> + * The right window in splice mode shares the plane state of the left one,
> + * only its registers and position on the screen are different.
> + */
> +static struct drm_plane_state *vop2_win_get_pstate(struct vop2_win *win)
> +{
> +	if (win->left_win)
> +		return win->left_win->base.state;
> +
> +	return win->base.state;
> +}

[ ... ]

> @@ -1894,7 +1913,7 @@ static void vop2_setup_cluster_alpha(struct vop2 *vop2, struct vop2_win *main_wi
>  {
>  	struct vop2_alpha_config alpha_config;
>  	struct vop2_alpha alpha;
> -	struct drm_plane_state *bottom_win_pstate;
> +	struct drm_plane_state *pstate = vop2_win_get_pstate(main_win);
>  	bool src_pixel_alpha_en = false;
>  	u16 src_glb_alpha_val, dst_glb_alpha_val;
>  	u32 src_color_ctrl_reg, dst_color_ctrl_reg, src_alpha_ctrl_reg, dst_alpha_ctrl_reg;
> @@ -1903,11 +1922,10 @@ static void vop2_setup_cluster_alpha(struct vop2 *vop2, struct vop2_win *main_wi
>  	bool swap = false;
>  
>  	/* At one win mode, win0 is dst/bottom win, and win1 is a all zero src/top win */
> -	bottom_win_pstate = main_win->base.state;
>  	src_glb_alpha_val = 0;
> -	dst_glb_alpha_val = main_win->base.state->alpha;
> +	dst_glb_alpha_val = pstate->alpha;
>  
> -	if (!bottom_win_pstate->fb)
> +	if (!pstate->fb)
>  		return;

[ ... ]

> @@ -2003,9 +2025,12 @@ static void vop2_setup_alpha(struct vop2_video_port *vp)
>  		dst_alpha_ctrl_reg = RK3576_OVL_MIX0_DST_ALPHA_CTRL(vp->id);
>  	}
>  
> -	drm_atomic_crtc_for_each_plane(plane, &vp->crtc) {
> -		struct vop2_win *win = to_vop2_win(plane);
> -		int zpos = plane->state->normalized_zpos;
> +	for_each_set_bit(phys_id, &win_mask, BITS_PER_TYPE(u32)) {
> +		int zpos;
> +
> +		win = vop2_find_win_by_phys_id(vop2, phys_id);
> +		pstate = vop2_win_get_pstate(win);
> +		zpos = pstate->normalized_zpos;
>  
>  		/*
>  		 * Need to configure alpha from second layer.
> @@ -2013,13 +2038,12 @@ static void vop2_setup_alpha(struct vop2_video_port *vp)
>  		if (zpos == 0)
>  			continue;
>  
> -		if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PREMULTI)
> +		if (pstate->pixel_blend_mode == DRM_MODE_BLEND_PREMULTI)
>  			premulti_en = 1;
>  		else
>  			premulti_en = 0;
>  
> -		plane = &win->base;
> -		fb = plane->state->fb;
> +		fb = pstate->fb;
>  
>  		pixel_alpha_en = fb->format->has_alpha;

[Severity: Critical]
In vop2_win_init(), win->splice_win->left_win = win is set unconditionally at
probe time for Cluster1, Cluster3, Esmart1, and Esmart3, and is never cleared.
However, vop2_win_get_pstate(win) unconditionally returns
win->left_win->base.state whenever win->left_win is non-NULL, even when the
video port is operating in normal non-splice mode (!vp->splice_mode_right) and
win is being used as an independent DRM plane.

When Cluster1, Cluster3, Esmart1, or Esmart3 is enabled on any CRTC in normal
non-splice mode while its corresponding left_win (Cluster0, Cluster2, Esmart0,
or Esmart2) is disabled (so left_win->base.state->fb is NULL):

1. vop2_setup_cluster_alpha() checks if (!pstate->fb) on left_win's state and
   returns early without configuring cluster alpha.

2. rk3568_vop2_setup_layer_mixer() computes layer_id from left_win's
   normalized_zpos instead of win->base.state->normalized_zpos.

3. Because vop2_plane_init() initializes default zpos to win->win_id (which is
   non-zero for Cluster2, Esmart0, and Esmart2) and
   __drm_atomic_helper_plane_duplicate_state() preserves normalized_zpos across
   plane disables, vop2_setup_alpha() can see zpos != 0 on the disabled
   left_win's state and dereference fb = pstate->fb (NULL) at
   fb->format->has_alpha.

4. Because win->left_win->base is a separate drm_plane that may be bound to
   another CRTC, the committing CRTC does not hold win->left_win->base.mutex,
   so a concurrent atomic commit on another CRTC can swap and free
   win->left_win->base.state while vop2_setup_cluster_alpha(),
   rk3568_vop2_setup_layer_mixer(), and vop2_setup_alpha() are dereferencing
   it.

Should vop2_win_get_pstate() only resolve through win->left_win when the video
port is actually acting as the right half in splice mode
(vp->splice_mode_right)?

[ ... ]

> @@ -2301,7 +2337,12 @@ static void rk3568_vop2_setup_layer_mixer(struct vop2_video_port *vp)
>  	if (port_sel != vop2->old_port_sel) {
>  		vop2->old_port_sel = port_sel;
>  		vop2_writel(vop2, RK3568_OVL_PORT_SEL, port_sel);
> -		vop2_cfg_done(vp);
> +		/*
> +		 * The port mux can only take effect together with the config
> +		 * done of the video port owning the CRTC state. In splice mode
> +		 * that also latches the registers of the right video port.
> +		 */
> +		vop2_cfg_done(vp->splice_mode_right ? vp->left_vp : vp);
>  		rk3568_vop2_wait_for_port_mux_done(vop2);
>  	}
>  
> @@ -2354,33 +2395,63 @@ static void rk3568_vop2_setup_dly_for_windows(struct vop2_video_port *vp)

[ ... ]

> +static void rk3568_vop2_setup_overlay(struct vop2_video_port *vp)
> +{
> +	struct vop2 *vop2 = vp->vop2;
> +	struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(vp->crtc.state);
> +	const struct vop2_video_port_data *vp_data = &vop2->data->vp[vp->id];
> +	struct vop2_video_port *splice_vp = NULL;
> +	struct drm_plane *plane;
> +
> +	vp->win_mask = 0;
> +	if (vcstate->splice_mode) {
> +		splice_vp = &vop2->vps[vp_data->splice_vp_id];
> +		splice_vp->win_mask = 0;
> +	}
> +
> +	/*
> +	 * The right video port shares the plane state of the left one, only
> +	 * the windows it uses and their position on the screen differ.
> +	 */
> +	drm_atomic_crtc_for_each_plane(plane, &vp->crtc) {
> +		struct vop2_win *win = to_vop2_win(plane);
> +
> +		vp->win_mask |= BIT(win->data->phys_id);
> +		if (splice_vp && win->splice_win)
> +			splice_vp->win_mask |= BIT(win->splice_win->data->phys_id);
> +	}
> +
> +	rk3568_vop2_setup_overlay_for_vp(vp);
> +	if (splice_vp)
> +		rk3568_vop2_setup_overlay_for_vp(splice_vp);
> +}

[Severity: High]
In rk3568_vop2_setup_overlay(), splice mode calls
rk3568_vop2_setup_overlay_for_vp(vp) followed immediately by
rk3568_vop2_setup_overlay_for_vp(splice_vp), which invokes
rk3568_vop2_setup_layer_mixer() twice back-to-back before
vop2_crtc_atomic_flush() has latched the first update:

    if (layer_sel != vop2->old_layer_sel && atv_layer_sel != vop2->old_layer_sel) {
        cfg_done = vop2_readl(vop2, RK3568_REG_CFG_DONE);
        cfg_done &= (BIT(vop2->data->nr_vps) - 1);
        cfg_done &= ~BIT(vp->id);
        /*
         * Changes of other VPs' overlays have not taken effect
         */
        if (cfg_done)
            rk3568_vop2_wait_for_layer_cfg_done(vop2);
    }

    if (layer_sel != vop2->old_layer_sel || port_sel != vop2->old_port_sel)
        ovl_ctrl |= FIELD_PREP(RK3568_OVL_CTRL__LAYERSEL_REGDONE_SEL, vp->id);
    vop2_writel(vop2, RK3568_OVL_CTRL, ovl_ctrl);

1. In the second call (for splice_vp), RK3568_OVL_CTRL__LAYERSEL_REGDONE_SEL
   is set to vp->id (the right VP, which is in standby with no running dclk or
   VSYNC) instead of vp->left_vp->id, overwriting the left VP ID programmed by
   the first call. Won't that prevent the active left VP's VSYNC from latching
   RK3568_OVL_PORT_SEL and RK3568_OVL_LAYER_SEL, causing
   rk3568_vop2_wait_for_port_mux_done() to time out?

2. Because the first call updates vop2->old_layer_sel = layer_sel before the
   register is latched at VSYNC, the immediate second call for splice_vp sees
   atv_layer_sel != vop2->old_layer_sel and layer_sel != vop2->old_layer_sel.
   Since cfg_done &= ~BIT(vp->id) only clears BIT(splice_vp->id) and leaves
   BIT(vp->left_vp->id) set (for example from vop2_crtc_atomic_enable()),
   won't the second call enter rk3568_vop2_wait_for_layer_cfg_done() and spin
   for 50ms waiting for the uncommitted layer_sel from the first call?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022071737.508-1-kernel@airkyi.com?part=3