[PATCH] drm/rockchip: vop2: fix suspend/resume

[PATCH] drm/rockchip: vop2: fix suspend/resume

[Sascha Hauer]

During a suspend/resume cycle the VO power domain will be disabled and
the VOP2 registers will reset to their default values. After that the
cached register values will be out of sync and the read/modify/write
operations we do on the window registers will result in bogus values
written. Fix this by re-initializing the register cache each time we
enable the VOP2. With this the VOP2 will show a picture after a
suspend/resume cycle whereas without this the screen stays dark.

Fixes: 604be85547ce4 ("drm/rockchip: Add VOP2 driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index ba3b817895091..d9daa686b014d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -215,6 +215,8 @@ struct vop2 {
 	struct vop2_win win[];
 };
 
+static const struct regmap_config vop2_regmap_config;
+
 static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
 {
 	return container_of(crtc, struct vop2_video_port, crtc);
@@ -839,6 +841,12 @@ static void vop2_enable(struct vop2 *vop2)
 		return;
 	}
 
+	ret = regmap_reinit_cache(vop2->map, &vop2_regmap_config);
+	if (ret) {
+		drm_err(vop2->drm, "failed to reinit cache: %d\n", ret);
+		return;
+	}
+
 	if (vop2->data->soc_id == 3566)
 		vop2_writel(vop2, RK3568_OTP_WIN_EN, 1);
 
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: vop2: fix suspend/resume

[Chris Morgan]

On Thu, Apr 13, 2023 at 04:43:47PM +0200, Sascha Hauer wrote:
> During a suspend/resume cycle the VO power domain will be disabled and
> the VOP2 registers will reset to their default values. After that the
> cached register values will be out of sync and the read/modify/write
> operations we do on the window registers will result in bogus values
> written. Fix this by re-initializing the register cache each time we
> enable the VOP2. With this the VOP2 will show a picture after a
> suspend/resume cycle whereas without this the screen stays dark.
> 
> Fixes: 604be85547ce4 ("drm/rockchip: Add VOP2 driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index ba3b817895091..d9daa686b014d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -215,6 +215,8 @@ struct vop2 {
>  	struct vop2_win win[];
>  };
>  
> +static const struct regmap_config vop2_regmap_config;
> +
>  static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
>  {
>  	return container_of(crtc, struct vop2_video_port, crtc);
> @@ -839,6 +841,12 @@ static void vop2_enable(struct vop2 *vop2)
>  		return;
>  	}
>  
> +	ret = regmap_reinit_cache(vop2->map, &vop2_regmap_config);
> +	if (ret) {
> +		drm_err(vop2->drm, "failed to reinit cache: %d\n", ret);
> +		return;
> +	}
> +
>  	if (vop2->data->soc_id == 3566)
>  		vop2_writel(vop2, RK3568_OTP_WIN_EN, 1);
>  
> -- 
> 2.39.2
> 

I confirmed this works on my Anbernic RG353P which uses the rk3566 SOC.
Before applying the patch I displayed a color pattern with modetest
before suspend and it appeared correctly. Then I suspended and resumed
the device, attempted to display the same color pattern, and only got
a single pixel on an otherwise blank display. After applying the patch
I performed the same test and the color pattern appeared correctly
both before and after suspend (and the display was no longer blank
after resume from suspend).

Tested-by: Chris Morgan <macromorgan@hotmail.com>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: vop2: fix suspend/resume

[Paul Kocialkowski]

[-- Attachment #1.1: Type: text/plain, Size: 3047 bytes --]

Hi,

On Thu 13 Apr 23, 10:27, Chris Morgan wrote:
> On Thu, Apr 13, 2023 at 04:43:47PM +0200, Sascha Hauer wrote:
> > During a suspend/resume cycle the VO power domain will be disabled and
> > the VOP2 registers will reset to their default values. After that the
> > cached register values will be out of sync and the read/modify/write
> > operations we do on the window registers will result in bogus values
> > written. Fix this by re-initializing the register cache each time we
> > enable the VOP2. With this the VOP2 will show a picture after a
> > suspend/resume cycle whereas without this the screen stays dark.

I was actually tracking the very same bug this week!

Thanks a lot for fixing this, it would certainly have taken me a while to
think about regmap cache maintenance. Good thinking :)

Your patch fixes the issue on my side but I have a suggestion below:

