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

[yakui]

On Wed, 2009-07-08 at 04:21 +0800, Jesse Barnes wrote:
> On Wed, 8 Jul 2009 06:08:31 +1000
> Dave Airlie <airlied at gmail.com> wrote:
> 
> > On 08/07/2009, at 4:57, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> > 
> > > On Tue, 07 Jul 2009 11:40:10 -0700
> > > Eric Anholt <eric at anholt.net> 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. However, note that
> > >> drm_helper_resume_force_mode doesn't turn off disabled connectors,
> > >> which seems wrong.
> > >
> > > Yeah, that's one problem; I think the code should also be
> > > structured a bit differently.  We really want to track whole KMS
> > > configurations (output configuration and associated buffers), and
> > > have functions for saving & restoring those (including forced
> > > restoration, e.g. for lastclose or panic).  Right now the two uses
> > > are a bit muddled, which means panic/lastclose can fail, and
> > > force_mode doesn't do the same thing as a regular set_crtc call.
> > 
> > Force mode can't do the same thing unless you tear down the crtcs
> > and store the current modes on suspend which is pointless. The whole
> > point of force mode is to force the mode the sw currently thinks is
> > in the hw but isn't due to us having suspended. It is not meant for
> > panic paths etc.
> 
> Ok, but it doesn't look like it works for the resume case either, since
> it won't turn off unused stuff like the normal mode set path does.  So
> we still need to handle both cases better.
Agree with what Dave said. 
If we want to turn off the unused stuff, how about calling the function
of drm_helper_disable_unused_functions explicitly after the function of
drm_helper_resume_force_mode?

thanks.
>