Re: [PATCH 4/8] 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 3 potential
issues to consider:
- [Medium] drm/rockchip: vop2: plane commit order skews
  vop2_win_get_axi_bus()
- [Medium] drm/rockchip: vop2: vop2_plane_atomic_update() omits UV plane
  AXI load
- [Medium] drm/rockchip: vop2: premature AXI bus release in
  vop2_win_put_axi_bus()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] drm/rockchip: vop2: wrong GEM object for UV plane in
  vop2_plane_atomic_update() [1]
--

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

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

This commit dynamically assigns newly enabled VOP2 windows on RK3588 to the
least loaded of the two AXI read ports while avoiding AXI read ID collisions
with other active windows.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 629ea7f112502..9aeb90c6aa4b1 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -1231,6 +1231,98 @@ static int vop2_plane_atomic_check(struct drm_plane *plane,

[ ... ]

> +static u8 vop2_win_get_axi_bus(struct vop2_win *win, u64 load)
> +{
> +	struct vop2 *vop2 = win->vop2;
> +	u64 bus_load[VOP2_SYS_AXI_BUS_NUM] = { };
> +	bool clash[VOP2_SYS_AXI_BUS_NUM] = { };
> +	u8 def = win->data->axi_bus_id;
> +	unsigned long flags;
> +	u8 bus = def;
> +	int i;
> +
> +	if (!(vop2->data->feature & VOP2_FEATURE_AXI_BALANCE))
> +		return def;
> +
> +	spin_lock_irqsave(&vop2->axi_lock, flags);
> +
> +	if (win->axi_active) {
> +		win->axi_load = load;
> +		bus = win->axi_bus;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < vop2->registered_num_wins; i++) {
> +		struct vop2_win *w = &vop2->win[i];
> +
> +		if (w == win || !w->axi_active)
> +			continue;
> +
> +		bus_load[w->axi_bus] += w->axi_load;
> +		if (vop2_win_axi_ids_clash(win, w))
> +			clash[w->axi_bus] = true;
> +	}
> +
> +	if (!clash[def ^ 1] &&
> +	    (clash[def] || max(bus_load[def ^ 1] + load, bus_load[def]) <
> +			   max(bus_load[def] + load, bus_load[def ^ 1])))
> +		bus = def ^ 1;
> +
> +	WARN_ON_ONCE(clash[bus]);
> +
> +	win->axi_bus = bus;
> +	win->axi_load = load;
> +	win->axi_active = true;
> +out:
> +	spin_unlock_irqrestore(&vop2->axi_lock, flags);
> +
> +	return bus;
> +}

[Severity: Medium]
Does vop2_win_get_axi_bus() see stale axi_active and axi_load state for
higher-indexed planes changed in the same atomic commit?

drm_atomic_helper_commit_planes() runs a single loop in ascending plane
index order, calling atomic_disable() or atomic_update() on each plane.
In rk3588_vop_win_data, all windows that default to AXI0 (Cluster0/1,
Esmart0/1) have lower indices than their AXI1 mirrors (Cluster2/3,
Esmart2/3).  When a commit enables Esmart0 while disabling Esmart2 (or
another higher-indexed plane on AXI1), vop2_win_get_axi_bus() runs for
Esmart0 before vop2_plane_atomic_disable() clears Esmart2's axi_active.
It therefore sees Esmart2's old load and read-ID clash on AXI1 and pins
Esmart0 to AXI0 alongside Cluster0 for as long as Esmart0 remains enabled,
causing the POST_BUF_EMPTY underflows this patch aims to avoid.

> +
> +static void vop2_win_put_axi_bus(struct vop2_win *win)
> +{
> +	struct vop2 *vop2 = win->vop2;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vop2->axi_lock, flags);
> +	win->axi_active = false;
> +	win->axi_load = 0;
> +	spin_unlock_irqrestore(&vop2->axi_lock, flags);
> +}
> +
>  static void vop2_plane_atomic_disable(struct drm_plane *plane,
>  				      struct drm_atomic_commit *state)
>  {
> @@ -1241,6 +1333,8 @@ static void vop2_plane_atomic_disable(struct drm_plane *plane,
>  
>  	drm_dbg(vop2->drm, "%s disable\n", win->data->name);
>  
> +	vop2_win_put_axi_bus(win);
> +
>  	if (state)
>  		old_pstate = drm_atomic_get_old_plane_state(state, plane);

[Severity: Medium]
Can clearing win->axi_active in vop2_plane_atomic_disable() cause an AXI
read ID collision before the disable is latched at VSYNC?

VOP2_WIN_ENABLE is a shadow register latched at the video port's next
VSYNC, so the disabled window keeps fetching over win->axi_bus until then.
If Esmart0 is on AXI1 on VP0 and is disabled while its mirror Esmart2
(which shares read IDs 0x0a and 0x0b) is enabled on VP1,
vop2_win_get_axi_bus() sees Esmart0->axi_active == false and assigns
Esmart2 to AXI1.  If VP1's VSYNC fires before VP0's VSYNC, both windows
fetch on AXI1 with identical read IDs until VP0 latches the disable.

> @@ -1416,7 +1510,9 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
>  		afbc_en ? "AFBC" : "", &yrgb_mst);
>  
>  	if (vop2->version > VOP_VERSION_RK3568) {
> -		vop2_win_write(win, VOP2_WIN_AXI_BUS_ID, win->data->axi_bus_id);
> +		u64 load = (u64)src_w * src_h * bpp / 8 * drm_mode_vrefresh(adjusted_mode);
> +
> +		vop2_win_write(win, VOP2_WIN_AXI_BUS_ID, vop2_win_get_axi_bus(win, load));
>  		vop2_win_write(win, VOP2_WIN_AXI_YRGB_R_ID, win->data->axi_yrgb_r_id);
>  		vop2_win_write(win, VOP2_WIN_AXI_UV_R_ID, win->data->axi_uv_r_id);
>  	}

[Severity: Medium]
Does computing load from bpp omit the UV plane bandwidth for multi-planar
YUV formats?

bpp comes from vop2_get_bpp(fb->format), which falls back to
drm_format_info_bpp(format, 0) and returns only plane 0's bits per pixel:

drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:vop2_get_bpp() {
	switch (format->format) {
	case DRM_FORMAT_YUV420_8BIT:
		return 12;
	case DRM_FORMAT_YUV420_10BIT:
		return 15;
	case DRM_FORMAT_VUY101010:
		return 30;
	default:
		return drm_format_info_bpp(format, 0);
	}
}

For semi-planar formats such as NV12, NV16 or NV24, plane 1 is fetched
through the same AXI port via VOP2_WIN_AXI_UV_R_ID, but bpp only counts
the Y plane (8 or 10 bpp instead of 12 to 30 bpp), underestimating
win->axi_load by 33% to 67%.

[Severity: High]
This problem wasn't introduced by this patch, but earlier in
vop2_plane_atomic_update() the UV plane DMA address is computed from
fb->obj[0] instead of fb->obj[1] [1]:

drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:vop2_plane_atomic_update() {
	...
	rk_obj = to_rockchip_obj(fb->obj[0]);

	yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0];
	if (fb->format->is_yuv) {
		...
		rk_obj = to_rockchip_obj(fb->obj[0]);
		uv_mst = rk_obj->dma_addr + offset + fb->offsets[1];
	}
}

When a multi-planar YUV framebuffer has separate GEM objects for the Y and
UV planes, VOP2_WIN_UV_MST is programmed with plane 0's DMA base address
plus plane 1's offset, causing corrupted scanout or out-of-bounds DMA
reads past fb->obj[0].

[1] https://sashiko.dev/#/bug/linux-46433ab8-0e41-4994-af45-5dfb41ba7894

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-vop2-fixes-v1-0-d336e9dd90d3@gmail.com?part=4