[PATCH v2 0/6] drm/: Add simple modeset suspend/resume helpers

[PATCH v2 0/6] drm/: Add simple modeset suspend/resume helpers

[Noralf Trønnes]

This patchset adds some simple modeset suspend/resume helpers which
probably most atomic drivers can use.

For those that haven't followed dri-devel closely the past few days,
this patch put in place the fbdev piece necessary to do this:
drm: Add drm_device->fb_helper pointer
https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/drm_fb_
helper.c?id=29ad20b22c8f3ab35e91c2f68b4c7956cee30fd0

I have converted the cma helper drivers as part of my ongoing cma helper
refactoring.

Noralf.

Changes since version 1:
- Improve driver commit message (Liviu)
- fsl-dcu: Fix build error: 'ret' undeclared

Noralf Trønnes (6):
  drm/probe-helper: Fix drm_kms_helper_poll_enable() docs
  drm/modeset-helper: Add simple modeset suspend/resume helpers
  drm/arm/mali: Use drm_mode_config_helper_suspend/resume()
  drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()
  drm/tinydrm: Use drm_mode_config_helper_suspend/resume()
  drm/docs: Add todo entry for simple modeset suspend/resume

 Documentation/gpu/todo.rst                  | 14 ++++--
 drivers/gpu/drm/arm/malidp_drv.c            | 24 ++-------
 drivers/gpu/drm/arm/malidp_drv.h            |  1 -
 drivers/gpu/drm/drm_modeset_helper.c        | 76 +++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c          |  3 +-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c   | 24 +++------
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h   |  1 -
 drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 67 -------------------------
 drivers/gpu/drm/tinydrm/mi0283qt.c          |  7 ++-
 include/drm/drm_mode_config.h               |  9 ++++
 include/drm/drm_modeset_helper.h            |  3 ++
 include/drm/tinydrm/tinydrm.h               |  4 --
 12 files changed, 112 insertions(+), 121 deletions(-)

-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/6] drm/probe-helper: Fix drm_kms_helper_poll_enable() docs

[Noralf Trønnes]

Fix docs to reflect code and drm_kms_helper_poll_disable() docs by saying
that calling drm_kms_helper_poll_enable() is fine even if output polling
is not enabled.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_probe_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 5840aabbf24e..024a89bf0ba7 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -216,8 +216,7 @@ enum drm_mode_status drm_connector_mode_valid(struct drm_connector *connector,
  * suspend/resume.
  *
  * Drivers can call this helper from their device resume implementation. It is
- * an error to call this when the output polling support has not yet been set
- * up.
+ * not an error to call this even when output polling isn't enabled.
  *
  * Note that calls to enable and disable polling must be strictly ordered, which
  * is automatically the case when they're only call from suspend/resume
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/6] drm/modeset-helper: Add simple modeset suspend/resume helpers

[Noralf Trønnes]

Add drm_mode_config_helper_suspend/resume() which takes care of
atomic modeset suspend/resume for simple use cases.
The suspend state is stored in struct drm_mode_config.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_modeset_helper.c | 76 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_mode_config.h        |  9 +++++
 include/drm/drm_modeset_helper.h     |  3 ++
 3 files changed, 88 insertions(+)

diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
index 9cb1eede0b4d..f1c24ab0ef09 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -20,6 +20,9 @@
  * OF THIS SOFTWARE.
  */
 
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_helper.h>
 #include <drm/drm_modeset_helper.h>
 #include <drm/drm_plane_helper.h>
 
@@ -156,3 +159,76 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 					 NULL);
 }
 EXPORT_SYMBOL(drm_crtc_init);
+
+/**
+ * drm_mode_config_helper_suspend - Modeset suspend helper
+ * @dev: DRM device
+ *
+ * This helper function takes care of suspending the modeset side. It disables
+ * output polling if initialized, suspends fbdev if used and finally calls
+ * drm_atomic_helper_suspend().
+ * If suspending fails, fbdev and polling is re-enabled.
+ *
+ * Returns:
+ * Zero on success, negative error code on error.
+ *
+ * See also:
+ * drm_kms_helper_poll_disable() and drm_fb_helper_set_suspend_unlocked().
+ */
+int drm_mode_config_helper_suspend(struct drm_device *dev)
+{
+	struct drm_atomic_state *state;
+
+	if (!dev)
+		return 0;
+
+	drm_kms_helper_poll_disable(dev);
+	drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
+	state = drm_atomic_helper_suspend(dev);
+	if (IS_ERR(state)) {
+		drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
+		drm_kms_helper_poll_enable(dev);
+		return PTR_ERR(state);
+	}
+
+	dev->mode_config.suspend_state = state;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_config_helper_suspend);
+
+/**
+ * drm_mode_config_helper_resume - Modeset resume helper
+ * @dev: DRM device
+ *
+ * This helper function takes care of resuming the modeset side. It calls
+ * drm_atomic_helper_resume(), resumes fbdev if used and enables output polling
+ * if initiaized.
+ *
+ * Returns:
+ * Zero on success, negative error code on error.
+ *
+ * See also:
+ * drm_fb_helper_set_suspend_unlocked() and drm_kms_helper_poll_enable().
+ */
+int drm_mode_config_helper_resume(struct drm_device *dev)
+{
+	int ret;
+
+	if (!dev)
+		return 0;
+
+	if (WARN_ON(!dev->mode_config.suspend_state))
+		return -EINVAL;
+
+	ret = drm_atomic_helper_resume(dev, dev->mode_config.suspend_state);
+	if (ret)
+		DRM_ERROR("Failed to resume (%d)\n", ret);
+	dev->mode_config.suspend_state = NULL;
+
+	drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
+	drm_kms_helper_poll_enable(dev);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_mode_config_helper_resume);
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 1b37368416c8..5a872496b409 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -766,6 +766,15 @@ struct drm_mode_config {
 	/* cursor size */
 	uint32_t cursor_width, cursor_height;
 
+	/**
+	 * @suspend_state:
+	 *
+	 * Atomic state when suspended.
+	 * Set by drm_mode_config_helper_suspend() and cleared by
+	 * drm_mode_config_helper_resume().
+	 */
+	struct drm_atomic_state *suspend_state;
+
 	const struct drm_mode_config_helper_funcs *helper_private;
 };
 
diff --git a/include/drm/drm_modeset_helper.h b/include/drm/drm_modeset_helper.h
index cb0ec92e11e6..efa337f03129 100644
--- a/include/drm/drm_modeset_helper.h
+++ b/include/drm/drm_modeset_helper.h
@@ -34,4 +34,7 @@ void drm_helper_mode_fill_fb_struct(struct drm_device *dev,
 int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		  const struct drm_crtc_funcs *funcs);
 
+int drm_mode_config_helper_suspend(struct drm_device *dev);
+int drm_mode_config_helper_resume(struct drm_device *dev);
+
 #endif
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/6] drm/arm/mali: Use drm_mode_config_helper_suspend/resume()

[Noralf Trønnes]

Replace driver's code with the generic helpers that do the same thing.

Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Brian Starkey <brian.starkey@arm.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
---
 drivers/gpu/drm/arm/malidp_drv.c | 24 +++---------------------
 drivers/gpu/drm/arm/malidp_drv.h |  1 -
 2 files changed, 3 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index b8944666a18f..75f0bce33941 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -27,6 +27,7 @@
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_modeset_helper.h>
 #include <drm/drm_of.h>
 
 #include "malidp_drv.h"
@@ -749,34 +750,15 @@ static int malidp_platform_remove(struct platform_device *pdev)
 static int __maybe_unused malidp_pm_suspend(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
-	struct malidp_drm *malidp = drm->dev_private;
 
-	drm_kms_helper_poll_disable(drm);
-	console_lock();
-	drm_fbdev_cma_set_suspend(malidp->fbdev, 1);
-	console_unlock();
-	malidp->pm_state = drm_atomic_helper_suspend(drm);
-	if (IS_ERR(malidp->pm_state)) {
-		console_lock();
-		drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
-		console_unlock();
-		drm_kms_helper_poll_enable(drm);
-		return PTR_ERR(malidp->pm_state);
-	}
-
-	return 0;
+	return drm_mode_config_helper_suspend(drm);
 }
 
 static int __maybe_unused malidp_pm_resume(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
-	struct malidp_drm *malidp = drm->dev_private;
 
-	drm_atomic_helper_resume(drm, malidp->pm_state);
-	console_lock();
-	drm_fbdev_cma_set_suspend(malidp->fbdev, 0);
-	console_unlock();
-	drm_kms_helper_poll_enable(drm);
+	drm_mode_config_helper_resume(drm);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index 2e2033140efc..70ed6aeccf05 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -24,7 +24,6 @@ struct malidp_drm {
 	struct drm_crtc crtc;
 	wait_queue_head_t wq;
 	atomic_t config_valid;
-	struct drm_atomic_state *pm_state;
 	u32 core_id;
 };
 
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Noralf Trønnes]

Replace driver's code with the generic helpers that do the same thing.

Cc: Stefan Agner <stefan@agner.ch>
Cc: Alison Wang <alison.wang@freescale.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 ++++++------------------
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
 2 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index 58e9e0601a61..1a9ee657bbac 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -27,6 +27,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_modeset_helper.h>
 
 #include "fsl_dcu_drm_crtc.h"
 #include "fsl_dcu_drm_drv.h"
@@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
 static int fsl_dcu_drm_pm_suspend(struct device *dev)
 {
 	struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
+	int ret;
 
 	if (!fsl_dev)
 		return 0;
 
 	disable_irq(fsl_dev->irq);
-	drm_kms_helper_poll_disable(fsl_dev->drm);
 
-	console_lock();
-	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
-	console_unlock();
-
-	fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
-	if (IS_ERR(fsl_dev->state)) {
-		console_lock();
-		drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
-		console_unlock();
-
-		drm_kms_helper_poll_enable(fsl_dev->drm);
+	ret = drm_mode_config_helper_suspend(fsl_dev->drm);
+	if (ret) {
 		enable_irq(fsl_dev->irq);
-		return PTR_ERR(fsl_dev->state);
+		return ret;
 	}
 
 	clk_disable_unprepare(fsl_dev->pix_clk);
@@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
 	if (fsl_dev->tcon)
 		fsl_tcon_bypass_enable(fsl_dev->tcon);
 	fsl_dcu_drm_init_planes(fsl_dev->drm);
-	drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
 
-	console_lock();
-	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
-	console_unlock();
+	drm_mode_config_helper_resume(fsl_dev->drm);
 
-	drm_kms_helper_poll_enable(fsl_dev->drm);
 	enable_irq(fsl_dev->irq);
 
 	return 0;
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
index da9bfd432ca6..93bfb98012d4 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
@@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
 	struct drm_encoder encoder;
 	struct fsl_dcu_drm_connector connector;
 	const struct fsl_dcu_soc_data *soc;
-	struct drm_atomic_state *state;
 };
 
 int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev);
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/6] drm/tinydrm: Use drm_mode_config_helper_suspend/resume()

[Noralf Trønnes]

Replace driver's code with the generic helpers that do the same thing.
Remove todo entry.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 Documentation/gpu/todo.rst                  |  5 ---
 drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 67 -----------------------------
 drivers/gpu/drm/tinydrm/mi0283qt.c          |  7 ++-
 include/drm/tinydrm/tinydrm.h               |  4 --
 4 files changed, 5 insertions(+), 78 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index e9840d693a86..a44f379d2b25 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -404,11 +404,6 @@ those drivers as simple as possible, so lots of room for refactoring:
   a drm_device wrong. Doesn't matter, since everyone else gets it wrong
   too :-)
 
-- With the fbdev pointer in dev->mode_config we could also make
-  suspend/resume helpers entirely generic, at least if we add a
-  dev->mode_config.suspend_state. We could even provide a generic pm_ops
-  structure with those.
-
 - also rework the drm_framebuffer_funcs->dirty hook wire-up, see above.
 
 Contact: Noralf Trønnes, Daniel Vetter
diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
index 1a8a57cad431..bd7b82824a34 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
@@ -292,71 +292,4 @@ void tinydrm_shutdown(struct tinydrm_device *tdev)
 }
 EXPORT_SYMBOL(tinydrm_shutdown);
 
-/**
- * tinydrm_suspend - Suspend tinydrm
- * @tdev: tinydrm device
- *
- * Used in driver PM operations to suspend tinydrm.
- * Suspends fbdev and DRM.
- * Resume with tinydrm_resume().
- *
- * Returns:
- * Zero on success, negative error code on failure.
- */
-int tinydrm_suspend(struct tinydrm_device *tdev)
-{
-	struct drm_atomic_state *state;
-
-	if (tdev->suspend_state) {
-		DRM_ERROR("Failed to suspend: state already set\n");
-		return -EINVAL;
-	}
-
-	drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 1);
-	state = drm_atomic_helper_suspend(tdev->drm);
-	if (IS_ERR(state)) {
-		drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
-		return PTR_ERR(state);
-	}
-
-	tdev->suspend_state = state;
-
-	return 0;
-}
-EXPORT_SYMBOL(tinydrm_suspend);
-
-/**
- * tinydrm_resume - Resume tinydrm
- * @tdev: tinydrm device
- *
- * Used in driver PM operations to resume tinydrm.
- * Suspend with tinydrm_suspend().
- *
- * Returns:
- * Zero on success, negative error code on failure.
- */
-int tinydrm_resume(struct tinydrm_device *tdev)
-{
-	struct drm_atomic_state *state = tdev->suspend_state;
-	int ret;
-
-	if (!state) {
-		DRM_ERROR("Failed to resume: state is not set\n");
-		return -EINVAL;
-	}
-
-	tdev->suspend_state = NULL;
-
-	ret = drm_atomic_helper_resume(tdev->drm, state);
-	if (ret) {
-		DRM_ERROR("Error resuming state: %d\n", ret);
-		return ret;
-	}
-
-	drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
-
-	return 0;
-}
-EXPORT_SYMBOL(tinydrm_resume);
-
 MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 6a83b3093254..70ae4f76f455 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -9,6 +9,7 @@
  * (at your option) any later version.
  */
 
+#include <drm/drm_modeset_helper.h>
 #include <drm/tinydrm/ili9341.h>
 #include <drm/tinydrm/mipi-dbi.h>
 #include <drm/tinydrm/tinydrm-helpers.h>
@@ -231,7 +232,7 @@ static int __maybe_unused mi0283qt_pm_suspend(struct device *dev)
 	struct mipi_dbi *mipi = dev_get_drvdata(dev);
 	int ret;
 
-	ret = tinydrm_suspend(&mipi->tinydrm);
+	ret = drm_mode_config_helper_suspend(mipi->tinydrm.drm);
 	if (ret)
 		return ret;
 
@@ -249,7 +250,9 @@ static int __maybe_unused mi0283qt_pm_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	return tinydrm_resume(&mipi->tinydrm);
+	drm_mode_config_helper_resume(mipi->tinydrm.drm);
+
+	return 0;
 }
 
 static const struct dev_pm_ops mi0283qt_pm_ops = {
diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
index 4774fe3d4273..fb0d86600ea3 100644
--- a/include/drm/tinydrm/tinydrm.h
+++ b/include/drm/tinydrm/tinydrm.h
@@ -20,7 +20,6 @@
  * @pipe: Display pipe structure
  * @dirty_lock: Serializes framebuffer flushing
  * @fbdev_cma: CMA fbdev structure
- * @suspend_state: Atomic state when suspended
  * @fb_funcs: Framebuffer functions used when creating framebuffers
  */
 struct tinydrm_device {
@@ -28,7 +27,6 @@ struct tinydrm_device {
 	struct drm_simple_display_pipe pipe;
 	struct mutex dirty_lock;
 	struct drm_fbdev_cma *fbdev_cma;
-	struct drm_atomic_state *suspend_state;
 	const struct drm_framebuffer_funcs *fb_funcs;
 };
 
@@ -92,8 +90,6 @@ int devm_tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
 		      struct drm_driver *driver);
 int devm_tinydrm_register(struct tinydrm_device *tdev);
 void tinydrm_shutdown(struct tinydrm_device *tdev);
-int tinydrm_suspend(struct tinydrm_device *tdev);
-int tinydrm_resume(struct tinydrm_device *tdev);
 
 void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
 				 struct drm_plane_state *old_state);
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/6] drm/docs: Add todo entry for simple modeset suspend/resume

[Noralf Trønnes]

Add entry for conversion of drivers to new helpers.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/gpu/todo.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index a44f379d2b25..6bce1beafabe 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -185,6 +185,15 @@ are better.
 
 Contact: Sean Paul, Maintainer of the driver you plan to convert
 
+Convert drivers to use simple modeset suspend/resume
+----------------------------------------------------
+
+Most drivers (except i915 and nouveau) that use
+drm_atomic_helper_suspend/resume() can probably be converted to use
+drm_mode_config_helper_suspend/resume().
+
+Contact: Maintainer of the driver you plan to convert
+
 Core refactorings
 =================
 
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/6] drm/modeset-helper: Add simple modeset suspend/resume helpers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]

Hi Noralf,

I love your patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.14-rc8 next-20171106]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Noralf-Tr-nnes/drm-Add-simple-modeset-suspend-resume-helpers/20171107-141931
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x000-201745 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/drm_modeset_helper.c: In function 'drm_mode_config_helper_suspend':
>> drivers/gpu/drm/drm_modeset_helper.c:186:40: error: 'struct drm_device' has no member named 'fb_helper'
     drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
                                           ^~
   drivers/gpu/drm/drm_modeset_helper.c:189:41: error: 'struct drm_device' has no member named 'fb_helper'
      drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
                                            ^~
   drivers/gpu/drm/drm_modeset_helper.c: In function 'drm_mode_config_helper_resume':
   drivers/gpu/drm/drm_modeset_helper.c:229:40: error: 'struct drm_device' has no member named 'fb_helper'
     drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
                                           ^~

vim +186 drivers/gpu/drm/drm_modeset_helper.c

   162	
   163	/**
   164	 * drm_mode_config_helper_suspend - Modeset suspend helper
   165	 * @dev: DRM device
   166	 *
   167	 * This helper function takes care of suspending the modeset side. It disables
   168	 * output polling if initialized, suspends fbdev if used and finally calls
   169	 * drm_atomic_helper_suspend().
   170	 * If suspending fails, fbdev and polling is re-enabled.
   171	 *
   172	 * Returns:
   173	 * Zero on success, negative error code on error.
   174	 *
   175	 * See also:
   176	 * drm_kms_helper_poll_disable() and drm_fb_helper_set_suspend_unlocked().
   177	 */
   178	int drm_mode_config_helper_suspend(struct drm_device *dev)
   179	{
   180		struct drm_atomic_state *state;
   181	
   182		if (!dev)
   183			return 0;
   184	
   185		drm_kms_helper_poll_disable(dev);
 > 186		drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
   187		state = drm_atomic_helper_suspend(dev);
   188		if (IS_ERR(state)) {
   189			drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
   190			drm_kms_helper_poll_enable(dev);
   191			return PTR_ERR(state);
   192		}
   193	
   194		dev->mode_config.suspend_state = state;
   195	
   196		return 0;
   197	}
   198	EXPORT_SYMBOL(drm_mode_config_helper_suspend);
   199	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31510 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Stefan Agner]

On 2017-11-06 20:18, Noralf Trønnes wrote:
> Replace driver's code with the generic helpers that do the same thing.

Tested using:
echo devices > /sys/power/pm_test
echo freeze > /sys/power/state


Note, currently I do get, but even without this patches, so this is
something else:

[  930.992433] ------------[ cut here ]------------
[  930.992494] WARNING: CPU: 0 PID: 361 at
drivers/gpu/drm/drm_atomic_helper.c:1249
drm_atomic_helper_wait_for_vblanks.part.1+0x284/0x288
[  930.992502] [CRTC:28:crtc-0] vblank wait timed out


Tested-by: Stefan Agner <stefan@agner.ch>
Acked-by: Stefan Agner <stefan@agner.ch>

Will you take the patch through drm-misc?

--
Stefan



> 
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Alison Wang <alison.wang@freescale.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 ++++++------------------
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
>  2 files changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> index 58e9e0601a61..1a9ee657bbac 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -27,6 +27,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_cma_helper.h>
>  #include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_modeset_helper.h>
>  
>  #include "fsl_dcu_drm_crtc.h"
>  #include "fsl_dcu_drm_drv.h"
> @@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
>  static int fsl_dcu_drm_pm_suspend(struct device *dev)
>  {
>  	struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
> +	int ret;
>  
>  	if (!fsl_dev)
>  		return 0;
>  
>  	disable_irq(fsl_dev->irq);
> -	drm_kms_helper_poll_disable(fsl_dev->drm);
>  
> -	console_lock();
> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
> -	console_unlock();
> -
> -	fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
> -	if (IS_ERR(fsl_dev->state)) {
> -		console_lock();
> -		drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
> -		console_unlock();
> -
> -		drm_kms_helper_poll_enable(fsl_dev->drm);
> +	ret = drm_mode_config_helper_suspend(fsl_dev->drm);
> +	if (ret) {
>  		enable_irq(fsl_dev->irq);
> -		return PTR_ERR(fsl_dev->state);
> +		return ret;
>  	}
>  
>  	clk_disable_unprepare(fsl_dev->pix_clk);
> @@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
>  	if (fsl_dev->tcon)
>  		fsl_tcon_bypass_enable(fsl_dev->tcon);
>  	fsl_dcu_drm_init_planes(fsl_dev->drm);
> -	drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>  
> -	console_lock();
> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
> -	console_unlock();
> +	drm_mode_config_helper_resume(fsl_dev->drm);
>  
> -	drm_kms_helper_poll_enable(fsl_dev->drm);
>  	enable_irq(fsl_dev->irq);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> index da9bfd432ca6..93bfb98012d4 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> @@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
>  	struct drm_encoder encoder;
>  	struct fsl_dcu_drm_connector connector;
>  	const struct fsl_dcu_soc_data *soc;
> -	struct drm_atomic_state *state;
>  };
>  
>  int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Noralf Trønnes]

Den 09.11.2017 15.34, skrev Stefan Agner:
> On 2017-11-06 20:18, Noralf Trønnes wrote:
>> Replace driver's code with the generic helpers that do the same thing.
> Tested using:
> echo devices > /sys/power/pm_test
> echo freeze > /sys/power/state
>
>
> Note, currently I do get, but even without this patches, so this is
> something else:
>
> [  930.992433] ------------[ cut here ]------------
> [  930.992494] WARNING: CPU: 0 PID: 361 at
> drivers/gpu/drm/drm_atomic_helper.c:1249
> drm_atomic_helper_wait_for_vblanks.part.1+0x284/0x288
> [  930.992502] [CRTC:28:crtc-0] vblank wait timed out
>
>
> Tested-by: Stefan Agner <stefan@agner.ch>
> Acked-by: Stefan Agner <stefan@agner.ch>
>
> Will you take the patch through drm-misc?

Yes if that's fine with you, thanks for testing.

Noralf.

> --
> Stefan
>
>
>
>> Cc: Stefan Agner <stefan@agner.ch>
>> Cc: Alison Wang <alison.wang@freescale.com>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 ++++++------------------
>>   drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
>>   2 files changed, 6 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>> index 58e9e0601a61..1a9ee657bbac 100644
>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>> @@ -27,6 +27,7 @@
>>   #include <drm/drm_crtc_helper.h>
>>   #include <drm/drm_fb_cma_helper.h>
>>   #include <drm/drm_gem_cma_helper.h>
>> +#include <drm/drm_modeset_helper.h>
>>   
>>   #include "fsl_dcu_drm_crtc.h"
>>   #include "fsl_dcu_drm_drv.h"
>> @@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
>>   static int fsl_dcu_drm_pm_suspend(struct device *dev)
>>   {
>>   	struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>> +	int ret;
>>   
>>   	if (!fsl_dev)
>>   		return 0;
>>   
>>   	disable_irq(fsl_dev->irq);
>> -	drm_kms_helper_poll_disable(fsl_dev->drm);
>>   
>> -	console_lock();
>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>> -	console_unlock();
>> -
>> -	fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>> -	if (IS_ERR(fsl_dev->state)) {
>> -		console_lock();
>> -		drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>> -		console_unlock();
>> -
>> -		drm_kms_helper_poll_enable(fsl_dev->drm);
>> +	ret = drm_mode_config_helper_suspend(fsl_dev->drm);
>> +	if (ret) {
>>   		enable_irq(fsl_dev->irq);
>> -		return PTR_ERR(fsl_dev->state);
>> +		return ret;
>>   	}
>>   
>>   	clk_disable_unprepare(fsl_dev->pix_clk);
>> @@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
>>   	if (fsl_dev->tcon)
>>   		fsl_tcon_bypass_enable(fsl_dev->tcon);
>>   	fsl_dcu_drm_init_planes(fsl_dev->drm);
>> -	drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>   
>> -	console_lock();
>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>> -	console_unlock();
>> +	drm_mode_config_helper_resume(fsl_dev->drm);
>>   
>> -	drm_kms_helper_poll_enable(fsl_dev->drm);
>>   	enable_irq(fsl_dev->irq);
>>   
>>   	return 0;
>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>> index da9bfd432ca6..93bfb98012d4 100644
>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>> @@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
>>   	struct drm_encoder encoder;
>>   	struct fsl_dcu_drm_connector connector;
>>   	const struct fsl_dcu_soc_data *soc;
>> -	struct drm_atomic_state *state;
>>   };
>>   
>>   int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Stefan Agner]

On 2017-11-09 17:49, Noralf Trønnes wrote:
> Den 09.11.2017 15.34, skrev Stefan Agner:
>> On 2017-11-06 20:18, Noralf Trønnes wrote:
>>> Replace driver's code with the generic helpers that do the same thing.
>> Tested using:
>> echo devices > /sys/power/pm_test
>> echo freeze > /sys/power/state
>>
>>
>> Note, currently I do get, but even without this patches, so this is
>> something else:
>>
>> [  930.992433] ------------[ cut here ]------------
>> [  930.992494] WARNING: CPU: 0 PID: 361 at
>> drivers/gpu/drm/drm_atomic_helper.c:1249
>> drm_atomic_helper_wait_for_vblanks.part.1+0x284/0x288
>> [  930.992502] [CRTC:28:crtc-0] vblank wait timed out
>>
>>
>> Tested-by: Stefan Agner <stefan@agner.ch>
>> Acked-by: Stefan Agner <stefan@agner.ch>
>>
>> Will you take the patch through drm-misc?
> 
> Yes if that's fine with you, thanks for testing.

Yes, fine for me!

Thanks,
Stefan

> 
> Noralf.
> 
>> --
>> Stefan
>>
>>
>>
>>> Cc: Stefan Agner <stefan@agner.ch>
>>> Cc: Alison Wang <alison.wang@freescale.com>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> ---
>>>   drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 ++++++------------------
>>>   drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
>>>   2 files changed, 6 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>> index 58e9e0601a61..1a9ee657bbac 100644
>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>> @@ -27,6 +27,7 @@
>>>   #include <drm/drm_crtc_helper.h>
>>>   #include <drm/drm_fb_cma_helper.h>
>>>   #include <drm/drm_gem_cma_helper.h>
>>> +#include <drm/drm_modeset_helper.h>
>>>     #include "fsl_dcu_drm_crtc.h"
>>>   #include "fsl_dcu_drm_drv.h"
>>> @@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
>>>   static int fsl_dcu_drm_pm_suspend(struct device *dev)
>>>   {
>>>   	struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>> +	int ret;
>>>     	if (!fsl_dev)
>>>   		return 0;
>>>     	disable_irq(fsl_dev->irq);
>>> -	drm_kms_helper_poll_disable(fsl_dev->drm);
>>>   -	console_lock();
>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>>> -	console_unlock();
>>> -
>>> -	fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>>> -	if (IS_ERR(fsl_dev->state)) {
>>> -		console_lock();
>>> -		drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>> -		console_unlock();
>>> -
>>> -		drm_kms_helper_poll_enable(fsl_dev->drm);
>>> +	ret = drm_mode_config_helper_suspend(fsl_dev->drm);
>>> +	if (ret) {
>>>   		enable_irq(fsl_dev->irq);
>>> -		return PTR_ERR(fsl_dev->state);
>>> +		return ret;
>>>   	}
>>>     	clk_disable_unprepare(fsl_dev->pix_clk);
>>> @@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
>>>   	if (fsl_dev->tcon)
>>>   		fsl_tcon_bypass_enable(fsl_dev->tcon);
>>>   	fsl_dcu_drm_init_planes(fsl_dev->drm);
>>> -	drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>>   -	console_lock();
>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>> -	console_unlock();
>>> +	drm_mode_config_helper_resume(fsl_dev->drm);
>>>   -	drm_kms_helper_poll_enable(fsl_dev->drm);
>>>   	enable_irq(fsl_dev->irq);
>>>     	return 0;
>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>> index da9bfd432ca6..93bfb98012d4 100644
>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>> @@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
>>>   	struct drm_encoder encoder;
>>>   	struct fsl_dcu_drm_connector connector;
>>>   	const struct fsl_dcu_soc_data *soc;
>>> -	struct drm_atomic_state *state;
>>>   };
>>>     int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Stefan Agner]

On 2017-11-09 17:49, Noralf Trønnes wrote:
> Den 09.11.2017 15.34, skrev Stefan Agner:
>> On 2017-11-06 20:18, Noralf Trønnes wrote:
>>> Replace driver's code with the generic helpers that do the same thing.
>> Tested using:
>> echo devices > /sys/power/pm_test
>> echo freeze > /sys/power/state
>>
>>
>> Note, currently I do get, but even without this patches, so this is
>> something else:
>>
>> [  930.992433] ------------[ cut here ]------------
>> [  930.992494] WARNING: CPU: 0 PID: 361 at
>> drivers/gpu/drm/drm_atomic_helper.c:1249
>> drm_atomic_helper_wait_for_vblanks.part.1+0x284/0x288
>> [  930.992502] [CRTC:28:crtc-0] vblank wait timed out

I resolved that issue and another related to suspend/resume for the DCU
driver:
https://patchwork.freedesktop.org/series/33616/

Suspend/resume is not supported for the platform (Vybrid) I usually test
on, so that is why I did not catch this earlier.

This two patches are now on-top of your changes. How can we make sure
this goes through smoothly? For which merge window are you targeting
your changes? 

--
Stefan

>>
>>
>> Tested-by: Stefan Agner <stefan@agner.ch>
>> Acked-by: Stefan Agner <stefan@agner.ch>
>>
>> Will you take the patch through drm-misc?
> 
> Yes if that's fine with you, thanks for testing.
> 
> Noralf.
> 
>> --
>> Stefan
>>
>>
>>
>>> Cc: Stefan Agner <stefan@agner.ch>
>>> Cc: Alison Wang <alison.wang@freescale.com>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> ---
>>>   drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 ++++++------------------
>>>   drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
>>>   2 files changed, 6 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>> index 58e9e0601a61..1a9ee657bbac 100644
>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>> @@ -27,6 +27,7 @@
>>>   #include <drm/drm_crtc_helper.h>
>>>   #include <drm/drm_fb_cma_helper.h>
>>>   #include <drm/drm_gem_cma_helper.h>
>>> +#include <drm/drm_modeset_helper.h>
>>>     #include "fsl_dcu_drm_crtc.h"
>>>   #include "fsl_dcu_drm_drv.h"
>>> @@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
>>>   static int fsl_dcu_drm_pm_suspend(struct device *dev)
>>>   {
>>>   	struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>> +	int ret;
>>>     	if (!fsl_dev)
>>>   		return 0;
>>>     	disable_irq(fsl_dev->irq);
>>> -	drm_kms_helper_poll_disable(fsl_dev->drm);
>>>   -	console_lock();
>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>>> -	console_unlock();
>>> -
>>> -	fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>>> -	if (IS_ERR(fsl_dev->state)) {
>>> -		console_lock();
>>> -		drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>> -		console_unlock();
>>> -
>>> -		drm_kms_helper_poll_enable(fsl_dev->drm);
>>> +	ret = drm_mode_config_helper_suspend(fsl_dev->drm);
>>> +	if (ret) {
>>>   		enable_irq(fsl_dev->irq);
>>> -		return PTR_ERR(fsl_dev->state);
>>> +		return ret;
>>>   	}
>>>     	clk_disable_unprepare(fsl_dev->pix_clk);
>>> @@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
>>>   	if (fsl_dev->tcon)
>>>   		fsl_tcon_bypass_enable(fsl_dev->tcon);
>>>   	fsl_dcu_drm_init_planes(fsl_dev->drm);
>>> -	drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>>   -	console_lock();
>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>> -	console_unlock();
>>> +	drm_mode_config_helper_resume(fsl_dev->drm);
>>>   -	drm_kms_helper_poll_enable(fsl_dev->drm);
>>>   	enable_irq(fsl_dev->irq);
>>>     	return 0;
>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>> index da9bfd432ca6..93bfb98012d4 100644
>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>> @@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
>>>   	struct drm_encoder encoder;
>>>   	struct fsl_dcu_drm_connector connector;
>>>   	const struct fsl_dcu_soc_data *soc;
>>> -	struct drm_atomic_state *state;
>>>   };
>>>     int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Noralf Trønnes]

Den 10.11.2017 17.39, skrev Stefan Agner:
> On 2017-11-09 17:49, Noralf Trønnes wrote:
>> Den 09.11.2017 15.34, skrev Stefan Agner:
>>> On 2017-11-06 20:18, Noralf Trønnes wrote:
>>>> Replace driver's code with the generic helpers that do the same thing.
>>> Tested using:
>>> echo devices > /sys/power/pm_test
>>> echo freeze > /sys/power/state
>>>
>>>
>>> Note, currently I do get, but even without this patches, so this is
>>> something else:
>>>
>>> [  930.992433] ------------[ cut here ]------------
>>> [  930.992494] WARNING: CPU: 0 PID: 361 at
>>> drivers/gpu/drm/drm_atomic_helper.c:1249
>>> drm_atomic_helper_wait_for_vblanks.part.1+0x284/0x288
>>> [  930.992502] [CRTC:28:crtc-0] vblank wait timed out
> I resolved that issue and another related to suspend/resume for the DCU
> driver:
> https://patchwork.freedesktop.org/series/33616/
>
> Suspend/resume is not supported for the platform (Vybrid) I usually test
> on, so that is why I did not catch this earlier.
>
> This two patches are now on-top of your changes. How can we make sure
> this goes through smoothly? For which merge window are you targeting
> your changes?

drm-misc is always open so I'm planning to apply it this weekend or 
maybe monday.
This means it will go into 4.16. Maybe you need to fix it before that?

Could you help me out by acking the tinydrm patch in this series?

Noralf.

> --
> Stefan
>
>>>
>>> Tested-by: Stefan Agner <stefan@agner.ch>
>>> Acked-by: Stefan Agner <stefan@agner.ch>
>>>
>>> Will you take the patch through drm-misc?
>> Yes if that's fine with you, thanks for testing.
>>
>> Noralf.
>>
>>> --
>>> Stefan
>>>
>>>
>>>
>>>> Cc: Stefan Agner <stefan@agner.ch>
>>>> Cc: Alison Wang <alison.wang@freescale.com>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>>>    drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 ++++++------------------
>>>>    drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
>>>>    2 files changed, 6 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>> index 58e9e0601a61..1a9ee657bbac 100644
>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>> @@ -27,6 +27,7 @@
>>>>    #include <drm/drm_crtc_helper.h>
>>>>    #include <drm/drm_fb_cma_helper.h>
>>>>    #include <drm/drm_gem_cma_helper.h>
>>>> +#include <drm/drm_modeset_helper.h>
>>>>      #include "fsl_dcu_drm_crtc.h"
>>>>    #include "fsl_dcu_drm_drv.h"
>>>> @@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
>>>>    static int fsl_dcu_drm_pm_suspend(struct device *dev)
>>>>    {
>>>>    	struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>>> +	int ret;
>>>>      	if (!fsl_dev)
>>>>    		return 0;
>>>>      	disable_irq(fsl_dev->irq);
>>>> -	drm_kms_helper_poll_disable(fsl_dev->drm);
>>>>    -	console_lock();
>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>>>> -	console_unlock();
>>>> -
>>>> -	fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>>>> -	if (IS_ERR(fsl_dev->state)) {
>>>> -		console_lock();
>>>> -		drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>> -		console_unlock();
>>>> -
>>>> -		drm_kms_helper_poll_enable(fsl_dev->drm);
>>>> +	ret = drm_mode_config_helper_suspend(fsl_dev->drm);
>>>> +	if (ret) {
>>>>    		enable_irq(fsl_dev->irq);
>>>> -		return PTR_ERR(fsl_dev->state);
>>>> +		return ret;
>>>>    	}
>>>>      	clk_disable_unprepare(fsl_dev->pix_clk);
>>>> @@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
>>>>    	if (fsl_dev->tcon)
>>>>    		fsl_tcon_bypass_enable(fsl_dev->tcon);
>>>>    	fsl_dcu_drm_init_planes(fsl_dev->drm);
>>>> -	drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>>>    -	console_lock();
>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>> -	console_unlock();
>>>> +	drm_mode_config_helper_resume(fsl_dev->drm);
>>>>    -	drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>    	enable_irq(fsl_dev->irq);
>>>>      	return 0;
>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>> index da9bfd432ca6..93bfb98012d4 100644
>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>> @@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
>>>>    	struct drm_encoder encoder;
>>>>    	struct fsl_dcu_drm_connector connector;
>>>>    	const struct fsl_dcu_soc_data *soc;
>>>> -	struct drm_atomic_state *state;
>>>>    };
>>>>      int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Noralf Trønnes]

Den 10.11.2017 19.06, skrev Noralf Trønnes:
>
> Den 10.11.2017 17.39, skrev Stefan Agner:
>> On 2017-11-09 17:49, Noralf Trønnes wrote:
>>> Den 09.11.2017 15.34, skrev Stefan Agner:
>>>> On 2017-11-06 20:18, Noralf Trønnes wrote:
>>>>> Replace driver's code with the generic helpers that do the same 
>>>>> thing.
>>>> Tested using:
>>>> echo devices > /sys/power/pm_test
>>>> echo freeze > /sys/power/state
>>>>
>>>>
>>>> Note, currently I do get, but even without this patches, so this is
>>>> something else:
>>>>
>>>> [  930.992433] ------------[ cut here ]------------
>>>> [  930.992494] WARNING: CPU: 0 PID: 361 at
>>>> drivers/gpu/drm/drm_atomic_helper.c:1249
>>>> drm_atomic_helper_wait_for_vblanks.part.1+0x284/0x288
>>>> [  930.992502] [CRTC:28:crtc-0] vblank wait timed out
>> I resolved that issue and another related to suspend/resume for the DCU
>> driver:
>> https://patchwork.freedesktop.org/series/33616/
>>
>> Suspend/resume is not supported for the platform (Vybrid) I usually test
>> on, so that is why I did not catch this earlier.
>>
>> This two patches are now on-top of your changes. How can we make sure
>> this goes through smoothly? For which merge window are you targeting
>> your changes?
>
> drm-misc is always open so I'm planning to apply it this weekend or 
> maybe monday.

s/drm-misc/drm-misc-next/

