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

From: Sascha Hauer <s.hauer@pengutronix.de>
To: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Cc: "Chris Morgan" <macroalpha82@gmail.com>,
	"Köry Maincent" <kory.maincent@bootlin.com>,
	"Sandy Huang" <hjc@rock-chips.com>,
	dri-devel@lists.freedesktop.org,
	linux-rockchip@lists.infradead.org,
	"Michael Riesch" <michael.riesch@wolfvision.net>,
	kernel@pengutronix.de, stable@vger.kernel.org
Subject: Re: [PATCH] drm/rockchip: vop2: fix suspend/resume
Date: Mon, 17 Apr 2023 11:45:16 +0200	[thread overview]
Message-ID: <20230417094516.GM15436@pengutronix.de> (raw)
In-Reply-To: <ZDlhGv0seSoxFlJ5@aptenodytes>

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