[PATCH] drm/nouveau/fb: fix suspend/resume fbcon

[PATCH] drm/nouveau/fb: fix suspend/resume fbcon

[Christoph Rudorff]

On resume of a hibernated notebook, I get garbled virtual consoles.

fb_set_suspend(*dev, state == 0 means dev is running ...)

This patch fixes that issue for me:

hibernate:
kernel: nouveau  [     DRM] suspending fbcon...
kernel: nouveau  [     DRM] suspending display...
kernel: nouveau  [     DRM] unpinning framebuffer(s)...
kernel: nouveau  [     DRM] evicting buffers...
kernel: nouveau  [     DRM] waiting for kernel channels to go idle...
kernel: nouveau  [     DRM] suspending client object trees...
kernel: nouveau  [     DRM] suspending kernel object tree...
resume:
kernel: nouveau  [     DRM] re-enabling device...
kernel: nouveau  [     DRM] resuming kernel object tree...
kernel: nouveau  [   VBIOS][0000:01:00.0] running init tables
kernel: nouveau  [     DRM] resuming client object trees...
kernel: nouveau  [     DRM] resuming display...
kernel: nouveau E[     PFB][0000:01:00.0] trapped write at 0x00007fe000 on channel 0x0000fee0 [unknown] BAR/PFIFO_WRITE/FB reason: PAGE_NOT_PRESENT
kernel: nouveau E[     PFB][0000:01:00.0] trapped write at 0x00007fe000 on channel 0x0000fee0 [unknown] BAR/PFIFO_WRITE/FB reason: PAGE_NOT_PRESENT
kernel: nouveau E[     PFB][0000:01:00.0] trapped write at 0x00007fe240 on channel 0x0000fee0 [unknown] BAR/PFIFO_WRITE/FB reason: PAGE_NOT_PRESENT
kernel: nouveau E[     PFB][0000:01:00.0] trapped write at 0x00007fe400 on channel 0x0000fee0 [unknown] BAR/PFIFO_WRITE/FB reason: PAGE_NOT_PRESENT

https://bugs.freedesktop.org/buglist.cgi?query_format=specific&order=relevance+desc&bug_status=__open__&product=&content=PAGE_NOT_PRESENT

https://bugs.freedesktop.org/show_bug.cgi?id=58556
https://bugs.freedesktop.org/show_bug.cgi?id=62835
https://bugs.freedesktop.org/show_bug.cgi?id=68037
https://bugs.freedesktop.org/show_bug.cgi?id=69029
https://bugs.freedesktop.org/show_bug.cgi?id=69928
---
 drivers/gpu/drm/nouveau/nouveau_drm.c   |    2 +-
 drivers/gpu/drm/nouveau/nouveau_fbcon.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 383f4e6..6148758 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -544,7 +544,7 @@ nouveau_do_resume(struct drm_device *dev)
 	nouveau_pm_resume(dev);
 
 	if (dev->mode_config.num_crtc) {
-		NV_INFO(drm, "resuming display...\n");
+		NV_INFO(drm, "resuming display and fbcon...\n");
 		nouveau_display_resume(dev);
 	}
 	return 0;
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index b035317..46e37c0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -514,10 +514,10 @@ void nouveau_fbcon_set_suspend(struct drm_device *dev, int state)
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	console_lock();
 	if (state == 0)
-		nouveau_fbcon_save_disable_accel(dev);
+		nouveau_fbcon_restore_accel(dev);
 	fb_set_suspend(drm->fbcon->helper.fbdev, state);
 	if (state == 1)
-		nouveau_fbcon_restore_accel(dev);
+		nouveau_fbcon_save_disable_accel(dev);
 	console_unlock();
 }
 
-- 
1.7.10.4

Re: [PATCH] drm/nouveau/fb: fix suspend/resume fbcon

[Emil Velikov]

On 03/10/13 15:41, Christoph Rudorff wrote:
> On resume of a hibernated notebook, I get garbled virtual consoles.
> 
> fb_set_suspend(*dev, state == 0 means dev is running ...)
> 
> This patch fixes that issue for me:
> 
Ouch, nice catch Christoph :)

