Dear all,
This patchset performs a significant cleanup of Exynos DRM suspend/resume code. A side effect of this cleanup is working system suspend/resume on Exynos5433 SoCs, where there are more dependencies between hardware device drivers, clock controllers and power domains than in case of the previous SoCs.
Best regards Marek Szyprowski Samsung R&D Institute Poland
Patch summary:
Marek Szyprowski (3): drm/exynos: Drop useless check from exynos_drm_{suspend,resume} drm/exynos: Suspend/resume display pipeline as early/late as possible drm/exynos: Ensure suspended runtime PM state during system suspend
drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 2 ++ drivers/gpu/drm/exynos/exynos7_drm_decon.c | 2 ++ drivers/gpu/drm/exynos/exynos_dp.c | 3 +++ drivers/gpu/drm/exynos/exynos_drm_drv.c | 15 ++++++--------- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 ++ drivers/gpu/drm/exynos/exynos_drm_fimd.c | 2 ++ drivers/gpu/drm/exynos/exynos_drm_mic.c | 2 ++ drivers/gpu/drm/exynos/exynos_hdmi.c | 2 ++ drivers/gpu/drm/exynos/exynos_mixer.c | 2 ++ 9 files changed, 23 insertions(+), 9 deletions(-)
The virtual Exynos DRM device has no runtime PM enabled, so checking for its runtime suspended state is useless.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_drv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index a81b4a5e24a7..c0b4a03ae1b6 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -153,7 +153,7 @@ static int exynos_drm_suspend(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev); struct exynos_drm_private *private;
- if (pm_runtime_suspended(dev) || !drm_dev) + if (!drm_dev) return 0;
private = drm_dev->dev_private; @@ -175,8 +175,8 @@ static int exynos_drm_resume(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev); struct exynos_drm_private *private;
- if (pm_runtime_suspended(dev) || !drm_dev) - return 0; + if (!drm_dev) + return;
private = drm_dev->dev_private; drm_atomic_helper_resume(drm_dev, private->suspend_state);
2018년 06월 11일 21:24에 Marek Szyprowski 이(가) 쓴 글:
The virtual Exynos DRM device has no runtime PM enabled, so checking for its runtime suspended state is useless.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/gpu/drm/exynos/exynos_drm_drv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index a81b4a5e24a7..c0b4a03ae1b6 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -153,7 +153,7 @@ static int exynos_drm_suspend(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev); struct exynos_drm_private *private;
- if (pm_runtime_suspended(dev) || !drm_dev)
if (!drm_dev) return 0;
private = drm_dev->dev_private;
@@ -175,8 +175,8 @@ static int exynos_drm_resume(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev); struct exynos_drm_private *private;
- if (pm_runtime_suspended(dev) || !drm_dev)
return 0;
- if (!drm_dev)
return;
return 0. I will fix it.
Thanks, Inki Dae
private = drm_dev->dev_private; drm_atomic_helper_resume(drm_dev, private->suspend_state);
Hi Inki,
On 2018-07-24 09:12, Inki Dae wrote:
2018년 06월 11일 21:24에 Marek Szyprowski 이(가) 쓴 글:
The virtual Exynos DRM device has no runtime PM enabled, so checking for its runtime suspended state is useless.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/gpu/drm/exynos/exynos_drm_drv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index a81b4a5e24a7..c0b4a03ae1b6 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -153,7 +153,7 @@ static int exynos_drm_suspend(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev); struct exynos_drm_private *private;
- if (pm_runtime_suspended(dev) || !drm_dev)
if (!drm_dev) return 0;
private = drm_dev->dev_private;
@@ -175,8 +175,8 @@ static int exynos_drm_resume(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev); struct exynos_drm_private *private;
- if (pm_runtime_suspended(dev) || !drm_dev)
return 0;
- if (!drm_dev)
return;return 0. I will fix it.
Ah, my fault. This is a result of reordering the patches in the final patchset. After patch 2/3 exynos_drm_resume is assigned to .complete callback, which use 'void' return signature, so the 'return 0' has to be changed to 'return' again then. Thanks for fixing this.
Best regards
2018년 07월 24일 16:49에 Marek Szyprowski 이(가) 쓴 글:
Hi Inki,
On 2018-07-24 09:12, Inki Dae wrote:
2018년 06월 11일 21:24에 Marek Szyprowski 이(가) 쓴 글:
The virtual Exynos DRM device has no runtime PM enabled, so checking for its runtime suspended state is useless.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/gpu/drm/exynos/exynos_drm_drv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index a81b4a5e24a7..c0b4a03ae1b6 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -153,7 +153,7 @@ static int exynos_drm_suspend(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev); struct exynos_drm_private *private;
- if (pm_runtime_suspended(dev) || !drm_dev)
if (!drm_dev) return 0;
private = drm_dev->dev_private;
@@ -175,8 +175,8 @@ static int exynos_drm_resume(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev); struct exynos_drm_private *private;
- if (pm_runtime_suspended(dev) || !drm_dev)
return 0;
- if (!drm_dev)
return;return 0. I will fix it.
Ah, my fault. This is a result of reordering the patches in the final patchset. After patch 2/3 exynos_drm_resume is assigned to .complete callback, which use 'void' return signature, so the 'return 0' has to be changed to 'return' again
Already done. :)
then. Thanks for fixing this.
Best regards
In the current code, exynos_drm_suspend() function is called after all real devices (CRTCs, Encoders, etc) are suspended, because Exynos DRM virtual platform device is created as last device in the system (as a part of DRM registration). None of the devices for real hardware modules has its own system suspend/resume callbacks, so it doesn't change any order of the executed code, but it has a side-effect: runtime PM callbacks for real devices are not executed, because those devices are considered by PM core as already suspended. This might cause issues on boards with complex pipelines, where something depends on the runtime PM state of the given device.
To ensure that exynos_drm_suspend() is called before any suspend callback from the real devices, assign it to .prepare callback. Same for exynos_drm_resume(), using .complete callback ensures that all real devices have been resumed when calling it.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_drv.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index c0b4a03ae1b6..d98bc15d490d 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -147,7 +147,6 @@ static struct drm_driver exynos_drm_driver = { .minor = DRIVER_MINOR, };
-#ifdef CONFIG_PM_SLEEP static int exynos_drm_suspend(struct device *dev) { struct drm_device *drm_dev = dev_get_drvdata(dev); @@ -170,7 +169,7 @@ static int exynos_drm_suspend(struct device *dev) return 0; }
-static int exynos_drm_resume(struct device *dev) +static void exynos_drm_resume(struct device *dev) { struct drm_device *drm_dev = dev_get_drvdata(dev); struct exynos_drm_private *private; @@ -182,13 +181,11 @@ static int exynos_drm_resume(struct device *dev) drm_atomic_helper_resume(drm_dev, private->suspend_state); exynos_drm_fbdev_resume(drm_dev); drm_kms_helper_poll_enable(drm_dev); - - return 0; } -#endif
static const struct dev_pm_ops exynos_drm_pm_ops = { - SET_SYSTEM_SLEEP_PM_OPS(exynos_drm_suspend, exynos_drm_resume) + .prepare = exynos_drm_suspend, + .complete = exynos_drm_resume, };
/* forward declaration */
Add calls to pm_runtime_force_{suspend,resume} as SYSTEM_SLEEP_PM_OPS for all drivers for the real Exynos DRM hardware modules. This ensures that the resources will be released for the system PM suspend/resume cycle. Exynos DRM core already takes care of suspending the whole display pipeline before PM callbacks of the real devices are called.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 2 ++ drivers/gpu/drm/exynos/exynos7_drm_decon.c | 2 ++ drivers/gpu/drm/exynos/exynos_dp.c | 3 +++ drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 ++ drivers/gpu/drm/exynos/exynos_drm_fimd.c | 2 ++ drivers/gpu/drm/exynos/exynos_drm_mic.c | 2 ++ drivers/gpu/drm/exynos/exynos_hdmi.c | 2 ++ drivers/gpu/drm/exynos/exynos_mixer.c | 2 ++ 8 files changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c index 82c95c34447f..aac0b383027e 100644 --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c @@ -673,6 +673,8 @@ static int exynos5433_decon_resume(struct device *dev) static const struct dev_pm_ops exynos5433_decon_pm_ops = { SET_RUNTIME_PM_OPS(exynos5433_decon_suspend, exynos5433_decon_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) };
static const struct of_device_id exynos5433_decon_driver_dt_match[] = { diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c index 3931d5e33fe0..88cbd000eb09 100644 --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c @@ -832,6 +832,8 @@ static int exynos7_decon_resume(struct device *dev) static const struct dev_pm_ops exynos7_decon_pm_ops = { SET_RUNTIME_PM_OPS(exynos7_decon_suspend, exynos7_decon_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) };
struct platform_driver decon_driver = { diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c index 86330f396784..7bda7cbb6666 100644 --- a/drivers/gpu/drm/exynos/exynos_dp.c +++ b/drivers/gpu/drm/exynos/exynos_dp.c @@ -16,6 +16,7 @@ #include <linux/clk.h> #include <linux/of_graph.h> #include <linux/component.h> +#include <linux/pm_runtime.h> #include <video/of_display_timing.h> #include <video/of_videomode.h> #include <video/videomode.h> @@ -276,6 +277,8 @@ static int exynos_dp_resume(struct device *dev)
static const struct dev_pm_ops exynos_dp_pm_ops = { SET_RUNTIME_PM_OPS(exynos_dp_suspend, exynos_dp_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) };
static const struct of_device_id exynos_dp_match[] = { diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 7c3030b7e586..95faea0d5f59 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1860,6 +1860,8 @@ static int __maybe_unused exynos_dsi_resume(struct device *dev)
static const struct dev_pm_ops exynos_dsi_pm_ops = { SET_RUNTIME_PM_OPS(exynos_dsi_suspend, exynos_dsi_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) };
struct platform_driver dsi_driver = { diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 01b1570d0c3a..b7f56935a46b 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -1192,6 +1192,8 @@ static int exynos_fimd_resume(struct device *dev)
static const struct dev_pm_ops exynos_fimd_pm_ops = { SET_RUNTIME_PM_OPS(exynos_fimd_suspend, exynos_fimd_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) };
struct platform_driver fimd_driver = { diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c index 2174814273e2..2fd299a58297 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_mic.c +++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c @@ -367,6 +367,8 @@ static int exynos_mic_resume(struct device *dev)
static const struct dev_pm_ops exynos_mic_pm_ops = { SET_RUNTIME_PM_OPS(exynos_mic_suspend, exynos_mic_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) };
static int exynos_mic_probe(struct platform_device *pdev) diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 09c4bc0b1859..2fef61144743 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -2093,6 +2093,8 @@ static int __maybe_unused exynos_hdmi_resume(struct device *dev)
static const struct dev_pm_ops exynos_hdmi_pm_ops = { SET_RUNTIME_PM_OPS(exynos_hdmi_suspend, exynos_hdmi_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) };
struct platform_driver hdmi_driver = { diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 272c79f5f5bf..e45278e076da 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -1271,6 +1271,8 @@ static int __maybe_unused exynos_mixer_resume(struct device *dev)
static const struct dev_pm_ops exynos_mixer_pm_ops = { SET_RUNTIME_PM_OPS(exynos_mixer_suspend, exynos_mixer_resume, NULL) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) };
struct platform_driver mixer_driver = {
dri-devel@lists.freedesktop.org
-
Inki Dae -
Marek Szyprowski