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@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> > >---
> > >
> > > 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);
