Hi!
Recently I noticed that suspend/resume support in Exynos DRM driver is broken since v4.13-rc1. After some investigation I found that there are 2 patches causing this problem. First one is specific to v4.13 kernel release (first patch fixes it). The second one is caused by a patch queued to linux-next for v4.14-rc1 merge window (second patch fixes it). Please queue both patches to v4.14-rcX fixes merge.
Best regards Marek Szyprowski Samsung R&D Institute Poland
Marek Szyprowski (2): drm/exynos: Fix locking in the suspend/resume paths drm/exynos: Fix suspend/resume support
drivers/gpu/drm/exynos/exynos_drm_drv.c | 36 +++++++++++-------------------- drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 + drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 20 +++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.h | 10 +++++++++ 4 files changed, 43 insertions(+), 24 deletions(-)
Commit 48a92916729b ("drm/exynos: use drm_for_each_connector_iter()") replaced unsafe drm_for_each_connector() with drm_for_each_connector_iter() and removed surrounding drm_modeset_lock calls. However, that lock was there not only to protect unsafe drm_for_each_connector(), but it was also required to be held by the dpms code which was called from the loop body. This patch restores those drm_modeset_lock calls to fix broken suspend and resume of Exynos DRM subsystem in v4.13 kernel.
Fixes: 48a92916729b ("drm/exynos: use drm_for_each_connector_iter()") CC: stable@vger.kernel.org # v4.13 Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index b1f7299600f0..7f3cfc5dd320 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -174,6 +174,7 @@ static int exynos_drm_suspend(struct device *dev) if (pm_runtime_suspended(dev) || !drm_dev) return 0;
+ drm_modeset_lock_all(drm_dev); drm_connector_list_iter_begin(drm_dev, &conn_iter); drm_for_each_connector_iter(connector, &conn_iter) { int old_dpms = connector->dpms; @@ -185,6 +186,7 @@ static int exynos_drm_suspend(struct device *dev) connector->dpms = old_dpms; } drm_connector_list_iter_end(&conn_iter); + drm_modeset_unlock_all(drm_dev);
return 0; } @@ -198,6 +200,7 @@ static int exynos_drm_resume(struct device *dev) if (pm_runtime_suspended(dev) || !drm_dev) return 0;
+ drm_modeset_lock_all(drm_dev); drm_connector_list_iter_begin(drm_dev, &conn_iter); drm_for_each_connector_iter(connector, &conn_iter) { if (connector->funcs->dpms) { @@ -208,6 +211,7 @@ static int exynos_drm_resume(struct device *dev) } } drm_connector_list_iter_end(&conn_iter); + drm_modeset_unlock_all(drm_dev);
return 0; }
On Thu, Sep 14, 2017 at 2:01 PM, Marek Szyprowski m.szyprowski@samsung.com wrote:
Commit 48a92916729b ("drm/exynos: use drm_for_each_connector_iter()") replaced unsafe drm_for_each_connector() with drm_for_each_connector_iter() and removed surrounding drm_modeset_lock calls. However, that lock was there not only to protect unsafe drm_for_each_connector(), but it was also required to be held by the dpms code which was called from the loop body. This patch restores those drm_modeset_lock calls to fix broken suspend and resume of Exynos DRM subsystem in v4.13 kernel.
Fixes: 48a92916729b ("drm/exynos: use drm_for_each_connector_iter()") CC: stable@vger.kernel.org # v4.13 Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 ++++ 1 file changed, 4 insertions(+)
Acked-by: Krzysztof Kozlowski krzk@kernel.org
Best regards, Krzysztof
Commit 7d902c05b480 ("drm: Nuke drm_atomic_helper_connector_dpms") removed drm_atomic_helper_connector_dpms() helper saying that it was a dead code. It was however indirectly used by Exynos DRM driver for implementing suspend/resume support. To fix this regression (after that patch Exynos DRM suspend/resume functions became no-ops and hardware fails to suspend), this patch rewrites them with drm_atomic_helper_suspend/resume() helpers.
Fixes: 7d902c05b480 ("drm: Nuke drm_atomic_helper_connector_dpms") Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_drv.c | 40 ++++++++++--------------------- drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 + drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 20 ++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.h | 10 ++++++++ 4 files changed, 43 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 7f3cfc5dd320..e651a58c18cf 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -168,25 +168,19 @@ static void exynos_drm_lastclose(struct drm_device *dev) static int exynos_drm_suspend(struct device *dev) { struct drm_device *drm_dev = dev_get_drvdata(dev); - struct drm_connector *connector; - struct drm_connector_list_iter conn_iter; + struct exynos_drm_private *private = drm_dev->dev_private;
if (pm_runtime_suspended(dev) || !drm_dev) return 0;
- drm_modeset_lock_all(drm_dev); - drm_connector_list_iter_begin(drm_dev, &conn_iter); - drm_for_each_connector_iter(connector, &conn_iter) { - int old_dpms = connector->dpms; - - if (connector->funcs->dpms) - connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); - - /* Set the old mode back to the connector for resume */ - connector->dpms = old_dpms; + drm_kms_helper_poll_disable(drm_dev); + exynos_drm_fbdev_suspend(drm_dev); + private->suspend_state = drm_atomic_helper_suspend(drm_dev); + if (IS_ERR(private->suspend_state)) { + exynos_drm_fbdev_resume(drm_dev); + drm_kms_helper_poll_enable(drm_dev); + return PTR_ERR(private->suspend_state); } - drm_connector_list_iter_end(&conn_iter); - drm_modeset_unlock_all(drm_dev);
return 0; } @@ -194,24 +188,14 @@ static int exynos_drm_suspend(struct device *dev) static int exynos_drm_resume(struct device *dev) { struct drm_device *drm_dev = dev_get_drvdata(dev); - struct drm_connector *connector; - struct drm_connector_list_iter conn_iter; + struct exynos_drm_private *private = drm_dev->dev_private;
if (pm_runtime_suspended(dev) || !drm_dev) return 0;
- drm_modeset_lock_all(drm_dev); - drm_connector_list_iter_begin(drm_dev, &conn_iter); - drm_for_each_connector_iter(connector, &conn_iter) { - if (connector->funcs->dpms) { - int dpms = connector->dpms; - - connector->dpms = DRM_MODE_DPMS_OFF; - connector->funcs->dpms(connector, dpms); - } - } - drm_connector_list_iter_end(&conn_iter); - drm_modeset_unlock_all(drm_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; } diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index cf131c2aa23e..f8bae4cb4823 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -202,6 +202,7 @@ struct drm_exynos_file_private { */ struct exynos_drm_private { struct drm_fb_helper *fb_helper; + struct drm_atomic_state *suspend_state;
struct device *dma_dev; void *mapping; diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index c3a068409b48..dfb66ecf417b 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -18,6 +18,8 @@ #include <drm/drm_crtc_helper.h> #include <drm/exynos_drm.h>
+#include <linux/console.h> + #include "exynos_drm_drv.h" #include "exynos_drm_fb.h" #include "exynos_drm_fbdev.h" @@ -285,3 +287,21 @@ void exynos_drm_output_poll_changed(struct drm_device *dev)
drm_fb_helper_hotplug_event(fb_helper); } + +void exynos_drm_fbdev_suspend(struct drm_device *dev) +{ + struct exynos_drm_private *private = dev->dev_private; + + console_lock(); + drm_fb_helper_set_suspend(private->fb_helper, 1); + console_unlock(); +} + +void exynos_drm_fbdev_resume(struct drm_device *dev) +{ + struct exynos_drm_private *private = dev->dev_private; + + console_lock(); + drm_fb_helper_set_suspend(private->fb_helper, 0); + console_unlock(); +} diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.h b/drivers/gpu/drm/exynos/exynos_drm_fbdev.h index 330eef87f718..8cdc2e8a1c6a 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.h +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.h @@ -21,6 +21,8 @@ void exynos_drm_fbdev_fini(struct drm_device *dev); void exynos_drm_fbdev_restore_mode(struct drm_device *dev); void exynos_drm_output_poll_changed(struct drm_device *dev); +void exynos_drm_fbdev_suspend(struct drm_device *drm); +void exynos_drm_fbdev_resume(struct drm_device *drm);
#else
@@ -39,6 +41,14 @@ static inline void exynos_drm_fbdev_restore_mode(struct drm_device *dev)
#define exynos_drm_output_poll_changed (NULL)
+static inline void exynos_drm_fbdev_suspend(struct drm_device *drm) +{ +} + +static inline void exynos_drm_fbdev_resume(struct drm_device *drm) +{ +} + #endif
#endif
On Thu, Sep 14, 2017 at 2:01 PM, Marek Szyprowski m.szyprowski@samsung.com wrote:
Commit 7d902c05b480 ("drm: Nuke drm_atomic_helper_connector_dpms") removed drm_atomic_helper_connector_dpms() helper saying that it was a dead code. It was however indirectly used by Exynos DRM driver for implementing suspend/resume support. To fix this regression (after that patch Exynos DRM suspend/resume functions became no-ops and hardware fails to suspend), this patch rewrites them with drm_atomic_helper_suspend/resume() helpers.
Fixes: 7d902c05b480 ("drm: Nuke drm_atomic_helper_connector_dpms") Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
drivers/gpu/drm/exynos/exynos_drm_drv.c | 40 ++++++++++--------------------- drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 + drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 20 ++++++++++++++++ drivers/gpu/drm/exynos/exynos_drm_fbdev.h | 10 ++++++++ 4 files changed, 43 insertions(+), 28 deletions(-)
Acked-by: Krzysztof Kozlowski krzk@kernel.org
Best regards, Krzysztof
dri-devel@lists.freedesktop.org