From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH] drm/atomic-helper: reset vblank on crtc reset Date: Wed, 27 May 2020 12:52:47 +0200 Message-ID: <20200527105247.GA2979001@ulmo> References: <20200527094757.1414174-2-daniel.vetter@ffwll.ch> <20200527095332.1439425-1-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="BOKacYhQ+x31HxR3" Return-path: Content-Disposition: inline In-Reply-To: <20200527095332.1439425-1-daniel.vetter-/w4YWyX8dFk@public.gmane.org> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Daniel Vetter Cc: DRI Development , Intel Graphics Development , Tetsuo Handa , syzbot+0871b14ca2e2fb64f6e3-Pl5Pbv+GP7P466ipTTIvnc23WoclnBCfAL8bYrjMMd8@public.gmane.org, "James (Qian) Wang" , Liviu Dudau , Mihail Atanassov , Brian Starkey , Sam Ravnborg , Boris Brezillon , Nicolas Ferre , Alexandre Belloni , Ludovic Desroches , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie List-Id: linux-tegra@vger.kernel.org --BOKacYhQ+x31HxR3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, May 27, 2020 at 11:53:32AM +0200, Daniel Vetter wrote: > Only when vblanks are supported ofc. >=20 > Some drivers do this already, but most unfortunately missed it. This > opens up bugs after driver load, before the crtc is enabled for the > first time. syzbot spotted this when loading vkms as a secondary > output. Given how many drivers are buggy it's best to solve this once > and for all in shared helper code. >=20 > Aside from moving the few existing calls to drm_crtc_vblank_reset into > helpers (i915 doesn't use helpers, so keeps its own) I think the > regression risk is minimal: atomic helpers already rely on drivers > calling drm_crtc_vblank_on/off correctly in their hooks when they > support vblanks. And driver that's failing to handle vblanks after > this is missing those calls already, and vblanks could only work by > accident when enabling a CRTC for the first time right after boot. >=20 > Big thanks to Tetsuo for helping track down what's going wrong here. >=20 > There's only a few drivers which already had the necessary call and > needed some updating: > - komeda, atmel and tidss also needed to be changed to call > __drm_atomic_helper_crtc_reset() intead of open coding it > - tegra and msm even had it in the same place already, just code > motion, and malidp already uses __drm_atomic_helper_crtc_reset(). >=20 > Only call left is in i915, which doesn't use drm_mode_config_reset, > but has its own fastboot infrastructure. So that's the only case where > we actually want this in the driver still. >=20 > I've also reviewed all other drivers which set up vblank support with > drm_vblank_init. After the previous patch fixing mxsfb all atomic > drivers do call drm_crtc_vblank_on/off as they should, the remaining > drivers are either legacy kms or legacy dri1 drivers, so not affected > by this change to atomic helpers. >=20 > v2: Use the drm_dev_has_vblank() helper. >=20 > Link: https://syzkaller.appspot.com/bug?id=3D0ba17d70d062b2595e1f06123147= 4800f076c7cb > Reported-by: Tetsuo Handa > Reported-by: syzbot+0871b14ca2e2fb64f6e3-Pl5Pbv+GP7P466ipTTIvnc23WoclnBCfAL8bYrjMMd8@public.gmane.org > Cc: Tetsuo Handa > Cc: "James (Qian) Wang" > Cc: Liviu Dudau > Cc: Mihail Atanassov > Cc: Brian Starkey > Cc: Sam Ravnborg > Cc: Boris Brezillon > Cc: Nicolas Ferre > Cc: Alexandre Belloni > Cc: Ludovic Desroches > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: Jyri Sarha > Cc: Tomi Valkeinen > Cc: Rob Clark > Cc: Sean Paul > Cc: Brian Masney > Cc: Emil Velikov > Cc: zhengbin > Cc: Thomas Gleixner > Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++----- > drivers/gpu/drm/arm/malidp_drv.c | 1 - > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++----- > drivers/gpu/drm/drm_atomic_state_helper.c | 4 ++++ > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 2 -- > drivers/gpu/drm/tegra/dc.c | 1 - > drivers/gpu/drm/tidss/tidss_crtc.c | 3 +-- > drivers/gpu/drm/tidss/tidss_kms.c | 4 ---- > 8 files changed, 9 insertions(+), 20 deletions(-) Looks good, and nice cleanup: Acked-by: Thierry Reding --BOKacYhQ+x31HxR3 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAl7ORn0ACgkQ3SOs138+ s6H2lA/8DvMIozjVlrshtTC7Qr9YxJxR6RFOy17iNH5ARYLPLbhBq1yRfsiX4FT3 l8PvDxGsrxfsxh+AhRRtQC1bNIHhzrZTMyqR3ynrphuGm73Q5A2sOzkNp6XjvfGj AoWxb+y+kxg62luapDOUKCnJdILpJ5EagC7enrCKKZo8ZCGjFvjWUivFejUaDmy/ lUX8X0a1PgRkCBubwk/Fr0P0oGg+VM6+DoMguR/9yTC5TrQ1F4JmNOGKpsVivm+K 3NUalHyzQlzYkXhhQhU858C+COIRErJCweeBYTIGPZQglpjXASEONWw+giiizKpm FwB4yKDQqLuW1hGfp8FToRO6YWZZXfrZU2YC8F+xSFmF+Ppox0NrTnnivjwpwzul EakIXUbxe0gYRobtcIJFLsQfYvEFYtDm675moCPijf8VG2XzQWEXTVJRWCHg2u3V zu/l1yRIsLbK+3mPBLf+3kxP4pvFjYwzniod4BI2RpHF/b7vY61CZROradHPAkSG h934cP2DCjJDPOTDRXhBuhFE6Sq8/mfoYnqToky5+RfRw4tiQUgUPMUCm/h/SvLr OAiZclZ4eawslPWXpYfeYv16WupR9Pk+2DZPg//7R1g1c15+CdGEejiuoZCsUl5K wVu8p5fh0Hei9TbND3EDMqyt+4lcMxIsYC8Qb5fmBB9amz4PZVs= =fudx -----END PGP SIGNATURE----- --BOKacYhQ+x31HxR3--