Re: [PATCH] drm/i915: fix OOPS in lid_notify

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Sep 17, 2012 at 02:38:14PM -0700, Jesse Barnes wrote:
> On Mon, 17 Sep 2012 22:27:21 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > This goes back to
> > 
> > commit c1c7af60892070e4b82ad63bbfb95ae745056de0
> > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 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 <torvalds@linux-foundation.org>
> > 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 <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  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 <jbarnes@virtuousgeek.org>

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