> This means it will go into 4.16. Maybe you need to fix it before that?
>
> Could you help me out by acking the tinydrm patch in this series?
>
> Noralf.
>
>> -- 
>> Stefan
>>
>>>>
>>>> Tested-by: Stefan Agner <stefan@agner.ch>
>>>> Acked-by: Stefan Agner <stefan@agner.ch>
>>>>
>>>> Will you take the patch through drm-misc?
>>> Yes if that's fine with you, thanks for testing.
>>>
>>> Noralf.
>>>
>>>> -- 
>>>> Stefan
>>>>
>>>>
>>>>
>>>>> Cc: Stefan Agner <stefan@agner.ch>
>>>>> Cc: Alison Wang <alison.wang@freescale.com>
>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> ---
>>>>>    drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 
>>>>> ++++++------------------
>>>>>    drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
>>>>>    2 files changed, 6 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>> index 58e9e0601a61..1a9ee657bbac 100644
>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>> @@ -27,6 +27,7 @@
>>>>>    #include <drm/drm_crtc_helper.h>
>>>>>    #include <drm/drm_fb_cma_helper.h>
>>>>>    #include <drm/drm_gem_cma_helper.h>
>>>>> +#include <drm/drm_modeset_helper.h>
>>>>>      #include "fsl_dcu_drm_crtc.h"
>>>>>    #include "fsl_dcu_drm_drv.h"
>>>>> @@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
>>>>>    static int fsl_dcu_drm_pm_suspend(struct device *dev)
>>>>>    {
>>>>>        struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>>>> +    int ret;
>>>>>          if (!fsl_dev)
>>>>>            return 0;
>>>>>          disable_irq(fsl_dev->irq);
>>>>> -    drm_kms_helper_poll_disable(fsl_dev->drm);
>>>>>    -    console_lock();
>>>>> -    drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>>>>> -    console_unlock();
>>>>> -
>>>>> -    fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>>>>> -    if (IS_ERR(fsl_dev->state)) {
>>>>> -        console_lock();
>>>>> -        drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>> -        console_unlock();
>>>>> -
>>>>> -        drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>> +    ret = drm_mode_config_helper_suspend(fsl_dev->drm);
>>>>> +    if (ret) {
>>>>>            enable_irq(fsl_dev->irq);
>>>>> -        return PTR_ERR(fsl_dev->state);
>>>>> +        return ret;
>>>>>        }
>>>>>          clk_disable_unprepare(fsl_dev->pix_clk);
>>>>> @@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct 
>>>>> device *dev)
>>>>>        if (fsl_dev->tcon)
>>>>>            fsl_tcon_bypass_enable(fsl_dev->tcon);
>>>>>        fsl_dcu_drm_init_planes(fsl_dev->drm);
>>>>> -    drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>>>>    -    console_lock();
>>>>> -    drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>> -    console_unlock();
>>>>> +    drm_mode_config_helper_resume(fsl_dev->drm);
>>>>>    -    drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>>        enable_irq(fsl_dev->irq);
>>>>>          return 0;
>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>> index da9bfd432ca6..93bfb98012d4 100644
>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>> @@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
>>>>>        struct drm_encoder encoder;
>>>>>        struct fsl_dcu_drm_connector connector;
>>>>>        const struct fsl_dcu_soc_data *soc;
>>>>> -    struct drm_atomic_state *state;
>>>>>    };
>>>>>      int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device 
>>>>> *fsl_dev);
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Stefan Agner]

On 2017-11-10 19:06, Noralf Trønnes wrote:
> Den 10.11.2017 17.39, skrev Stefan Agner:
>> On 2017-11-09 17:49, Noralf Trønnes wrote:
>>> Den 09.11.2017 15.34, skrev Stefan Agner:
>>>> On 2017-11-06 20:18, Noralf Trønnes wrote:
>>>>> Replace driver's code with the generic helpers that do the same thing.
>>>> Tested using:
>>>> echo devices > /sys/power/pm_test
>>>> echo freeze > /sys/power/state
>>>>
>>>>
>>>> Note, currently I do get, but even without this patches, so this is
>>>> something else:
>>>>
>>>> [  930.992433] ------------[ cut here ]------------
>>>> [  930.992494] WARNING: CPU: 0 PID: 361 at
>>>> drivers/gpu/drm/drm_atomic_helper.c:1249
>>>> drm_atomic_helper_wait_for_vblanks.part.1+0x284/0x288
>>>> [  930.992502] [CRTC:28:crtc-0] vblank wait timed out
>> I resolved that issue and another related to suspend/resume for the DCU
>> driver:
>> https://patchwork.freedesktop.org/series/33616/
>>
>> Suspend/resume is not supported for the platform (Vybrid) I usually test
>> on, so that is why I did not catch this earlier.
>>
>> This two patches are now on-top of your changes. How can we make sure
>> this goes through smoothly? For which merge window are you targeting
>> your changes?
> 
> drm-misc is always open so I'm planning to apply it this weekend or
> maybe monday.
> This means it will go into 4.16. Maybe you need to fix it before that?

The issues were already present in 4.14, so anyway to late for that. The
suspend/resume platform support for the SoC using this IP is missing in
mainline anyway, so in practice this patches are not that critical. I
probably will backport it for v4.14 since its LTS once it hits mainline.

So from my point of view, 4.16 is fine.

Should I create a separate pull request for those or can you pick them
up directly?

--
Stefan


> 
> Could you help me out by acking the tinydrm patch in this series?
> 
> Noralf.
> 
>> --
>> Stefan
>>
>>>>
>>>> Tested-by: Stefan Agner <stefan@agner.ch>
>>>> Acked-by: Stefan Agner <stefan@agner.ch>
>>>>
>>>> Will you take the patch through drm-misc?
>>> Yes if that's fine with you, thanks for testing.
>>>
>>> Noralf.
>>>
>>>> --
>>>> Stefan
>>>>
>>>>
>>>>
>>>>> Cc: Stefan Agner <stefan@agner.ch>
>>>>> Cc: Alison Wang <alison.wang@freescale.com>
>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> ---
>>>>>    drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 ++++++------------------
>>>>>    drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
>>>>>    2 files changed, 6 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>> index 58e9e0601a61..1a9ee657bbac 100644
>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>> @@ -27,6 +27,7 @@
>>>>>    #include <drm/drm_crtc_helper.h>
>>>>>    #include <drm/drm_fb_cma_helper.h>
>>>>>    #include <drm/drm_gem_cma_helper.h>
>>>>> +#include <drm/drm_modeset_helper.h>
>>>>>      #include "fsl_dcu_drm_crtc.h"
>>>>>    #include "fsl_dcu_drm_drv.h"
>>>>> @@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
>>>>>    static int fsl_dcu_drm_pm_suspend(struct device *dev)
>>>>>    {
>>>>>    	struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>>>> +	int ret;
>>>>>      	if (!fsl_dev)
>>>>>    		return 0;
>>>>>      	disable_irq(fsl_dev->irq);
>>>>> -	drm_kms_helper_poll_disable(fsl_dev->drm);
>>>>>    -	console_lock();
>>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>>>>> -	console_unlock();
>>>>> -
>>>>> -	fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>>>>> -	if (IS_ERR(fsl_dev->state)) {
>>>>> -		console_lock();
>>>>> -		drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>> -		console_unlock();
>>>>> -
>>>>> -		drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>> +	ret = drm_mode_config_helper_suspend(fsl_dev->drm);
>>>>> +	if (ret) {
>>>>>    		enable_irq(fsl_dev->irq);
>>>>> -		return PTR_ERR(fsl_dev->state);
>>>>> +		return ret;
>>>>>    	}
>>>>>      	clk_disable_unprepare(fsl_dev->pix_clk);
>>>>> @@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
>>>>>    	if (fsl_dev->tcon)
>>>>>    		fsl_tcon_bypass_enable(fsl_dev->tcon);
>>>>>    	fsl_dcu_drm_init_planes(fsl_dev->drm);
>>>>> -	drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>>>>    -	console_lock();
>>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>> -	console_unlock();
>>>>> +	drm_mode_config_helper_resume(fsl_dev->drm);
>>>>>    -	drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>>    	enable_irq(fsl_dev->irq);
>>>>>      	return 0;
>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>> index da9bfd432ca6..93bfb98012d4 100644
>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>> @@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
>>>>>    	struct drm_encoder encoder;
>>>>>    	struct fsl_dcu_drm_connector connector;
>>>>>    	const struct fsl_dcu_soc_data *soc;
>>>>> -	struct drm_atomic_state *state;
>>>>>    };
>>>>>      int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/6] drm/tinydrm: Use drm_mode_config_helper_suspend/resume()

[Stefan Agner]

On 2017-11-06 20:18, Noralf Trønnes wrote:
> Replace driver's code with the generic helpers that do the same thing.
> Remove todo entry.

This patch looks good to me:
Reviewed-by: Stefan Agner <stefan@agner.ch>

One question below though:

> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  Documentation/gpu/todo.rst                  |  5 ---
>  drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 67 -----------------------------
>  drivers/gpu/drm/tinydrm/mi0283qt.c          |  7 ++-
>  include/drm/tinydrm/tinydrm.h               |  4 --
>  4 files changed, 5 insertions(+), 78 deletions(-)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index e9840d693a86..a44f379d2b25 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -404,11 +404,6 @@ those drivers as simple as possible, so lots of
> room for refactoring:
>    a drm_device wrong. Doesn't matter, since everyone else gets it wrong
>    too :-)
>  
> -- With the fbdev pointer in dev->mode_config we could also make
> -  suspend/resume helpers entirely generic, at least if we add a
> -  dev->mode_config.suspend_state. We could even provide a generic pm_ops
> -  structure with those.
> -
>  - also rework the drm_framebuffer_funcs->dirty hook wire-up, see above.
>  
>  Contact: Noralf Trønnes, Daniel Vetter
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> index 1a8a57cad431..bd7b82824a34 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
> @@ -292,71 +292,4 @@ void tinydrm_shutdown(struct tinydrm_device *tdev)
>  }
>  EXPORT_SYMBOL(tinydrm_shutdown);
>  
> -/**
> - * tinydrm_suspend - Suspend tinydrm
> - * @tdev: tinydrm device
> - *
> - * Used in driver PM operations to suspend tinydrm.
> - * Suspends fbdev and DRM.
> - * Resume with tinydrm_resume().
> - *
> - * Returns:
> - * Zero on success, negative error code on failure.
> - */
> -int tinydrm_suspend(struct tinydrm_device *tdev)
> -{
> -	struct drm_atomic_state *state;
> -
> -	if (tdev->suspend_state) {
> -		DRM_ERROR("Failed to suspend: state already set\n");
> -		return -EINVAL;
> -	}

Here you used to check whether suspend_state is already set. However, in
drm_mode_config_helper_suspend you don't (while you still do in
drm_mode_config_helper_resume). I think we should be consistent (check
in suspend and resume or in nono of those).

--
Stefan

