Problems with suspend on recent kernel (not necessary a bug)

Problems with suspend on recent kernel (not necessary a bug)

[Klaus Ethgen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Hi list,

first, just punish me if I post on the wrong place for that but I think
it it. :-)

I moved recently my laptop (levovon X61s) from kernel version v3.13.11
to v3.18.6. Everything was fine except that suspend to RAM is not
working anymore. Right after blanking the console the suspend stuck with
still running fans and not reacting to any interrupt except hard
switching of the box.

I tried to debug that but with a blanked console it is pretty difficult
to do so. At least I was able to set pm_trace and it gave me "tty ptyxc:
hash matches". So there seems to be something with that subsystem I
believe.

Is there any further way to debug suspend that could give me more
informations?

As told in the subject, it might be a bug or not, I cannot say as there
are several differences in config between that two versions without
doing changes manually. This even makes it difficult to bisec the
problem. Moreover that compiling need some time on that box. However,
if there is no easier way, I plan to test at least with major
versions...

A small side note: I using the bfq patch. Currently I have no pointer of
that being the source of the problem but could try to eliminate that
too.

So if anybody has an easy way to catch down that suspend beast, I would
appreciate the help.

Regards
   Klaus
- -- 
Klaus Ethgen                              http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16   Klaus Ethgen <Klaus@Ethgen.de>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQGcBAEBCgAGBQJU2z9dAAoJEKZ8CrGAGfase8kL/3NEj1cHA/bsbOc6Xo8T+9AY
St43xkTX/Ex0zHYfCNfm4/s2gAi7yHgmeni/GXCYLBCEpcN4jBbrwYFHfsd02USm
KglqkFqjklXw7lHaE6sclinSn5txe8tZbRqjWtRgrxgLU+7EoUQqF0Ozvvpc2teK
d0VXdO+RpsGg7ZIcwyCr5b8dRKhLshjGgGIpk12kkdZZmRipu8V5u9Ls6j3tZj7h
xCQ97GLsYd3bDhNWok/YVXNRPAmdLYtmnqkAYTSMDTUqtJZoNahVCZs0Z6qFfdPS
ycPK1NtksNQSnlAYa7n7RlvbnWcytNklKBn1jIcdDKMH61+YnrQ9wYwpNkKu0VP7
idL84OlY85t5nJUhNpseJ3o6iwUu6826AKSzt76tlXNKY+ZhoqdMAC+3Ku58nsTP
CFS63zjxmFMBTXjFak3bKnk7ZHKzJ7G24j257Qed/wnVE82kQ+17F0xHPjhbMj8I
lc9goaDgiPRxCOA6NqM6RmlgC604/xPOrAKMH3AkMg==
=OYh9
-----END PGP SIGNATURE-----

Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Klaus Ethgen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

After three days of bisect I found the bug for my problem with a broken
suspend on x61p of lenovo.

The problem is commit e11aa36. It seems to be a wrong assumption that
disabling the interrupts is enough.

I'll try to revert the commit on top of 3.18.7. (It does not clean
revert.)

Regards
   Klaus
- -- 
Klaus Ethgen                              http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16   Klaus Ethgen <Klaus@Ethgen.de>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQGcBAEBCgAGBQJU4mseAAoJEKZ8CrGAGfas9skMAIeYCOSbbA2kQbC0AQT17y5L
TUayHwW/4pDYxIOqS8cuCkiSGxokiemBYDdLgtVzPCp90suhB0ZhZY0o1VolwQTP
CHTqYAGTNarq7c9gXWmpCGlpYyPzce38rlDMzu4fQwOqegziO3TBrAm7m5rwv/mK
cBtPK4anx6qrXjUrttn8LEt0Qlr5n+HIKVBrbZcPLuLJmxA1KclY742V53Z2uEuz
CozDMjKrVhAKG3ehRxXhpcWQRRNOplKH7cY74yWLA3eepQg82CjEcM5mP7cZWDPT
Hv+B1H6wZ38STtKctiXUUukCphaT0AU97saCr7k4xkY/6gM+BNCZNQnQAOGzW0J3
ybhTT/oZVxY2qPo/hk3x/XGoAkdQq/7axYEYRN/urocklLUcy2Dlc2kuN8K6QRqk
VY4USl6U3c4mbK3bOB35oPPqrqFzFgbntJ7VHaIRip5VUjEsrHYox5lKFQJOt2Rg
fHLyD423SuXE5pCQxWiXl+Hq5MC7n80N132SQSxa4Q==
=9G0k
-----END PGP SIGNATURE-----

Re: Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Dave Airlie]

ccing some ppl.


