* [PATCH] drm/i915: fix OOPS in lid_notify
@ 2012-09-17 20:27 Daniel Vetter
2012-09-17 21:38 ` Jesse Barnes
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2012-09-17 20:27 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
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;
--
1.7.11.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: fix OOPS in lid_notify
2012-09-17 20:27 [PATCH] drm/i915: fix OOPS in lid_notify Daniel Vetter
@ 2012-09-17 21:38 ` Jesse Barnes
2012-09-17 22:57 ` Daniel Vetter
2012-09-18 10:42 ` Chris Wilson
0 siblings, 2 replies; 5+ messages in thread
From: Jesse Barnes @ 2012-09-17 21:38 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: fix OOPS in lid_notify
2012-09-17 21:38 ` Jesse Barnes
@ 2012-09-17 22:57 ` Daniel Vetter
2012-09-18 10:42 ` Chris Wilson
1 sibling, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2012-09-17 22:57 UTC (permalink / raw)
To: Jesse Barnes; +Cc: Daniel Vetter, Intel Graphics Development
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: fix OOPS in lid_notify
2012-09-17 21:38 ` Jesse Barnes
2012-09-17 22:57 ` Daniel Vetter
@ 2012-09-18 10:42 ` Chris Wilson
2012-09-19 7:26 ` Daniel Vetter
1 sibling, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2012-09-18 10:42 UTC (permalink / raw)
To: Jesse Barnes, Daniel Vetter; +Cc: Intel Graphics Development
On Mon, 17 Sep 2012 14:38:14 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> 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).
Not even a squeak on my 855gm. On a positive note modeset-rework is able
to light the LVDS even when that machine is booted with its lid close,
which is a first! It complains vehemently in the process though.
> Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Tested-by: Chris Wilson <chris@chris-wilson.co.uk> # 855gm with an evil BIOS
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: fix OOPS in lid_notify
2012-09-18 10:42 ` Chris Wilson
@ 2012-09-19 7:26 ` Daniel Vetter
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2012-09-19 7:26 UTC (permalink / raw)
To: Chris Wilson; +Cc: Daniel Vetter, Intel Graphics Development
On Tue, Sep 18, 2012 at 11:42:17AM +0100, Chris Wilson wrote:
> Not even a squeak on my 855gm. On a positive note modeset-rework is able
> to light the LVDS even when that machine is booted with its lid close,
> which is a first! It complains vehemently in the process though.
Wohoo for better lvds. And yep, gen2 is a bit noise here, too. I've shut
it up with the fixed pipe/plane code, but that seems to kill resume on my
i855gm :(
I'll poached Kristians X30 with a i830M here at xdc, hopefully I'll figure
out what we're wrong.
> > Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk> # 855gm with an evil BIOS
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-09-19 7:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-17 20:27 [PATCH] drm/i915: fix OOPS in lid_notify Daniel Vetter
2012-09-17 21:38 ` Jesse Barnes
2012-09-17 22:57 ` Daniel Vetter
2012-09-18 10:42 ` Chris Wilson
2012-09-19 7:26 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.