resume()

Noralf Trønnes Tue, 14 Nov 2017 08:03:07 -0800


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 <ste...@agner.ch>


Thanks!

One question below though:

Signed-off-by: Noralf Trønnes <nor...@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 5/6] drm/tinydrm: Use drm_mode_config_helper_suspend/resume()

Reply via email to