On 17 February 2015 at 08:11, Klaus Ethgen <Klaus+lkml@ethgen.de> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> After three days of bisect I found the bug for my problem with a broken
> suspend on x61p of lenovo.
>
> The problem is commit e11aa36. It seems to be a wrong assumption that
> disabling the interrupts is enough.
>
> I'll try to revert the commit on top of 3.18.7. (It does not clean
> revert.)
>
> Regards
>    Klaus
> - --
> Klaus Ethgen                              http://www.ethgen.ch/
> pub  4096R/4E20AF1C 2011-05-16   Klaus Ethgen <Klaus@Ethgen.de>
> Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1
>
> iQGcBAEBCgAGBQJU4mseAAoJEKZ8CrGAGfas9skMAIeYCOSbbA2kQbC0AQT17y5L
> TUayHwW/4pDYxIOqS8cuCkiSGxokiemBYDdLgtVzPCp90suhB0ZhZY0o1VolwQTP
> CHTqYAGTNarq7c9gXWmpCGlpYyPzce38rlDMzu4fQwOqegziO3TBrAm7m5rwv/mK
> cBtPK4anx6qrXjUrttn8LEt0Qlr5n+HIKVBrbZcPLuLJmxA1KclY742V53Z2uEuz
> CozDMjKrVhAKG3ehRxXhpcWQRRNOplKH7cY74yWLA3eepQg82CjEcM5mP7cZWDPT
> Hv+B1H6wZ38STtKctiXUUukCphaT0AU97saCr7k4xkY/6gM+BNCZNQnQAOGzW0J3
> ybhTT/oZVxY2qPo/hk3x/XGoAkdQq/7axYEYRN/urocklLUcy2Dlc2kuN8K6QRqk
> VY4USl6U3c4mbK3bOB35oPPqrqFzFgbntJ7VHaIRip5VUjEsrHYox5lKFQJOt2Rg
> fHLyD423SuXE5pCQxWiXl+Hq5MC7n80N132SQSxa4Q==
> =9G0k
> -----END PGP SIGNATURE-----
> --
> 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/

Re: [KERNEL] Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Klaus Ethgen]

