Re: [PATCH] drm/rcar-du: Convert drm_atomic_helper_suspend/resume()

[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@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