[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 at agner.ch>

Thanks!

> One question below though:
>
>> Signed-off-by: Noralf Trønnes <noralf at 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);