convert drm_atomic_helper_suspend/resume() to use
drm_mode_config_helper_suspend/resume().
Signed-off-by: Souptick Joarder <[email protected]>
Signed-off-by: Ajit Negi <[email protected]>
---
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
--
1.9.1
On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder <[email protected]> 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 <[email protected]>
> Signed-off-by: Ajit Negi <[email protected]>
> ---
> 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
<[email protected]> wrote:
> On Sat, Jul 28, 2018 at 9:10 PM, Souptick Joarder <[email protected]> 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 <[email protected]>
>> Signed-off-by: Ajit Negi <[email protected]>
>> ---
>> 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 <[email protected]>
> >> Signed-off-by: Ajit Negi <[email protected]>
> >> ---
> >>
> >> 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
--
Regards,
Laurent Pinchart
On Sun, Jul 29, 2018 at 1:50 AM, Laurent Pinchart
<[email protected]> 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 <[email protected]>
>> >> Signed-off-by: Ajit Negi <[email protected]>
>> >> ---
>> >>
>> >> 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 <[email protected]>
> >>>> Signed-off-by: Ajit Negi <[email protected]>
> >>>> ---
> >>>>
> >>>> 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
--
Regards,
Laurent Pinchart
On Mon, Jul 30, 2018 at 7:54 PM, Laurent Pinchart
<[email protected]> 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 <[email protected]>
>> >>>> Signed-off-by: Ajit Negi <[email protected]>
>> >>>> ---
>> >>>>
>> >>>> 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 <[email protected]>
> >>>>>> Signed-off-by: Ajit Negi <[email protected]>
> >>>>>> ---
> >>>>>>
> >>>>>> 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
--
Regards,
Laurent Pinchart
On Mon, Jul 30, 2018 at 9:15 PM, Laurent Pinchart
<[email protected]> 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 <[email protected]>
>> >>>>>> Signed-off-by: Ajit Negi <[email protected]>
>> >>>>>> ---
>> >>>>>>
>> >>>>>> 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
>
>
>