> -
> -	drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 1);
> -	state = drm_atomic_helper_suspend(tdev->drm);
> -	if (IS_ERR(state)) {
> -		drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
> -		return PTR_ERR(state);
> -	}
> -
> -	tdev->suspend_state = state;
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(tinydrm_suspend);
> -
> -/**
> - * tinydrm_resume - Resume tinydrm
> - * @tdev: tinydrm device
> - *
> - * Used in driver PM operations to resume tinydrm.
> - * Suspend with tinydrm_suspend().
> - *
> - * Returns:
> - * Zero on success, negative error code on failure.
> - */
> -int tinydrm_resume(struct tinydrm_device *tdev)
> -{
> -	struct drm_atomic_state *state = tdev->suspend_state;
> -	int ret;
> -
> -	if (!state) {
> -		DRM_ERROR("Failed to resume: state is not set\n");
> -		return -EINVAL;
> -	}
> -
> -	tdev->suspend_state = NULL;
> -
> -	ret = drm_atomic_helper_resume(tdev->drm, state);
> -	if (ret) {
> -		DRM_ERROR("Error resuming state: %d\n", ret);
> -		return ret;
> -	}
> -
> -	drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(tinydrm_resume);
> -
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
> b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 6a83b3093254..70ae4f76f455 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -9,6 +9,7 @@
>   * (at your option) any later version.
>   */
>  
> +#include <drm/drm_modeset_helper.h>
>  #include <drm/tinydrm/ili9341.h>
>  #include <drm/tinydrm/mipi-dbi.h>
>  #include <drm/tinydrm/tinydrm-helpers.h>
> @@ -231,7 +232,7 @@ static int __maybe_unused
> mi0283qt_pm_suspend(struct device *dev)
>  	struct mipi_dbi *mipi = dev_get_drvdata(dev);
>  	int ret;
>  
> -	ret = tinydrm_suspend(&mipi->tinydrm);
> +	ret = drm_mode_config_helper_suspend(mipi->tinydrm.drm);
>  	if (ret)
>  		return ret;
>  
> @@ -249,7 +250,9 @@ static int __maybe_unused
> mi0283qt_pm_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	return tinydrm_resume(&mipi->tinydrm);
> +	drm_mode_config_helper_resume(mipi->tinydrm.drm);
> +
> +	return 0;
>  }
>  
>  static const struct dev_pm_ops mi0283qt_pm_ops = {
> diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
> index 4774fe3d4273..fb0d86600ea3 100644
> --- a/include/drm/tinydrm/tinydrm.h
> +++ b/include/drm/tinydrm/tinydrm.h
> @@ -20,7 +20,6 @@
>   * @pipe: Display pipe structure
>   * @dirty_lock: Serializes framebuffer flushing
>   * @fbdev_cma: CMA fbdev structure
> - * @suspend_state: Atomic state when suspended
>   * @fb_funcs: Framebuffer functions used when creating framebuffers
>   */
>  struct tinydrm_device {
> @@ -28,7 +27,6 @@ struct tinydrm_device {
>  	struct drm_simple_display_pipe pipe;
>  	struct mutex dirty_lock;
>  	struct drm_fbdev_cma *fbdev_cma;
> -	struct drm_atomic_state *suspend_state;
>  	const struct drm_framebuffer_funcs *fb_funcs;
>  };
>  
> @@ -92,8 +90,6 @@ int devm_tinydrm_init(struct device *parent, struct
> tinydrm_device *tdev,
>  		      struct drm_driver *driver);
>  int devm_tinydrm_register(struct tinydrm_device *tdev);
>  void tinydrm_shutdown(struct tinydrm_device *tdev);
> -int tinydrm_suspend(struct tinydrm_device *tdev);
> -int tinydrm_resume(struct tinydrm_device *tdev);
>  
>  void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
>  				 struct drm_plane_state *old_state);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Noralf Trønnes]

Den 14.11.2017 16.23, skrev Stefan Agner:
> On 2017-11-10 19:06, Noralf Trønnes wrote:
>> Den 10.11.2017 17.39, skrev Stefan Agner:
>>> On 2017-11-09 17:49, Noralf Trønnes wrote:
>>>> Den 09.11.2017 15.34, skrev Stefan Agner:
>>>>> On 2017-11-06 20:18, Noralf Trønnes wrote:
>>>>>> Replace driver's code with the generic helpers that do the same thing.
>>>>> Tested using:
>>>>> echo devices > /sys/power/pm_test
>>>>> echo freeze > /sys/power/state
>>>>>
>>>>>
>>>>> Note, currently I do get, but even without this patches, so this is
>>>>> something else:
>>>>>
>>>>> [  930.992433] ------------[ cut here ]------------
>>>>> [  930.992494] WARNING: CPU: 0 PID: 361 at
>>>>> drivers/gpu/drm/drm_atomic_helper.c:1249
>>>>> drm_atomic_helper_wait_for_vblanks.part.1+0x284/0x288
>>>>> [  930.992502] [CRTC:28:crtc-0] vblank wait timed out
>>> I resolved that issue and another related to suspend/resume for the DCU
>>> driver:
>>> https://patchwork.freedesktop.org/series/33616/
>>>
>>> Suspend/resume is not supported for the platform (Vybrid) I usually test
>>> on, so that is why I did not catch this earlier.
>>>
>>> This two patches are now on-top of your changes. How can we make sure
>>> this goes through smoothly? For which merge window are you targeting
>>> your changes?
>> drm-misc is always open so I'm planning to apply it this weekend or
>> maybe monday.
>> This means it will go into 4.16. Maybe you need to fix it before that?
> The issues were already present in 4.14, so anyway to late for that. The
> suspend/resume platform support for the SoC using this IP is missing in
> mainline anyway, so in practice this patches are not that critical. I
> probably will backport it for v4.14 since its LTS once it hits mainline.

In that case shouldn't you fix that issue first with fixes tag and cc 
stable?
Then I can rebase this patch on top of that. This way we avoid merge 
conflict
when your fix is backported to 4.14 and 4.15.

Or have I missed something here?

Noralf.


> So from my point of view, 4.16 is fine.
>
> Should I create a separate pull request for those or can you pick them
> up directly?
>
> --
> Stefan
>
>
>> Could you help me out by acking the tinydrm patch in this series?
>>
>> Noralf.
>>
>>> --
>>> Stefan
>>>
>>>>> Tested-by: Stefan Agner <stefan@agner.ch>
>>>>> Acked-by: Stefan Agner <stefan@agner.ch>
>>>>>
>>>>> Will you take the patch through drm-misc?
>>>> Yes if that's fine with you, thanks for testing.
>>>>
>>>> Noralf.
>>>>
>>>>> --
>>>>> Stefan
>>>>>
>>>>>
>>>>>
>>>>>> Cc: Stefan Agner <stefan@agner.ch>
>>>>>> Cc: Alison Wang <alison.wang@freescale.com>
>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>> ---
>>>>>>     drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 ++++++------------------
>>>>>>     drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
>>>>>>     2 files changed, 6 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>> index 58e9e0601a61..1a9ee657bbac 100644
>>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>> @@ -27,6 +27,7 @@
>>>>>>     #include <drm/drm_crtc_helper.h>
>>>>>>     #include <drm/drm_fb_cma_helper.h>
>>>>>>     #include <drm/drm_gem_cma_helper.h>
>>>>>> +#include <drm/drm_modeset_helper.h>
>>>>>>       #include "fsl_dcu_drm_crtc.h"
>>>>>>     #include "fsl_dcu_drm_drv.h"
>>>>>> @@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
>>>>>>     static int fsl_dcu_drm_pm_suspend(struct device *dev)
>>>>>>     {
>>>>>>     	struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>>>>> +	int ret;
>>>>>>       	if (!fsl_dev)
>>>>>>     		return 0;
>>>>>>       	disable_irq(fsl_dev->irq);
>>>>>> -	drm_kms_helper_poll_disable(fsl_dev->drm);
>>>>>>     -	console_lock();
>>>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>>>>>> -	console_unlock();
>>>>>> -
>>>>>> -	fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>>>>>> -	if (IS_ERR(fsl_dev->state)) {
>>>>>> -		console_lock();
>>>>>> -		drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>>> -		console_unlock();
>>>>>> -
>>>>>> -		drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>>> +	ret = drm_mode_config_helper_suspend(fsl_dev->drm);
>>>>>> +	if (ret) {
>>>>>>     		enable_irq(fsl_dev->irq);
>>>>>> -		return PTR_ERR(fsl_dev->state);
>>>>>> +		return ret;
>>>>>>     	}
>>>>>>       	clk_disable_unprepare(fsl_dev->pix_clk);
>>>>>> @@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
>>>>>>     	if (fsl_dev->tcon)
>>>>>>     		fsl_tcon_bypass_enable(fsl_dev->tcon);
>>>>>>     	fsl_dcu_drm_init_planes(fsl_dev->drm);
>>>>>> -	drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>>>>>     -	console_lock();
>>>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>>> -	console_unlock();
>>>>>> +	drm_mode_config_helper_resume(fsl_dev->drm);
>>>>>>     -	drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>>>     	enable_irq(fsl_dev->irq);
>>>>>>       	return 0;
>>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>> index da9bfd432ca6..93bfb98012d4 100644
>>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>> @@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
>>>>>>     	struct drm_encoder encoder;
>>>>>>     	struct fsl_dcu_drm_connector connector;
>>>>>>     	const struct fsl_dcu_soc_data *soc;
>>>>>> -	struct drm_atomic_state *state;
>>>>>>     };
>>>>>>       int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Stefan Agner]

On 2017-11-14 16:40, Noralf Trønnes wrote:
> Den 14.11.2017 16.23, skrev Stefan Agner:
>> On 2017-11-10 19:06, Noralf Trønnes wrote:
>>> Den 10.11.2017 17.39, skrev Stefan Agner:
>>>> On 2017-11-09 17:49, Noralf Trønnes wrote:
>>>>> Den 09.11.2017 15.34, skrev Stefan Agner:
>>>>>> On 2017-11-06 20:18, Noralf Trønnes wrote:
>>>>>>> Replace driver's code with the generic helpers that do the same thing.
>>>>>> Tested using:
>>>>>> echo devices > /sys/power/pm_test
>>>>>> echo freeze > /sys/power/state
>>>>>>
>>>>>>
>>>>>> Note, currently I do get, but even without this patches, so this is
>>>>>> something else:
>>>>>>
>>>>>> [  930.992433] ------------[ cut here ]------------
>>>>>> [  930.992494] WARNING: CPU: 0 PID: 361 at
>>>>>> drivers/gpu/drm/drm_atomic_helper.c:1249
>>>>>> drm_atomic_helper_wait_for_vblanks.part.1+0x284/0x288
>>>>>> [  930.992502] [CRTC:28:crtc-0] vblank wait timed out
>>>> I resolved that issue and another related to suspend/resume for the DCU
>>>> driver:
>>>> https://patchwork.freedesktop.org/series/33616/
>>>>
>>>> Suspend/resume is not supported for the platform (Vybrid) I usually test
>>>> on, so that is why I did not catch this earlier.
>>>>
>>>> This two patches are now on-top of your changes. How can we make sure
>>>> this goes through smoothly? For which merge window are you targeting
>>>> your changes?
>>> drm-misc is always open so I'm planning to apply it this weekend or
>>> maybe monday.
>>> This means it will go into 4.16. Maybe you need to fix it before that?
>> The issues were already present in 4.14, so anyway to late for that. The
>> suspend/resume platform support for the SoC using this IP is missing in
>> mainline anyway, so in practice this patches are not that critical. I
>> probably will backport it for v4.14 since its LTS once it hits mainline.
> 
> In that case shouldn't you fix that issue first with fixes tag and cc stable?
> Then I can rebase this patch on top of that. This way we avoid merge conflict
> when your fix is backported to 4.14 and 4.15.
> 
> Or have I missed something here?