Seems like the following commit flipped the logic unintentionally, thus
causing the issue. Stange enough I have no problems with s2d although I
must admit it's not the most common thing I do.

commit cf41d53bf5b95d77673b185cc3b20ae3257f79e2
Author: Ben Skeggs <bskeggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Wed Nov 9 14:31:16 2011 +1000

    drm/nouveau: re-jig fbcon suspend/resume process a little

    Signed-off-by: Ben Skeggs <bskeggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

> hibernate:
> kernel: nouveau  [     DRM] suspending fbcon...
> kernel: nouveau  [     DRM] suspending display...
> kernel: nouveau  [     DRM] unpinning framebuffer(s)...
> kernel: nouveau  [     DRM] evicting buffers...
> kernel: nouveau  [     DRM] waiting for kernel channels to go idle...
> kernel: nouveau  [     DRM] suspending client object trees...
> kernel: nouveau  [     DRM] suspending kernel object tree...
> resume:
> kernel: nouveau  [     DRM] re-enabling device...
> kernel: nouveau  [     DRM] resuming kernel object tree...
> kernel: nouveau  [   VBIOS][0000:01:00.0] running init tables
> kernel: nouveau  [     DRM] resuming client object trees...
> kernel: nouveau  [     DRM] resuming display...
> kernel: nouveau E[     PFB][0000:01:00.0] trapped write at 0x00007fe000 on channel 0x0000fee0 [unknown] BAR/PFIFO_WRITE/FB reason: PAGE_NOT_PRESENT
> kernel: nouveau E[     PFB][0000:01:00.0] trapped write at 0x00007fe000 on channel 0x0000fee0 [unknown] BAR/PFIFO_WRITE/FB reason: PAGE_NOT_PRESENT
> kernel: nouveau E[     PFB][0000:01:00.0] trapped write at 0x00007fe240 on channel 0x0000fee0 [unknown] BAR/PFIFO_WRITE/FB reason: PAGE_NOT_PRESENT
> kernel: nouveau E[     PFB][0000:01:00.0] trapped write at 0x00007fe400 on channel 0x0000fee0 [unknown] BAR/PFIFO_WRITE/FB reason: PAGE_NOT_PRESENT
> 
> https://bugs.freedesktop.org/buglist.cgi?query_format=specific&order=relevance+desc&bug_status=__open__&product=&content=PAGE_NOT_PRESENT
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=58556
> https://bugs.freedesktop.org/show_bug.cgi?id=62835
> https://bugs.freedesktop.org/show_bug.cgi?id=68037
> https://bugs.freedesktop.org/show_bug.cgi?id=69029
> https://bugs.freedesktop.org/show_bug.cgi?id=69928
> ---
>  drivers/gpu/drm/nouveau/nouveau_drm.c   |    2 +-
>  drivers/gpu/drm/nouveau/nouveau_fbcon.c |    4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 383f4e6..6148758 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -544,7 +544,7 @@ nouveau_do_resume(struct drm_device *dev)
>  	nouveau_pm_resume(dev);
>  
>  	if (dev->mode_config.num_crtc) {
> -		NV_INFO(drm, "resuming display...\n");
> +		NV_INFO(drm, "resuming display and fbcon...\n");
>  		nouveau_display_resume(dev);
>  	}
>  	return 0;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> index b035317..46e37c0 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> @@ -514,10 +514,10 @@ void nouveau_fbcon_set_suspend(struct drm_device *dev, int state)
>  	struct nouveau_drm *drm = nouveau_drm(dev);
>  	console_lock();
>  	if (state == 0)
> -		nouveau_fbcon_save_disable_accel(dev);
> +		nouveau_fbcon_restore_accel(dev);
>  	fb_set_suspend(drm->fbcon->helper.fbdev, state);
>  	if (state == 1)
> -		nouveau_fbcon_restore_accel(dev);
> +		nouveau_fbcon_save_disable_accel(dev);
>  	console_unlock();
>  }
I'm not entirely sure this is correct. One needs to save and disable
accleration before suspending the fb. Please try the following

