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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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