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

[yakui]

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. 
> 
> > 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.

Thanks.
>