[-- Attachment #1.1: Type: text/plain, Size: 729 bytes --]

After solving  the conflicts, I applied the revert (see attachment) to
v3.18.7. I think it should also apply to the current head. With that
patch, suspend is working again on that version.

However, I have not to deep knowledge of that subsystem, so please,
someone who have, have a deeper look into it. I especially do not know
if the lines in .../intel_pm.c are correct or better leaving them as
they are in v3.18.7.

I want to have it working on a version that I know is stable before
asking to pull it to head.

Regards
   Klaus
-- 
Klaus Ethgen                              http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16   Klaus Ethgen <Klaus@Ethgen.de>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C

[-- Attachment #1.2: 0001-Revert-drm-i915-use-runtime-irq-suspend-resume-in-fr.patch --]
[-- Type: text/x-diff, Size: 2153 bytes --]

>From b9831d771ea7f416f3ab1c992f67c7523f34ecbd Mon Sep 17 00:00:00 2001
From: Klaus Ethgen <Klaus@Ethgen.de>
Date: Mon, 16 Feb 2015 23:20:41 +0100
Subject: [PATCH] Revert "drm/i915: use runtime irq suspend/resume in
 freeze/thaw"

It is a wrong assumption that disabling the interrupts is enough.

This reverts commit e11aa362308f5de467ce355a2a2471321b15a35c.
---
 drivers/gpu/drm/i915/i915_drv.c | 5 +++--
 drivers/gpu/drm/i915/intel_pm.c | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9256973..bc390da 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -575,7 +575,7 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 		flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
-		intel_runtime_pm_disable_interrupts(dev);
+		drm_irq_uninstall(dev);
 		intel_hpd_cancel_work(dev_priv);
 
 		intel_suspend_encoders(dev_priv);
@@ -680,7 +680,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 		}
 		mutex_unlock(&dev->struct_mutex);
 
-		intel_runtime_pm_restore_interrupts(dev);
+		/* We need working interrupts for modeset enabling ... */
+		drm_irq_install(dev, dev->pdev->irq);
 
 		intel_modeset_init_hw(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 83c7ecf..ee68ba9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5195,7 +5195,7 @@ void intel_suspend_gt_powersave(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	/* Interrupts should be disabled already to avoid re-arming. */
-	WARN_ON(intel_irqs_enabled(dev_priv));
+	WARN_ON(dev->irq_enabled);
 
 	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
@@ -5210,7 +5210,7 @@ void intel_disable_gt_powersave(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	/* Interrupts should be disabled already to avoid re-arming. */
-	WARN_ON(intel_irqs_enabled(dev_priv));
+	WARN_ON(dev->irq_enabled);
 
 	if (IS_IRONLAKE_M(dev)) {
 		ironlake_disable_drps(dev);
-- 
2.1.4


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 648 bytes --]

Re: [KERNEL] Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Jani Nikula]

On Tue, 17 Feb 2015, Klaus Ethgen <Klaus+lkml@ethgen.de> wrote:
> After solving  the conflicts, I applied the revert (see attachment) to
> v3.18.7. I think it should also apply to the current head. With that
> patch, suspend is working again on that version.
>
> However, I have not to deep knowledge of that subsystem, so please,
> someone who have, have a deeper look into it. I especially do not know
> if the lines in .../intel_pm.c are correct or better leaving them as
> they are in v3.18.7.
>
> I want to have it working on a version that I know is stable before
> asking to pull it to head.

Hi Klaus, we fear this patch may hide the actual cause. Would be useful
to get a better description of what happens, along with a dmesg with
drm.debug=14 module parameter set. This might clutter the mailing list,
would you mind filing a bug at [1] and attach the info there?

Thanks,
Jani.

[1] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel



-- 
Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] [KERNEL] Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Imre Deak]

On ke, 2015-02-18 at 17:39 +0200, Jani Nikula wrote:
> On Tue, 17 Feb 2015, Klaus Ethgen <Klaus+lkml@ethgen.de> wrote:
> > After solving  the conflicts, I applied the revert (see attachment) to
> > v3.18.7. I think it should also apply to the current head. With that
> > patch, suspend is working again on that version.
> >
> > However, I have not to deep knowledge of that subsystem, so please,
> > someone who have, have a deeper look into it. I especially do not know
> > if the lines in .../intel_pm.c are correct or better leaving them as
> > they are in v3.18.7.
> >
> > I want to have it working on a version that I know is stable before
> > asking to pull it to head.
> 
> Hi Klaus, we fear this patch may hide the actual cause. Would be useful
> to get a better description of what happens, along with a dmesg with
> drm.debug=14 module parameter set. This might clutter the mailing list,
> would you mind filing a bug at [1] and attach the info there?
> 
> Thanks,
> Jani.
> 
> [1] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel

In addition to the above could you also try the following patch and
provide a dmesg log on the bugzilla ticket - at this point only to try
to narrow down the issue?:

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d358ce8..02c65f4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4466,6 +4466,14 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
 		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
 
+	if (!intel_irqs_enabled(dev_priv)) {
+		if (printk_ratelimit())
+			DRM_ERROR("spurious/shared interrupt iir %08x imr %08x ier %08x\n",
+				  I915_READ(IIR), I915_READ(IMR), I915_READ(IER));
+
+		return IRQ_NONE;
+	}
+
 	iir = I915_READ(IIR);
 
 	for (;;) {
@@ -4766,7 +4774,10 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	dev->driver->irq_uninstall(dev);
+
+	spin_lock_irq(&dev_priv->irq_lock);
 	dev_priv->pm._irqs_disabled = true;
+	spin_unlock_irq(&dev_priv->irq_lock);
 }
 
 /* Restore interrupts so we can recover from runtime PM. */
@@ -4774,7 +4785,10 @@ void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	spin_lock_irq(&dev_priv->irq_lock);
 	dev_priv->pm._irqs_disabled = false;
+	spin_unlock_irq(&dev_priv->irq_lock);
+
 	dev->driver->irq_preinstall(dev);
 	dev->driver->irq_postinstall(dev);
 }

Re: [Intel-gfx] [KERNEL] Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Dave Gordon]

On 18/02/15 16:24, Imre Deak wrote:
> On ke, 2015-02-18 at 17:39 +0200, Jani Nikula wrote:
>> On Tue, 17 Feb 2015, Klaus Ethgen <Klaus+lkml@ethgen.de> wrote:
>>> After solving  the conflicts, I applied the revert (see attachment) to
>>> v3.18.7. I think it should also apply to the current head. With that
>>> patch, suspend is working again on that version.
>>>
>>> However, I have not to deep knowledge of that subsystem, so please,
>>> someone who have, have a deeper look into it. I especially do not know
>>> if the lines in .../intel_pm.c are correct or better leaving them as
>>> they are in v3.18.7.
>>>
>>> I want to have it working on a version that I know is stable before
>>> asking to pull it to head.
>>
>> Hi Klaus, we fear this patch may hide the actual cause. Would be useful
>> to get a better description of what happens, along with a dmesg with
>> drm.debug=14 module parameter set. This might clutter the mailing list,
>> would you mind filing a bug at [1] and attach the info there?
>>
>> Thanks,
>> Jani.
>>
>> [1] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel
> 
> In addition to the above could you also try the following patch and
> provide a dmesg log on the bugzilla ticket - at this point only to try
> to narrow down the issue?:
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d358ce8..02c65f4 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4466,6 +4466,14 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
>  		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
>  		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
>  
> +	if (!intel_irqs_enabled(dev_priv)) {
> +		if (printk_ratelimit())
> +			DRM_ERROR("spurious/shared interrupt iir %08x imr %08x ier %08x\n",
> +				  I915_READ(IIR), I915_READ(IMR), I915_READ(IER));
> +
> +		return IRQ_NONE;
> +	}
> +
>  	iir = I915_READ(IIR);
>  
>  	for (;;) {
> @@ -4766,7 +4774,10 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	dev->driver->irq_uninstall(dev);
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
>  	dev_priv->pm._irqs_disabled = true;
> +	spin_unlock_irq(&dev_priv->irq_lock);
>  }
>  
>  /* Restore interrupts so we can recover from runtime PM. */
> @@ -4774,7 +4785,10 @@ void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	spin_lock_irq(&dev_priv->irq_lock);
>  	dev_priv->pm._irqs_disabled = false;
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
>  	dev->driver->irq_preinstall(dev);
>  	dev->driver->irq_postinstall(dev);
>  }

