[PATCH 01/42] drm/omap: fix suspend/resume handling

[Laurent Pinchart]

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 at ti.com>
> ---
>  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