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@xxxxxxxx>
One question below though:
>
> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
