Re: [PATCH 22/22] drm: rockchip: Add VOP2 driver

From: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
To: dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, devicetree@vger.kernel.org,
	kernel@pengutronix.de, "Andy Yan" <andy.yan@rock-chips.com>,
	"Benjamin Gaignard" <benjamin.gaignard@collabora.com>,
	"Michael Riesch" <michael.riesch@wolfvision.net>,
	"Sandy Huang" <hjc@rock-chips.com>,
	"Heiko Stübner" <heiko@sntech.de>,
	"Peter Geis" <pgwipeout@gmail.com>,
	"Sascha Hauer" <s.hauer@pengutronix.de>,
	"Sascha Hauer" <s.hauer@pengutronix.de>,
	"Nicolas Frattaroli" <frattaroli.nicolas@gmail.com>
Subject: Re: [PATCH 22/22] drm: rockchip: Add VOP2 driver
Date: Wed, 22 Dec 2021 18:07:02 +0100	[thread overview]
Message-ID: <26571551.24qHfsk75X@archbook> (raw)
In-Reply-To: <1761858.GBYTvM79DV@archbook>

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