Surely surrounding (what ought to be) an atomic assignment to a single
variable cannot make a difference? Unless it's the memory barrier
semantics that have some effect? It seems unlikely that the compiler has
deferred the write to the variable past the pre/postinstall calls that
actually enable the h/w interrupts, but maybe we should add "volatile"
just in case?

.Dave.

Re: [Intel-gfx] [KERNEL] Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Imre Deak]

On Thu, 2015-02-19 at 10:47 +0000, Dave Gordon wrote:
> On 18/02/15 16:24, Imre Deak wrote:
> > On ke, 2015-02-18 at 17:39 +0200, Jani Nikula wrote:
> >> On Tue, 17 Feb 2015, Klaus Ethgen <Klaus+lkml@ethgen.de> wrote:
> >>> After solving  the conflicts, I applied the revert (see attachment) to
> >>> v3.18.7. I think it should also apply to the current head. With that
> >>> patch, suspend is working again on that version.
> >>>
> >>> However, I have not to deep knowledge of that subsystem, so please,
> >>> someone who have, have a deeper look into it. I especially do not know
> >>> if the lines in .../intel_pm.c are correct or better leaving them as
> >>> they are in v3.18.7.
> >>>
> >>> I want to have it working on a version that I know is stable before
> >>> asking to pull it to head.
> >>
> >> Hi Klaus, we fear this patch may hide the actual cause. Would be useful
> >> to get a better description of what happens, along with a dmesg with
> >> drm.debug=14 module parameter set. This might clutter the mailing list,
> >> would you mind filing a bug at [1] and attach the info there?
> >>
> >> Thanks,
> >> Jani.
> >>
> >> [1] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel
> > 
> > In addition to the above could you also try the following patch and
> > provide a dmesg log on the bugzilla ticket - at this point only to try
> > to narrow down the issue?:
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index d358ce8..02c65f4 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -4466,6 +4466,14 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
> >  		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
> >  		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
> >  
> > +	if (!intel_irqs_enabled(dev_priv)) {
> > +		if (printk_ratelimit())
> > +			DRM_ERROR("spurious/shared interrupt iir %08x imr %08x ier %08x\n",
> > +				  I915_READ(IIR), I915_READ(IMR), I915_READ(IER));
> > +
> > +		return IRQ_NONE;
> > +	}
> > +
> >  	iir = I915_READ(IIR);
> >  
> >  	for (;;) {
> > @@ -4766,7 +4774,10 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> >  	dev->driver->irq_uninstall(dev);
> > +
> > +	spin_lock_irq(&dev_priv->irq_lock);
> >  	dev_priv->pm._irqs_disabled = true;
> > +	spin_unlock_irq(&dev_priv->irq_lock);
> >  }
> >  
> >  /* Restore interrupts so we can recover from runtime PM. */
> > @@ -4774,7 +4785,10 @@ void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	spin_lock_irq(&dev_priv->irq_lock);
> >  	dev_priv->pm._irqs_disabled = false;
> > +	spin_unlock_irq(&dev_priv->irq_lock);
> > +
> >  	dev->driver->irq_preinstall(dev);
> >  	dev->driver->irq_postinstall(dev);
> >  }
> 
> Surely surrounding (what ought to be) an atomic assignment to a single
> variable cannot make a difference? Unless it's the memory barrier
> semantics that have some effect? It seems unlikely that the compiler has
> deferred the write to the variable past the pre/postinstall calls that
> actually enable the h/w interrupts, but maybe we should add "volatile"
> just in case?

spinlocks also serve as a memory barrier. volatile would guarantee only
that the compiler doesn't reorder the write, so it wouldn't be enough
alone. Otoh, we may also need to add spinlocking to the interrupt
handler if the issue turns out to be that we can't access some register
past/before intel_runtime_pm_{disable,enable}_interrupts.

--Imre

Re: [Intel-gfx] [KERNEL] Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Dave Gordon]