-	if (state == 0)
+	if (state == 1)
		nouveau_fbcon_save_disable_accel(dev);
	fb_set_suspend(drm->fbcon->helper.fbdev, state);
-	if (state == 1)
+	if (state == 0)
		nouveau_fbcon_restore_accel(dev);
	console_unlock();

Cheers,
Emil

Re: [PATCH] drm/nouveau/fb: fix suspend/resume fbcon

[Christoph Rudorff]

Am Donnerstag, den 03.10.2013, 23:50 +0100 schrieb Emil Velikov:
> I'm not entirely sure this is correct. One needs to save and disable
> accleration before suspending the fb. Please try the following
> 
> -	if (state == 0)
> +	if (state == 1)
> 		nouveau_fbcon_save_disable_accel(dev);
> 	fb_set_suspend(drm->fbcon->helper.fbdev, state);
> -	if (state == 1)
> +	if (state == 0)
> 		nouveau_fbcon_restore_accel(dev);
> 	console_unlock();
> 
> Cheers,
> Emil

Hi!

That was my first try! I guessed the same but I got exactly one trap
message on resume.

So it's about first put the bucket and then open the water tap.

;)

chris

ps: just found these macros for the state in fb.h:

FBINFO_STATE_RUNNING = 0
FBINFO_STATE_SUSPENDED = 1

Re: [PATCH] drm/nouveau/fb: fix suspend/resume fbcon

[Emil Velikov]

On 04/10/13 01:54, Christoph Rudorff wrote:
> Am Donnerstag, den 03.10.2013, 23:50 +0100 schrieb Emil Velikov:
>> I'm not entirely sure this is correct. One needs to save and disable
>> accleration before suspending the fb. Please try the following
>>
>> -	if (state == 0)
>> +	if (state == 1)
>> 		nouveau_fbcon_save_disable_accel(dev);
>> 	fb_set_suspend(drm->fbcon->helper.fbdev, state);
>> -	if (state == 1)
>> +	if (state == 0)
>> 		nouveau_fbcon_restore_accel(dev);
>> 	console_unlock();
>>
>> Cheers,
>> Emil
> 
> Hi!
> 
> That was my first try! I guessed the same but I got exactly one trap
> message on resume.
> 
Hi Chris,

Just got around to playing with s2disk on my laptop(nv96) and AFAICS
it seems to be OK without either patch.

Can you provide some more context regarding the issue ?
* What hardware are you running
* Which kernel are you having problems with
* Can you resume from s2disk correctly if you never start X
* Do you have the problem with s2ram
* dmesg without and with either patch

It might be useful if you can open a bug report and attach the
information in there [1]

Cheers,
Emil

[1] http://nouveau.freedesktop.org/wiki/Bugs/

> So it's about first put the bucket and then open the water tap.
> 
> ;)
> 
> chris
> 
> ps: just found these macros for the state in fb.h:
> 
> FBINFO_STATE_RUNNING = 0
> FBINFO_STATE_SUSPENDED = 1
> 
> 

Re: [PATCH] drm/nouveau/fb: fix suspend/resume fbcon

[Christoph Rudorff]

On 17.11.2013 00:22, Emil Velikov wrote:
> On 04/10/13 01:54, Christoph Rudorff wrote:
>> Am Donnerstag, den 03.10.2013, 23:50 +0100 schrieb Emil Velikov:
>>> I'm not entirely sure this is correct. One needs to save and disable
>>> accleration before suspending the fb. Please try the following
>>>
>>> -	if (state == 0)
>>> +	if (state == 1)
>>> 		nouveau_fbcon_save_disable_accel(dev);
>>> 	fb_set_suspend(drm->fbcon->helper.fbdev, state);
>>> -	if (state == 1)
>>> +	if (state == 0)
>>> 		nouveau_fbcon_restore_accel(dev);
>>> 	console_unlock();
>>>
>>> Cheers,
>>> Emil
>>
>> Hi!
>>
>> That was my first try! I guessed the same but I got exactly one trap
>> message on resume.
>>
> Hi Chris,
> 

Hi Emil!

