Hi Tomi,
Thank you for the patch.
On Monday 22 February 2016 19:10:07 Tomi Valkeinen wrote:
> For legacy reasons omapdss handles system suspend/resume via PM notifier
> callback, where the driver disables/resumes all the outputs.
>
> This doesn't work well with omapdrm. What happens on suspend is that the
> omapdss disables the displays while omapdrm is still happily continuing
> 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 <tomi.valkeinen@xxxxxx>
> ---
> 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, void
> (*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 = {
> - .notifier_call = 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_device
> *pdev) else if (pdata->default_device)
> core.default_display_name = 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..24c2bffa0036
> 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_device
> *dssdev, }
> EXPORT_SYMBOL(omapdss_default_get_timings);
>
> -int dss_suspend_all_devices(void)
> -{
> - struct omap_dss_device *dssdev = NULL;
> -
> - for_each_dss_dev(dssdev) {
> - if (!dssdev->driver)
> - continue;
> -
> - if (dssdev->state == OMAP_DSS_DISPLAY_ACTIVE) {
> - dssdev->driver->disable(dssdev);
> - dssdev->activate_after_resume = true;
> - } else {
> - dssdev->activate_after_resume = false;
> - }
> - }
> -
> - return 0;
> -}
> -
> -int dss_resume_all_devices(void)
> -{
> - struct omap_dss_device *dssdev = NULL;
> -
> - for_each_dss_dev(dssdev) {
> - if (!dssdev->driver)
> - continue;
> -
> - if (dssdev->activate_after_resume) {
> - dssdev->driver->enable(dssdev);
> - dssdev->activate_after_resume = false;
> - }
> - }
> -
> - return 0;
> -}
> -
> void dss_disable_all_devices(void)
> {
> struct omap_dss_device *dssdev = NULL;
> diff --git a/drivers/gpu/drm/omapdrm/dss/dss.h
> b/drivers/gpu/drm/omapdrm/dss/dss.h index 9a6453235585..a974d46672db 100644
> --- 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, unsigned
> 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 *device)
> }
>
> #ifdef CONFIG_PM_SLEEP
> +static int omap_drm_suspend_all_displays(void)
> +{
> + struct omap_dss_device *dssdev = NULL;
> +
> + for_each_dss_dev(dssdev) {
> + if (!dssdev->driver)
> + continue;
> +
> + if (dssdev->state == OMAP_DSS_DISPLAY_ACTIVE) {
> + dssdev->driver->disable(dssdev);
> + dssdev->activate_after_resume = true;
> + } else {
> + dssdev->activate_after_resume = false;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int omap_drm_resume_all_displays(void)
> +{
> + struct omap_dss_device *dssdev = NULL;
> +
> + for_each_dss_dev(dssdev) {
> + if (!dssdev->driver)
> + continue;
> +
> + if (dssdev->activate_after_resume) {
> + dssdev->driver->enable(dssdev);
> + dssdev->activate_after_resume = false;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int omap_drm_suspend(struct device *dev)
> {
> struct drm_device *drm_dev = dev_get_drvdata(dev);
>
> drm_kms_helper_poll_disable(drm_dev);
>
> + drm_modeset_lock_all(drm_dev);
The omapdss implementation of the suspend and resume handlers didn't take the
modeset locks. I wonder what we're trying to protect against here, what other
concurrent task(s) could race us ?
> + omap_drm_suspend_all_displays();
> + drm_modeset_unlock_all(drm_dev);
> +
> return 0;
> }
>
> @@ -913,6 +953,10 @@ static int omap_drm_resume(struct device *dev)
> {
> struct drm_device *drm_dev = dev_get_drvdata(dev);
>
> + drm_modeset_lock_all(drm_dev);
> + omap_drm_resume_all_displays();
> + drm_modeset_unlock_all(drm_dev);
> +
> drm_kms_helper_poll_enable(drm_dev);
>
> return omap_gem_resume(dev);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
