On Dienstag, 21. Dezember 2021 14:44:39 CET Nicolas Frattaroli wrote:
On Montag, 20. Dezember 2021 12:06:30 CET Sascha Hauer wrote:
From: Andy Yan andy.yan@rock-chips.com
The VOP2 unit is found on Rockchip SoCs beginning with rk3566/rk3568. It replaces the VOP unit found in the older Rockchip SoCs.
This driver has been derived from the downstream Rockchip Kernel and heavily modified:
- All nonstandard DRM properties have been removed
- dropped struct vop2_plane_state and pass around less data between functions
- Dropped all DRM_FORMAT_* not known on upstream
- rework register access to get rid of excessively used macros
- Drop all waiting for framesyncs
The driver is tested with HDMI and MIPI-DSI display on a RK3568-EVB board. Overlay support is tested with the modetest utility. AFBC support on the cluster windows is tested with weston-simple-dmabuf-egl on weston using the (yet to be upstreamed) panfrost driver support.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
Hi Sascha,
quick partial review of the code in-line.
For reference, I debugged locking issues with the kernel lock debug config options and assert_spin_locked in the reg write functions, as well as some manual deduction.
As a small follow-up, I've completely mapped out the calls to vop2_writel, vop2_readl, vop2_vp_write and vop2_win_write and coloured in whether they were called with the lock held or not.
The conclusion is startling: Most of the code absolutely does not care about the reg_lock.
Here's the graph as an SVG: https://overviewer.org/~pillow/up/6800427ef3/vop2_callgraph_modified.svg
vop2_isr here needs to be paid special attention, as it also acquires a different spinlock, and we want to avoid deadlocks.
Perhaps we should precisely define which lock must be held for what registers, such that the vop2_isr can write its interrupt related registers without acquiring the "big" reg_lock.
I'm also not entirely sure whether I should assume vop2_readl needs to be called with the lock held. This needs some investigating both in terms of whether the hardware presents a writel as an atomic write of a long, and whether the code assumes the state between readl calls is ever a consistent view.
Regards, Nicolas Frattaroli