> > Fixes: 604be85547ce4 ("drm/rockchip: Add VOP2 driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > index ba3b817895091..d9daa686b014d 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > @@ -215,6 +215,8 @@ struct vop2 {
> >  	struct vop2_win win[];
> >  };
> >  
> > +static const struct regmap_config vop2_regmap_config;
> > +
> >  static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
> >  {
> >  	return container_of(crtc, struct vop2_video_port, crtc);
> > @@ -839,6 +841,12 @@ static void vop2_enable(struct vop2 *vop2)
> >  		return;
> >  	}
> >  
> > +	ret = regmap_reinit_cache(vop2->map, &vop2_regmap_config);
> > +	if (ret) {
> > +		drm_err(vop2->drm, "failed to reinit cache: %d\n", ret);
> > +		return;
> > +	}

It seems that regmap has regcache_mark_dirty() for this purpose, which is
perhaps more adapted than reinitializing cache (unless I'm missing something).
Note that I haven't tested it at this point.

Cheers,

Paul

> >  	if (vop2->data->soc_id == 3566)
> >  		vop2_writel(vop2, RK3568_OTP_WIN_EN, 1);
> >  
> > -- 
> > 2.39.2
> > 
> 
> I confirmed this works on my Anbernic RG353P which uses the rk3566 SOC.
> Before applying the patch I displayed a color pattern with modetest
> before suspend and it appeared correctly. Then I suspended and resumed
> the device, attempted to display the same color pattern, and only got
> a single pixel on an otherwise blank display. After applying the patch
> I performed the same test and the color pattern appeared correctly
> both before and after suspend (and the display was no longer blank
> after resume from suspend).
> 
> Tested-by: Chris Morgan <macromorgan@hotmail.com>

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: vop2: fix suspend/resume

[Sascha Hauer]

On Fri, Apr 14, 2023 at 04:20:10PM +0200, Paul Kocialkowski wrote:
> Hi,
> 
> On Thu 13 Apr 23, 10:27, Chris Morgan wrote:
> > On Thu, Apr 13, 2023 at 04:43:47PM +0200, Sascha Hauer wrote:
> > > During a suspend/resume cycle the VO power domain will be disabled and
> > > the VOP2 registers will reset to their default values. After that the
> > > cached register values will be out of sync and the read/modify/write
> > > operations we do on the window registers will result in bogus values
> > > written. Fix this by re-initializing the register cache each time we
> > > enable the VOP2. With this the VOP2 will show a picture after a
> > > suspend/resume cycle whereas without this the screen stays dark.
> 
> I was actually tracking the very same bug this week!
> 
> Thanks a lot for fixing this, it would certainly have taken me a while to
> think about regmap cache maintenance. Good thinking :)
> 
> Your patch fixes the issue on my side but I have a suggestion below:
> 
> > > Fixes: 604be85547ce4 ("drm/rockchip: Add VOP2 driver")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > index ba3b817895091..d9daa686b014d 100644
> > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > @@ -215,6 +215,8 @@ struct vop2 {
> > >  	struct vop2_win win[];
> > >  };
> > >  
> > > +static const struct regmap_config vop2_regmap_config;
> > > +
> > >  static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
> > >  {
> > >  	return container_of(crtc, struct vop2_video_port, crtc);
> > > @@ -839,6 +841,12 @@ static void vop2_enable(struct vop2 *vop2)
> > >  		return;
> > >  	}
> > >  
> > > +	ret = regmap_reinit_cache(vop2->map, &vop2_regmap_config);
> > > +	if (ret) {
> > > +		drm_err(vop2->drm, "failed to reinit cache: %d\n", ret);
> > > +		return;
> > > +	}
> 
> It seems that regmap has regcache_mark_dirty() for this purpose, which is
> perhaps more adapted than reinitializing cache (unless I'm missing something).
> Note that I haven't tested it at this point.

I wasn't aware of this function. regcache_mark_dirty() alone is not
enough, we need regcache_sync() as well. This looks better, I just sent
a v2.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: vop2: fix suspend/resume

[Heiko Stuebner]

On Thu, 13 Apr 2023 16:43:47 +0200, Sascha Hauer wrote:
> During a suspend/resume cycle the VO power domain will be disabled and
> the VOP2 registers will reset to their default values. After that the
> cached register values will be out of sync and the read/modify/write
> operations we do on the window registers will result in bogus values
> written. Fix this by re-initializing the register cache each time we
> enable the VOP2. With this the VOP2 will show a picture after a
> suspend/resume cycle whereas without this the screen stays dark.
> 
> [...]

Applied, thanks!

[1/1] drm/rockchip: vop2: fix suspend/resume
      commit: afa965a45e01e541cdbe5c8018226eff117610f0

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH] drm/rockchip: vop2: fix suspend/resume

[Sascha Hauer]

During a suspend/resume cycle the VO power domain will be disabled and
the VOP2 registers will reset to their default values. After that the
cached register values will be out of sync and the read/modify/write
operations we do on the window registers will result in bogus values
written. Fix this by marking the regcache as dirty each time we disable
the VOP2 and call regcache_sync() each time we enable it again. With
this the VOP2 will show a picture after a suspend/resume cycle whereas
without this the screen stays dark.

Fixes: 604be85547ce4 ("drm/rockchip: Add VOP2 driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

Changes since v1:
- Use regcache_mark_dirty()/regcache_sync() instead of regmap_reinit_cache()

 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index ba3b817895091..293c228a83f90 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -839,6 +839,8 @@ static void vop2_enable(struct vop2 *vop2)
 		return;
 	}
 
+	regcache_sync(vop2->map);
+
 	if (vop2->data->soc_id == 3566)
 		vop2_writel(vop2, RK3568_OTP_WIN_EN, 1);
 
@@ -867,6 +869,8 @@ static void vop2_disable(struct vop2 *vop2)
 
 	pm_runtime_put_sync(vop2->dev);
 
+	regcache_mark_dirty(vop2->map);
+
 	clk_disable_unprepare(vop2->aclk);
 	clk_disable_unprepare(vop2->hclk);
 }
-- 
2.39.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: vop2: fix suspend/resume

[Heiko Stübner]

Hi Sascha,

Am Montag, 17. April 2023, 11:42:15 CEST schrieb Sascha Hauer:
> During a suspend/resume cycle the VO power domain will be disabled and
> the VOP2 registers will reset to their default values. After that the
> cached register values will be out of sync and the read/modify/write
> operations we do on the window registers will result in bogus values
> written. Fix this by marking the regcache as dirty each time we disable
> the VOP2 and call regcache_sync() each time we enable it again. With
> this the VOP2 will show a picture after a suspend/resume cycle whereas
> without this the screen stays dark.
> 
> Fixes: 604be85547ce4 ("drm/rockchip: Add VOP2 driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

somehow we overlapped with this v2 and me applying the original one [0]
to drm-misc. With drm-misc being a shared tree there is also no way back.

So if this v2 is better suited, could do a follow-up patch instead - on
top of your original one?

Thanks
Heiko



[0] https://cgit.freedesktop.org/drm/drm-misc/commit/?h=drm-misc-fixes&id=afa965a45e01e541cdbe5c8018226eff117610f0

> ---
> 
> Changes since v1:
> - Use regcache_mark_dirty()/regcache_sync() instead of regmap_reinit_cache()
> 
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index ba3b817895091..293c228a83f90 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -839,6 +839,8 @@ static void vop2_enable(struct vop2 *vop2)
>  		return;
>  	}
>  
> +	regcache_sync(vop2->map);
> +
>  	if (vop2->data->soc_id == 3566)
>  		vop2_writel(vop2, RK3568_OTP_WIN_EN, 1);
>  
> @@ -867,6 +869,8 @@ static void vop2_disable(struct vop2 *vop2)
>  
>  	pm_runtime_put_sync(vop2->dev);
>  
> +	regcache_mark_dirty(vop2->map);
> +
>  	clk_disable_unprepare(vop2->aclk);
>  	clk_disable_unprepare(vop2->hclk);
>  }
> 





_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: vop2: fix suspend/resume

[Sascha Hauer]

On Mon, Apr 17, 2023 at 12:46:05PM +0200, Heiko Stübner wrote:
> Hi Sascha,
> 
> Am Montag, 17. April 2023, 11:42:15 CEST schrieb Sascha Hauer:
> > During a suspend/resume cycle the VO power domain will be disabled and
> > the VOP2 registers will reset to their default values. After that the
> > cached register values will be out of sync and the read/modify/write
> > operations we do on the window registers will result in bogus values
> > written. Fix this by marking the regcache as dirty each time we disable
> > the VOP2 and call regcache_sync() each time we enable it again. With
> > this the VOP2 will show a picture after a suspend/resume cycle whereas
> > without this the screen stays dark.
> > 
> > Fixes: 604be85547ce4 ("drm/rockchip: Add VOP2 driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> somehow we overlapped with this v2 and me applying the original one [0]
> to drm-misc. With drm-misc being a shared tree there is also no way back.
> 
> So if this v2 is better suited, could do a follow-up patch instead - on
> top of your original one?

Alright, just did that. You should find it in your inbox.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip