RFC: Add drm_dev_suspend/resume() ?

[Daniel Vetter]

On Wed, Nov 01, 2017 at 01:59:00PM +0100, Noralf Trønnes wrote:
> 
> Den 01.11.2017 09.47, skrev Daniel Vetter:
> > On Tue, Oct 31, 2017 at 05:37:23PM +0100, Noralf Trønnes wrote:
> > > Den 30.10.2017 10.34, skrev Daniel Vetter:
> > > > Hi Noralf,
> > > > 
> > > > On Sun, Oct 22, 2017 at 06:52:41PM +0200, Noralf Trønnes wrote:
> > > > > Hi,
> > > > > 
> > > > > I've spent some time in the fbdev emulation code and discovered a
> > > > > recurring pattern around suspend/resume.
> > > > > Should we add some more helpers :-)
> > > > You're maybe a bit too good at spotting these for your own good :-)
> > > > 
> > > > But yeah, a "suspend for dummies" is one of the things which would be nice
> > > > I think ... Especially since we now have the atomic suspend/resume
> > > > helpers.
> > > > 
> > > > > struct drm_device {
> > > > >       /**
> > > > >        * @suspend_state:
> > > > >        *
> > > > >        * Atomic state when suspended.
> > > > >        * Set by drm_dev_suspend(), cleared by drm_dev_resume().
> > > > >        */
> > > > >       struct drm_atomic_state *suspend_state;
> > > > > };
> > > > Imo fits better when we put it into drm_mode_config.
> > > > 
> > > > > int drm_dev_suspend(struct drm_device *dev)
> > > > > {
> > > > >       struct drm_atomic_state *state;
> > > > > 
> > > > >       if (!dev)
> > > > >           return 0;
> > > > > 
> > > > >       drm_kms_helper_poll_disable(dev);
> > > > >       drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
> > > > >       state = drm_atomic_helper_suspend(dev);
> > > > >       if (IS_ERR(state)) {
> > > > >           drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 0);
> > > > >           drm_kms_helper_poll_enable(dev);
> > > > >           return PTR_ERR(state);
> > > > >       }
> > > > > 
> > > > >       dev->suspend_state = state;
> > > > > 
> > > > >       return 0;
> > > > > }
> > > > This is all about suspending the modeset side ... I'd give it a
> > > > drm_mode_config prefix instead of the drm_dev_ (that's maybe a bit too
> > > > generic), but then maybe type a general suspend/resume kernel-doc text in
> > > > the drm-internals.rst (maybe pulled in from drm_dev.c) which references
> > > > these 2 functions as the recommended way to suspend/resume the modeset
> > > > side of a driver. These won't suspend/resume a render part (if present),
> > > > so drm_dev_ seems a bit too much.
> > > I just realised that this is pulling helpers (atomic, crtc, fbdev) into
> > > the core which IIRC is something that you didn't want?
> > Ugh right. I think starting a new drm_mode_config_helper.c for top-level
> > helper stuff would be a reasonable solution. Or some other name if you
> > have a better one.
> 
> Does it fit in drm_modeset_helper.c ?
> 
> /**
>  * DOC: aux kms helpers
>  *
>  * This helper library contains various one-off functions which don't really
> fit
>  * anywhere else in the DRM modeset helper library.
>  */

Even better I think, avoids another file no one remembers exists :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch