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

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

[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.

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 <jbarnes@virtuousgeek.org>

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index f97122a..54ed0e1 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -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);
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e2dd903..69e25ab 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -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");

Re: [PATCH] drm/agp/i915: trim stolen space to 32M

[Simon Farnsworth]

On Wednesday 7 July 2010, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 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.

> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index f97122a..54ed0e1 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -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

> +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);

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

> +	} 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);
> diff --git a/drivers/gpu/drm/i915/i915_dma.c
> b/drivers/gpu/drm/i915/i915_dma.c index e2dd903..69e25ab 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -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;

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

> +	}
> +
>  	dev_priv->wq = create_singlethread_workqueue("i915");
>  	if (dev_priv->wq == NULL) {
>  		DRM_ERROR("Failed to create our workqueue.\n");
-- 
Simon Farnsworth
Software Engineer
ONELAN Limited
http://www.onelan.com/

Re: [PATCH] drm/agp/i915: trim stolen space to 32M

[Jesse Barnes]

On Thu, 8 Jul 2010 10:58:21 +0100
Simon Farnsworth <simon.farnsworth@onelan.com> wrote:

> On Wednesday 7 July 2010, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > 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.

> 
> > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> > index f97122a..54ed0e1 100644
> > --- a/drivers/char/agp/intel-gtt.c
> > +++ b/drivers/char/agp/intel-gtt.c
> > @@ -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
> 
> > +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);
> 
> 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...

> 
> > +	} 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);
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c index e2dd903..69e25ab 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -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;
> 
> And again here, you appear to limit to intel_max_stolen, not to twice that.
> 
> > +	}
> > +
> >  	dev_priv->wq = create_singlethread_workqueue("i915");
> >  	if (dev_priv->wq == NULL) {
> >  		DRM_ERROR("Failed to create our workqueue.\n");


-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/agp/i915: trim stolen space to 32M

[Jesse Barnes]

On Thu, 8 Jul 2010 09:01:44 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Thu, 8 Jul 2010 10:58:21 +0100
> Simon Farnsworth <simon.farnsworth@onelan.com> wrote:
> 
> > On Wednesday 7 July 2010, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > 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.

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.

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[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.

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 <jbarnes@virtuousgeek.org>
---
 drivers/char/agp/intel-gtt.c    |   11 ++++++++++-
 drivers/gpu/drm/i915/i915_dma.c |    8 ++++++++
 2 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index f97122a..a61a87c 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -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 = 32 * 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);
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e2dd903..69e25ab 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -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");
-- 
1.7.0.1

Re: [PATCH] drm/agp/i915: trim stolen space to 32M

[Simon Farnsworth]

On Thursday 8 July 2010, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 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 <jbarnes@virtuousgeek.org>

FWIW, given how simple the code actually is:
Reviewed-by: Simon Farnsworth <simon.farnsworth@onelan.com>

> ---
>  drivers/char/agp/intel-gtt.c    |   11 ++++++++++-
>  drivers/gpu/drm/i915/i915_dma.c |    8 ++++++++
>  2 files changed, 18 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index f97122a..a61a87c 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -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 = 32 * 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);
> diff --git a/drivers/gpu/drm/i915/i915_dma.c
> b/drivers/gpu/drm/i915/i915_dma.c index e2dd903..69e25ab 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -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
Software Engineer
ONELAN Limited
http://www.onelan.com/

Re: [PATCH] drm/agp/i915: trim stolen space to 32M

[Chris Wilson]

On Thu, 8 Jul 2010 17:37:10 +0100, Simon Farnsworth <simon.farnsworth@onelan.com> wrote:
> On Thursday 8 July 2010, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > 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 <jbarnes@virtuousgeek.org>
> 
> FWIW, given how simple the code actually is:
> Reviewed-by: Simon Farnsworth <simon.farnsworth@onelan.com>

Heh, you were meant to say:

"What? Export a magic parameter from intel-gtt.c to indicate how much
space to remove from the aperture guestimate, but not replace the *broken*
code in i915_dma.c with an interface from intel-gtt.c to query and control
the populated GTT more precisely?"

;-)
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/agp/i915: trim stolen space to 32M

[Jesse Barnes]

On Thu, 08 Jul 2010 17:42:19 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 8 Jul 2010 17:37:10 +0100, Simon Farnsworth <simon.farnsworth@onelan.com> wrote:
> > On Thursday 8 July 2010, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > 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 <jbarnes@virtuousgeek.org>
> > 
> > FWIW, given how simple the code actually is:
> > Reviewed-by: Simon Farnsworth <simon.farnsworth@onelan.com>
> 
> Heh, you were meant to say:
> 
> "What? Export a magic parameter from intel-gtt.c to indicate how much
> space to remove from the aperture guestimate, but not replace the *broken*
> code in i915_dma.c with an interface from intel-gtt.c to query and control
> the populated GTT more precisely?"

Yeah, that's what this patch does.  It'll all be killed by Daniel's AGP
rework though!

-- 
Jesse Barnes, Intel Open Source Technology Center