> Just got around to playing with s2disk on my laptop(nv96) and AFAICS
> it seems to be OK without either patch.

That does not make me wonder.

The "nouveau_fbcon_restore_accel" is restoring from uninitialized memory.
So the resulting behavior is undefined. It must not fail.

(My experience with vga-drivers is, it may makes a difference if u just
reboot or power down the machine when testing another kernel!
And I do believe we can't catch the remaining bugs just with testing.)

> Can you provide some more context regarding the issue ?
> * What hardware are you running

A MacBookPro6,2 with GT216 [GeForce GT 330M]
Debian 6
Boot via UEFI, not using pc-bios emulation.

It's my working horse and usually an uptime monster thanks to s2disk.
So I can't test that much and only during a mandatory kernel upgrade.

> * Which kernel are you having problems with

That's the fun part. I was stuck with 3.6.9 which was fine and later
kernels were a mess. I did not upgrade any more until longterm 3.10 came
out. However, this bug was already in 3.6.9, but it did not trigger:

3.6.9  ok
3.6.10 problems  *)
3.7.1  problems
... never touch a running system ... until
3.10.13 ok
3.10.14 problems
...

*) There were no changes below drivers/gpu/drm/nouveau !

I think the difference is, that the kernel size increased so memory
allocations were just shifted ... so we may get an used memory page, no?
It's a race condition.

> * Can you resume from s2disk correctly if you never start X

The machine never locked up and X was also fine. The console gets messed:
just snow. A s2ram clears it up. I may test this later.

> * Do you have the problem with s2ram

No.

> * dmesg without and with either patch

Blame me. I didn't kept them. But I remember I diffed them but that did
not gave any hint. I found it by reading the source code.

> 
> It might be useful if you can open a bug report and attach the
> information in there [1]

I didn't opened a report because there are already many reports about
garbled console.

I just thought that the actual nouveau_fbcon_set_suspend() is obviously
doing the wrong thing and the fix is too easy.

And the remaining snow on the console, which other users get, are other
causes.

Actually reproducible is this (also with my patch):

1. Fresh boot up, run mplayer (-vo xv), switch to fullscreen: GPU lockup.
If I do a s2ram before, it works.

https://bugs.freedesktop.org/show_bug.cgi?id=68037
^^ same issue?! Are we missing some initialization?

2. Try a sysrq-"space"-key on console: The help text, which should appear,
is just snow (Can you reproduce this?).

3. On boot up, I'll still get some snow just before X start, _sometimes_,
not always: 3-4 lines of
nouveau E[     PFB][0000:01:00.0] trapped write at 0x0000710540 on channel
0x0000fee0 [unknown] BAR/PFIFO_WRITE/FB reason: PAGE_NOT_PRESENT

greets,

chris


> 
> Cheers,
> Emil
> 
> [1] http://nouveau.freedesktop.org/wiki/Bugs/
> 
>> So it's about first put the bucket and then open the water tap.
>>
>> ;)
>>
>> chris
>>
>> ps: just found these macros for the state in fb.h:
>>
>> FBINFO_STATE_RUNNING = 0
>> FBINFO_STATE_SUSPENDED = 1
>>
>>
> 

Re: [PATCH] drm/nouveau/fb: fix suspend/resume fbcon

[Emil Velikov]