On 19/02/15 11:08, Deak, Imre wrote:
> On Thu, 2015-02-19 at 10:47 +0000, Dave Gordon wrote:
>> On 18/02/15 16:24, Imre Deak wrote:
>>> On ke, 2015-02-18 at 17:39 +0200, Jani Nikula wrote:
>>>> On Tue, 17 Feb 2015, Klaus Ethgen <Klaus+lkml@ethgen.de> wrote:
>>>>> After solving  the conflicts, I applied the revert (see attachment) to
>>>>> v3.18.7. I think it should also apply to the current head. With that
>>>>> patch, suspend is working again on that version.
>>>>>
>>>>> However, I have not to deep knowledge of that subsystem, so please,
>>>>> someone who have, have a deeper look into it. I especially do not know
>>>>> if the lines in .../intel_pm.c are correct or better leaving them as
>>>>> they are in v3.18.7.
>>>>>
>>>>> I want to have it working on a version that I know is stable before
>>>>> asking to pull it to head.
>>>>
>>>> Hi Klaus, we fear this patch may hide the actual cause. Would be useful
>>>> to get a better description of what happens, along with a dmesg with
>>>> drm.debug=14 module parameter set. This might clutter the mailing list,
>>>> would you mind filing a bug at [1] and attach the info there?
>>>>
>>>> Thanks,
>>>> Jani.
>>>>
>>>> [1] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel
>>>
>>> In addition to the above could you also try the following patch and
>>> provide a dmesg log on the bugzilla ticket - at this point only to try
>>> to narrow down the issue?:
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>> index d358ce8..02c65f4 100644
>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>> @@ -4466,6 +4466,14 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
>>>  		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
>>>  		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
>>>  
>>> +	if (!intel_irqs_enabled(dev_priv)) {
>>> +		if (printk_ratelimit())
>>> +			DRM_ERROR("spurious/shared interrupt iir %08x imr %08x ier %08x\n",
>>> +				  I915_READ(IIR), I915_READ(IMR), I915_READ(IER));
>>> +
>>> +		return IRQ_NONE;
>>> +	}
>>> +
>>>  	iir = I915_READ(IIR);
>>>  
>>>  	for (;;) {
>>> @@ -4766,7 +4774,10 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>>  
>>>  	dev->driver->irq_uninstall(dev);
>>> +
>>> +	spin_lock_irq(&dev_priv->irq_lock);
>>>  	dev_priv->pm._irqs_disabled = true;
>>> +	spin_unlock_irq(&dev_priv->irq_lock);
>>>  }
>>>  
>>>  /* Restore interrupts so we can recover from runtime PM. */
>>> @@ -4774,7 +4785,10 @@ void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
>>>  {
>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>>  
>>> +	spin_lock_irq(&dev_priv->irq_lock);
>>>  	dev_priv->pm._irqs_disabled = false;
>>> +	spin_unlock_irq(&dev_priv->irq_lock);
>>> +
>>>  	dev->driver->irq_preinstall(dev);
>>>  	dev->driver->irq_postinstall(dev);
>>>  }
>>
>> Surely surrounding (what ought to be) an atomic assignment to a single
>> variable cannot make a difference? Unless it's the memory barrier
>> semantics that have some effect? It seems unlikely that the compiler has
>> deferred the write to the variable past the pre/postinstall calls that
>> actually enable the h/w interrupts, but maybe we should add "volatile"
>> just in case?
> 
> spinlocks also serve as a memory barrier. volatile would guarantee only
> that the compiler doesn't reorder the write, so it wouldn't be enough
> alone. Otoh, we may also need to add spinlocking to the interrupt
> handler if the issue turns out to be that we can't access some register
> past/before intel_runtime_pm_{disable,enable}_interrupts.
> 
> --Imre

If we were getting interrupts during the enabling sequence, there would
be three possibilities:
1.	compiler has delayed writeback. This would be a compiler bug
	(IMHO) but 'volatile' might fix it.
2.	the 'restore' thread has written the value, but it isn't seen
	by the thread handling the interrupt (on a different cpu?).
	This would be a coherency issue, and a memory barrier should
	fix it. But I would have expected the variable to be in
	coherent memory anyway.
3.	the GPU h/w sending interrupts before they're enabled :(

But I suspect this might be during *disabling* the interrupt. Possibly a
race condition in which the h/w has already generated an interrupt
before it's disabled, but that interrupt has not yet been fielded; so
that by the time the interrupt handler runs (on a different CPU from the
'suspend' thread), the write to the variable can have happened and then
the new value is seen by the interrupt handler.

In which case the tweak above will reduce but not necessarily eliminate
the window; it will ensure that if the handler has got at least as far
as taking the spinlock before the suspend thread, then the write will be
delayed until the handler has finished. OTOH if the interrupt were
delayed a little bit more, this code might still get to take the
spinlock before the handler, so the unexpected value would still be seen :(

Maybe you need a two-phase uninstall?

.Dave.

Re: [Intel-gfx] [KERNEL] Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Imre Deak]

On to, 2015-02-19 at 15:42 +0000, Dave Gordon wrote:
> On 19/02/15 11:08, Deak, Imre wrote:
> > On Thu, 2015-02-19 at 10:47 +0000, Dave Gordon wrote:
> >> On 18/02/15 16:24, Imre Deak wrote:
> >>> On ke, 2015-02-18 at 17:39 +0200, Jani Nikula wrote:
> >>>> On Tue, 17 Feb 2015, Klaus Ethgen <Klaus+lkml@ethgen.de> wrote:
> >>>>> After solving  the conflicts, I applied the revert (see attachment) to
> >>>>> v3.18.7. I think it should also apply to the current head. With that
> >>>>> patch, suspend is working again on that version.
> >>>>>
> >>>>> However, I have not to deep knowledge of that subsystem, so please,
> >>>>> someone who have, have a deeper look into it. I especially do not know
> >>>>> if the lines in .../intel_pm.c are correct or better leaving them as
> >>>>> they are in v3.18.7.
> >>>>>
> >>>>> I want to have it working on a version that I know is stable before
> >>>>> asking to pull it to head.
> >>>>
> >>>> Hi Klaus, we fear this patch may hide the actual cause. Would be useful
> >>>> to get a better description of what happens, along with a dmesg with
> >>>> drm.debug=14 module parameter set. This might clutter the mailing list,
> >>>> would you mind filing a bug at [1] and attach the info there?
> >>>>
> >>>> Thanks,
> >>>> Jani.
> >>>>
> >>>> [1] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel
> >>>
> >>> In addition to the above could you also try the following patch and
> >>> provide a dmesg log on the bugzilla ticket - at this point only to try
> >>> to narrow down the issue?:
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >>> index d358ce8..02c65f4 100644
> >>> --- a/drivers/gpu/drm/i915/i915_irq.c
> >>> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >>> @@ -4466,6 +4466,14 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
> >>>  		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
> >>>  		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
> >>>  
> >>> +	if (!intel_irqs_enabled(dev_priv)) {
> >>> +		if (printk_ratelimit())
> >>> +			DRM_ERROR("spurious/shared interrupt iir %08x imr %08x ier %08x\n",
> >>> +				  I915_READ(IIR), I915_READ(IMR), I915_READ(IER));
> >>> +
> >>> +		return IRQ_NONE;
> >>> +	}
> >>> +
> >>>  	iir = I915_READ(IIR);
> >>>  
> >>>  	for (;;) {
> >>> @@ -4766,7 +4774,10 @@ void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
> >>>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>  
> >>>  	dev->driver->irq_uninstall(dev);
> >>> +
> >>> +	spin_lock_irq(&dev_priv->irq_lock);
> >>>  	dev_priv->pm._irqs_disabled = true;
> >>> +	spin_unlock_irq(&dev_priv->irq_lock);
> >>>  }
> >>>  
> >>>  /* Restore interrupts so we can recover from runtime PM. */
> >>> @@ -4774,7 +4785,10 @@ void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
> >>>  {
> >>>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>>  
> >>> +	spin_lock_irq(&dev_priv->irq_lock);
> >>>  	dev_priv->pm._irqs_disabled = false;
> >>> +	spin_unlock_irq(&dev_priv->irq_lock);
> >>> +
> >>>  	dev->driver->irq_preinstall(dev);
> >>>  	dev->driver->irq_postinstall(dev);
> >>>  }
> >>
> >> Surely surrounding (what ought to be) an atomic assignment to a single
> >> variable cannot make a difference? Unless it's the memory barrier
> >> semantics that have some effect? It seems unlikely that the compiler has
> >> deferred the write to the variable past the pre/postinstall calls that
> >> actually enable the h/w interrupts, but maybe we should add "volatile"
> >> just in case?
> > 
> > spinlocks also serve as a memory barrier. volatile would guarantee only
> > that the compiler doesn't reorder the write, so it wouldn't be enough
> > alone. Otoh, we may also need to add spinlocking to the interrupt
> > handler if the issue turns out to be that we can't access some register
> > past/before intel_runtime_pm_{disable,enable}_interrupts.
> > 
> > --Imre
> 
> If we were getting interrupts during the enabling sequence, there would
> be three possibilities:
> 1.	compiler has delayed writeback. This would be a compiler bug
> 	(IMHO) but 'volatile' might fix it.
> 2.	the 'restore' thread has written the value, but it isn't seen
> 	by the thread handling the interrupt (on a different cpu?).
> 	This would be a coherency issue, and a memory barrier should
> 	fix it. But I would have expected the variable to be in
> 	coherent memory anyway.
> 3.	the GPU h/w sending interrupts before they're enabled :(

For enabling I want to make sure _irqs_disabled=false is stored to
memory before the interrupt handler can run. This flag is in normal
write-back memory, hence you need a memory barrier which is provided by
the spin lock. I could have also used here mb() or rely on the implicit
memory barrier of the iowrite in preinstall/postinstall hooks; I didn't
since as I said we may want to add spin locking to the irq handler too
anyway.

> But I suspect this might be during *disabling* the interrupt. Possibly a
> race condition in which the h/w has already generated an interrupt
> before it's disabled, but that interrupt has not yet been fielded; so
> that by the time the interrupt handler runs (on a different CPU from the
> 'suspend' thread), the write to the variable can have happened and then
> the new value is seen by the interrupt handler.
> 
> In which case the tweak above will reduce but not necessarily eliminate
> the window; it will ensure that if the handler has got at least as far
> as taking the spinlock before the suspend thread, then the write will be
> delayed until the handler has finished. OTOH if the interrupt were
> delayed a little bit more, this code might still get to take the
> spinlock before the handler, so the unexpected value would still be seen :(
> 
> Maybe you need a two-phase uninstall?

In case we add the locking to the interrupt handler too, it doesn't
matter how much the handler is delayed. We are either before taking the
lock in intel_runtime_pm_disable_interrupts() and so the handler will
see _irqs_disabled=false, or we are after releasing the lock in
intel_runtime_pm_disable_interrupts() and the handler will see
_irqs_disabled=true and just return without doing anything.

But this patch makes already sure that the handler will see
_irqs_disabled=true eventually and I think that's enough for this test.

--Imre

Re: [KERNEL] Re: [KERNEL] Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Klaus Ethgen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Hi,

Am Mi den 18. Feb 2015 um 16:39 schrieb Jani Nikula:
> On Tue, 17 Feb 2015, Klaus Ethgen <Klaus+lkml@ethgen.de> wrote:
> > After solving  the conflicts, I applied the revert (see attachment) to
> > v3.18.7. I think it should also apply to the current head. With that
> > patch, suspend is working again on that version.
> >
> > However, I have not to deep knowledge of that subsystem, so please,
> > someone who have, have a deeper look into it. I especially do not know
> > if the lines in .../intel_pm.c are correct or better leaving them as
> > they are in v3.18.7.
> >
> > I want to have it working on a version that I know is stable before
> > asking to pull it to head.
> 
> Hi Klaus, we fear this patch may hide the actual cause.

That might be. But that might be a second step, to find the actual
cause. I think first the patch that causes the problem should be
fixed/reverted to have a working kernel and the next step would be to
find the real cause.

I would happily help to solve both.

> Would be useful to get a better description of what happens,

Ask.. English is not my mother thong and I am not that good in finding
descriptions. ;-)

> along with a dmesg with drm.debug=14 module parameter set.

Oh, I would be happy to be able to provide that. But if you look at the
first mail in this thread, I asked days before how to debug that cause.
The problem is that it stops in the middle of the suspend right after
switching of screen. There is no way to get it back working after that
and so no dmesg. I have to hard switch off the laptop after that.

> This might clutter the mailing list, would you mind filing a bug at
> [1] and attach the info there?

One another account to just fill one bug? Or is there a way to fill the
bug without creating an account? I would really like to not create
another account. I still have that many from systems that forced me to
create an account for just one bug report.

Am Mi den 18. Feb 2015 um 17:24 schrieb Imre Deak:
> In addition to the above could you also try the following patch and
> provide a dmesg log on the bugzilla ticket - at this point only to try
> to narrow down the issue?:
> 
[PATCH]

Yes, I can do that. But can you tell me how to get dmesg output when the
problem happened? After that the box is unusable and I have to hard
switch off it. There is no way to get any dmesg output after the bug
happened.

Regards
   Klaus

Ps. I can forward the first two messages that I posted on kernel ML.
    That should also be available online.
- -- 
Klaus Ethgen                              http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16   Klaus Ethgen <Klaus@Ethgen.de>
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQGcBAEBCgAGBQJU5OIMAAoJEKZ8CrGAGfas/MUL/1EYB/BKzdXl2JKyktSqt/TF
ojXfGKXF6RPYukwIU4+ACXUVGWyZ2llWetbNxrP48MBjBHvXfPLUG0FcxagtE8hH
yw61xt3uhO6YPso4fBBZfaCeiU2A5sO3RDb5yQ9kKdpK1GkrCQoG1VsQRKDFZGdl
99dEXQ4mMFMv27HR22YKtVnqW+Iwzdifu3NcdVw7ioqfZrMlRJSPmJQpJ7avfqFO
qQzTOv9tFC2G29j6K1J4IeDRd5uagE/IoMQWMF1hvDJlx9iSr6dHSbq/7hZ6aOKZ
nVWJ8mF5VB8ReqwI6j+akP+SgShCSG1HqdBJ57/Z2ygDys1N4Yr2EzJ1Ee8qyYp/
ibjwTbCfNKR4Ugq95WY8Dhpr9nUXDB+0pyVYjLvUABkkguDchQeQdUkY50IUodVh
82QkGoATJx+CLY7o+xNRj0W8m/e/VuSMPiJwxwNgJbmipPgG0JhlptRCO5Go8VCA
woyPNWdaiLriPIo19mr2NTK8BlV2pT2HD6D7ehlJ7Q==
=CE8V
-----END PGP SIGNATURE-----

Re: [KERNEL] Re: [KERNEL] Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Imre Deak]

On Wed, 2015-02-18 at 20:03 +0100, Klaus Ethgen wrote:
> Hi,
> 
> Am Mi den 18. Feb 2015 um 16:39 schrieb Jani Nikula:
> > On Tue, 17 Feb 2015, Klaus Ethgen <Klaus+lkml@ethgen.de> wrote:
> > > After solving  the conflicts, I applied the revert (see attachment) to
> > > v3.18.7. I think it should also apply to the current head. With that
> > > patch, suspend is working again on that version.
> > >
> > > However, I have not to deep knowledge of that subsystem, so please,
> > > someone who have, have a deeper look into it. I especially do not know
> > > if the lines in .../intel_pm.c are correct or better leaving them as
> > > they are in v3.18.7.
> > >
> > > I want to have it working on a version that I know is stable before
> > > asking to pull it to head.
> > 
> > Hi Klaus, we fear this patch may hide the actual cause.
> 
> That might be. But that might be a second step, to find the actual
> cause. I think first the patch that causes the problem should be
> fixed/reverted to have a working kernel and the next step would be to
> find the real cause.
> 
> I would happily help to solve both.
> 
> > Would be useful to get a better description of what happens,
> 
> Ask.. English is not my mother thong and I am not that good in finding
> descriptions. ;-)
> 
> > along with a dmesg with drm.debug=14 module parameter set.
> 
> Oh, I would be happy to be able to provide that. But if you look at the
> first mail in this thread, I asked days before how to debug that cause.
> The problem is that it stops in the middle of the suspend right after
> switching of screen. There is no way to get it back working after that
> and so no dmesg. I have to hard switch off the laptop after that.
> 
> > This might clutter the mailing list, would you mind filing a bug at
> > [1] and attach the info there?
> 
> One another account to just fill one bug? Or is there a way to fill the
> bug without creating an account? I would really like to not create
> another account. I still have that many from systems that forced me to
> create an account for just one bug report.

Yes, you need an account to file a freedesktop bug. This would be really
great to keep records, but if this isn't possible for you, then please
email the logs to us. In the latter case I would also suggest not CC'ing
the mailing lists to reduce the clutter.

> Am Mi den 18. Feb 2015 um 17:24 schrieb Imre Deak:
> > In addition to the above could you also try the following patch and
> > provide a dmesg log on the bugzilla ticket - at this point only to try
> > to narrow down the issue?:
> > 
> [PATCH]
> 
> Yes, I can do that. But can you tell me how to get dmesg output when the
> problem happened? After that the box is unusable and I have to hard
> switch off it. There is no way to get any dmesg output after the bug
> happened.

Thanks. I'm hoping that the patch also gets rid of the problem, so you
could get the dmesg log easily. If the patch doesn't get rid of the hang
it'd be great if you could still try setting up netconsole and get the
log from another machine. To get a trace closer to the hang you could
also try
# echo N > /sys/module/printk/parameters/console_suspend
before suspending.

Thanks,
Imre

> Ps. I can forward the first two messages that I posted on kernel ML.
>     That should also be available online.

Re: [KERNEL] Re: [KERNEL] Regression bug in drm/i915, Wrong assumption in commit e11aa36 breaks suspend on at least lenovo x61

[Daniel Vetter]

On Wed, Feb 18, 2015 at 8:03 PM, Klaus Ethgen <Klaus+lkml@ethgen.de> wrote:
> Am Mi den 18. Feb 2015 um 16:39 schrieb Jani Nikula:
>> On Tue, 17 Feb 2015, Klaus Ethgen <Klaus+lkml@ethgen.de> wrote:
>> > After solving  the conflicts, I applied the revert (see attachment) to
>> > v3.18.7. I think it should also apply to the current head. With that
>> > patch, suspend is working again on that version.
>> >
>> > However, I have not to deep knowledge of that subsystem, so please,
>> > someone who have, have a deeper look into it. I especially do not know
>> > if the lines in .../intel_pm.c are correct or better leaving them as
>> > they are in v3.18.7.
>> >
>> > I want to have it working on a version that I know is stable before
>> > asking to pull it to head.
>>
>> Hi Klaus, we fear this patch may hide the actual cause.
>
> That might be. But that might be a second step, to find the actual
> cause. I think first the patch that causes the problem should be
> fixed/reverted to have a working kernel and the next step would be to
> find the real cause.

I think the rever also breaks soix on newer platforms. Instead I think
intel_runtime_pm_disable_interrupts needs to grow a synchronize_irq()
after having disabled all the interrupt sources. But that's just
drive-by debugging, I might be off the charts ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch