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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