On 17/11/13 18:30, Christoph Rudorff wrote:
> On 17.11.2013 00:22, Emil Velikov wrote:
>> On 04/10/13 01:54, Christoph Rudorff wrote:
>>> Am Donnerstag, den 03.10.2013, 23:50 +0100 schrieb Emil Velikov:
>>>> I'm not entirely sure this is correct. One needs to save and disable
>>>> accleration before suspending the fb. Please try the following
>>>>
>>>> -	if (state == 0)
>>>> +	if (state == 1)
>>>> 		nouveau_fbcon_save_disable_accel(dev);
>>>> 	fb_set_suspend(drm->fbcon->helper.fbdev, state);
>>>> -	if (state == 1)
>>>> +	if (state == 0)
>>>> 		nouveau_fbcon_restore_accel(dev);
>>>> 	console_unlock();
>>>>
>>>> Cheers,
>>>> Emil
>>>
>>> Hi!
>>>
>>> That was my first try! I guessed the same but I got exactly one trap
>>> message on resume.
>>>
>> Hi Chris,
>>
> 
> Hi Emil!
> 
>> Just got around to playing with s2disk on my laptop(nv96) and AFAICS
>> it seems to be OK without either patch.
> 
> That does not make me wonder.
> 
> The "nouveau_fbcon_restore_accel" is restoring from uninitialized memory.
> So the resulting behavior is undefined. It must not fail.
> 
> (My experience with vga-drivers is, it may makes a difference if u just
> reboot or power down the machine when testing another kernel!
> And I do believe we can't catch the remaining bugs just with testing.)
> 
>> Can you provide some more context regarding the issue ?
>> * What hardware are you running
> 
> A MacBookPro6,2 with GT216 [GeForce GT 330M]
> Debian 6
> Boot via UEFI, not using pc-bios emulation.
> 
> It's my working horse and usually an uptime monster thanks to s2disk.
> So I can't test that much and only during a mandatory kernel upgrade.
> 
>> * Which kernel are you having problems with
> 
> That's the fun part. I was stuck with 3.6.9 which was fine and later
> kernels were a mess. I did not upgrade any more until longterm 3.10 came
> out. However, this bug was already in 3.6.9, but it did not trigger:
> 
> 3.6.9  ok
> 3.6.10 problems  *)
> 3.7.1  problems
> ... never touch a running system ... until
> 3.10.13 ok
> 3.10.14 problems
> ...
> 
> *) There were no changes below drivers/gpu/drm/nouveau !
> 
> I think the difference is, that the kernel size increased so memory
> allocations were just shifted ... so we may get an used memory page, no?
> It's a race condition.
> 
>> * Can you resume from s2disk correctly if you never start X
> 
> The machine never locked up and X was also fine. The console gets messed:
> just snow. A s2ram clears it up. I may test this later.
> 
>> * Do you have the problem with s2ram
> 
> No.
> 
>> * dmesg without and with either patch
> 
> Blame me. I didn't kept them. But I remember I diffed them but that did
> not gave any hint. I found it by reading the source code.
> 
>>
>> It might be useful if you can open a bug report and attach the
>> information in there [1]
> 
> I didn't opened a report because there are already many reports about
> garbled console.
> 
> I just thought that the actual nouveau_fbcon_set_suspend() is obviously
> doing the wrong thing and the fix is too easy.
> 
> And the remaining snow on the console, which other users get, are other
> causes.
> 
> Actually reproducible is this (also with my patch):
> 
> 1. Fresh boot up, run mplayer (-vo xv), switch to fullscreen: GPU lockup.
> If I do a s2ram before, it works.
> 
Hi Chris,

This seems like an orthogonal issue.
When you say "before", I'm assuming you mean before running mplayer ?
Does the lockup occur only with xv, or does 3d/gl apps trigger it as well ?

You can avoid the s2ram cycle my telling nouveau to forcepost the card
during boot by appending nouveau.config=NvForcePost=1 to your kernel
command line.

When you have some time you can bisect which init script (executed
during post) resolves the problem.

> https://bugs.freedesktop.org/show_bug.cgi?id=68037
> ^^ same issue?! Are we missing some initialization?
> 

Good idea, I normally avoid comparing parts of dmesg/nouveau output
especially when different cards are used.

Back to the patch/original issue:
Seems I was rather too thick to pick up on your "put the bucket and then
open the tap" comment.

Would you mind resending your original patch with a slightly more
descriptive commit message (silly example below) and Cc
stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org + bskeggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org ?

drm/nouveau/fbcon: fix suspend/resume fbcon

Current code disables fbcon acceleration before fbcon is suspended,
leading to corrupted console after resume from s2disk. In a similar
fashion we must make sure that fbcon acceleration is enabled before we
revive the console.

With this patch s2disk works correctly on my A MacBookPro6,2 with GT216
[GeForce GT 330M]

Thanks,
Emil