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

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- 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