i915 / PM: Fix hibernate regression caused by suspend/resume splitting
diff mbox series

Message ID 201002182306.28052.rjw@sisk.pl
State New, archived
Headers show
Series
  • i915 / PM: Fix hibernate regression caused by suspend/resume splitting
Related show

Commit Message

Rafael J. Wysocki Feb. 18, 2010, 10:06 p.m. UTC
On Thursday 18 February 2010, Pedro Ribeiro wrote:
> On 17 February 2010 22:20, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday 17 February 2010, Pedro Ribeiro wrote:
> >> 2010/2/17 Rafael J. Wysocki <rjw@sisk.pl>:
> >> > On Wednesday 17 February 2010, Rafael J. Wysocki wrote:
> >> >> On Wednesday 17 February 2010, Pedro Ribeiro wrote:
> >> >> > Hi,
> >> >> >
> >> >> > commit 84b79f8d2882b0a84330c04839ed4d3cefd2ff77 (drm/i915: Fix crash
> >> >> > while aborting hibernation) introduced two new issues which were not
> >> >> > present in 2.6.33-rc7:
> >> >> >
> >> >> > - every second resume from hibernate results in a blank screen
> >> >> > - the annoying flash at the end of atomic copy/restore during the
> >> >> > hibernate process is back (present in kernels < 2.6.33)
> >> >> >
> >> >> > The first issue is serious, the second is just an annoyance.
> >> >>
> >> >> The second one is an expected price of fixing the aborted hibernation
> >> >> regression.
> >> >>
> >> >> The first one shouldn't happen, though.
> >> >>
> >> >> I'll see if I can reproduce that locally.
> >> >
> >> > No, I can't.
> >> >
> >> > Is the driver compiled directly into the kernel or modular?
> >>
> >> The driver is modular.
> >> And sorry, I forgot to tell you I'm using TuxOnIce, it certainly makes
> >> a difference.
> >
> > It shouldn't in fact, although I'm not sure.
> >
> >> However, every since I reverted that commit I've done 10 test
> >> hibernations and no hang so far.
> >
> > First, please try if you can reproduce it with non-modular driver.
> >
> > Second, please check if the appended patch helps.
> >
> > Rafael
> >
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c |   30 +++++++++---------------------
> >  1 file changed, 9 insertions(+), 21 deletions(-)
> >
> > Index: linux-2.6/drivers/gpu/drm/i915/i915_drv.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/gpu/drm/i915/i915_drv.c
> > +++ linux-2.6/drivers/gpu/drm/i915/i915_drv.c
> > @@ -177,6 +177,8 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
> >
> >  static int i915_drm_freeze(struct drm_device *dev)
> >  {
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> >        pci_save_state(dev->pdev);
> >
> >        /* If KMS is active, we do the leavevt stuff here */
> > @@ -192,17 +194,12 @@ static int i915_drm_freeze(struct drm_de
> >
> >        i915_save_state(dev);
> >
> > -       return 0;
> > -}
> > -
> > -static void i915_drm_suspend(struct drm_device *dev)
> > -{
> > -       struct drm_i915_private *dev_priv = dev->dev_private;
> > -
> >        intel_opregion_free(dev, 1);
> >
> >        /* Modeset on resume, not lid events */
> >        dev_priv->modeset_on_lid = 0;
> > +
> > +       return 0;
> >  }
> >
> >  static int i915_suspend(struct drm_device *dev, pm_message_t state)
> > @@ -222,8 +219,6 @@ static int i915_suspend(struct drm_devic
> >        if (error)
> >                return error;
> >
> > -       i915_drm_suspend(dev);
> > -
> >        if (state.event == PM_EVENT_SUSPEND) {
> >                /* Shut down the device */
> >                pci_disable_device(dev->pdev);
> > @@ -238,6 +233,10 @@ static int i915_drm_thaw(struct drm_devi
> >        struct drm_i915_private *dev_priv = dev->dev_private;
> >        int error = 0;
> >
> > +       i915_restore_state(dev);
> > +
> > +       intel_opregion_init(dev, 1);
> > +
> >        /* KMS EnterVT equivalent */
> >        if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> >                mutex_lock(&dev->struct_mutex);
> > @@ -264,10 +263,6 @@ static int i915_resume(struct drm_device
> >
> >        pci_set_master(dev->pdev);
> >
> > -       i915_restore_state(dev);
> > -
> > -       intel_opregion_init(dev, 1);
> > -
> >        return i915_drm_thaw(dev);
> >  }
> >
> > @@ -424,8 +419,6 @@ static int i915_pm_suspend(struct device
> >        if (error)
> >                return error;
> >
> > -       i915_drm_suspend(drm_dev);
> > -
> >        pci_disable_device(pdev);
> >        pci_set_power_state(pdev, PCI_D3hot);
> >
> > @@ -465,13 +458,8 @@ static int i915_pm_poweroff(struct devic
> >  {
> >        struct pci_dev *pdev = to_pci_dev(dev);
> >        struct drm_device *drm_dev = pci_get_drvdata(pdev);
> > -       int error;
> > -
> > -       error = i915_drm_freeze(drm_dev);
> > -       if (!error)
> > -               i915_drm_suspend(drm_dev);
> >
> > -       return error;
> > +       return i915_drm_freeze(drm_dev);
> >  }
> >
> >  const struct dev_pm_ops i915_pm_ops = {
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >
> 
> The patch fixes this issue for me.
> 
> Thanks for your help.

OK, thanks for testing, let's submit it appropriately.

Jesse, Eric, the appended patch fixes a very recent hibernate regression in
i915, please push it to Linus ASAP.

Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: i915 / PM: Fix hibernate regression caused by suspend/resume splitting

Commit 84b79f8d2882b0a84330c04839ed4d3cefd2ff77 (drm/i915: Fix crash
while aborting hibernation) attempted to fix a regression introduced
by commit cbda12d77ea590082edb6d30bd342a67ebc459e0 (drm/i915:
implement new pm ops for i915), but it went too far trying to split
the freeze/suspend and resume/thaw parts of the code.  As a result,
it introduced another regression, which only is visible on some systems.

Fix the problem by merging i915_drm_suspend() with
i915_drm_freeze() and moving some code from i915_resume()
into i915_drm_thaw(), so that intel_opregion_free() and
intel_opregion_init() are also executed in the freeze and thaw code
paths, respectively.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Reported-and-tested-by: Pedro Ribeiro <pedrib@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.c |   30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Tino Keitel Feb. 19, 2010, 6:39 a.m. UTC | #1
On Thu, Feb 18, 2010 at 23:06:27 +0100, Rafael J. Wysocki wrote:

[...]

> Jesse, Eric, the appended patch fixes a very recent hibernate regression in
> i915, please push it to Linus ASAP.

FYI: I got no resume failure during normal usage after 9 suspend to RAM
and 8 suspend to disk cycles (using both suspend types alternately)
with the patch applied.

Regards,
Tino
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Linus Torvalds Feb. 22, 2010, 3:47 p.m. UTC | #2
Jesse, Eric, what's the status of this patch? 

Should I take it directly (I'd really like an Ack) or is it queued up in 
some tree waiting for me to pull? Or is there some reason not to have it, 
despite it fixing things for some people?

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jesse Barnes Feb. 22, 2010, 4:32 p.m. UTC | #3
On Mon, 22 Feb 2010 07:47:53 -0800 (PST)
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> Jesse, Eric, what's the status of this patch? 
> 
> Should I take it directly (I'd really like an Ack) or is it queued up in 
> some tree waiting for me to pull? Or is there some reason not to have it, 
> despite it fixing things for some people?

Hm, I thought Eric had applied that to his for-linus branch, but it
looks like that branch is empty now.  Maybe Rafael posted an update
that Eric missed and so you just have a partial fix in your tree?
Eric Anholt Feb. 22, 2010, 4:36 p.m. UTC | #4
On Mon, 22 Feb 2010 07:47:53 -0800 (PST), Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> Jesse, Eric, what's the status of this patch? 
> 
> Should I take it directly (I'd really like an Ack) or is it queued up in 
> some tree waiting for me to pull? Or is there some reason not to have it, 
> despite it fixing things for some people?

Acked-by: Eric Anholt <eric@anholt.net>

I just reviewed the rest of my tag:todo and I don't think I have
anything else I want to pull request for this kernel (finally), so if
you could just grab this patch it would be great.

(Well, there's also "quit trying to _LID on 855 because it's always a
pack of lies", but I'd rather do it as a backport later in case someone
screams)

Patch
diff mbox series

Index: linux-2.6/drivers/gpu/drm/i915/i915_drv.c
===================================================================
--- linux-2.6.orig/drivers/gpu/drm/i915/i915_drv.c
+++ linux-2.6/drivers/gpu/drm/i915/i915_drv.c
@@ -177,6 +177,8 @@  MODULE_DEVICE_TABLE(pci, pciidlist);
 
 static int i915_drm_freeze(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
 	pci_save_state(dev->pdev);
 
 	/* If KMS is active, we do the leavevt stuff here */
@@ -192,17 +194,12 @@  static int i915_drm_freeze(struct drm_de
 
 	i915_save_state(dev);
 
-	return 0;
-}
-
-static void i915_drm_suspend(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
 	intel_opregion_free(dev, 1);
 
 	/* Modeset on resume, not lid events */
 	dev_priv->modeset_on_lid = 0;
+
+	return 0;
 }
 
 static int i915_suspend(struct drm_device *dev, pm_message_t state)
@@ -222,8 +219,6 @@  static int i915_suspend(struct drm_devic
 	if (error)
 		return error;
 
-	i915_drm_suspend(dev);
-
 	if (state.event == PM_EVENT_SUSPEND) {
 		/* Shut down the device */
 		pci_disable_device(dev->pdev);
@@ -238,6 +233,10 @@  static int i915_drm_thaw(struct drm_devi
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int error = 0;
 
+	i915_restore_state(dev);
+
+	intel_opregion_init(dev, 1);
+
 	/* KMS EnterVT equivalent */
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		mutex_lock(&dev->struct_mutex);
@@ -264,10 +263,6 @@  static int i915_resume(struct drm_device
 
 	pci_set_master(dev->pdev);
 
-	i915_restore_state(dev);
-
-	intel_opregion_init(dev, 1);
-
 	return i915_drm_thaw(dev);
 }
 
@@ -424,8 +419,6 @@  static int i915_pm_suspend(struct device
 	if (error)
 		return error;
 
-	i915_drm_suspend(drm_dev);
-
 	pci_disable_device(pdev);
 	pci_set_power_state(pdev, PCI_D3hot);
 
@@ -465,13 +458,8 @@  static int i915_pm_poweroff(struct devic
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct drm_device *drm_dev = pci_get_drvdata(pdev);
-	int error;
-
-	error = i915_drm_freeze(drm_dev);
-	if (!error)
-		i915_drm_suspend(drm_dev);
 
-	return error;
+	return i915_drm_freeze(drm_dev);
 }
 
 const struct dev_pm_ops i915_pm_ops = {