Skip to contentSkip to conversation

drm/nouveau: fix early vram corruption originating from vgacon

PATCH In review Marcin Slusarz 2 participants 4 messages Sep 12, 2012 – Oct 5, 2012 3 messages not in the archive

Subsystems (3)
DRM drivers DRM misc nouveau
lore
Share & tools
lore

Marcin Slusarz

author
#0 lore raw

PATCH 1/1 drm/nouveau: fix early vram corruption originating from vgacon

There's a short window between module load and fbcon initalization when it's possible for vgacon to write to VGA RAM. Nouveau uses this memory for different purposes, so if we are unlucky, it causes mysterious memory corruptions.

For me, booting with nv_printk debug levels set to 5 was enough to trigger it. It manifested as long stream of:
"trapped write at ... on channel 0x0001fea0 BAR/PFIFO_WRITE/IN reason:
DMAOBJ_LIMIT / PT_NOT_PRESENT / PAGE_SYSTEM_ONLY / PAGE_NOT_PRESENT" which eventually lead to complete hang.

Disabling access to VGA memory (through 0x54 PCI config space register) is enough to fix it, but it breaks copying screen data between old and new console (because old data is inaccessible). But blanking console (with entering_gfx==1) is enough to move vgacon screen buffer from VRAM to RAM and let handover to work correctly.

Signed-off-by Marcin Slusarz <[email protected]>
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)
drivers/gpu/drm/nouveau/nouveau_drm.c +30 -1
index 6826525..1641bd9 100644
@@ -25,6 +25,7 @@
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/vt_kern.h>
 
 #include <core/device.h>
 #include <core/client.h>

@@ -51,6 +52,8 @@
 
 #include "nouveau_ttm.h"
 
+#define NV_PCI_VGAMEM_ENABLE 0x54
+
 MODULE_PARM_DESC(config, "option string to pass to driver core");
 static char *nouveau_config;
 module_param_named(config, nouveau_config, charp, 0400);

@@ -247,9 +250,20 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags)
 	struct nouveau_drm *drm;
 	int ret;
 
+	/* Blank initial console to prevent VRAM corruption while we initialize
+	 * the HW. For vgacon it will move console memory from VGA VRAM to RAM.
+	 */
+	console_lock();
+	do_blank_screen(1);
+	console_unlock();
+
+	/* Completely disable access to VGA IO/memory, just to be sure no one
+	 * will change it. */
+	pci_write_config_byte(pdev, NV_PCI_VGAMEM_ENABLE, 0);
+
 	ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm), (void**)&drm);
 	if (ret)
-		return ret;
+		goto fail_cli;
 
 	dev->dev_private = drm;
 	drm->dev = dev;

@@ -336,6 +350,11 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags)
 
 	nouveau_accel_init(drm);
 	nouveau_fbcon_init(dev);
+
+	console_lock();
+	do_unblank_screen(1);
+	console_unlock();
+
 	return 0;
 
 fail_dispinit:

@@ -351,12 +370,20 @@ fail_ttm:
 	nouveau_vga_fini(drm);
 fail_device:
 	nouveau_cli_destroy(&drm->client);
+fail_cli:
+	pci_write_config_byte(pdev, NV_PCI_VGAMEM_ENABLE, 1);
+
+	console_lock();
+	do_unblank_screen(1);
+	console_unlock();
+
 	return ret;
 }
 
 static int
 nouveau_drm_unload(struct drm_device *dev)
 {
+	struct pci_dev *pdev = dev->pdev;
 	struct nouveau_drm *drm = nouveau_drm(dev);
 
 	nouveau_fbcon_fini(dev);

@@ -375,6 +402,8 @@ nouveau_drm_unload(struct drm_device *dev)
 	nouveau_vga_fini(drm);
 
 	nouveau_cli_destroy(&drm->client);
+
+	pci_write_config_byte(pdev, NV_PCI_VGAMEM_ENABLE, 1);
 	return 0;
 }
Signature · 1 line
1.7.12

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#2 lore raw
Quoted 100 lines · Marcin Slusarz

There's a short window between module load and fbcon initalization when it's possible for vgacon to write to VGA RAM. Nouveau uses this memory for different purposes, so if we are unlucky, it causes mysterious memory corruptions.

For me, booting with nv_printk debug levels set to 5 was enough to trigger it. It manifested as long stream of:
"trapped write at ... on channel 0x0001fea0 BAR/PFIFO_WRITE/IN reason:
DMAOBJ_LIMIT / PT_NOT_PRESENT / PAGE_SYSTEM_ONLY / PAGE_NOT_PRESENT" which eventually lead to complete hang.

Disabling access to VGA memory (through 0x54 PCI config space register) is enough to fix it, but it breaks copying screen data between old and new console (because old data is inaccessible). But blanking console (with entering_gfx==1) is enough to move vgacon screen buffer from VRAM to RAM and let handover to work correctly.

Signed-off-by Marcin Slusarz <[email protected]>
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)
drivers/gpu/drm/nouveau/nouveau_drm.c +28 -1
index 6826525..1641bd9 100644
@@ -25,6 +25,7 @@
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/vt_kern.h>
 
 #include <core/device.h>
 #include <core/client.h>

@@ -51,6 +52,8 @@
 
 #include "nouveau_ttm.h"
 
+#define NV_PCI_VGAMEM_ENABLE 0x54
+
 MODULE_PARM_DESC(config, "option string to pass to driver core");
 static char *nouveau_config;
 module_param_named(config, nouveau_config, charp, 0400);

@@ -247,9 +250,20 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags)
 	struct nouveau_drm *drm;
 	int ret;
 
+	/* Blank initial console to prevent VRAM corruption while we initialize
+	 * the HW. For vgacon it will move console memory from VGA VRAM to RAM.
+	 */
+	console_lock();
+	do_blank_screen(1);
+	console_unlock();
+
+	/* Completely disable access to VGA IO/memory, just to be sure no one
+	 * will change it. */
+	pci_write_config_byte(pdev, NV_PCI_VGAMEM_ENABLE, 0);
+
 	ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm), (void**)&drm);
 	if (ret)
-		return ret;
+		goto fail_cli;
 
 	dev->dev_private = drm;
 	drm->dev = dev;

@@ -336,6 +350,11 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags)
 
 	nouveau_accel_init(drm);
 	nouveau_fbcon_init(dev);
+
+	console_lock();
+	do_unblank_screen(1);
+	console_unlock();
+
 	return 0;
 
 fail_dispinit:

@@ -351,12 +370,20 @@ fail_ttm:
 	nouveau_vga_fini(drm);
 fail_device:
 	nouveau_cli_destroy(&drm->client);
+fail_cli:
+	pci_write_config_byte(pdev, NV_PCI_VGAMEM_ENABLE, 1);
+
+	console_lock();
+	do_unblank_screen(1);
+	console_unlock();
+
 	return ret;
 }
 
 static int
 nouveau_drm_unload(struct drm_device *dev)
 {
+	struct pci_dev *pdev = dev->pdev;
 	struct nouveau_drm *drm = nouveau_drm(dev);
 
 	nouveau_fbcon_fini(dev);

drivers/gpu/drm/nouveau/nouveau_drm.c nouveau_drm_unload(struct drm_device *dev) · line 409

Quoted 8 lines
drivers/gpu/drm/nouveau/nouveau_drm.c +2 -0
@@ -375,6 +402,8 @@ nouveau_drm_unload(struct drm_device *dev)
 	nouveau_vga_fini(drm);
 
 	nouveau_cli_destroy(&drm->client);
+
+	pci_write_config_byte(pdev, NV_PCI_VGAMEM_ENABLE, 1);
 	return 0;
 }

What's up with this patch?

A message that is not in the archive (sent off-list or not yet received): [email protected]

#4 lore raw
Quoted 77 lines · Marcin Slusarz
Quoted 76 lines · Marcin Slusarz

There's a short window between module load and fbcon initalization when it's possible for vgacon to write to VGA RAM. Nouveau uses this memory for different purposes, so if we are unlucky, it causes mysterious memory corruptions.

For me, booting with nv_printk debug levels set to 5 was enough to trigger it. It manifested as long stream of:
"trapped write at ... on channel 0x0001fea0 BAR/PFIFO_WRITE/IN reason:
DMAOBJ_LIMIT / PT_NOT_PRESENT / PAGE_SYSTEM_ONLY / PAGE_NOT_PRESENT" which eventually lead to complete hang.

Disabling access to VGA memory (through 0x54 PCI config space register) is enough to fix it, but it breaks copying screen data between old and new console (because old data is inaccessible). But blanking console (with entering_gfx==1) is enough to move vgacon screen buffer from VRAM to RAM and let handover to work correctly.

Signed-off-by Marcin Slusarz <[email protected]>
---
  drivers/gpu/drm/nouveau/nouveau_drm.c | 31 ++++++++++++++++++++++++++++++-
  1 file changed, 30 insertions(+), 1 deletion(-)
drivers/gpu/drm/nouveau/nouveau_drm.c +20 -1
index 6826525..1641bd9 100644
@@ -25,6 +25,7 @@
  #include <linux/console.h>
  #include <linux/module.h>
  #include <linux/pci.h>
+#include <linux/vt_kern.h>
 
  #include <core/device.h>
  #include <core/client.h>

@@ -51,6 +52,8 @@
 
  #include "nouveau_ttm.h"
 
+#define NV_PCI_VGAMEM_ENABLE 0x54
+
  MODULE_PARM_DESC(config, "option string to pass to driver core");
  static char *nouveau_config;
  module_param_named(config, nouveau_config, charp, 0400);

@@ -247,9 +250,20 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags)
  	struct nouveau_drm *drm;
  	int ret;
 
+	/* Blank initial console to prevent VRAM corruption while we initialize
+	 * the HW. For vgacon it will move console memory from VGA VRAM to RAM.
+	 */
+	console_lock();
+	do_blank_screen(1);
+	console_unlock();
+
+	/* Completely disable access to VGA IO/memory, just to be sure no one
+	 * will change it. */
+	pci_write_config_byte(pdev, NV_PCI_VGAMEM_ENABLE, 0);
+
  	ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm), (void**)&drm);
  	if (ret)
-		return ret;
+		goto fail_cli;
 
  	dev->dev_private = drm;
  	drm->dev = dev;

@@ -336,6 +350,11 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags)
 
  	nouveau_accel_init(drm);
  	nouveau_fbcon_init(dev);
+
+	console_lock();
+	do_unblank_screen(1);
+	console_unlock();
+
  	return 0;
Quoted 7 lines

drivers/gpu/drm/nouveau/nouveau_drm.c fail_ttm: · line 374

Quoted 7 lines
drivers/gpu/drm/nouveau/nouveau_drm.c +2 -0
  fail_dispinit:

@@ -351,12 +370,20 @@ fail_ttm:
  	nouveau_vga_fini(drm);
  fail_device:
  	nouveau_cli_destroy(&drm->client);
+fail_cli:
+	pci_write_config_byte(pdev, NV_PCI_VGAMEM_ENABLE, 1);

This (and corresponding unload line) is a bug: you should NOT blindly set this register to 1 on unload, use its previous value instead. Otherwise you can get into sticky situations involving two GPUs responding to VGA address space. Better yet, think up a proper solution involving vga arb.

Quoted 33 lines
Quoted 16 lines
drivers/gpu/drm/nouveau/nouveau_drm.c +6 -0
+
+	console_lock();
+	do_unblank_screen(1);
+	console_unlock();
+
  	return ret;
  }
 
  static int
  nouveau_drm_unload(struct drm_device *dev)
  {
+	struct pci_dev *pdev = dev->pdev;
  	struct nouveau_drm *drm = nouveau_drm(dev);
 
  	nouveau_fbcon_fini(dev);

drivers/gpu/drm/nouveau/nouveau_drm.c nouveau_drm_unload(struct drm_device *dev)

Quoted 9 lines
drivers/gpu/drm/nouveau/nouveau_drm.c +2 -1
@@ -375,6 +402,8 @@ nouveau_drm_unload(struct drm_device *dev)
  	nouveau_vga_fini(drm);
 
  	nouveau_cli_destroy(&drm->client);
+
+	pci_write_config_byte(pdev, NV_PCI_VGAMEM_ENABLE, 1);
  	return 0;
  }
 
--

What's up with this patch?

_______________________________________________
Nouveau mailing list [email protected]
http://lists.freedesktop.org/mailman/listinfo/nouveau

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#6 lore raw
Quoted 7 lines · Marcin Kościelnicki
Quoted 1 line · Marcin Slusarz

On Thu, Sep 13, 2012 at 12:52:30AM +0200, Marcin Slusarz wrote:

This (and corresponding unload line) is a bug: you should NOT blindly set this register to 1 on unload, use its previous value instead. Otherwise you can get into sticky situations involving two GPUs responding to VGA address space. Better yet, think up a proper solution involving vga arb.

Thanks to your comment I noticed two more things:
- I probably should have used dword API variant.
- There is already code in nouveau which bashes this register (nouveau_vga.c / nouveau_vga_set_decode) and it's wired to vgaarb. It seems it's set to 0 only when there is more than one card. I'm not sure how to handle it yet...

Marcin
_______________________________________________
Nouveau mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/nouveau