From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH] drm/i915: no longer call drm_helper_resume_force_mode Date: Thu, 6 Sep 2012 13:47:35 -0700 Message-ID: <20120906134735.271dbf1f@jbarnes-desktop> References: <1345403595-9678-58-git-send-email-daniel.vetter@ffwll.ch> <1346274809-31155-1-git-send-email-daniel.vetter@ffwll.ch> <20120905113155.0286756f@jbarnes-desktop> <20120905130454.30ba4cfc@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from oproxy7-pub.bluehost.com (oproxy7-pub.bluehost.com [67.222.55.9]) by gabe.freedesktop.org (Postfix) with SMTP id 1ADFCA0C86 for ; Thu, 6 Sep 2012 13:47:35 -0700 (PDT) In-Reply-To: <20120905130454.30ba4cfc@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Jesse Barnes Cc: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Wed, 5 Sep 2012 13:04:54 -0700 Jesse Barnes wrote: > On Wed, 5 Sep 2012 21:56:08 +0200 > Daniel Vetter wrote: > > > On Wed, Sep 5, 2012 at 8:31 PM, Jesse Barnes wrote: > > > On Wed, 29 Aug 2012 23:13:29 +0200 > > > Daniel Vetter wrote: > > > > > >> Since this only calls crtc helper functions, of which a shocking > > >> amount are NULL. > > >> > > >> Now the curious thing is how the new modeset code worked with this > > >> function call still present: > > >> > > >> Thanks to the hw state readout and the suspend fixes to properly > > >> quiescent the register state, nothing is actually enabled at resume > > >> (if the bios doesn't set up anything). Which means resume_force_mode > > >> doesn't actually do anything and hence nothing blows up at resume > > >> time. > > >> > > >> The other reason things do work is that the fbcon layer has it's own > > >> resume notifier callback, which restores the mode. And thanks to the > > >> force vt switch at suspend/resume, that then forces X to restore it's > > >> own mode. > > >> > > >> Hence everything still worked (as long as the bios doesn't enable > > >> anything). And we can just kill the call to resume_force_mode. > > >> > > >> The upside of both this patch and the preceeding patch to quiescent > > >> the modeset state is that our resume path is much simpler: > > >> - We now longer restore bogus register values (which most often would > > >> enable the backlight a bit and a few ports), causing flickering. > > >> - We now longer call resume_force_mode to restore a mode that the > > >> fbcon layer would overwrite right away anyway. > > >> > > >> Signed-off-by: Daniel Vetter > > >> --- > > >> drivers/gpu/drm/i915/i915_drv.c | 5 ----- > > >> 1 file changed, 5 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > >> index fe7512a..cd6697c 100644 > > >> --- a/drivers/gpu/drm/i915/i915_drv.c > > >> +++ b/drivers/gpu/drm/i915/i915_drv.c > > >> @@ -549,11 +549,6 @@ static int i915_drm_thaw(struct drm_device *dev) > > >> intel_modeset_setup_hw_state(dev); > > >> drm_mode_config_reset(dev); > > >> drm_irq_install(dev); > > >> - > > >> - /* Resume the modeset for every activated CRTC */ > > >> - mutex_lock(&dev->mode_config.mutex); > > >> - drm_helper_resume_force_mode(dev); > > >> - mutex_unlock(&dev->mode_config.mutex); > > >> } > > >> > > >> intel_opregion_init(dev); > > > > > > Wouldn't the fb layer's modeset end up being a no-op if the suspended > > > mode was the same as the fb mode (often the case)? Or at the very > > > least just a flip rather than a full mode set. > > > > I guess most of the flicker was because the register restoring > > restored a bunch of crap (since the old modeset state wasn't properly > > cleared before suspending). > > > > > Though we do need to deal with non-fb, non-X resumes as well. kmscon > > > and wayland will expect to be restored at resume time even if CONFIG_VT > > > and the fb layer aren't compiled into the kernel. > > > > Tbh I was rather surprised that when I've noticed this little issue > > here the restore still worked - until I've noticed by looking at the > > logs that both the fbcon and the X server restore their modes. > > > > I'm not sure what exactly we should do here, since even with the > > current code the concept of a controlling node isn't really defined in > > the kms interface (fbcon uses a bunch of funky checks to ensure it > > doesn't clobber the output state of someone else). But for now (with > > fbcon pretty much being non-optional) things keep on working, and > > afaict actually work a bit better overall. > > It's probably ok for now, but at some point we'll want some code that > restores the suspend mode if fbcon isn't enabled... > So acked/reviewed-by: Jesse Barnes But note that if someone complains we'll need to add this back... -- Jesse Barnes, Intel Open Source Technology Center