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

[Detlev Casanova <detlev.casanova@collabora.com>]

On Friday, 8 November 2024 11:39:57 EST Detlev Casanova wrote:
> 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@collabora.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@collabora.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.

Actually, re-thinking about it, the check is done to avoid the udelay(10); if 
there is no resets configured, so I'd rather keep it that way.

> > >+
> > >+	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);