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@xxxxxxxxx>
> >>>>>> Signed-off-by: Ajit Negi <ajitn.linux@xxxxxxxxx>
> >>>>>> ---
> >>>>>>
> >>>>>> 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
