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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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>

-- 
Jesse Barnes, Intel Open Source Technology Center