From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: fix OOPS in lid_notify Date: Tue, 18 Sep 2012 00:57:49 +0200 Message-ID: <20120917225749.GD5522@phenom.ffwll.local> References: <1347913641-8229-1-git-send-email-daniel.vetter@ffwll.ch> <20120917143814.22392d56@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f171.google.com (mail-wi0-f171.google.com [209.85.212.171]) by gabe.freedesktop.org (Postfix) with ESMTP id 74C489E7BA for ; Mon, 17 Sep 2012 15:57:13 -0700 (PDT) Received: by wibhq4 with SMTP id hq4so2784084wib.12 for ; Mon, 17 Sep 2012 15:57:12 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20120917143814.22392d56@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 Mon, Sep 17, 2012 at 02:38:14PM -0700, Jesse Barnes wrote: > On Mon, 17 Sep 2012 22:27:21 +0200 > Daniel Vetter wrote: > > > This goes back to > > > > commit c1c7af60892070e4b82ad63bbfb95ae745056de0 > > Author: Jesse Barnes > > Date: Thu Sep 10 15:28:03 2009 -0700 > > > > drm/i915: force mode set at lid open time > > > > It was used to fix an issue on a i915GM based Thinkpad X41, which > > somehow clobbered the modeset state at lid close time. Since then > > massive amounts of things changed: Tons of fixes to the modeset > > sequence, OpRegion support, better integration with the acpi code. > > Especially OpRegion /should/ allow us to control the display hw > > cooperatively with the firmware, without the firmware clobbering the > > hw state behind our backs. > > > > So it's dubious whether we still need this. > > > > The second issue is that it's unclear who's responsibility it actually > > is to restore the mode - Chris Wilson suggests to just emit a hotplug > > event and let userspace figure things out. > > > > The real reason I've stumbled over this is that the new modeset code > > breaks drm_helper_resume_force_mode - it OOPSes derefing a NULL vfunc > > pointer. The reason this wasn't caught in testing earlier is that in > > > > commit c9354c85c1c7bac788ce57d3c17f2016c1c45b1d > > Author: Linus Torvalds > > Date: Mon Nov 2 09:29:55 2009 -0800 > > > > i915: fix intel graphics suspend breakage due to resume/lid event > > confusion > > > > logic was added to _not_ restore the modeset state after a resume. And > > since most machines are configured to auto-suspend on lid-close, this > > neatly papered over the issue. > > > > Summarizing, this shouldn't be required on any platform supporting > > OpRegion. And none of the really old machines I have here seem to > > require it either. Hence I'm inclined to just rip it out. > > > > But in case that there are really firmwares out there that clobber the > > hw state, replace it with a call to intel_modset_check_state. This > > will ensure that we catch any issues as soon as they happen. > > > > Cc: Chris Wilson > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/i915/intel_lvds.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > > index b1b36de..4e22d46 100644 > > --- a/drivers/gpu/drm/i915/intel_lvds.c > > +++ b/drivers/gpu/drm/i915/intel_lvds.c > > @@ -581,7 +581,7 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val, > > dev_priv->modeset_on_lid = 0; > > > > mutex_lock(&dev->mode_config.mutex); > > - drm_helper_resume_force_mode(dev); > > + intel_modeset_check_state(dev); > > mutex_unlock(&dev->mode_config.mutex); > > > > return NOTIFY_OK; > > Yeah, I *hope* we don't need this anymore. Needs some good tested-by > activity on old machines especially (some of them definitely do mess > with display regs across lid events). > > Acked-by: Jesse Barnes Ok, since I head of to xdc tomorrow I've merged this right away. We can take it out again if it blows up on ickles fancy old machine ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch