convert drm_atomic_helper_suspend/resume() to use drm_mode_config_helper_suspend/resume().
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); - drm_kms_helper_poll_enable(rcdu->ddev); - return PTR_ERR(state); - } - - rcdu->suspend_state = state; - - 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
On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder jrdr.linux@gmail.com 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. 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.
Thanks.
drm_kms_helper_poll_enable(rcdu->ddev);return PTR_ERR(state);}rcdu->suspend_state = state;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
-- 1.9.1
On Sat, Jul 28, 2018 at 11:20 PM, Vaishali Thakkar vthakkar@vaishalithakkar.in wrote:
On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder jrdr.linux@gmail.com 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().
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;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
-- 1.9.1
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
On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
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.
No, any extention of drm_fbdev_cma_set_suspend_unlocked() will not have any impact on driver because with this patch we will be retaining the original suspend/resume logic of the rcar-du driver and further this driver is not going to use drm_fbdev_cma_set_suspend_unlocked().
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().
The consumers of drm_fbdev_cma_set_suspend_unlocked() are - drivers/gpu/drm/arm/hdlcd_drv.c drivers/gpu/drm/drm_fb_cma_helper.c
and both will be converted to use API drm_mode_config_helper_suspend/resume(). As there will be no more consumer of drm_fbdev_cma_set_suspend_unlocked() , we can remove this wrapper API forever :)
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.
Sure, I will remove it in v2.
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
-- Regards,
Laurent Pinchart
Hi Souptick,
On Monday, 30 July 2018 16:58:09 EEST Souptick Joarder wrote:
On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart wrote:
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.
No, any extention of drm_fbdev_cma_set_suspend_unlocked() will not have any impact on driver because with this patch we will be retaining the original suspend/resume logic of the rcar-du driver and further this driver is not going to use drm_fbdev_cma_set_suspend_unlocked().
My point is that if the fb cma helpers gets later extended with a feature that need special handling and suspend/resume time, with the drm_fbdev_cma_set_suspend_unlocked() function properly updated to take that feature into account, driver using those helpers but converted to drm_atomic_helper_suspend/resume() will break.
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().
The consumers of drm_fbdev_cma_set_suspend_unlocked() are - drivers/gpu/drm/arm/hdlcd_drv.c drivers/gpu/drm/drm_fb_cma_helper.c
and both will be converted to use API drm_mode_config_helper_suspend/resume(). As there will be no more consumer of drm_fbdev_cma_set_suspend_unlocked() , we can remove this wrapper API forever :)
OK, if you remove the function completely then anyone wanting to extend the fbdev cma helpers in the way described above will notice that something will need to be done, so it's fine. Please thus make sure that you go all the way to removing that function.
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.
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.
Sure, I will remove it in v2.
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
On Mon, Jul 30, 2018 at 7:54 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Souptick,
On Monday, 30 July 2018 16:58:09 EEST Souptick Joarder wrote:
On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart wrote:
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.
No, any extention of drm_fbdev_cma_set_suspend_unlocked() will not have any impact on driver because with this patch we will be retaining the original suspend/resume logic of the rcar-du driver and further this driver is not going to use drm_fbdev_cma_set_suspend_unlocked().
My point is that if the fb cma helpers gets later extended with a feature that need special handling and suspend/resume time, with the drm_fbdev_cma_set_suspend_unlocked() function properly updated to take that feature into account, driver using those helpers but converted to drm_atomic_helper_suspend/resume() will break.
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().
The consumers of drm_fbdev_cma_set_suspend_unlocked() are - drivers/gpu/drm/arm/hdlcd_drv.c drivers/gpu/drm/drm_fb_cma_helper.c
and both will be converted to use API drm_mode_config_helper_suspend/resume(). As there will be no more consumer of drm_fbdev_cma_set_suspend_unlocked() , we can remove this wrapper API forever :)
OK, if you remove the function completely then anyone wanting to extend the fbdev cma helpers in the way described above will notice that something will need to be done, so it's fine. Please thus make sure that you go all the way to removing that function.
Sure, once both the drivers are converted to use drm_mode_config_helper_suspend/resume() and goes into linus's tree, then we can remove it.
But will wait for some more feedback before concluding on this.
Dave/ Daniel, Would you like to add any feedback ?
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.
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.
Sure, I will remove it in v2.
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
-- Regards,
Laurent Pinchart
Hi Souptick,
On Monday, 30 July 2018 18:13:13 EEST Souptick Joarder wrote:
On Mon, Jul 30, 2018 at 7:54 PM, Laurent Pinchart wrote:
On Monday, 30 July 2018 16:58:09 EEST Souptick Joarder wrote:
On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart wrote:
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.
No, any extention of drm_fbdev_cma_set_suspend_unlocked() will not have any impact on driver because with this patch we will be retaining the original suspend/resume logic of the rcar-du driver and further this driver is not going to use drm_fbdev_cma_set_suspend_unlocked().
My point is that if the fb cma helpers gets later extended with a feature that need special handling and suspend/resume time, with the drm_fbdev_cma_set_suspend_unlocked() function properly updated to take that feature into account, driver using those helpers but converted to drm_atomic_helper_suspend/resume() will break.
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().
The consumers of drm_fbdev_cma_set_suspend_unlocked() are - drivers/gpu/drm/arm/hdlcd_drv.c drivers/gpu/drm/drm_fb_cma_helper.c
and both will be converted to use API drm_mode_config_helper_suspend/resume(). As there will be no more consumer of drm_fbdev_cma_set_suspend_unlocked() , we can remove this wrapper API forever :)
OK, if you remove the function completely then anyone wanting to extend the fbdev cma helpers in the way described above will notice that something will need to be done, so it's fine. Please thus make sure that you go all the way to removing that function.
Sure, once both the drivers are converted to use drm_mode_config_helper_suspend/resume() and goes into linus's tree, then we can remove it.
Could we get the two driver changes and the function removal merged all together ?
But will wait for some more feedback before concluding on this.
Dave/ Daniel, Would you like to add any feedback ?
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.
> - 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.
Sure, I will remove it in v2.
> - 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
On Mon, Jul 30, 2018 at 9:15 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Souptick,
On Monday, 30 July 2018 18:13:13 EEST Souptick Joarder wrote:
On Mon, Jul 30, 2018 at 7:54 PM, Laurent Pinchart wrote:
On Monday, 30 July 2018 16:58:09 EEST Souptick Joarder wrote:
On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart wrote:
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.
No, any extention of drm_fbdev_cma_set_suspend_unlocked() will not have any impact on driver because with this patch we will be retaining the original suspend/resume logic of the rcar-du driver and further this driver is not going to use drm_fbdev_cma_set_suspend_unlocked().
My point is that if the fb cma helpers gets later extended with a feature that need special handling and suspend/resume time, with the drm_fbdev_cma_set_suspend_unlocked() function properly updated to take that feature into account, driver using those helpers but converted to drm_atomic_helper_suspend/resume() will break.
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().
The consumers of drm_fbdev_cma_set_suspend_unlocked() are - drivers/gpu/drm/arm/hdlcd_drv.c drivers/gpu/drm/drm_fb_cma_helper.c
and both will be converted to use API drm_mode_config_helper_suspend/resume(). As there will be no more consumer of drm_fbdev_cma_set_suspend_unlocked() , we can remove this wrapper API forever :)
OK, if you remove the function completely then anyone wanting to extend the fbdev cma helpers in the way described above will notice that something will need to be done, so it's fine. Please thus make sure that you go all the way to removing that function.
Sure, once both the drivers are converted to use drm_mode_config_helper_suspend/resume() and goes into linus's tree, then we can remove it.
Could we get the two driver changes and the function removal merged all together ?
Sure, I will send it.
But will wait for some more feedback before concluding on this.
Dave/ Daniel, Would you like to add any feedback ?
> 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.
>> - 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.
Sure, I will remove it in v2.
>> - 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
-- Regards,
Laurent Pinchart
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Souptick Joarder -
Vaishali Thakkar