[PATCH] imx-drm: imx-drm-core: add suspend/resume support

[Daniel Vetter]

On Thu, Jul 24, 2014 at 10:59:36PM +0800, Shawn Guo wrote:
> On Thu, Jul 24, 2014 at 12:38:59PM +0200, Daniel Vetter wrote:
> > > > > +static int imx_drm_suspend(struct device *dev)
> > > > > +{
> > > > > +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > > > +	struct drm_connector *connector;
> > > > > +
> > > > > +	drm_kms_helper_poll_disable(drm_dev);
> > > > > +
> > > > 
> > > > That's ok.
> > > > 
> > > > > +	drm_modeset_lock_all(drm_dev);
> > > > > +	list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) {
> > > > > +		if (connector->funcs->dpms)
> > > > > +			connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
> > > > > +	}
> > > > 
> > > > Don't touch DPMS state here. See below.
> > > 
> > > In which case, how else does the hardware get placed into a low power
> > > mode on suspend?
> > > 
> > > DRM has nothing provided, and this is left up to each DRM driver to
> > > implement (probably because it tends to be very driver specific.)
> > > i915 for example calls the CRTC DPMS function with DRM_MODE_DPMS_OFF.
> > > 
> > > This provides a similar mechanism, but also informs the connector, any
> > > bridge, and encoder associated with the connector as well as the CRTC
> > > to place themselves into a low power mode (which is what
> > > DRM_MODE_DPMS_OFF should be doing anyway.)
> > 
> > Well you need to call internal functions to make sure you can restore the
> > state again. Not sure any more how that all works with the crtc helpers
> > and whether they restore dpms state properly at all. i915 uses something
> > completely different nowadays.
> 
> Is it okay to do what exynos driver does, i.e. saving/restoring the dpms
> state before/after calling connector .dpms function?
> 
>         list_for_each_entry(connector, &drm_dev->mode_config.connector_list, head) {
>                 int old_dpms = connector->dpms;
> 
>                 if (connector->funcs->dpms)
>                         connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
> 
>                 /* Set the old mode back to the connector for resume */
>                 connector->dpms = old_dpms;
>         }

I'm not sure whether that will get you correct behavior since iirc the
crtc helpers aren't terribly good at restoring the right dpms state.
drm_helper_resume_force_mode simple does a modeset, and that has an
implicit dpms on when enabling the crtc. Someone might want to tackle
this, but thus far it doesn't seem to have been too annoying. I think the
best way would be to do this as part of atomic modeset, where we can
supply the desired dpms state directly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch