From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomi Valkeinen Subject: Re: [PATCH 01/42] drm/omap: fix suspend/resume handling Date: Mon, 7 Mar 2016 10:04:34 +0200 Message-ID: <56DD3612.8050207@ti.com> References: <1456161048-21240-1-git-send-email-tomi.valkeinen@ti.com> <1456161048-21240-2-git-send-email-tomi.valkeinen@ti.com> <2736448.dXU553MJfV@avalon> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0688546813==" Return-path: Received: from bear.ext.ti.com (bear.ext.ti.com [192.94.94.41]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2F2FE6E05A for ; Mon, 7 Mar 2016 08:04:41 +0000 (UTC) In-Reply-To: <2736448.dXU553MJfV@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0688546813== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xJaBB8gHC60l3kQrXLfbK8whgafG6Bv1v" --xJaBB8gHC60l3kQrXLfbK8whgafG6Bv1v Content-Type: multipart/mixed; boundary="No553Jri10qpqKJDi1PA0TtmmTWUe2EII" From: Tomi Valkeinen To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org, Rob Clark Message-ID: <56DD3612.8050207@ti.com> Subject: Re: [PATCH 01/42] drm/omap: fix suspend/resume handling References: <1456161048-21240-1-git-send-email-tomi.valkeinen@ti.com> <1456161048-21240-2-git-send-email-tomi.valkeinen@ti.com> <2736448.dXU553MJfV@avalon> In-Reply-To: <2736448.dXU553MJfV@avalon> --No553Jri10qpqKJDi1PA0TtmmTWUe2EII Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/03/16 09:52, Laurent Pinchart wrote: > Hi Tomi, >=20 > Thank you for the patch. >=20 > On Monday 22 February 2016 19:10:07 Tomi Valkeinen wrote: >> For legacy reasons omapdss handles system suspend/resume via PM notifi= er >> callback, where the driver disables/resumes all the outputs. >> >> This doesn't work well with omapdrm. What happens on suspend is that t= he >> omapdss disables the displays while omapdrm is still happily continuin= g >> its work, possibly waiting for an vsync irq, which will never come if >> the display output is disabled, leading to timeouts and errors sent to= >> userspace. >> >> This patch moves the suspend/resume handling to omapdrm, and the >> suspend/resume is now done safely inside modeset lock. >> >> Signed-off-by: Tomi Valkeinen >> --- >> drivers/gpu/drm/omapdrm/dss/core.c | 29 ----------------------- >> drivers/gpu/drm/omapdrm/dss/display.c | 36 --------------------------= -- >> drivers/gpu/drm/omapdrm/dss/dss.h | 2 -- >> drivers/gpu/drm/omapdrm/omap_drv.c | 44 ++++++++++++++++++++++++++= ++++++ >> 4 files changed, 44 insertions(+), 67 deletions(-) >> >> diff --git a/drivers/gpu/drm/omapdrm/dss/core.c >> b/drivers/gpu/drm/omapdrm/dss/core.c index 54eeb507f9b3..1f55d0aae03d >> 100644 >> --- a/drivers/gpu/drm/omapdrm/dss/core.c >> +++ b/drivers/gpu/drm/omapdrm/dss/core.c >> @@ -165,31 +165,6 @@ int dss_debugfs_create_file(const char *name, voi= d >> (*write)(struct seq_file *)) #endif /* CONFIG_OMAP2_DSS_DEBUGFS */ >> >> /* PLATFORM DEVICE */ >> -static int omap_dss_pm_notif(struct notifier_block *b, unsigned long = v, >> void *d) -{ >> - DSSDBG("pm notif %lu\n", v); >> - >> - switch (v) { >> - case PM_SUSPEND_PREPARE: >> - case PM_HIBERNATION_PREPARE: >> - case PM_RESTORE_PREPARE: >> - DSSDBG("suspending displays\n"); >> - return dss_suspend_all_devices(); >> - >> - case PM_POST_SUSPEND: >> - case PM_POST_HIBERNATION: >> - case PM_POST_RESTORE: >> - DSSDBG("resuming displays\n"); >> - return dss_resume_all_devices(); >> - >> - default: >> - return 0; >> - } >> -} >> - >> -static struct notifier_block omap_dss_pm_notif_block =3D { >> - .notifier_call =3D omap_dss_pm_notif, >> -}; >> >> static int __init omap_dss_probe(struct platform_device *pdev) >> { >> @@ -211,8 +186,6 @@ static int __init omap_dss_probe(struct platform_d= evice >> *pdev) else if (pdata->default_device) >> core.default_display_name =3D pdata->default_device->name; >> >> - register_pm_notifier(&omap_dss_pm_notif_block); >> - >> return 0; >> >> err_debugfs: >> @@ -222,8 +195,6 @@ err_debugfs: >> >> static int omap_dss_remove(struct platform_device *pdev) >> { >> - unregister_pm_notifier(&omap_dss_pm_notif_block); >> - >> dss_uninitialize_debugfs(); >> >> return 0; >> diff --git a/drivers/gpu/drm/omapdrm/dss/display.c >> b/drivers/gpu/drm/omapdrm/dss/display.c index ef5b9027985d..24c2bffa00= 36 >> 100644 >> --- a/drivers/gpu/drm/omapdrm/dss/display.c >> +++ b/drivers/gpu/drm/omapdrm/dss/display.c >> @@ -78,42 +78,6 @@ void omapdss_default_get_timings(struct omap_dss_de= vice >> *dssdev, } >> EXPORT_SYMBOL(omapdss_default_get_timings); >> >> -int dss_suspend_all_devices(void) >> -{ >> - struct omap_dss_device *dssdev =3D NULL; >> - >> - for_each_dss_dev(dssdev) { >> - if (!dssdev->driver) >> - continue; >> - >> - if (dssdev->state =3D=3D OMAP_DSS_DISPLAY_ACTIVE) { >> - dssdev->driver->disable(dssdev); >> - dssdev->activate_after_resume =3D true; >> - } else { >> - dssdev->activate_after_resume =3D false; >> - } >> - } >> - >> - return 0; >> -} >> - >> -int dss_resume_all_devices(void) >> -{ >> - struct omap_dss_device *dssdev =3D NULL; >> - >> - for_each_dss_dev(dssdev) { >> - if (!dssdev->driver) >> - continue; >> - >> - if (dssdev->activate_after_resume) { >> - dssdev->driver->enable(dssdev); >> - dssdev->activate_after_resume =3D false; >> - } >> - } >> - >> - return 0; >> -} >> - >> void dss_disable_all_devices(void) >> { >> struct omap_dss_device *dssdev =3D NULL; >> diff --git a/drivers/gpu/drm/omapdrm/dss/dss.h >> b/drivers/gpu/drm/omapdrm/dss/dss.h index 9a6453235585..a974d46672db 1= 00644 >> --- a/drivers/gpu/drm/omapdrm/dss/dss.h >> +++ b/drivers/gpu/drm/omapdrm/dss/dss.h >> @@ -206,8 +206,6 @@ int dss_set_min_bus_tput(struct device *dev, unsig= ned >> long tput); int dss_debugfs_create_file(const char *name, void >> (*write)(struct seq_file *)); >> >> /* display */ >> -int dss_suspend_all_devices(void); >> -int dss_resume_all_devices(void); >> void dss_disable_all_devices(void); >> >> int display_init_sysfs(struct platform_device *pdev); >> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c >> b/drivers/gpu/drm/omapdrm/omap_drv.c index dfafdb602ad2..e21433c3fda4 >> 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_drv.c >> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c >> @@ -900,12 +900,52 @@ static int pdev_remove(struct platform_device *d= evice) >> } >> >> #ifdef CONFIG_PM_SLEEP >> +static int omap_drm_suspend_all_displays(void) >> +{ >> + struct omap_dss_device *dssdev =3D NULL; >> + >> + for_each_dss_dev(dssdev) { >> + if (!dssdev->driver) >> + continue; >> + >> + if (dssdev->state =3D=3D OMAP_DSS_DISPLAY_ACTIVE) { >> + dssdev->driver->disable(dssdev); >> + dssdev->activate_after_resume =3D true; >> + } else { >> + dssdev->activate_after_resume =3D false; >> + } >> + } >> + >> + return 0; >> +} >> + >> +static int omap_drm_resume_all_displays(void) >> +{ >> + struct omap_dss_device *dssdev =3D NULL; >> + >> + for_each_dss_dev(dssdev) { >> + if (!dssdev->driver) >> + continue; >> + >> + if (dssdev->activate_after_resume) { >> + dssdev->driver->enable(dssdev); >> + dssdev->activate_after_resume =3D false; >> + } >> + } >> + >> + return 0; >> +} >> + >> static int omap_drm_suspend(struct device *dev) >> { >> struct drm_device *drm_dev =3D dev_get_drvdata(dev); >> >> drm_kms_helper_poll_disable(drm_dev); >> >> + drm_modeset_lock_all(drm_dev); >=20 > The omapdss implementation of the suspend and resume handlers didn't ta= ke the=20 > modeset locks. I wonder what we're trying to protect against here, what= other=20 > concurrent task(s) could race us ? The description explains at least one case I was encountering. To be honest, the new code doesn't feel very good either, but at least the entry point is now in omapdrm, and protected with a lock, so it should be much safer. As for what else is there... I hope not much. The panel/encoder drivers can execute code in an interrupt or possibly even via a sysfs file if someone would add things like that, and at the moment we don't have means to properly deal with it. But afaik the current drivers don't do any of those. Tomi --No553Jri10qpqKJDi1PA0TtmmTWUe2EII-- --xJaBB8gHC60l3kQrXLfbK8whgafG6Bv1v Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJW3TYSAAoJEPo9qoy8lh71tHcQAJkSTdUI+DKTuwiUxe0eEqs9 utKNyN6WZiI586IkLtf0IfpR3a8aTeg4IY3CdIBDylkz5xHXnLCf5+6XFDDeN1Fn lCkVr9Nd1FooPJbExCCifbWU62b0tZEsuIrJ0F4rPCjwQi/tsMH2BPW50Wjri3nn hSwloZD89mRAb9URy2RNk4UfrMCK2cRKaLdNk+dNQ3eeUSypxDeVvgVXxS4uULa4 Rd68GUZ40X157asLruNx918aoykL6w5glhEHBTJvvqM5ajJOFgfmbWCPQY9os7lv IpXTgPGb9D++AnD5Fr8t8pKfcveBXl0fmWTCo2xLesB3dmeGHC9nsKexqsJGMol8 vz6dBCOswjcKMatFOdyaVkpxqNd7AUjSi7+XOdGr60pvO08TTUah3L3bZOiTGqH6 2WbqA7xn5PwMNd3CT/LeeTBSeDlKxBTviV68qYqCEsMNsOD0ktWnvlw+EjTaLNq+ hJsxmlJFBBuoE39epnEWXqFwRMARx2WU9yeFJGmATOqgZYFwknjGswGlcq6fGaTZ 4an4A8LdRL1T2u8AtgbzcGmg8B017rsmrLImIFFlzUZicEzn2Fb1ChPn6P9Et8Or TVqxKEolxNdsyDD+7MvbHkQQ5IePmB5cue4ojZW7ttD8TMUaEXYk728LAR2igaIV nS+uKZLdXaW2C1aIgJwz =i8AC -----END PGP SIGNATURE----- --xJaBB8gHC60l3kQrXLfbK8whgafG6Bv1v-- --===============0688546813== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0688546813==--