That is probably the right approach. Whatever causes the least amount of
friction.

Try to understand how to go about that:
https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-misc.html#where-do-i-apply-my-patch

Release already happened, but rc1 not yet, so I guess I should base on
drm-misc-next-fixes now, right?

So I prepare a drm-fsl-dcu-next fixes-forbranch, based on
drm-misc-next-fixes and send it to Dave.

Rebasing once it is merged rebasing should rather trivial.

--
Stefan


> 
> Noralf.
> 
> 
>> So from my point of view, 4.16 is fine.
>>
>> Should I create a separate pull request for those or can you pick them
>> up directly?
>>
>> --
>> Stefan
>>
>>
>>> Could you help me out by acking the tinydrm patch in this series?
>>>
>>> Noralf.
>>>
>>>> --
>>>> Stefan
>>>>
>>>>>> Tested-by: Stefan Agner <stefan@agner.ch>
>>>>>> Acked-by: Stefan Agner <stefan@agner.ch>
>>>>>>
>>>>>> Will you take the patch through drm-misc?
>>>>> Yes if that's fine with you, thanks for testing.
>>>>>
>>>>> Noralf.
>>>>>
>>>>>> --
>>>>>> Stefan
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Cc: Stefan Agner <stefan@agner.ch>
>>>>>>> Cc: Alison Wang <alison.wang@freescale.com>
>>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 ++++++------------------
>>>>>>>     drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
>>>>>>>     2 files changed, 6 insertions(+), 19 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>> index 58e9e0601a61..1a9ee657bbac 100644
>>>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>> @@ -27,6 +27,7 @@
>>>>>>>     #include <drm/drm_crtc_helper.h>
>>>>>>>     #include <drm/drm_fb_cma_helper.h>
>>>>>>>     #include <drm/drm_gem_cma_helper.h>
>>>>>>> +#include <drm/drm_modeset_helper.h>
>>>>>>>       #include "fsl_dcu_drm_crtc.h"
>>>>>>>     #include "fsl_dcu_drm_drv.h"
>>>>>>> @@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
>>>>>>>     static int fsl_dcu_drm_pm_suspend(struct device *dev)
>>>>>>>     {
>>>>>>>     	struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>>>>>> +	int ret;
>>>>>>>       	if (!fsl_dev)
>>>>>>>     		return 0;
>>>>>>>       	disable_irq(fsl_dev->irq);
>>>>>>> -	drm_kms_helper_poll_disable(fsl_dev->drm);
>>>>>>>     -	console_lock();
>>>>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>>>>>>> -	console_unlock();
>>>>>>> -
>>>>>>> -	fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>>>>>>> -	if (IS_ERR(fsl_dev->state)) {
>>>>>>> -		console_lock();
>>>>>>> -		drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>>>> -		console_unlock();
>>>>>>> -
>>>>>>> -		drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>>>> +	ret = drm_mode_config_helper_suspend(fsl_dev->drm);
>>>>>>> +	if (ret) {
>>>>>>>     		enable_irq(fsl_dev->irq);
>>>>>>> -		return PTR_ERR(fsl_dev->state);
>>>>>>> +		return ret;
>>>>>>>     	}
>>>>>>>       	clk_disable_unprepare(fsl_dev->pix_clk);
>>>>>>> @@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
>>>>>>>     	if (fsl_dev->tcon)
>>>>>>>     		fsl_tcon_bypass_enable(fsl_dev->tcon);
>>>>>>>     	fsl_dcu_drm_init_planes(fsl_dev->drm);
>>>>>>> -	drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>>>>>>     -	console_lock();
>>>>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>>>> -	console_unlock();
>>>>>>> +	drm_mode_config_helper_resume(fsl_dev->drm);
>>>>>>>     -	drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>>>>     	enable_irq(fsl_dev->irq);
>>>>>>>       	return 0;
>>>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>> index da9bfd432ca6..93bfb98012d4 100644
>>>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>> @@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
>>>>>>>     	struct drm_encoder encoder;
>>>>>>>     	struct fsl_dcu_drm_connector connector;
>>>>>>>     	const struct fsl_dcu_soc_data *soc;
>>>>>>> -	struct drm_atomic_state *state;
>>>>>>>     };
>>>>>>>       int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/6] drm/tinydrm: Use drm_mode_config_helper_suspend/resume()

[Noralf Trønnes]

Den 14.11.2017 16.40, skrev Stefan Agner:
> On 2017-11-06 20:18, Noralf Trønnes wrote:
>> Replace driver's code with the generic helpers that do the same thing.
>> Remove todo entry.
> This patch looks good to me:
> Reviewed-by: Stefan Agner <stefan@agner.ch>

Thanks!

> One question below though:
>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   Documentation/gpu/todo.rst                  |  5 ---
>>   drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 67 -----------------------------
>>   drivers/gpu/drm/tinydrm/mi0283qt.c          |  7 ++-
>>   include/drm/tinydrm/tinydrm.h               |  4 --
>>   4 files changed, 5 insertions(+), 78 deletions(-)
>>
>> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
>> index e9840d693a86..a44f379d2b25 100644
>> --- a/Documentation/gpu/todo.rst
>> +++ b/Documentation/gpu/todo.rst
>> @@ -404,11 +404,6 @@ those drivers as simple as possible, so lots of
>> room for refactoring:
>>     a drm_device wrong. Doesn't matter, since everyone else gets it wrong
>>     too :-)
>>   
>> -- With the fbdev pointer in dev->mode_config we could also make
>> -  suspend/resume helpers entirely generic, at least if we add a
>> -  dev->mode_config.suspend_state. We could even provide a generic pm_ops
>> -  structure with those.
>> -
>>   - also rework the drm_framebuffer_funcs->dirty hook wire-up, see above.
>>   
>>   Contact: Noralf Trønnes, Daniel Vetter
>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>> b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>> index 1a8a57cad431..bd7b82824a34 100644
>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c
>> @@ -292,71 +292,4 @@ void tinydrm_shutdown(struct tinydrm_device *tdev)
>>   }
>>   EXPORT_SYMBOL(tinydrm_shutdown);
>>   
>> -/**
>> - * tinydrm_suspend - Suspend tinydrm
>> - * @tdev: tinydrm device
>> - *
>> - * Used in driver PM operations to suspend tinydrm.
>> - * Suspends fbdev and DRM.
>> - * Resume with tinydrm_resume().
>> - *
>> - * Returns:
>> - * Zero on success, negative error code on failure.
>> - */
>> -int tinydrm_suspend(struct tinydrm_device *tdev)
>> -{
>> -	struct drm_atomic_state *state;
>> -
>> -	if (tdev->suspend_state) {
>> -		DRM_ERROR("Failed to suspend: state already set\n");
>> -		return -EINVAL;
>> -	}
> Here you used to check whether suspend_state is already set. However, in
> drm_mode_config_helper_suspend you don't (while you still do in
> drm_mode_config_helper_resume). I think we should be consistent (check
> in suspend and resume or in nono of those).

Indeed I missed out on that one. I think the checks are nice to have
in a helper since they catch unbalanced suspend/resume and are cheap.
I'll make a new version. Thanks.

Noralf.


