[Intel-gfx] [PATCH] drm/i915: Don't allow binding objects into the last page of the aperture.

[Jesse Barnes]

On Tue, 12 May 2009 17:12:29 -0700
Eric Anholt <eric at anholt.net> wrote:

> On Tue, 2009-05-12 at 15:34 -0700, Jesse Barnes wrote:
> > On Tue, 12 May 2009 15:29:56 -0700
> > Eric Anholt <eric at anholt.net> wrote:
> > 
> > > This should avoid a class of bugs where the hardware prefetches
> > > past the end of the object, and walks into unallocated memory
> > > when the object is bound to the last page of the aperture.
> > > 
> > > fd.o bug #21488
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c |   12 ++++++++++--
> > >  1 files changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > > b/drivers/gpu/drm/i915/i915_dma.c index 051134c..3133f99 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1011,8 +1011,16 @@ static int i915_load_modeset_init(struct
> > > drm_device *dev) /* Basic memrange allocator for stolen space (aka
> > > vram) */ drm_mm_init(&dev_priv->vram, 0, prealloc_size);
> > >  
> > > -	/* Let GEM Manage from end of prealloc space to end of
> > > aperture */
> > > -	i915_gem_do_init(dev, prealloc_size, agp_size);
> > > +	/* Let GEM Manage from end of prealloc space to end of
> > > aperture.
> > > +	 *
> > > +	 * However, leave one page at the end still bound to the
> > > scratch page.
> > > +	 * There are a number of places where the hardware
> > > apparently
> > > +	 * prefetches past the end of the object, and we've seen
> > > multiple
> > > +	 * hangs with the GPU head pointer stuck in a batchbuffer
> > > bound
> > > +	 * at the last page of the aperture.  One page should be
> > > enough to
> > > +	 * keep any prefetching inside of the aperture.
> > > +	 */
> > > +	i915_gem_do_init(dev, prealloc_size, agp_size - 4096);
> > 
> > What about objects in the middle of the aperture that are
> > immediately followed by unbound pages?  Do we already handle that
> > case by adding a padding page to objects (iirc one of the docs
> > mentions that this is necessary).
> 
> We never have unbound pages inside of the aperture once AGP has
> initialized -- they're always mapped to the scratch page, like the
> docs recommend.  Thus why this patch should work -- it leaves the
> last page unused and just pointing at scratch.

Yeah forgot about that bit, good.

> Note that we've talked about a quick and dirty hack taking advantage
> of the scratch -- make a sysfs knob that says "operate execbuffers
> synchronously, and compare the contents of the scratch page before and
> after each execbuf".  This would let us catch some failures with
> writing beyond the boundaries of objects at a very low code cost.

Yeah that would be great.

-- 
Jesse Barnes, Intel Open Source Technology Center