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

[Stefan Agner <stefan@agner.ch>]

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