> --
> Stefan
>
>> -
>> -	drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 1);
>> -	state = drm_atomic_helper_suspend(tdev->drm);
>> -	if (IS_ERR(state)) {
>> -		drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
>> -		return PTR_ERR(state);
>> -	}
>> -
>> -	tdev->suspend_state = state;
>> -
>> -	return 0;
>> -}
>> -EXPORT_SYMBOL(tinydrm_suspend);
>> -
>> -/**
>> - * tinydrm_resume - Resume tinydrm
>> - * @tdev: tinydrm device
>> - *
>> - * Used in driver PM operations to resume tinydrm.
>> - * Suspend with tinydrm_suspend().
>> - *
>> - * Returns:
>> - * Zero on success, negative error code on failure.
>> - */
>> -int tinydrm_resume(struct tinydrm_device *tdev)
>> -{
>> -	struct drm_atomic_state *state = tdev->suspend_state;
>> -	int ret;
>> -
>> -	if (!state) {
>> -		DRM_ERROR("Failed to resume: state is not set\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	tdev->suspend_state = NULL;
>> -
>> -	ret = drm_atomic_helper_resume(tdev->drm, state);
>> -	if (ret) {
>> -		DRM_ERROR("Error resuming state: %d\n", ret);
>> -		return ret;
>> -	}
>> -
>> -	drm_fbdev_cma_set_suspend_unlocked(tdev->fbdev_cma, 0);
>> -
>> -	return 0;
>> -}
>> -EXPORT_SYMBOL(tinydrm_resume);
>> -
>>   MODULE_LICENSE("GPL");
>> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> index 6a83b3093254..70ae4f76f455 100644
>> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
>> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
>> @@ -9,6 +9,7 @@
>>    * (at your option) any later version.
>>    */
>>   
>> +#include <drm/drm_modeset_helper.h>
>>   #include <drm/tinydrm/ili9341.h>
>>   #include <drm/tinydrm/mipi-dbi.h>
>>   #include <drm/tinydrm/tinydrm-helpers.h>
>> @@ -231,7 +232,7 @@ static int __maybe_unused
>> mi0283qt_pm_suspend(struct device *dev)
>>   	struct mipi_dbi *mipi = dev_get_drvdata(dev);
>>   	int ret;
>>   
>> -	ret = tinydrm_suspend(&mipi->tinydrm);
>> +	ret = drm_mode_config_helper_suspend(mipi->tinydrm.drm);
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -249,7 +250,9 @@ static int __maybe_unused
>> mi0283qt_pm_resume(struct device *dev)
>>   	if (ret)
>>   		return ret;
>>   
>> -	return tinydrm_resume(&mipi->tinydrm);
>> +	drm_mode_config_helper_resume(mipi->tinydrm.drm);
>> +
>> +	return 0;
>>   }
>>   
>>   static const struct dev_pm_ops mi0283qt_pm_ops = {
>> diff --git a/include/drm/tinydrm/tinydrm.h b/include/drm/tinydrm/tinydrm.h
>> index 4774fe3d4273..fb0d86600ea3 100644
>> --- a/include/drm/tinydrm/tinydrm.h
>> +++ b/include/drm/tinydrm/tinydrm.h
>> @@ -20,7 +20,6 @@
>>    * @pipe: Display pipe structure
>>    * @dirty_lock: Serializes framebuffer flushing
>>    * @fbdev_cma: CMA fbdev structure
>> - * @suspend_state: Atomic state when suspended
>>    * @fb_funcs: Framebuffer functions used when creating framebuffers
>>    */
>>   struct tinydrm_device {
>> @@ -28,7 +27,6 @@ struct tinydrm_device {
>>   	struct drm_simple_display_pipe pipe;
>>   	struct mutex dirty_lock;
>>   	struct drm_fbdev_cma *fbdev_cma;
>> -	struct drm_atomic_state *suspend_state;
>>   	const struct drm_framebuffer_funcs *fb_funcs;
>>   };
>>   
>> @@ -92,8 +90,6 @@ int devm_tinydrm_init(struct device *parent, struct
>> tinydrm_device *tdev,
>>   		      struct drm_driver *driver);
>>   int devm_tinydrm_register(struct tinydrm_device *tdev);
>>   void tinydrm_shutdown(struct tinydrm_device *tdev);
>> -int tinydrm_suspend(struct tinydrm_device *tdev);
>> -int tinydrm_resume(struct tinydrm_device *tdev);
>>   
>>   void tinydrm_display_pipe_update(struct drm_simple_display_pipe *pipe,
>>   				 struct drm_plane_state *old_state);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm/fsl-dcu: Use drm_mode_config_helper_suspend/resume()

[Noralf Trønnes]

Den 14.11.2017 16.56, skrev Stefan Agner:
> On 2017-11-14 16:40, Noralf Trønnes wrote:
>> Den 14.11.2017 16.23, skrev Stefan Agner:
>>> On 2017-11-10 19:06, Noralf Trønnes wrote:
>>>> Den 10.11.2017 17.39, skrev Stefan Agner:
>>>>> On 2017-11-09 17:49, Noralf Trønnes wrote:
>>>>>> Den 09.11.2017 15.34, skrev Stefan Agner:
>>>>>>> On 2017-11-06 20:18, Noralf Trønnes wrote:
>>>>>>>> Replace driver's code with the generic helpers that do the same thing.
>>>>>>> Tested using:
>>>>>>> echo devices > /sys/power/pm_test
>>>>>>> echo freeze > /sys/power/state
>>>>>>>
>>>>>>>
>>>>>>> Note, currently I do get, but even without this patches, so this is
>>>>>>> something else:
>>>>>>>
>>>>>>> [  930.992433] ------------[ cut here ]------------
>>>>>>> [  930.992494] WARNING: CPU: 0 PID: 361 at
>>>>>>> drivers/gpu/drm/drm_atomic_helper.c:1249
>>>>>>> drm_atomic_helper_wait_for_vblanks.part.1+0x284/0x288
>>>>>>> [  930.992502] [CRTC:28:crtc-0] vblank wait timed out
>>>>> I resolved that issue and another related to suspend/resume for the DCU
>>>>> driver:
>>>>> https://patchwork.freedesktop.org/series/33616/
>>>>>
>>>>> Suspend/resume is not supported for the platform (Vybrid) I usually test
>>>>> on, so that is why I did not catch this earlier.
>>>>>
>>>>> This two patches are now on-top of your changes. How can we make sure
>>>>> this goes through smoothly? For which merge window are you targeting
>>>>> your changes?
>>>> drm-misc is always open so I'm planning to apply it this weekend or
>>>> maybe monday.
>>>> This means it will go into 4.16. Maybe you need to fix it before that?
>>> The issues were already present in 4.14, so anyway to late for that. The
>>> suspend/resume platform support for the SoC using this IP is missing in
>>> mainline anyway, so in practice this patches are not that critical. I
>>> probably will backport it for v4.14 since its LTS once it hits mainline.
>> In that case shouldn't you fix that issue first with fixes tag and cc stable?
>> Then I can rebase this patch on top of that. This way we avoid merge conflict
>> when your fix is backported to 4.14 and 4.15.
>>
>> Or have I missed something here?
> That is probably the right approach. Whatever causes the least amount of
> friction.
>
> Try to understand how to go about that:
> https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-misc.html#where-do-i-apply-my-patch
>
> Release already happened, but rc1 not yet, so I guess I should base on
> drm-misc-next-fixes now, right?

I really can't say, I've never done this before.
I suggest you ask a maintainer.
I'll respin this series with your other suggestion and drop fsl-dcu for
now until you have fixed the resume issue.

Please ping me when that is sorted and I'll send a rebased version.
No need hurry on my behalf, it doesn't block anything that can't wait a
few weeks.

Noralf.

> So I prepare a drm-fsl-dcu-next fixes-forbranch, based on
> drm-misc-next-fixes and send it to Dave.
>
> Rebasing once it is merged rebasing should rather trivial.
>
> --
> Stefan
>
>
>> Noralf.
>>
>>
>>> So from my point of view, 4.16 is fine.
>>>
>>> Should I create a separate pull request for those or can you pick them
>>> up directly?
>>>
>>> --
>>> Stefan
>>>
>>>
>>>> Could you help me out by acking the tinydrm patch in this series?
>>>>
>>>> Noralf.
>>>>
>>>>> --
>>>>> Stefan
>>>>>
>>>>>>> Tested-by: Stefan Agner <stefan@agner.ch>
>>>>>>> Acked-by: Stefan Agner <stefan@agner.ch>
>>>>>>>
>>>>>>> Will you take the patch through drm-misc?
>>>>>> Yes if that's fine with you, thanks for testing.
>>>>>>
>>>>>> Noralf.
>>>>>>
>>>>>>> --
>>>>>>> Stefan
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Cc: Stefan Agner <stefan@agner.ch>
>>>>>>>> Cc: Alison Wang <alison.wang@freescale.com>
>>>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 24 ++++++------------------
>>>>>>>>      drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h |  1 -
>>>>>>>>      2 files changed, 6 insertions(+), 19 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>>> index 58e9e0601a61..1a9ee657bbac 100644
>>>>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
>>>>>>>> @@ -27,6 +27,7 @@
>>>>>>>>      #include <drm/drm_crtc_helper.h>
>>>>>>>>      #include <drm/drm_fb_cma_helper.h>
>>>>>>>>      #include <drm/drm_gem_cma_helper.h>
>>>>>>>> +#include <drm/drm_modeset_helper.h>
>>>>>>>>        #include "fsl_dcu_drm_crtc.h"
>>>>>>>>      #include "fsl_dcu_drm_drv.h"
>>>>>>>> @@ -188,26 +189,17 @@ static struct drm_driver fsl_dcu_drm_driver = {
>>>>>>>>      static int fsl_dcu_drm_pm_suspend(struct device *dev)
>>>>>>>>      {
>>>>>>>>      	struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
>>>>>>>> +	int ret;
>>>>>>>>        	if (!fsl_dev)
>>>>>>>>      		return 0;
>>>>>>>>        	disable_irq(fsl_dev->irq);
>>>>>>>> -	drm_kms_helper_poll_disable(fsl_dev->drm);
>>>>>>>>      -	console_lock();
>>>>>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 1);
>>>>>>>> -	console_unlock();
>>>>>>>> -
>>>>>>>> -	fsl_dev->state = drm_atomic_helper_suspend(fsl_dev->drm);
>>>>>>>> -	if (IS_ERR(fsl_dev->state)) {
>>>>>>>> -		console_lock();
>>>>>>>> -		drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>>>>> -		console_unlock();
>>>>>>>> -
>>>>>>>> -		drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>>>>> +	ret = drm_mode_config_helper_suspend(fsl_dev->drm);
>>>>>>>> +	if (ret) {
>>>>>>>>      		enable_irq(fsl_dev->irq);
>>>>>>>> -		return PTR_ERR(fsl_dev->state);
>>>>>>>> +		return ret;
>>>>>>>>      	}
>>>>>>>>        	clk_disable_unprepare(fsl_dev->pix_clk);
>>>>>>>> @@ -233,13 +225,9 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
>>>>>>>>      	if (fsl_dev->tcon)
>>>>>>>>      		fsl_tcon_bypass_enable(fsl_dev->tcon);
>>>>>>>>      	fsl_dcu_drm_init_planes(fsl_dev->drm);
>>>>>>>> -	drm_atomic_helper_resume(fsl_dev->drm, fsl_dev->state);
>>>>>>>>      -	console_lock();
>>>>>>>> -	drm_fbdev_cma_set_suspend(fsl_dev->fbdev, 0);
>>>>>>>> -	console_unlock();
>>>>>>>> +	drm_mode_config_helper_resume(fsl_dev->drm);
>>>>>>>>      -	drm_kms_helper_poll_enable(fsl_dev->drm);
>>>>>>>>      	enable_irq(fsl_dev->irq);
>>>>>>>>        	return 0;
>>>>>>>> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>>> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>>> index da9bfd432ca6..93bfb98012d4 100644
>>>>>>>> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>>> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
>>>>>>>> @@ -196,7 +196,6 @@ struct fsl_dcu_drm_device {
>>>>>>>>      	struct drm_encoder encoder;
>>>>>>>>      	struct fsl_dcu_drm_connector connector;
>>>>>>>>      	const struct fsl_dcu_soc_data *soc;
>>>>>>>> -	struct drm_atomic_state *state;
>>>>>>>>      };
>>>>>>>>        int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel