Hi Souptick,
Thank you for the patch.
On Saturday, 28 July 2018 21:50:58 EEST Souptick Joarder wrote:
On Sat, Jul 28, 2018 at 11:20 PM, Vaishali Thakkar wrote:
On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder wrote:
convert drm_atomic_helper_suspend/resume() to use drm_mode_config_helper_suspend/resume().
Hi Souptick,
Thanks for your patch.
Signed-off-by: Souptick Joarder jrdr.linux@gmail.com Signed-off-by: Ajit Negi ajitn.linux@gmail.com
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 02aee6c..288220f 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -357,32 +357,15 @@ static void rcar_du_lastclose(struct drm_device *dev)
static int rcar_du_pm_suspend(struct device *dev) { struct rcar_du_device *rcdu = dev_get_drvdata(dev);
struct drm_atomic_state *state;drm_kms_helper_poll_disable(rcdu->ddev);drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, true);state = drm_atomic_helper_suspend(rcdu->ddev);if (IS_ERR(state)) {drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false);I don't think we can use drm_mode_config_helper_(suspend/resume) API here as this file uses CMA functions.
drm_fbdev_cma_set_suspend_unlocked() is wrapper function which invokes drm_fb_helper_set_suspend_unlocked().
Where the new API drm_mode_config_helper_suspend/resume() directly invokes drm_fb_helper_set_suspend_unlocked(). So it is safe to replace exiting code with API drm_mode_config_helper_suspend/resume().
I agree that they're functionally equivalent for now, but what if drm_fbdev_cma_set_suspend_unlocked() gets extended later ? This change risks introducing a breakage that could could unnoticed at that point. At the very least you should add a comment in drm_fbdev_cma_set_suspend_unlocked() to explain that any extension of the function should also address all drivers using drm_mode_config_helper_suspend() and drm_mode_config_helper_resume().
And from git grep it seems that there are very few drivers using it at the moment, so not sure if introducing new API functions similar to drm_mode_config will make sense or not.
https://www.kernel.org/doc/html/latest/gpu/todo.html
It was picked up from TODO list after discussing with Daniel.
Thanks.
drm_kms_helper_poll_enable(rcdu->ddev);return PTR_ERR(state);}rcdu->suspend_state = state;
Additionally, I think you can remove the suspend_state field from the rcdu structure.
return 0;
return drm_mode_config_helper_suspend(rcdu->ddev);}
static int rcar_du_pm_resume(struct device *dev) { struct rcar_du_device *rcdu = dev_get_drvdata(dev);
drm_atomic_helper_resume(rcdu->ddev, rcdu->suspend_state);drm_fbdev_cma_set_suspend_unlocked(rcdu->fbdev, false);drm_kms_helper_poll_enable(rcdu->ddev);return 0;
return drm_mode_config_helper_resume(rcdu->ddev);} #endif