[Intel-gfx] [DRM/I915]: Restore the modeset for every activated CRTC

[Jesse Barnes]

On Wed, 08 Jul 2009 10:01:47 +0800
yakui <yakui.zhao at intel.com> wrote:

> On Wed, 2009-07-08 at 09:29 +0800, Jesse Barnes wrote:
> > On Wed, 08 Jul 2009 09:25:44 +0800
> > yakui <yakui.zhao at intel.com> wrote:
> > 
> > > On Wed, 2009-07-08 at 02:40 +0800, Eric Anholt wrote:
> > > > On Thu, 2009-07-02 at 15:16 +0800, yakui.zhao at intel.com wrote:
> > > > > From: Zhao Yakui <yakui.zhao at intel.com>
> > > > > 
> > > > > Restore the modeset for every activated CRTC in course of
> > > > > resume. This is realized by calling the function of
> > > > > drm_helper_resume_force_mode. Note: it is meaningful only for
> > > > > the KMS mode.
> > > > > 
> > > > > Signed-off-by: Zhao Yakui <yakui.zhao at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.c |    5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > Index: linux-2.6/drivers/gpu/drm/i915/i915_drv.c
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/drivers/gpu/drm/i915/i915_drv.c
> > > > > 2009-07-02 14:24:44.000000000 +0800 +++
> > > > > linux-2.6/drivers/gpu/drm/i915/i915_drv.c	2009-07-02
> > > > > 14:25:52.000000000 +0800 @@ -35,6 +35,7 @@ 
> > > > >  #include "drm_pciids.h"
> > > > >  #include <linux/console.h>
> > > > > +#include "drm_crtc_helper.h"
> > > > >  
> > > > >  static unsigned int i915_modeset = -1;
> > > > >  module_param_named(modeset, i915_modeset, int, 0400);
> > > > > @@ -115,6 +116,10 @@
> > > > >  
> > > > >  		drm_irq_install(dev);
> > > > >  	}
> > > > > +	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > > > > +		/* Resume the modeset for every activated
> > > > > CRTC */
> > > > > +		drm_helper_resume_force_mode(dev);
> > > > > +	}
> > > > >  
> > > > >  	return ret;
> > > > >  }
> > > > 
> > > > So, this patch does a full modeset on top of the almost-full
> > > > modeset done by the suspend/resume code.  That seems bogus --
> > > > if we're going to do this, we shouldn't do the rest of the
> > > > save/restore for mdoesetting.
> > > In course of suspend we will save some registers related with
> > > modesetting. Of course the registers related with interrupt/vga
> > > are also saved.
> > > And in course of resume we will restore the registers.
> > > After doing so, many boxes can work well.
> > > But if the modesetting registers are touched by BIOS in course of
> > > suspend/resume, maybe it is not enough to only restore the
> > > registers related with modesetting. It will be better that we
> > > restore the full modeset for the activated crtc. 
> > > And when the system is resumed from S4, the registers related with
> > > modesetting are not initialized correctly. In such case it will be
> > > better that we restore the modeset.
> > 
> > Right, we want to do the mode set.  But we don't want to touch the
> > mode setting regs twice at resume time (the current resume function
> > basically does mode sets on each output, since it restores the
> > previous config).
> > 
> > So your patch should split the suspend & resume functions into code
> > that executes in a non-KMS config and code that executes in a KMS
> > config.  In the non-KMS case, we'll need to save/restore everything,
> > like we do currently.  In the non-KMS case we only need to
> > save/restore certain global chip state.  Anything that the existing
> > mode set functions set doesn't have to be saved & restored, since
> > the resume mode set will take care of it.
> > Doing it that way will speed up suspend/resume and reduce flicker.
> What you said is right. 
> We can remove the registers related with the modesetting in
> save/restore if it works in KMS mode.
> 
> But it seems that we can do it in another patch.

I'd rather see one patch that splits the KMS and non-KMS suspend
paths.  Or at least 2 patches in this series (so we apply them both
at the same time).

> > > This patch has no side effect for the box that can work well after
> > > using save/restore. And it can assure that the modeset before
> > > entering suspended state is restored correctly.
> > > 
> > > If the patch can be shipped, we can remove the register related
> > > with modesetting in save/restore when it works in KMS mode. In
> > > fact it doesn't matter if the we don't remove them. The side
> > > effect is that some registers are restored twice.
> > > > However, note that drm_helper_resume_force_mode doesn't turn off
> > > > disabled connectors, which seems wrong.
> > > the drm_helper_resume_force_mode doesn't touch the disabled
> > > connectors. Why is it necessary to turn off the disabled
> > > connectors again?
> > 
> > Two main reasons: one is that it restores the actual config of the
> > machine prior to suspend, and the other is that it helps save power.
> > If you resume your machine and an extra, unused pipe is turned on,
> > your battery will drain faster.
> Understand what you are concerned.
> This is to assure that the unused connectors are turned off if they
> are turned on by BIOS.
> 
> IMO it will be ok to call the function of
> drm_helper_disable_unused_function.

Yeah, Dave suggested doing that in the force routine itself.  I think
radeon also uses it so that would be a good place to share code.

-- 
Jesse Barnes, Intel Open Source Technology Center