linux-kernel - Re: [PATCH v2 1/3] vop2: Add clock resets support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4605629.LvFx2qVVIh@trenzalore>
Date: Fri, 08 Nov 2024 11:39:57 -0500
From: Detlev Casanova <detlev.casanova@...labora.com>
To: Andy Yan <andyshrk@....com>
Cc: linux-kernel@...r.kernel.org, Sandy Huang <hjc@...k-chips.com>,
 Heiko Stübner <heiko@...ech.de>,
 Andy Yan <andy.yan@...k-chips.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Heiko Stuebner <heiko.stuebner@...rry.de>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Dragan Simic <dsimic@...jaro.org>, Chris Morgan <macromorgan@...mail.com>,
 Diederik de Haas <didi.debian@...ow.org>,
 Boris Brezillon <boris.brezillon@...labora.com>,
 dri-devel@...ts.freedesktop.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, devicetree@...r.kernel.org,
 kernel@...labora.com
Subject: Re: [PATCH v2 1/3] vop2: Add clock resets support

On Thursday, 23 May 2024 23:09:26 EST Andy Yan wrote:
> Hi Detlev，
> 
> At 2024-05-23 02:57:48, "Detlev Casanova" <detlev.casanova@...labora.com> 
wrote:
> >At the end of initialization, each VP clock needs to be reset before
> >they can be used.
> >
> >Failing to do so can put the VOP in an undefined state where the
> >generated HDMI signal is either lost or not matching the selected mode.
> 
> Would you please provide a detailed description of your test case?

The test case was to switch modes (using modetest) until the HDMI signal was 
lost on the TV side. It was also possible to detect the issue by tracking the 
HDMI TX Controller_VIDEO_MONITOR_STATUS[1-6] registers, especially at address 
0x890, where the register would take the value `0x0000018c`.

After adding these resets, the issue cannot be reproduced. I can share a 
script that reproduced this in the past (but this is an old patchset now, so 
things could have changed)

> >Signed-off-by: Detlev Casanova <detlev.casanova@...labora.com>
> >---
> >
> > drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 30 ++++++++++++++++++++
> > 1 file changed, 30 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c index
> >fdd768bbd487c..e81a67161d29a 100644
> >--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >@@ -17,6 +17,7 @@
> >
> > #include <linux/platform_device.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/regmap.h>
> >
> >+#include <linux/reset.h>
> >
> > #include <linux/swab.h>
> > 
> > #include <drm/drm.h>
> >
> >@@ -157,6 +158,7 @@ struct vop2_win {
> >
> > struct vop2_video_port {
> > 
> > 	struct drm_crtc crtc;
> > 	struct vop2 *vop2;
> >
> >+	struct reset_control *dclk_rst;
> >
> > 	struct clk *dclk;
> > 	unsigned int id;
> > 	const struct vop2_video_port_data *data;
> >
> >@@ -1915,6 +1917,26 @@ static int us_to_vertical_line(struct
> >drm_display_mode *mode, int us)>
> > 	return us * mode->clock / mode->htotal / 1000;
> > 
> > }
> >
> >+static int vop2_clk_reset(struct vop2_video_port *vp)
> >+{
> >+	struct reset_control *rstc = vp->dclk_rst;
> >+	struct vop2 *vop2 = vp->vop2;
> >+	int ret;
> >+
> >+	if (!rstc)
> >+		return 0;
> 
> In fact, this check is not necessary here.  The following reset control api
> will check for NULL pointer

Agreed, I'll do a rebased v3 and remove the check.

> >+
> >+	ret = reset_control_assert(rstc);
> >+	if (ret < 0)
> >+		drm_warn(vop2->drm, "failed to assert reset\n");
> >+	udelay(10);
> >+	ret = reset_control_deassert(rstc);
> >+	if (ret < 0)
> >+		drm_warn(vop2->drm, "failed to deassert reset\n");
> >+
> >+	return ret;
> >+}
> >+
> >
> > static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> > 
> > 				    struct drm_atomic_state 
*state)
> > 
> > {
> >
> >@@ -2055,6 +2077,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc
> >*crtc,>
> > 	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
> >
> >+	vop2_clk_reset(vp);
> >+
> >
> > 	drm_crtc_vblank_on(crtc);
> > 	
> > 	vop2_unlock(vop2);
> >
> >@@ -2706,6 +2730,12 @@ static int vop2_create_crtcs(struct vop2 *vop2)
> >
> > 		vp->data = vp_data;
> > 		
> > 		snprintf(dclk_name, sizeof(dclk_name), "dclk_vp%d", vp-
>id);
> >
> >+		vp->dclk_rst = devm_reset_control_get_optional(vop2-
>dev, dclk_name);
> >+		if (IS_ERR(vp->dclk_rst)) {
> >+		        drm_err(vop2->drm, "failed to get %s reset\n", 
dclk_name);
> >+		        return PTR_ERR(vp->dclk_rst);
> >+		}
> >+
> >
> > 		vp->dclk = devm_clk_get(vop2->dev, dclk_name);
> > 		if (IS_ERR(vp->dclk)) {
> > 		
> > 			drm_err(vop2->drm, "failed to get %s\n", 
dclk_name);