drm/agp/i915: trim stolen space to 32M

PATCH In review Jesse Barnes 2 participants 4 messages Jul 7, 2010 – Jul 8, 2010

Subsystems (3)

DRM drivers Intel i915 Agpgart driver

lore

Share & tools

lore

Jesse Barnes

author

2010-07-07 21:40 UTC #0 lore raw

PATCH 1/1 drm/agp/i915: trim stolen space to 32M

Some BIOSes will claim a large chunk of stolen space. Unless we reclaim it, our aperture for remapping buffer objects will be constrained. So clamp the stolen space to 32M and ignore the rest.

Fixes https://bugzilla.kernel.org/show_bug.cgi?id=15469 among others.

Adding the ignored stolen memory back into the general pool using the memory hotplug code is left as an exercise for the reader.

Signed-off-by Jesse Barnes <[email protected]>

drivers/char/agp/intel-gtt.c +10 -1

index f97122a..54ed0e1 100644
@@ -25,6 +25,10 @@
 #define USE_PCI_DMA_API 1
 #endif
 
+/* Max amount of stolen space, anything above will be returned to Linux */
+int intel_max_stolen = 16 * 1024 * 1024;
+EXPORT_SYMBOL(intel_max_stolen);
+
 static const struct aper_size_info_fixed intel_i810_sizes[] =
 {
 	{64, 16384, 4},

@@ -710,7 +714,12 @@ static void intel_i830_init_gtt_entries(void)
 			break;
 		}
 	}
-	if (gtt_entries > 0) {
+	if (!local && gtt_entries > intel_max_stolen) {
+		dev_info(&agp_bridge->dev->dev,
+			 "detected %dK stolen memory, trimming to %dK\n",
+			 gtt_entries / KB(1), intel_max_stolen / KB(1));
+		gtt_entries = intel_max_stolen / KB(4);
+	} else if (gtt_entries > 0) {
 		dev_info(&agp_bridge->dev->dev, "detected %dK %s memory\n",
 		       gtt_entries / KB(1), local ? "local" : "stolen");
 		gtt_entries /= KB(4);

drivers/gpu/drm/i915/i915_dma.c +8 -0

index e2dd903..69e25ab 100644
@@ -40,6 +40,8 @@
 #include <linux/vga_switcheroo.h>
 #include <linux/slab.h>
 
+extern int intel_max_stolen; /* from AGP driver */
+
 /**
  * Sets up the hardware status page for devices that need a physical address
  * in the register.

@@ -2105,6 +2107,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	if (ret)
 		goto out_iomapfree;
 
+	if (prealloc_size > intel_max_stolen) {
+		DRM_INFO("detected %dM stolen memory, trimming to %dM\n",
+			 prealloc_size >> 20, intel_max_stolen >> 20);
+		prealloc_size = intel_max_stolen;
+	}
+
 	dev_priv->wq = create_singlethread_workqueue("i915");
 	if (dev_priv->wq == NULL) {
 		DRM_ERROR("Failed to create our workqueue.\n");

Simon Farnsworth

2010-07-08 09:58 UTC #1 lore raw

Quoted 4 lines · Jesse Barnes

Some BIOSes will claim a large chunk of stolen space. Unless we reclaim it, our aperture for remapping buffer objects will be constrained. So clamp the stolen space to 32M and ignore the rest.

I'm not sure that this changelog fits the patch - if I'm understanding the code correctly, you're clamping to 16M, not 32M.

Apart from that, the code looks sensible.

drivers/char/agp/intel-gtt.c line 29

Quoted 10 lines

drivers/char/agp/intel-gtt.c +2 -0

index f97122a..54ed0e1 100644
@@ -25,6 +25,10 @@
 #define USE_PCI_DMA_API 1
 #endif
 
+/* Max amount of stolen space, anything above will be returned to Linux */
+int intel_max_stolen = 16 * 1024 * 1024;

This is 16M, not 32M

Quoted 6 lines

drivers/char/agp/intel-gtt.c +2 -0

+EXPORT_SYMBOL(intel_max_stolen);
+
 static const struct aper_size_info_fixed intel_i810_sizes[] =
 {
 	{64, 16384, 4},

drivers/char/agp/intel-gtt.c static void intel_i830_init_gtt_entries(void) · line 721

Quoted 9 lines

drivers/char/agp/intel-gtt.c +5 -1

@@ -710,7 +714,12 @@ static void intel_i830_init_gtt_entries(void)
 			break;
 		}
 	}
-	if (gtt_entries > 0) {
+	if (!local && gtt_entries > intel_max_stolen) {
+		dev_info(&agp_bridge->dev->dev,
+			 "detected %dK stolen memory, trimming to %dK\n",
+			 gtt_entries / KB(1), intel_max_stolen / KB(1));
+		gtt_entries = intel_max_stolen / KB(4);

This appears to limit to intel_max_stolen bytes, not intel_max_stolen * 2 bytes.

Quoted 18 lines

drivers/char/agp/intel-gtt.c +1 -0

+	} else if (gtt_entries > 0) {
 		dev_info(&agp_bridge->dev->dev, "detected %dK %s memory\n",
 		       gtt_entries / KB(1), local ? "local" : "stolen");
 		gtt_entries /= KB(4);

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_dma.c index e2dd903..69e25ab 100644

drivers/gpu/drm/i915/i915_dma.c +2 -0

@@ -40,6 +40,8 @@
 #include <linux/vga_switcheroo.h>
 #include <linux/slab.h>
 
+extern int intel_max_stolen; /* from AGP driver */
+
 /**
  * Sets up the hardware status page for devices that need a physical

address * in the register.

drivers/gpu/drm/i915/i915_dma.c int i915_driver_load(struct drm_device *dev, · line 2113

Quoted 7 lines

drivers/gpu/drm/i915/i915_dma.c +4 -0

@@ -2105,6 +2107,12 @@ int i915_driver_load(struct drm_device *dev,
 unsigned long flags) if (ret)
 		goto out_iomapfree;
 
+	if (prealloc_size > intel_max_stolen) {
+		DRM_INFO("detected %dM stolen memory, trimming to %dM\n",
+			 prealloc_size >> 20, intel_max_stolen >> 20);
+		prealloc_size = intel_max_stolen;

And again here, you appear to limit to intel_max_stolen, not to twice that.

Quoted 5 lines

drivers/gpu/drm/i915/i915_dma.c +2 -0

+	}
+
 	dev_priv->wq = create_singlethread_workqueue("i915");
 	if (dev_priv->wq == NULL) {
 		DRM_ERROR("Failed to create our workqueue.\n");

Signature · 4 lines

Simon Farnsworth
Software Engineer
ONELAN Limited
http://www.onelan.com/

Jesse Barnes

author

2010-07-08 16:02 UTC #2 lore raw

Quoted 9 lines · On Thu, 8 Jul 2010 10:58:21 +0100 Simon Farnsworth…

Quoted 4 lines · Jesse Barnes
Some BIOSes will claim a large chunk of stolen space. Unless we reclaim it, our aperture for remapping buffer objects will be constrained. So clamp the stolen space to 32M and ignore the rest.
I'm not sure that this changelog fits the patch - if I'm understanding the code correctly, you're clamping to 16M, not 32M.
Apart from that, the code looks sensible.

Oops updated to 32M for Ironlake but didn't update the changelog or comments.

Quoted 24 lines

drivers/char/agp/intel-gtt.c line 29

Quoted 11 lines

drivers/char/agp/intel-gtt.c +2 -0

index f97122a..54ed0e1 100644
@@ -25,6 +25,10 @@
 #define USE_PCI_DMA_API 1
 #endif
 
+/* Max amount of stolen space, anything above will be returned to Linux */
+int intel_max_stolen = 16 * 1024 * 1024;

This is 16M, not 32M

Quoted 10 lines

drivers/char/agp/intel-gtt.c +2 -1

+EXPORT_SYMBOL(intel_max_stolen);
+
 static const struct aper_size_info_fixed intel_i810_sizes[] =
 {
 	{64, 16384, 4},

@@ -710,7 +714,12 @@ static void intel_i830_init_gtt_entries(void)
 			break;
 		}
 	}
-	if (gtt_entries > 0) {

Quoted 8 lines

drivers/char/agp/intel-gtt.c static void intel_i830_init_gtt_entries(void) · line 721
Quoted 5 lines
drivers/char/agp/intel-gtt.c +5 -0
+	if (!local && gtt_entries > intel_max_stolen) {
+		dev_info(&agp_bridge->dev->dev,
+			 "detected %dK stolen memory, trimming to %dK\n",
+			 gtt_entries / KB(1), intel_max_stolen / KB(1));
+		gtt_entries = intel_max_stolen / KB(4);
This appears to limit to intel_max_stolen bytes, not intel_max_stolen * 2 bytes.

I think that's what I want to do, so I'm not sure what you mean? Each GTT entry covers a 4k page so we have to convert the total size into the number of entries...

Quoted 34 lines

Quoted 19 lines
drivers/char/agp/intel-gtt.c +1 -0
+	} else if (gtt_entries > 0) {
 		dev_info(&agp_bridge->dev->dev, "detected %dK %s memory\n",
 		       gtt_entries / KB(1), local ? "local" : "stolen");
 		gtt_entries /= KB(4);
(quoted diff) +0 -0
b/drivers/gpu/drm/i915/i915_dma.c index e2dd903..69e25ab 100644
drivers/gpu/drm/i915/i915_dma.c +2 -0
@@ -40,6 +40,8 @@
 #include <linux/vga_switcheroo.h>
 #include <linux/slab.h>
 
+extern int intel_max_stolen; /* from AGP driver */
+
 /**
  * Sets up the hardware status page for devices that need a physical
address * in the register.
drivers/gpu/drm/i915/i915_dma.c int i915_driver_load(struct drm_device *dev, · line 2113
Quoted 7 lines
drivers/gpu/drm/i915/i915_dma.c +4 -0
@@ -2105,6 +2107,12 @@ int i915_driver_load(struct drm_device *dev,
 unsigned long flags) if (ret)
 		goto out_iomapfree;
 
+	if (prealloc_size > intel_max_stolen) {
+		DRM_INFO("detected %dM stolen memory, trimming to %dM\n",
+			 prealloc_size >> 20, intel_max_stolen >> 20);
+		prealloc_size = intel_max_stolen;
And again here, you appear to limit to intel_max_stolen, not to twice that.
Quoted 5 lines
drivers/gpu/drm/i915/i915_dma.c +2 -0
+	}
+
 	dev_priv->wq = create_singlethread_workqueue("i915");
 	if (dev_priv->wq == NULL) {
 		DRM_ERROR("Failed to create our workqueue.\n");

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Jesse Barnes

author

2010-07-08 16:15 UTC #3 lore raw

Quoted 8 lines · On Thu, 8 Jul 2010 09:01:44 -0700 Jesse Barnes <[email protected]> wrote:

Quoted 5 lines · On Thu, 8 Jul 2010 10:58:21 +0100 Simon Farnsworth…
Quoted 4 lines · Jesse Barnes
Some BIOSes will claim a large chunk of stolen space. Unless we reclaim it, our aperture for remapping buffer objects will be constrained. So clamp the stolen space to 32M and ignore the rest.

Quoted 7 lines

Quoted 4 lines
I'm not sure that this changelog fits the patch - if I'm understanding the code correctly, you're clamping to 16M, not 32M.
Apart from that, the code looks sensible.
Oops updated to 32M for Ironlake but didn't update the changelog or comments.

Oh and I see you were trying to tell me that I failed to update the code too. Sigh.

I'll post an updated version with the code and comments fixed.

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center